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

Coercion is attempted on argument values but not on variable values. #123

Closed
johanatan opened this issue Aug 12, 2015 · 10 comments
Closed

Comments

@johanatan
Copy link
Contributor

Schema:

enum Color { blue, orange, green } // mapped to 1, 2, 3

type Favorite {
  id: ID!
  color: Color
}

This doesn't work:

curl -XPOST -H 'Content-Type:application/graphql' -d 'mutation a { createFavorite(color:2) { color }  }' http://localhost:3000/graphql   
{
  "errors": [
    {
      "message": "Argument \"color\" expected type \"Color\" but got: \"2\".",
      "stack": "Argument \"color\" expected type \"Color\" but got: \"2\".",
      "locations": [
        {
          "line": 1,
          "column": 35
        }
      ]
    }
  ]
}

while this does:

curl -XPOST -H 'Content-Type:application/graphql' -d '{"query":"mutation createFavorite($color: Color!) { createFavorite(color:$color) { color } }", "params":{"color":2}}' http://localhost:3000/graphql    
{
  "data": {
    "createFavorite": null // Null is returned by server but server printed log below showing coercion success.
  }
}

Server:

Received insert request: uid: caf02691-a167-453f-aced-65a2fac0aa8f, entity: Favorite, params: {"color" "{\"orange\" {
Returning: null
@leebyron
Copy link
Contributor

Thanks for the report. What should happen here is the variables provided should also have required "params":{"color":"orange"}} and "params":{"color":2}} should have reported an error.

The values mapped to the enums are for internal use only and should not be leaked outside of the graphql public facade.

@johanatan
Copy link
Contributor Author

Ahh, I see. If the variable were a primitive type (i.e., not requiring coercion) would it be ok to embed the value in the mutation itself rather than the params object? If so, it seems a bit inconsistent to have this distinction and I would prefer the condensed [inline] syntax; i.e., params-less, behavior for both primitives and coerced values.

@leebyron
Copy link
Contributor

I'm not sure what you're referring to by primitive in this sense. All scalars in GraphQL go through a coercion step.

The issue here, as I understand it, is that enum coercion from variables is currently incorrect.

# currently correctly works
mutation a { createFavorite(color: orange) { color }  } 

# currently correctly reports incorrect value error
mutation a { createFavorite(color: 2) { color }  }

# currently incorrectly works
mutation createFavorite($color: Color!) { createFavorite(color: $color) { color } }
{ "color": 2 }

# expected to currently work
mutation createFavorite($color: Color!) { createFavorite(color: $color) { color } }
{ "color": "orange" }

@johanatan
Copy link
Contributor Author

Coercion to enum from string is a separate issue (filed separately). This current issue is simply that your first code example does not in fact work.

However if the type of 'color' were changed to string (i.e., a primitive rather than a user-defined type), then it would work.

On Aug 12, 2015, at 4:13 PM, Lee Byron notifications@github.com wrote:

I'm not sure what you're referring to by primitive in this sense. All scalars in GraphQL go through a coercion step.

The issue here, as I understand it, is that enum coercion from variables is currently incorrect.

currently correctly works

mutation a { createFavorite(color: orange) { color } }

currently correctly reports incorrect value error

mutation a { createFavorite(color: 2) { color } }

currently incorrectly works

mutation createFavorite($color: Color!) { createFavorite(color: $color) { color } }
{ "color": 2 }

expected to currently work

mutation createFavorite($color: Color!) { createFavorite(color: $color) { color } }
{ "color": "red" }

Reply to this email directly or view it on GitHub.

@leebyron
Copy link
Contributor

Thanks for the report, I will write a new suite of tests to ensure the correct behavior.

@johanatan
Copy link
Contributor Author

Sorry, actually it is your second and third cases which demonstrate the current issue and, if this issue were fixed, then #1 would demonstrate the other one.

@johanatan
Copy link
Contributor Author

And, you're welcome! Thanks for all of your work on GraphQL as well! 😀

@johanatan
Copy link
Contributor Author

Btw, since your fix for #122 removed the ability to coerce ints into enums, I can't really test if this one is fixed yet (also since the fix for #122 seems to have not actually fixed the coercion of strings into enums).

@leebyron
Copy link
Contributor

removed the ability to coerce ints into enums

To be clear, that was the bug. We were leaking the internal representation (sometimes ints) through to the external API. The test suite verified that bug existed, and the fix 2cf2f4c#diff-15f6b6c92ff954f80995d31726f89851R655 replaced parsing the variables as an internal representation with the external representation, as specified.

@johanatan
Copy link
Contributor Author

Got it. And also my previous comment here was misguided as the failure for the fix to #122 to apply for me seems to be a bug in my code at this point, Will keep you posted if I eliminate that possibility.

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

No branches or pull requests

2 participants