Deduplicate directives when building schema

[MaeIsBad]

When importing directives from the prototype some of them were duplicated, which caused multiple functions with the same clause to be generated.
This should (partially?) fix #1233

Thanks @maartenvanvliet for looking into the original issue 💙

[benwilson512]

Hey thanks for taking a look at this! I have some concerns about solving it this way that I have noted.

[benwilson512]

Unless I am missing something, shouldn't directives with duplicate identifiers be flagged with errors just like objects with duplicate identifiers? Silently removing them doesn't seem good either because there is no guarantee that the directives are logically identical.

[MaeIsBad]

if duplicate directives cause an error, currently it will cause the code generation to always fail, since absinthe imports duplicate directives from the prototype schema.

I'm not a fan for a few reasons but would using {v.expand, v.identifier} instead work for you?

[MaeIsBad]

Also note that this doesn't change the current behavior.
Absinthe already will overwrite directives if build is called with duplicates, all this does is remove a confusing elixir warning that the users can't really deal with themselves

[maartenvanvliet]

There's a check for duplicate directive identifiers, only this happens before the prototype directives are imported into the schema.

I think alternatively we can prevent the issue from happening by not importing the default directives into the prototype schema. They are of no use there as include/skip only work at the runtime level of a document's execution.

--- a/lib/absinthe/schema/prototype/notation.ex
+++ b/lib/absinthe/schema/prototype/notation.ex
@@ -42,6 +42,10 @@ defmodule Absinthe.Schema.Prototype.Notation do
           Absinthe.Phase.Schema.TypeExtensionImports,
           {Absinthe.Phase.Schema.TypeExtensionImports, []}
         )
+        |> Absinthe.Pipeline.replace(
+          Absinthe.Phase.Schema.DirectiveImports,
+          {Absinthe.Phase.Schema.DirectiveImports, []}
+        )
       end

[benwilson512]

Ah! So that's the issue then. Could we simply do without(Absinthe.Phase.Schema.DirectiveImports) ?

[MaeIsBad]

@maartenvanvliet you mentioned here that

Whilst investigating, I found a similar bug happens with the extend notation, this can also cause multiple functions with the same function heads to be generated

is this the same issue?

[maartenvanvliet]

It's related, the Schema.Notation module does a pass over the blueprint to lift the functions defined in the schema to the schema module.
When it walks over the blueprint with a custom scalar, that is later extended using extend if will generate the same lifted function for both of them.

E.g.

  def __absinthe_function__(
      {Absinthe.Blueprint.Schema.ScalarTypeDefinition, :my_custom_scalar},
      :serialize
  ) do
    nil
  end

will be generated twice.

What I am as of yet unsure of is whether the first or second generated function can simply be removed. In the above case the second can be removed as serialize cannot be overridden by the extended scalar. I don't know whether there are cases currently where the second cannot be removed.

Regardless, supporting the overriding of serialize and similar in the extend syntax is something I've been thinking about for a while. Though it is beyond the graphql spec it would make it easier to override a custom scalar or even a resolver. A use case I have is overriding the introspection resolvers with my own.

I'll have a look at the directive issue you're having

[maartenvanvliet]

The problem with this solution is(I think) that on every "level" of schema inheritance a new :deprecated directive is defined

absinthe/lib/absinthe/schema/prototype/notation.ex

Line 14 in 5d4ef4c

directive :deprecated do

which still causes more of the same errors to appear

I cannot reproduce this. Do you have a repo showing this issue? Ideally with the OTP/elixir version.

[MaeIsBad]

hm, I am unable to reproduce this today as well, no idea why I thought that was what was happening yesterday.

I found that using the schema do notation causes more errors to be generated

defmodule MyAppWeb.Schema do
  use Absinthe.Schema

  schema do
   field :query, :query
   field :subscription, :subscription
  end

  subscription do
   field :name, :string
  end

  query do
    field :item, :string
  end
end

this doesn't happen if I remove the subscription

Compiling 1 file (.ex)
warning: this clause cannot match because a previous clause at line 1 always matches
  lib/asdf.ex:1

warning: this clause cannot match because a previous clause at line 1 always matches
  lib/asdf.ex:1

warning: this clause cannot match because a previous clause at line 1 always matches
  lib/asdf.ex:1

warning: this clause cannot match because a previous clause at line 1 always matches
  lib/asdf.ex:1

Compilation failed due to warnings while using the --warnings-as-errors option

[maartenvanvliet]

yes, the schema notation has the same issues as the extend notation. The fix should be similar as well.

[benwilson512]

@maartenvanvliet do you have any concerns with not importing the default directives into prototypes?

[maartenvanvliet]

@maartenvanvliet do you have any concerns with not importing the default directives into prototypes?

No, that would work.

[benwilson512]

Thanks for handling this @MaeIsBad !