Fix #650 (@desc not working on field arguments)

[darrenclark]

• Fix @desc does not work for field arguments #650 (@desc not working on field arguments)
• Also fixes the :name option not working for arguments

Approach I took

Since Absinthe.Blueprint.Schema.build_types/3 doesn't push a InputValueDefinition onto the stack (it sets is on the FieldDefinition), a {:desc, _} tuple doesn't get applied to the Schema.InputValueDefinition.

I added a clause to build_types/3 that peeks ahead of a InputValueDefinition to consume the {:desc, _} tuple

Other thoughts

Looking back, would it make more sense to instead to:

• Have Absinthe.Schema.Notation create a :close element after Schema.InputValueDefinition (and any {:desc, _} for that argument), and,
• put the Schema.InputValueDefinition on the stack in build_types/3, then wait for the :close to set it on the Schema.FieldDefinition, and
• let the existing build_types/3 clause for {:desc, _} handle setting description on the Schema.InputValueDefinition

Thoughts?

[benwilson512]

Thanks for taking this on!

In theory, the only clause that should be necessary within build types is

absinthe/lib/absinthe/blueprint/schema.ex

Line 110 in ad2e4c2

defp build_types([{:desc, desc} | rest], [item | stack], buff) do

. Doing a lookahead as you did also works but the look behind is more generalized and ought to work here as well. One issue you fixed was

[benwilson512]

Ah yeah, if we weren't adding the get_desc call here, then it wouldn't have ended up on the stack at all.

[benwilson512]

Good catch!

[benwilson512]

What's weird is that this shouldn't be necessary because the

absinthe/lib/absinthe/blueprint/schema.ex

Line 110 in ad2e4c2

defp build_types([{:desc, desc} | rest], [item | stack], buff) do

clause should already handle this. Doing a lookahead to find the desc should be exactly identical to just moving one more item through the stack, matching on desc, and modifying the previous thing.

However if I remove this clause locally, the tests no longer pass. Investigating.

[benwilson512]

Ah of course, this is what you were saying, I see it now.

absinthe/lib/absinthe/blueprint/schema.ex

Line 149 in ad2e4c2

defp build_types([%Schema.InputValueDefinition{} = arg | rest], [field | stack], buff) do

eagerly removes the InputValue definition from the stack, so further items can't do anything to it.

[benwilson512]

@darrenclark Thanks again for taking this on, and your thoughtful remarks in the description. I do think after looking through this that I'd prefer to treat arg as scoped so that there is only one {:desc oriented clause within build_types. This may be helpful for other macros as well, if I had to guess value probably has this issue too. I don't really want to add special case look-aheads for each of those macros.

[darrenclark]

Any thoughts on if I should go as far as adding it to @scoped_defs and having it handled via record!?

Or just update record_arg! to emit a :close as well?

absinthe/lib/absinthe/schema/notation.ex

Lines 1182 to 1197 in 7f7cb9e

	@scoped_types [
	Schema.ObjectTypeDefinition,
	Schema.FieldDefinition,
	Schema.ScalarTypeDefinition,
	Schema.EnumTypeDefinition,
	Schema.EnumValueDefinition,
	Schema.InputObjectTypeDefinition,
	Schema.UnionTypeDefinition,
	Schema.InterfaceTypeDefinition,
	Schema.DirectiveDefinition
	]
	def record!(env, type, identifier, attrs, block) when type in @scoped_types do
	attrs = expand_ast(attrs, env)
	scoped_def(env, type, identifier, attrs, block)
	end

[benwilson512]

@darrenclark go ahead and add it to scoped defs.

[darrenclark]

@benwilson512 updated

[benwilson512]

Thank you!!!

[FloatingFront]

Likewise thanks!