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

Support for null Enum values in 0.9.5 does not work for arguments #840

Closed
exogen opened this issue May 2, 2017 · 4 comments · Fixed by #848
Closed

Support for null Enum values in 0.9.5 does not work for arguments #840

exogen opened this issue May 2, 2017 · 4 comments · Fixed by #848

Comments

@exogen
Copy link
Contributor

exogen commented May 2, 2017

A GraphBrainz test started failing with graphql-js v0.9.5 and above. I have an enum like:

export const CoverArtImageSize = new GraphQLEnumType({
  name: 'CoverArtImageSize',
  description: `The image sizes that may be requested at the [Cover Art
Archive](https://musicbrainz.org/doc/Cover_Art_Archive).`,
  values: {
    SMALL: {
      name: 'Small',
      description: 'A maximum dimension of 250px.',
      value: 250
    },
    LARGE: {
      name: 'Large',
      description: 'A maximum dimension of 500px.',
      value: 500
    },
    FULL: {
      name: 'Full',
      description: 'The image’s original dimensions, with no maximum.',
      value: null
    }
  }
})

Note that FULL has the value null. (Yes, I had this null valued Enum even before it was 'officially' supported, and simply worked around it by checking for 'FULL' in the resolver.)

As of v0.9.5, I'm no longer able to supply FULL as an argument, as in this query:

  {
    lookup {
      release(mbid: "b84ee12a-09ef-421b-82de-0441a926375b") {
        coverArt {
          front(size: LARGE)
          back(size: SMALL)
          fullFront: front(size: FULL)
        }
      }
    }
  }

This now results in an error from the server:

GraphQLError: Argument "size" has invalid value FULL.
Expected type "CoverArtImageSize", found FULL.

But clearly FULL is a valid enum value there. It seems the new null support from #836 didn't extend support to arguments and resulted in breaking what was at least workaround-able.

(I can see how this could indeed pose problems for people when determining "no argument provided" vs. "null/undefined enum value provided", but that seems solvable by checking for 'arg' in args and making sure graphql-js only populates passed-in args.)

@exogen
Copy link
Contributor Author

exogen commented May 2, 2017

(I guess a more accurate title would be that 0.9.5 claimed to add support for null valued Enums, but it's not actually full support. I realize that 'FULL' and not null was actually being passed as the value in my code before.)

@exogen exogen changed the title 0.9.5 broke Enum null values provided as arguments Support for null Enum values in 0.9.5 does not work for arguments May 2, 2017
@exogen
Copy link
Contributor Author

exogen commented May 5, 2017

I can work on a PR for this if some details are fleshed out (it's currently keeping me from upgrading GraphQL for GraphBrainz, unless I change my enum's value back to 'FULL' like it was before implicitly). @leebyron When an arg isn't provided, should its value be undefined, treated distinctly from passing an enum whose value is null?

@wincent
Copy link
Contributor

wincent commented May 5, 2017

If I understand your issue correctly, seems like there's an oversight here. Your query is indeed valid and should be accepted.

@exogen
Copy link
Contributor Author

exogen commented May 13, 2017

@wincent I agree! I'm working on a fix for this and have narrowed it down to the isNullish check in isValidLiteralValue, which wasn't updated to account for null-valued Enums.

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

Successfully merging a pull request may close this issue.

2 participants