Fix description function calls

[dylan-chong]

Fixes #935

We use unquote to make sure that descriptions are evaluated in the scope of the caller (inside the YourSchema.__absinthe_blueprint__ function).

we (multiple committers) apply this change to:

• description macro
• enum with description keyword argument
• value with description keyword argument
• object with description keyword argument
• input_object with description keyword argument
• field with description keyword argument
• arg with description keyword argument
• scalar with description keyword argument
• union with description keyword argument
• mutation with description keyword argument
• query with description keyword argument
• directive with description keyword argument

@desc macros are left as is.

we implement this in notation.ex (we recommend looking at this file first for code review)

we also add tests for all of these cases (yes i think all of them) (including using description macro and @desc macros for all of them). We have extracted out duplication in these tests where possible. There does seem to be a lack of unit tests in this library (which is probably why issue #935 appeared in version 1.5 unnoticed for a little while), so this is a step in the right direction

I have tested this version of Absinthe my company's codebase and everything seems to be running smooth! I

Phew, what a journey. Took about a week to get to this point 😅 . Thanks for the suggestion on how to do this @benwilson512!

---

Known issues:

When using a @module_attribute as a description keyword argument (or description macro), you will get an unused module attribute warning. This is reported and fixed in elixir elixir-lang/elixir#10579 (comment)

---

Where to from here?

Well, firstly, @solvedata needs this fix to upgrade to 1.5 - our (thousands of) descriptions are written with function calls to and module attributes. It would be great to get this merged and released so we can start using the new features of absinthe 1.5. (Otherwise, will have to refactor the thousands of descriptions to use hydrate - the whole point of this PR is to avoid doing that, and hopefully saving other people hassle too.)

For the future, we have enum values to evaluate. This is mentioned in issue #946, and this PR does not fix that. (Although it wouldn't be too hard to do, it would be more scope creep for this already big PR). There will be more cases to fix too. Need to think about all the places where this can happen. I am happy to submit another PR to fix a few more things.

We should also eventually deprecate the hydrate functionality as that API is not the most intuitive, and it is unpractically verbose.

Anyway, let me know if you have any questions.

---

Also, we have multiple committers so could you not squash so that @justinakoh can get commit contributions

[dylan-chong]

I also tried fixing the build by force adding the plt after running mix dialyzer git add priv/plts/absinthe.plt -f . Didn't change the build error

Need some help here

[benwilson512]

This is an absolutely monumental amount of work, and your commitment to thorough testing is extremely laudable, so thank you.

From what I've seen, this approach is exactly right. I will continue to look into it further over the holiday.

With respect to your general concerns about functions calls elsewhere (like with values), I think my plan here is to rework how the record! macros work to simply unquote basically everything. I realize that this used to be "supported" in the past without shenanigans but in 1.4 there are non trivial performance and memory foot guns that can happen.

However a few things have changed since we have first released 1.5. The most important change is that :persistent_term has worked fantastically, and it removes the limits we've had in the past with respect to requiring data that was Macro.escape friendly. The second is that we way underestimated how many people were doing dynamic schema content and the feedback we've received makes it clear that we need to have a cleaner story on that. hydrate/2 was a good effort but I agree, its API is hard to use and perhaps as or more importantly, it's a completely different API from the actual blueprint schema transformation phases.

Setting aside where we go with respect to schema manipulation, this PR is extremely good and is very appreciated, thank you!

[benwilson512]

I haven't quite worked out why this work, because I was sure this would fail with:

defmodule Foo do
  use Absinthe.Schema
  
  query do
  end
  @msg "It's a User"

  @desc @msg
  object :user do
  end

  @msg "It's a dog"

  @desc @msg
  object :dog do
  end
end

However based on my local testing:

iex(4)> Foo.__absinthe_type__(:user).description
"It's a User"
iex(5)> Foo.__absinthe_type__(:dog).description 
"It's a dog"

Works great!

[dylan-chong]

The value assigned to @desc module attribute is evaluated outside of Absinthe (@msg is evaluated when setting the @desc), so actually the example you showed there works without any magic (aside from elixir's built in magic itself)

[dylan-chong]

Thanks for the feedback!

So far i can think of extending this PR to

• enum values (as a list)
• enum value (singular values)
• field/arg default_value

You make an interesting point about macros. I do not have any tests for macros at this point.

I look forward to hearing from you!

[dylan-chong]

Dialyzer seems to be passing locally but not in the build

> mix dialyzer
Finding suitable PLTs
Checking PLT...
[:benchee, :compiler, :crypto, :dataloader, :decimal, :deep_merge, :earmark_parser, :eex, :elixir, :ex_doc, :ex_unit, :kernel, :logger, :makeup, :makeup_elixir, :mix, :nimble_parsec, :stdlib, :telemetry]
PLT is up to date!
No :ignore_warnings opt specified in mix.exs. Using default: .dialyzer_ignore.exs.

Starting Dialyzer
[
  check_plt: false,
  init_plt: '/Users/Dylan/Development/solve/absinthe/priv/plts/absinthe.plt',
  files: ['/Users/Dylan/Development/solve/absinthe/_build/dev/lib/absinthe/ebin/Elixir.Absinthe.Language.ObjectField.beam',
   '/Users/Dylan/Development/solve/absinthe/_build/dev/lib/absinthe/ebin/Elixir.Mix.Tasks.Absinthe.Schema.Sdl.beam',
   '/Users/Dylan/Development/solve/absinthe/_build/dev/lib/absinthe/ebin/Elixir.Absinthe.Phase.Document.Validation.NoUnusedVariables.beam',
   '/Users/Dylan/Development/solve/absinthe/_build/dev/lib/absinthe/ebin/Elixir.Absinthe.Blueprint.Draft.Absinthe.Language.ObjectField.beam',
   '/Users/Dylan/Development/solve/absinthe/_build/dev/lib/absinthe/ebin/Elixir.Absinthe.Blueprint.TypeReference.Name.beam',
   ...],
  warnings: [:unknown]
]
Total errors: 0, Skipped: 0, Unnecessary Skips: 2
done in 0m28.96s
done (passed successfully)

[benwilson512]

@binaryseed seems like some sort of artifact caching issue with dialyzer?

[dylan-chong]

Have just added a Known issue section to the PR description

[binaryseed]

I suspect the dialyzer issue is an incorrect cache key in the config: #1007

Probably will need to rebase once the fix gets merged

[benwilson512]

Hey folks, I'm gonna go ahead and merge this. I've run tests locally and confirmed that all are passing, and the underlying changes here are pretty simple. Combined with the extensive testing in the PR, I feel very confident that this is working as intended. Thanks everyone who worked on it!

[dylan-chong]

Thanks @benwilson512 🎉 !!!

Can you think of any other places to make the unquote work? I’m actually keen to make a PR for doing that if #1005 (comment) mentions all the places that need changing

[dylan-chong]

I guess that raises the question would someone want to dynamically set the name of fields? And the types of those fields and I guess types themselves? Seems like an extremely rare case. Dynamically setting field/type names could be an issue with uniqueness

[benwilson512]

@dylan-chong dynamic field / object construction is definitely a case where we will want to advocate that you use a custom compilation phase.

[dylan-chong]

Ah, do you mean write a custom set of macros?

…

On 25/12/2020, at 8:49 PM, Ben Wilson ***@***.***> wrote:  @dylan-chong dynamic field / object construction is definitely a case where we will want to advocate that you use a custom compilation phase. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[benwilson512]

@dylan-chong no, schemas are manipulated and constructed via the blueprint system as of 1.5, allowing data structure manipulation instead of macros. For an example, see: https://hexdocs.pm/absinthe/Absinthe.Schema.html#module-custom-schema-manipulation-in-progress

[dylan-chong]

Ah I see, thanks

…

On 28/12/2020, at 3:15 PM, Ben Wilson ***@***.***> wrote: @dylan-chong <https://github.com/dylan-chong> no, schemas are manipulated and constructed via the blueprint system as of 1.5, allowing data structure manipulation instead of macros. For an example, see: https://hexdocs.pm/absinthe/Absinthe.Schema.html#module-custom-schema-manipulation-in-progress <https://hexdocs.pm/absinthe/Absinthe.Schema.html#module-custom-schema-manipulation-in-progress> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1005 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEFME3IZLFDNEPDEFMPBEXLSW7S2PANCNFSM4VE5UY5Q>.

[binaryseed]

Looks like we lost the String.trim call on descriptions, I suspect this is something we'll want to get back, right @benwilson512?

[binaryseed]

@benwilson512 I'm curious which additional cases we should try to include before releasing 1.6... the dynamic enum values have been requested multiple times since we released 1.5...

And one note regarding hydrate, it's not going to go away - it's the main API for attaching resolvers & other details to SDL based schemas. Its main use case isn't for macro schemas, though it does work to enable them to be much more dynamic.

[binaryseed]

Might have been a good time to do a "Squash and Merge" here ;)

[dylan-chong]

I intentionally removed the String.trim because it was only used on the description macro and not the other 2 ways of doing descriptions (I thought I noted this in a Comment somewhere but I guess I missed that)

…

> On 28/12/2020, at 6:34 PM, Vince Foley ***@***.***> wrote:  @binaryseed commented on this pull request. In lib/absinthe/schema/notation.ex: > @@ -1401,16 +1420,27 @@ defmodule Absinthe.Schema.Notation do scoped_def(env, :enum, identifier, attrs, block) end - defp reformat_description(text), do: String.trim(text) Looks like we lost the String.trim call on descriptions, I suspect this is something we'll want to get back, right @benwilson512? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[binaryseed]

I'd rather we not change the existing behavior, I suspect there are APIs out there that implicitly rely on this. (I know we do, and this would cause the Introspection result to change, which we SHA to tell if any additions happened)

[benwilson512]

@binaryseed Arguably the absence of trimming from the other mechanisms though is a bug. the presence of a new line at the end of a description is really just a quirk of """.

[binaryseed]

Here's a PR that brings back the trimming logic, and does so for all mechanisms: #1014