Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1.5.0-rc.2: Calling functions in enum value 'as:' option results in compile error. #843

Closed
aspett opened this issue Jan 13, 2020 · 10 comments · Fixed by #859
Closed

1.5.0-rc.2: Calling functions in enum value 'as:' option results in compile error. #843

aspett opened this issue Jan 13, 2020 · 10 comments · Fixed by #859
Labels

Comments

@aspett
Copy link

aspett commented Jan 13, 2020

Environment

  • Elixir version (elixir -v): Elixir 1.9.4 (OTP 22)
  • Absinthe version (mix deps | grep absinthe): locked at 1.5.0-rc.2 (absinthe) 132ceb90
  • Client Framework and version (Relay, Apollo, etc): N/A

Expected behavior

Expected successful compilation, with enum values defined by function calls. (Worked in 1.4)

Actual behavior

== Compilation error in file lib/app_web/schemas/animal.ex ==                                                                                                                                                             
** (ArgumentError) you attempted to apply :aliases on [as: {{:., [line: 9], [{:__aliases__, [counter: {AppWeb.Schemas.Animal, 5}, line: 9], [:Animal]}, :female]}, [line: 9], []}]. If you are using apply/3, make sure the 
module is an atom. If you are using the dot syntax, such as map.field or module.function, make sure the left side of the dot is an atom or a map                                                                                              
    :erlang.apply([as: {{:., [line: 9], [{:__aliases__, [counter: {AppWeb.Schemas.Animal, 5}, line: 9], [:Animal]}, :female]}, [line: 9], []}], :aliases, [])                                                               
    (elixir) lib/macro.ex:1217: Macro.do_expand_once/2                                                                                                                                                                                        
    (elixir) lib/macro.ex:1308: Macro.do_expand_once/2                                                                                                                                                                                        
    (elixir) lib/macro.ex:1379: Macro.expand_until/2                                                                                                                                                                                          
    (elixir) lib/macro.ex:300: anonymous fn/3 in Macro.prewalk/2                                                                                                                                                                              
    (elixir) lib/macro.ex:270: Macro.do_traverse/4                                                                                                                                                                                            
    (elixir) lib/enum.ex:1440: Enum."-map_reduce/3-lists^mapfoldl/2-0-"/3                                                                                                                                                                     
    (elixir) lib/macro.ex:276: Macro.do_traverse/4

Relevant Schema/Middleware Code

  # ...
  alias App.Context.Animal
  # ...

  enum :animal_sex do
    value :female, as: Animal.female()
    value :male, as: Animal.male()
  end
@binaryseed
Copy link
Contributor

The schema compilation step is operating on the AST representation of the function call, it's not executing the function directly (by design).

@benwilson512 Is this type of thing intended to work?

@benwilson512
Copy link
Contributor

No, you'd want to construct this type via the hydrate call back. We need to document how to do that though, and I'm not actually sure that hydrate supports a {:add, typedef} return yet, although it easily could.

@binaryseed
Copy link
Contributor

@benwilson512 do you mean {:as, value}?

@bgentry
Copy link
Contributor

bgentry commented Feb 20, 2020

I believe I'm encountering this same issue. This is the code which fails to compile:

defmodule MyApp.GraphQL.Schema.Types.Purchase do
  use Absinthe.Schema.Notation

  alias MyApp.Purchase

  enum :purchase_payment_status do
    @desc "The purchase has not yet been paid"
    value(:not_paid, as: Purchase.payment_status().not_paid)
  end
end

Although the error message I get is slightly different, at least until I remove the use of an alias:

== Compilation error in file web/graphql/schema_types/purchase.ex ==
** (FunctionClauseError) no function clause matching in :elixir_aliases.expand/2    
    
    The following arguments were given to :elixir_aliases.expand/2:
    
        # 1
        {:__aliases__, [counter: {MyApp.GraphQL.Schema.Types.Purchase, 6}, line: 12], [:Constants, :Purchase]}
    
        # 2
        [as: {{:., [line: 12], [{{:., [line: 12], [{:__aliases__, [counter: {MyApp.GraphQL.Schema.Types.Purchase, 6}, line: 12], [:Constants, :Purchase]}, :payment_status]}, [line: 12], []}, :not_paid]}, [no_parens: true, line: 12], []}]
    
    (elixir 1.10.0) lib/macro.ex:1372: Macro.do_expand_once/2
    (elixir 1.10.0) lib/macro.ex:1455: Macro.do_expand_once/2
    (elixir 1.10.0) lib/macro.ex:1553: Macro.expand_until/2
    (elixir 1.10.0) lib/macro.ex:439: anonymous fn/3 in Macro.prewalk/2
    (elixir 1.10.0) lib/macro.ex:409: Macro.do_traverse/4
    (elixir 1.10.0) lib/enum.ex:1520: Enum."-map_reduce/3-lists^mapfoldl/2-0-"/3
    (elixir 1.10.0) lib/macro.ex:415: Macro.do_traverse/4

And this works:

defmodule MyApp.GraphQL.Schema.Types.Purchase do
  use Absinthe.Schema.Notation

  enum :purchase_payment_status do
    @desc "The purchase has not yet been paid"
    value(:not_paid, as: "Not Paid")
  end
end

@bgentry
Copy link
Contributor

bgentry commented Feb 20, 2020

Here's another slightly different case where I'm reusing a more complex fragment as my internal value (which I use in my Relay dataloader implementation):

  import Ecto.Query, only: :macros

  enum :item_sort_field do
    value(:id, as: :internal_id, description: "Order by ID")
    value(:name, as: item_name_sort(), description: "Order by Item name")
  end

  defp item_name_sort do
    dynamic([mi], fragment("?->>?", mi.payload, "Name"))
  end

And the error for that:

== Compilation error in file web/graphql/schema_types/item.ex ==
** (ArgumentError) you attempted to apply :module on [as: {:item_name_sort, [line: 97], []}, description: "Order by Item name"]. If you are using apply/3, make sure the module is an atom. If you are using the dot syntax, such as map.field or module.function, make sure the left side of the dot is an atom or a map
    :erlang.apply([as: {:tem_name_sort, [line: 97], []}, description: "Order by Item name"], :module, [])
    (elixir 1.10.0) lib/macro.ex:1422: Macro.do_expand_once/2
    (elixir 1.10.0) lib/macro.ex:1553: Macro.expand_until/2
    (elixir 1.10.0) lib/macro.ex:439: anonymous fn/3 in Macro.prewalk/2
    (elixir 1.10.0) lib/macro.ex:409: Macro.do_traverse/4
    (elixir 1.10.0) lib/enum.ex:1520: Enum."-map_reduce/3-lists^mapfoldl/2-0-"/3
    (elixir 1.10.0) lib/macro.ex:415: Macro.do_traverse/4
    (elixir 1.10.0) lib/macro.ex:439: Macro.prewalk/2

It doesn't appear that there's much of a workaround in this particular case since I'm actually trying to pass a value that needs to be generated by that function (rather than a constant string). I guess I could try something like passing an anonymous fn and handling that in the place where this value gets used but that could get quite messy.

@benwilson512
Copy link
Contributor

Hi @bgentry the thing you're seeing is unrelated. Expanding modules is supposed to work, having function calls in type definitions is not supposed to work. Specifically:

    value(:name, as: item_name_sort(), description: "Order by Item name")

Is not supposed to work, even if item_name_sort simply returned a value. The schema definition macros require constants, which you can either supply directly as literals or supply via other macros. You'll want to use the new hydrate callback to generate types in a more dynamic or programatic fashion.

Taking a step further though, it sort of looks like you're trying to use an Enum in a way that differs from their intended use. What do you imagine Absinthe doing with the Ecto query returned from item_name_sort()? Absinthe and Ecto are two entirely unrelated projects, Absinthe has no idea what to do with an Ecto query.

@bgentry
Copy link
Contributor

bgentry commented Feb 20, 2020

@benwilson512 yeah, I understand that Ecto and Absinthe are different projects — that's not my issue 🙂

In existing released versions of Absinthe, you can use any value at all in the :as of an enum: an atom, string, a random Ecto fragment, whatever. This turned out to be quite useful when I wanted to add additional sorting options to my Relay dataloader connections which are not simple columns in the database: I could use a fragment as in the example above, and my underlying code could just sort by whatever value was passed in that param (whether it was a plain atom field name, or something more complicated). This works fine pre-v1.5.

However, due to the significant rewriting of Absinthe in v1.5, some previously functional bits appear to be broken. That includes:

  • Enum values derived from aliases
  • Enum values derived from function calls, which now apparently have to use the hydrate pattern
  • Enum values are limited to simple hardcoded values, and can no longer be complex or dynamically defined.

I believe the dynamic example above does actually work once I move to the hydrate pattern (as shown in #859), but I'll be sure to report back (on a different issue) if that turns out not to be the case. Edit: actually this doesn't work, it looks like it would require the code changes in #859 too.

Regarding this alias thing being a separate issue, my apologies for posting on the wrong issue. I had a separate new issue written up before I discovered this one which seemed similar enough to warrant posting here instead. If you'd like, I can still go ahead and open a separate issue for the alias thing, but AFAICT it seems like we're supposed to migrate to using hydrate to interpret plain values in enums after compilation anyway, so I don't think I'd have a use case for alias in enum definitions anymore. Let me know if you'd like me to open that issue anyway though!

@aspett
Copy link
Author

aspett commented Feb 20, 2020

Yeah it's unfortunate that calling (simple) functions will no longer work going forward. It means we will be having state or enumeration strings defined in multiple places

@binaryseed
Copy link
Contributor

Check out #859 , it's still possible via hydrate

@aspett
Copy link
Author

aspett commented Feb 20, 2020

Check out #859 , it's still possible via hydrate

Ah, my bad. Thanks for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants