From 71237145fd6fd5b6d80dd50ddc826eb6b213bdbc Mon Sep 17 00:00:00 2001 From: Maarten van Vliet Date: Sat, 10 Sep 2022 22:01:46 +0200 Subject: [PATCH 1/2] Validate type references for invalid wrapped types In some places we were not checking for invalid wrapped types (non-null/list). E.g. ```elixir union :my_union do types [:boolean, list_of(:id)] end ``` Or when using `import_fields` or `interface` with a wrapped type. This PR adds the missing checks in the TypeReferencesExist phase. All cases where a wrapped type is invalid are now caught in this phase. What still needs to be tackled is whether the types passed in are valid for that case. A union's types can only consist of object types, interfaces can only refer to interface definitions etc. This could be done in a separate phase. Some of this work is (incidentally) tackled by other phases but I think it makes sense to group this. Fixes #1048 --- lib/absinthe/blueprint/schema.ex | 6 + lib/absinthe/blueprint/type_reference.ex | 4 + .../validation/type_references_exist.ex | 98 ++++++++---- lib/absinthe/schema/notation.ex | 15 +- .../schema/type_references_exist_test.exs | 147 ++++++++++++++++++ test/absinthe/schema/notation/import_test.exs | 1 + 6 files changed, 237 insertions(+), 34 deletions(-) create mode 100644 test/absinthe/phase/schema/type_references_exist_test.exs diff --git a/lib/absinthe/blueprint/schema.ex b/lib/absinthe/blueprint/schema.ex index d45f7e679d..4657c299a9 100644 --- a/lib/absinthe/blueprint/schema.ex +++ b/lib/absinthe/blueprint/schema.ex @@ -160,6 +160,11 @@ defmodule Absinthe.Blueprint.Schema do build_types(rest, [obj | stack], buff) end + defp build_types([{:type, type} | rest], [obj | stack], buff) do + obj = Map.update!(obj, :types, &[type | &1]) + build_types(rest, [obj | stack], buff) + end + defp build_types([{:__private__, private} | rest], [entity | stack], buff) do entity = Map.update!(entity, :__private__, &update_private(&1, private)) build_types(rest, [entity | stack], buff) @@ -265,6 +270,7 @@ defmodule Absinthe.Blueprint.Schema do end defp build_types([:close | rest], [%Schema.UnionTypeDefinition{} = union, schema | stack], buff) do + union = Map.update!(union, :types, &Enum.reverse/1) build_types(rest, [push(schema, :type_definitions, union) | stack], buff) end diff --git a/lib/absinthe/blueprint/type_reference.ex b/lib/absinthe/blueprint/type_reference.ex index 91cc22e72d..a60d7a22a0 100644 --- a/lib/absinthe/blueprint/type_reference.ex +++ b/lib/absinthe/blueprint/type_reference.ex @@ -45,6 +45,10 @@ defmodule Absinthe.Blueprint.TypeReference do name end + def name(name) do + name |> to_string() |> String.capitalize() + end + def to_type(%__MODULE__.NonNull{of_type: type}, schema) do %Absinthe.Type.NonNull{of_type: to_type(type, schema)} end diff --git a/lib/absinthe/phase/schema/validation/type_references_exist.ex b/lib/absinthe/phase/schema/validation/type_references_exist.ex index 74eaf61bbb..badac8900c 100644 --- a/lib/absinthe/phase/schema/validation/type_references_exist.ex +++ b/lib/absinthe/phase/schema/validation/type_references_exist.ex @@ -1,6 +1,9 @@ defmodule Absinthe.Phase.Schema.Validation.TypeReferencesExist do @moduledoc false + # Checks whether all types referenced in the schema exist and + # are of the correct kind. + use Absinthe.Phase alias Absinthe.Blueprint alias Absinthe.Blueprint.Schema @@ -24,29 +27,37 @@ defmodule Absinthe.Phase.Schema.Validation.TypeReferencesExist do def validate_schema(node), do: node def validate_types(%Blueprint.Schema.FieldDefinition{} = field, types) do - check_or_error(field, field.type, types) + check_or_error(field, field.type, types, inner_type: true) end def validate_types(%Blueprint.Schema.ObjectTypeDefinition{} = object, types) do object - |> check_types(:interfaces, &check_or_error(&2, &1, types)) - |> check_types(:imports, fn {type, _}, obj -> check_or_error(obj, type, types) end) + |> check_types(:interfaces, &check_or_error(&2, &1, types, inner_type: false)) + |> check_types(:imports, fn {type, _}, obj -> + check_or_error(obj, type, types, inner_type: false) + end) end def validate_types(%Blueprint.Schema.InterfaceTypeDefinition{} = interface, types) do - check_types(interface, :interfaces, &check_or_error(&2, &1, types)) + interface + |> check_types(:interfaces, &check_or_error(&2, &1, types, inner_type: false)) + |> check_types(:imports, fn {type, _}, obj -> + check_or_error(obj, type, types, inner_type: false) + end) end def validate_types(%Blueprint.Schema.InputObjectTypeDefinition{} = object, types) do - check_types(object, :imports, fn {type, _}, obj -> check_or_error(obj, type, types) end) + check_types(object, :imports, fn {type, _}, obj -> + check_or_error(obj, type, types, inner_type: false) + end) end def validate_types(%Blueprint.Schema.InputValueDefinition{} = input, types) do - check_or_error(input, input.type, types) + check_or_error(input, input.type, types, inner_type: true) end def validate_types(%Blueprint.Schema.UnionTypeDefinition{} = union, types) do - check_types(union, :types, &check_or_error(&2, &1, types)) + check_types(union, :types, &check_or_error(&2, &1, types, inner_type: false)) end def validate_types(%Blueprint.Schema.TypeExtensionDefinition{} = extension, types) do @@ -55,7 +66,7 @@ defmodule Absinthe.Phase.Schema.Validation.TypeReferencesExist do declaration definition -> - check_or_error(extension, definition.identifier, types) + check_or_error(extension, definition.identifier, types, inner_type: false) end end @@ -63,14 +74,13 @@ defmodule Absinthe.Phase.Schema.Validation.TypeReferencesExist do Blueprint.Schema.DirectiveDefinition, Blueprint.Schema.EnumTypeDefinition, Blueprint.Schema.EnumValueDefinition, - Blueprint.Schema.InterfaceTypeDefinition, - Blueprint.Schema.ObjectTypeDefinition, Blueprint.Schema.ScalarTypeDefinition, Blueprint.Schema.SchemaDefinition, Blueprint.Schema.SchemaDeclaration, Blueprint.TypeReference.NonNull, Blueprint.TypeReference.ListOf, - Absinthe.Blueprint.TypeReference.Name + Blueprint.TypeReference.Name, + Blueprint.TypeReference.Identifier ] def validate_types(%struct{} = type, _) when struct in @no_types do type @@ -86,34 +96,46 @@ defmodule Absinthe.Phase.Schema.Validation.TypeReferencesExist do |> Enum.reduce(entity, fun) end - defp check_or_error(thing, type, types) do - type = unwrap(type) + defp check_or_error(thing, type, types, opts) when is_list(opts) do + check_or_error(thing, type, types, Map.new(opts)) + end + + defp check_or_error(thing, type, types, %{inner_type: true}) do + type = inner_type(type) + check_or_error(thing, type, types, inner_type: false) + end + + defp check_or_error(thing, type, types, %{inner_type: false}) do + case unwrapped?(type) do + {:ok, type} -> + if type in types do + thing + else + put_error(thing, error(thing, type)) + end - if type in types do - thing - else - put_error(thing, error(thing, type)) + :error -> + put_error(thing, wrapped_error(thing, type)) end end - defp unwrap(value) when is_binary(value) or is_atom(value) do + defp inner_type(value) when is_binary(value) or is_atom(value) do value end - defp unwrap(%Absinthe.Blueprint.TypeReference.Name{name: name}) do - name + defp inner_type(%{of_type: type}) do + inner_type(type) end - defp unwrap(type) do - unwrap_type = Absinthe.Blueprint.TypeReference.unwrap(type) - - if unwrap_type == type do - type - else - unwrap(unwrap_type) - end + defp inner_type(%Absinthe.Blueprint.TypeReference.Name{name: name}) do + name end + defp unwrapped?(value) when is_binary(value) or is_atom(value), do: {:ok, value} + defp unwrapped?(%Absinthe.Blueprint.TypeReference.Name{name: name}), do: {:ok, name} + defp unwrapped?(%Absinthe.Blueprint.TypeReference.Identifier{id: id}), do: {:ok, id} + defp unwrapped?(_), do: :error + defp error(thing, type) do %Absinthe.Phase.Error{ message: message(thing, type), @@ -141,4 +163,24 @@ defmodule Absinthe.Phase.Schema.Validation.TypeReferencesExist do Types must exist if referenced. """ end + + defp wrapped_error(thing, type) do + %Absinthe.Phase.Error{ + message: wrapped_message(thing, type), + locations: [thing.__reference__.location], + phase: __MODULE__ + } + end + + defp wrapped_message(thing, type) do + kind = Absinthe.Blueprint.Schema.struct_to_kind(thing) + artifact_name = String.capitalize(thing.name) + + """ + In #{kind} #{artifact_name}, cannot accept a non-null or a list type. + + Got: #{Blueprint.TypeReference.name(type)} + + """ + end end diff --git a/lib/absinthe/schema/notation.ex b/lib/absinthe/schema/notation.ex index 59d0a3bd47..c025345ddb 100644 --- a/lib/absinthe/schema/notation.ex +++ b/lib/absinthe/schema/notation.ex @@ -1706,11 +1706,9 @@ defmodule Absinthe.Schema.Notation do @doc false # Record an implemented interface in the current scope - def record_interface!(env, identifier) do - put_attr(env.module, {:interface, identifier}) - # Scope.put_attribute(env.module, :interfaces, identifier, accumulate: true) - # Scope.recorded!(env.module, :attr, :interface) - # :ok + def record_interface!(env, type) do + type = expand_ast(type, env) + put_attr(env.module, {:interface, type}) end @doc false @@ -1730,7 +1728,12 @@ defmodule Absinthe.Schema.Notation do @doc false # Record a list of member types for a union in the current scope def record_types!(env, types) do - put_attr(env.module, {:types, types}) + Enum.each(types, &record_type!(env, &1)) + end + + defp record_type!(env, type) do + type = expand_ast(type, env) + put_attr(env.module, {:type, type}) end @doc false diff --git a/test/absinthe/phase/schema/type_references_exist_test.exs b/test/absinthe/phase/schema/type_references_exist_test.exs new file mode 100644 index 0000000000..f50cbcf285 --- /dev/null +++ b/test/absinthe/phase/schema/type_references_exist_test.exs @@ -0,0 +1,147 @@ +defmodule Absinthe.Schema.Validation.TypeReferencesExistTest do + use Absinthe.Case, async: true + + describe "fields" do + @schema ~S{ + defmodule FieldSchema do + use Absinthe.Schema + + query do + field :foo, :string + field :bar, non_null(:string) + field :baz, :qux + end + end + } + + test "errors unknown type reference" do + error = ~r/In field Baz, :qux is not defined in your schema./ + + assert_raise(Absinthe.Schema.Error, error, fn -> + Code.eval_string(@schema, [], __ENV__) + end) + end + end + + describe "objects" do + @schema ~S{ + defmodule ObjectImportSchema do + use Absinthe.Schema + + query do + field :foo, :string + end + + object :baz do + field :name, :string + end + + object :bar do + import_fields non_null(:baz) + end + end + } + test "errors on import_fields with wrapped type" do + error = ~r/In object Bar, cannot accept a non-null or a list type.\n\nGot: Baz!/ + + assert_raise(Absinthe.Schema.Error, error, fn -> + Code.eval_string(@schema, [], __ENV__) + end) + end + + @schema ~S{ + defmodule ObjectInterfaceSchema do + use Absinthe.Schema + + query do + field :foo, :string + end + + object :baz do + field :name, :string + end + + object :qux do + interface list_of(:baz) + end + end + } + test "errors on interface with wrapped type" do + error = ~r/In object Qux, cannot accept a non-null or a list type./ + + assert_raise(Absinthe.Schema.Error, error, fn -> + Code.eval_string(@schema, [], __ENV__) + end) + end + end + + describe "interface" do + @schema ~S{ + defmodule InterfaceSchema do + use Absinthe.Schema + + query do + field :foo, :string + end + + interface :qux do + interface list_of(:baz) + end + end + } + test "errors on interface with wrapped type" do + error = ~r/In interface Qux, cannot accept a non-null or a list type./ + + assert_raise(Absinthe.Schema.Error, error, fn -> + Code.eval_string(@schema, [], __ENV__) + end) + end + end + + describe "input object" do + @schema ~S{ + defmodule InputObjectSchema do + use Absinthe.Schema + + query do + field :foo, :string + end + + input_object :bar do + import_fields non_null(:baz) + end + end + } + test "errors on import_fields with wrapped type" do + error = ~r/In input object Bar, cannot accept a non-null or a list type.\n\nGot: Baz!/ + + assert_raise(Absinthe.Schema.Error, error, fn -> + Code.eval_string(@schema, [], __ENV__) + end) + end + end + + describe "union type" do + @schema ~S{ + defmodule UnionSchema do + use Absinthe.Schema + + query do + field :foo, :string + end + + union :bar do + types [list_of(:baz)] + end + + end + } + test "errors on types with wrapped type" do + error = ~r/In union Bar, cannot accept a non-null or a list type./ + + assert_raise(Absinthe.Schema.Error, error, fn -> + Code.eval_string(@schema, [], __ENV__) + end) + end + end +end diff --git a/test/absinthe/schema/notation/import_test.exs b/test/absinthe/schema/notation/import_test.exs index 0ea984d0c0..8699572e0d 100644 --- a/test/absinthe/schema/notation/import_test.exs +++ b/test/absinthe/schema/notation/import_test.exs @@ -106,6 +106,7 @@ defmodule Absinthe.Schema.Notation.ImportTest do object :baz do import_fields :bar + field :age, :integer end end From b2cbbd51071a73156d32f85e01dd8e5cc26d912e Mon Sep 17 00:00:00 2001 From: Maarten van Vliet Date: Sun, 11 Sep 2022 14:31:05 +0200 Subject: [PATCH 2/2] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d5a99d5ea0..e85eea8208 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## Unreleased +- Bug Fix: [Validate type references for invalid wrapped types](https://github.com/absinthe-graphql/absinthe/pull/1195) - Breaking Bugfix: [Validate repeatable directives on schemas](https://github.com/absinthe-graphql/absinthe/pull/1179) - Bug Fix: Adds **optional fix** for non compliant built-in scalar Int type. `use Absinthe.Schema, use_spec_compliant_int_scalar: true` in your schema to use the fixed Int type. It is also advisable to upgrade for custom types if you are leveraging the use of integers outside the GraphQl standard. [#1131](https://github.com/absinthe-graphql/absinthe/pull/1131). - Feature: [Support error tuples when scalar parsing fails](https://github.com/absinthe-graphql/absinthe/pull/1187)