Add nullvalue

[jaemk]

Continues work from pr #119 addressing issue #118

Most of the work was implemented by @yen223
This adds extra tests found in the ref. implementation

[syrusakbary]

Hi @jaemk, there are still some code that is missing to be updated (apart from the ast one), such as:

• ast_from_value
• value_from_ast
  As well as tests checking that a null value provided as a field argument is properly retrieved from the resolver.

You can look into the original PR and check what are all the changes needed:
https://github.com/graphql/graphql-js/pull/544/files

[jaemk]

Thanks for reviewing, @syrusakbary! ast_from_value/value_from_ast are updated now, along with a couple more tests. I believe there's already a test for checking that null is allowed as a field name in test_null_as_name. Let me know if I missed anything else!

Edit: Sorry, I see I missed a couple things from that original PR -- I'll ping you once they're added.

[pbeckham]

@jaemk What's left to do here except fixing the python 2 syntax error picked up by travis-ci? Would be happy to help get this merged, as it is causing us issues in querying currently.

[jaemk]

@pbeckham I've been sitting on some more changes, I'll push those later today. The last issue I think I'm down to is dealing with null default values during schema introspection

[svilendobrev]

apart of above conflicts (caused by changed formating/style of code), the test graphql/type/tests/test_introspection.test_introspects_on_input_object() needs fixing, seems List has no defaultvalue=None anymore

See my fork for the result: master@ https://github.com/svilendobrev/graphql-core

[svilendobrev]

after using it, there is some side-effect: get-query argument-fields that are optional and are not specified, do appear as None in resolvers. If they are declared with explicit default_value=graphql.Undefined, they still do appear - as Undefined.
This will break old code that expects omitted arguments to stay omitted...

[svilendobrev]

and, same goes for mutation.. optional args that are not send in, are being filled with their default-value, and passed to resolver

[svilendobrev]

ahha, Argument/InputValue .defaultValue should be set to (internal) graphql.utils.undefined.UndefinedDefaultValue (and not the visible graphql.Undefined), then they are not auto-added each time. But, then, such an argument cannot be set to null, because execution.values.get_argument_values() has wrong logic (at end) that omits Nones instead of passing them through to result. :/

[jaemk]

@syrusakbary I think I've checked all the boxes. Mind taking another look?

[josh-williams]

@jaemk @syrusakbary Any update on this PR and when it might be merged/released?

[jaemk]

@syrusakbary - checking in to see if there's anything else I can do

[clarkmoody]

Ready for this to land here as well.

[schedutron]

Anyone knows when this will be merged?

[Cito]

Note that this is the repository for the legacy v2 branch. Active development is happening in the v3 branch only (graphql-core-next repository). Adding new features here is difficult since this branch currently does not have a dedicated maintainer any more after Syrus left and we would make it also more forward incompatible with v3 which will lead to frustration for users trying to upgrade. We need to rename repositories and make this more clear in the docs and issue templates, I know.

[syrusakbary]

after Syrus left

I'm still around, just not as active 😜

[schedutron]

@Cito Does graphql-core-next repo have support for nulls?

[Cito]

@Syrus 😃 Good to know. In fact as you see we could need a little help with maintaining v2.

Also, since I'm having you on the phone: Do you think we should rename the repositories (graphql-core -> graphql-core-2, graphql-core-next -> graphql-core-3 or graphql-core) to make this less confusing after we released v3? Renaming repos is always a bit problematic, since direct links to issues, PRs etc. will break and GitHub automatically redirects from old to new names. But merging the repos is also problematic, and we would also need to merge the PRs and issues. We can discuss this in #249.

@schedutron Yes, Core v3 has support for nulls and everything that is supported in the current GraphQL.js. Sorry, my comment was a bit misleading because it actually was meant for another PR that suggested a completely new feature that did not even exist in GraphQL.js.

It would in fact make sense to add features that already exist in Core v3 to v2. However we need to take care that it does not break backward compatibility, or make sure that other dependent libraries like Graphene 2 (and there are many others) are still working or updated as well. And it should also not break forward compatibility for people who want to move to v3, i.e. the feature should be implemented and tested in a similar way as in Core v3. Personally, my spare time is barely enough to maintain v3 and keep it up to date, but if anybody wants to jump in and help, of course I do not want to discourage you from that! We had a couple of volunteers last year, but currently nobody seems to be active any more so I felt compelled to answer so that people know the curent state and are not waiting forever.

[Cito]

Btw, regarding compatibility between v2 and v3, can you have a look at graphql-python/graphql-core#77 which is a bit related?