Consider adding defaultValue to @Argument

[marceloverdijk]

Similar as for traditional controllers the @RequestParam annotation supports a defaultValue, it would be nice to have this for the GraphQL @Argument annotation as well.

Rationale:

Imagine this Query definition:

seasons(first: Int = 10, offset: Int = 0): SeasonPage!

here default values are specified in the GraphQL schema, and if not provided by the user the defaults are used automatically.

But the above Season type could have the following field:

type Season {

   standings(first: Int, offset: Int = 0): StandingPage!
}

here the first argument has no default value in the GraphQL schema, basically meaning there is no limit and by default all standings are returned unless the limit is explicitly set by the client.

Now having a SchemaMapping for this like:

@SchemaMapping
public Page<Standing> standings(Season season, @Argument int first, @Argument int offset) { .. }

will fail as null is an illegal argument for the first argument.

It would be nice to define the SchemaMapping like:

@SchemaMapping
public Page<Standing> standings(Season season, @Argument(defaultValue = "99999") int first, @Argument int offset) { .. }

See the arbitrary 99999 as default value which in this proposal will be used when the argument is not passed, or set to null by the client.

I don't want the 99999 in the GraphQL schema itself as it is an arbitrary value without a real meaning,
unlike offset = 0 or first = 10 in the query mapping.

Without a defaultValue I can solve it on other ways to:

• Like mentioned above, add an arbitrary value like 99999 to the GraphQL schema. But even then a client could pass null which will break the controller
• Change the code to @Argument Integer first and test myself on null; this will also make sure the controller won't break.

[jord1e]

Why not add the possibility of doing this: (already possible)

@QueryMapping
public long idLongOrDefault(@Argument Optional<Long> id) {
  return id.orElse(-1);
}

So as to allow Optional<T> to be argument values, including OptionalInt, OptionalLong, and OptionalDouble for performance.

Spring Data only allows Optional<T>

Your proposal would introduce extra complexity, since there would be two places to define default values (the schema, and in your code).

[marceloverdijk]

@jord1e yes it's indeed in 2 places, but with different behaviour.
The one in the schema is public user facing, the one in the controller is more of a technical default value. But I agree it could be confusing... nevertheless I would use it if available ;-) (it would also be consistent with @RequestParam).

As I said I can easily workaround it by using Integer or maybe better Optional<Integer> as you mentioned.

Thinking of it, would it maybe be possible to use custom argument resolvers in GraphQL controllers?
I could introduce a custom Pageable (not to confuse with Spring Data's one) that holds the first and offset arguments and would globally assign default values if not provided...

[jord1e]

Thinking of it, would it maybe be possible to use custom argument resolvers in GraphQL controllers?
I could introduce a custom Pageable (not to confuse with Spring Data's one) that holds the first and offset arguments and would globally assign default values if not provided...

This is currently not possible, the resolvers are hardcoded.

spring-graphql/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerConfigurer.java

Lines 122 to 148 in 5919178

	@Override
	public void afterPropertiesSet() {
	this.argumentResolvers = new HandlerMethodArgumentResolverComposite();
	// Annotation based
	if (springDataPresent) {
	// Must be ahead of ArgumentMethodArgumentResolver
	this.argumentResolvers.addResolver(new ProjectedPayloadMethodArgumentResolver());
	}
	this.argumentResolvers.addResolver(new ArgumentMapMethodArgumentResolver());
	GraphQlArgumentInitializer initializer = new GraphQlArgumentInitializer(this.conversionService);
	this.argumentResolvers.addResolver(new ArgumentMethodArgumentResolver(initializer));
	this.argumentResolvers.addResolver(new ContextValueMethodArgumentResolver());
	// Type based
	this.argumentResolvers.addResolver(new DataFetchingEnvironmentMethodArgumentResolver());
	this.argumentResolvers.addResolver(new DataLoaderMethodArgumentResolver());
	if (springSecurityPresent) {
	this.argumentResolvers.addResolver(new PrincipalMethodArgumentResolver());
	}
	if (KotlinDetector.isKotlinPresent()) {
	this.argumentResolvers.addResolver(new ContinuationHandlerMethodArgumentResolver());
	}
	// This works as a fallback, after other resolvers
	this.argumentResolvers.addResolver(new SourceMethodArgumentResolver());
	}

Better yet

type Query {
    test(id: Int): Int!
}

@QueryMapping
public int test(@Argument Optional<Integer> id) {
    return id.orElse(-1);
}

works
{ test(id: -2) } > -2
{ test } > -1

See

spring-graphql/spring-graphql/src/main/java/org/springframework/graphql/data/GraphQlArgumentInitializer.java

Line 93 in 5919178

if (targetClass == Optional.class) {

[jord1e]

This feature not kind of listed in the documentation:
https://docs.spring.io/spring-graphql/docs/current-SNAPSHOT/reference/html/#controllers-schema-mapping-argument

Edit: "Use @argument to access an argument for the field that maps to the handler method. You can declare such a method parameter to be of any type." is a little vague considering using OptionalInt leads to

{
	"errors": [
		{
			"message": "Failed to convert value of type 'java.lang.Integer' to required type 'java.util.OptionalInt'; nested exception is java.lang.IllegalStateException: Cannot convert value of type 'java.lang.Integer' to required type 'java.util.OptionalInt': no matching editors or conversion strategy found",
			"locations": [
				{
					"line": 1,
					"column": 2
				}
			],
			"path": [
				"test"
			],
			"extensions": {
				"classification": "INTERNAL_ERROR"
			}
		}
	],
	"data": null
}

Edit 2: Found the list of supported "any type":
https://github.com/spring-projects/spring-framework/blob/b848acd86de11605ba14f08175a695cc55d00c97/spring-beans/src/main/java/org/springframework/beans/PropertyEditorRegistrySupport.java#L210-L281

[marceloverdijk]

Thx for analyzing this 👍

So besides my proposal for defaultValue on @Argument (and questionable if desired per your comment) it would also be nice to open up AnnotatedControllerConfigurer to be able to define custom argument resolvers ;-)

[marceloverdijk]

Explaining the bigger picture for my case and what I'm trying to achieve in a more declarative way:

In my GraphQL schema I have several collections fields which all return a Page containing the actual items and pageInfo.
Like:

type Query {
    seasons(first: Int = 10, offset: Int = 0): SeasonPage!
    teams(first: Int = 10, offset: Int = 0): TeamPage!
}

type Season {
    standings(first: Int, offset: Int = 0): StandingPage!
}

So it the Query type I have some top level collections which by default do paging with 10 items per page. Note I also want to limit it with a max value of 100 items per page.

The standings field within the Season will return by default all standing items, but clients could limit it only to 1; e.g. to display multiple seasons with just the champion.

I have many more similar collections and I want to avoid repetitively having to code checking for null, min value, max value etc.

I would love to define a custom argument handler like:

@QueryMapping
public Page<Season> seasons(@PageableDefault(first=10, maxFirst=100, offset=0) Pageable pageable) {
    ..
}

@SchemaMapping
public Page<Standing> standings(
        Season season, 
        @PageableDefault(first=Integer.MAX VALUE, maxFirst=Integer.MAX VALUE, offset=0) Pageable pageable) {
    ..
}

Note that the Integer.MAX VALUE could also be used as default values in my @PageableDefault to avoid having that.

My my names used above are confusing as it is basically a copy from Spring Data....
but it at least points outs what I'm trying to accomplish is on a similar level already possible with MVC and Spring Data.

[rstoyanchev]

Just a clarification question for now. In the snippets, the query returns SeasonPage but I see no definition for a type called that. Instead, you have a type called Season. Is that meant to be SeasonPage, i.e. a typo?

[marceloverdijk]

Besides the GraphQL Season type I have also a SeasonPage type:

type SeasonPage {
    items: [Season!]!
    pageInfo: PageInfo!
}

type PageInfo {
    hasNextPage: Boolean!
    hasPreviousPage: Boolean!
    totalCount: Int!
}

Note this has a bit of similarity with the Relay connection spec, but is different. It uses a simpler limit/offset paging model sufficient and more easier in my case.
If you would like I can share more of the schema.

[marceloverdijk]

It would be nice to understand Spring's team take on this (without having an immediate implementation).
That way I could choose a current approach for my project that that suits future directions of spring-graphql best.

[rstoyanchev]

There are several requests in this, so let me start with the original question only for now.

Initially we had both required and defaultValue attributes on @Argument, mirroring @RequestParam, but dropped those in #150 because we saw no reason for a duplicate mechanism that creates confusing choices. It also makes sense that clients should be aware of such constraints.

You raise a good point that a default value in the schema does not seem to apply to null and it's tempting to bring back defaultValue on @Argument, as it would serve a real purpose. Nevertheless, it still encourages either duplication of the default value, or not exposing it at all. I think neither is good a choice in the general case.

Perhaps we can consider some boolean flag on @Argument to ignore nulls, and that would cause the resolver to fall back on the schema-defined default value, which seems accessible through the GraphQLFieldDefinitionfromDataFetchingEnvironment`. What do you think?

[jord1e]

Perhaps we can consider some boolean flag on @argument to ignore nulls, and that would cause the resolver to fall back on the schema-defined default value, which seems accessible through the GraphQLFieldDefinitionfromDataFetchingEnvironment`. What do you think?

Do you mean:

type query {
  a(myArg: Int = 5): Int!
}

with a resolver

@QueryMapping
public int a(@Argument(fallbackIfNull = true) int myArg) {
  return myArg;
}

Which would return
The value of myArg when it is not-null
and
5 when myArg is null
?

I would strongly favour just using Integer, or adding an argument resolver for OptionalInt. I really don't find this an ideal situation, why would you want this in the first place?.

There is even a difference between null and not specifying the argument at all. This is being discussed in #140 - how would you handle this difference in combination with this proposal?

As far as I am aware no other JVM or GraphQL framework implements this functionality.

[marceloverdijk]

Thx @rstoyanchev for replying.

You raise a good point that a default value in the schema does not seem to apply to null and it's tempting to bring back defaultValue on @argument, as it would serve a real purpose. Nevertheless, it still encourages either duplication of the default value, or not exposing it at all. I think neither is good a choice in the general case.

Clients passing null is indeed a nasty issue at the moment for number types in the GraphQL schema... at least when defined as int in the controller methods.
I would almost tend to always use Optional<Integer> or as @jord1e mentioned OptionalInt (but I believe the latter is not supported at the moment).

---

Other question (and yes I understand in this threads many questions / alternatives are discussed 🙈 ) what is your take on the ability to provide / register custom custom argument resolvers as in normal MVC controllers?

E.g. to do something like:

@QueryMapping
public Page<Season> seasons(Pageable pageable) { .. }

or even:

@QueryMapping
public Page<Season> seasons(@PageableDefault(first=10, offset=0) Pageable pageable) { .. }

[rstoyanchev]

Custom argument resolvers is just something that hasn't come up but nothing against it. It does feel though like @Argument should be able to support creating a Pageable in this case. The capability is there and where ArgumentMethodArgumentResolver calls GraphQlArgumentInitializer, if it passed no name, the target object would be initialized from the full arguments map. This is already how it works for @ProjectedPayload and for Query by Example. So just a matter of having a way to express that this is what you want, and it should be easy to do.

[rstoyanchev]

@jord1e, I understand no value vs null is an important distinction, but it is surprising that there is no a way to express an optional argument with a guaranteed value. You always have to deal with the possibility of null even though the distinction between null vs no value (or vs a default value) may be unnecessary.

This undermines the value of declaring a default value in the schema, especially for Java applications where there isn't a distinction between null vs no value. The end result is you have to check anyway and provide a fall back. Optional<Integer> doesn't solve this. It's just another flavor of handling it. The default value still needs to be duplicated.

I'm now thinking that @Argument should always fill in the default value from the schema, but also look for signs that null is expected, like on a method parameter that is Optional or marked @Nullable.

[marceloverdijk]

As this thread was growing and requesting multiple things I also created 2 separate issues:

#258 Custom @Argument object to access root properties does not work
#259 Add support for custom argument resolvers in GraphQL controllers

[rstoyanchev]

Team decision: null is an explicit input that requires additional effort from the client and if present it needs to be taken into account. Therefore applications should be prepared to handle it. In practice, the extra effort required to send null means it's unlikely to be an issue if it makes no sense to send null like with first and offset.

In terms of options on how we can help to handle the null. A defaultValue on @Argument is no good as it duplicates the schema mechanism. Transparently filling in the schema declared default value is also no good because we don't know enough. We would need something more explicit to get involved. The best place for that would be in the schema, e.g. an argument directive is something we might consider in the future.

[marceloverdijk]

I agree and it's a difficult "problem".
For now probably best to never use primitives as arguments in GraphQL @Controller's as clients can send null explicitly,
and instead e.g. use Optional<Integer> or Integer.

[marceloverdijk]

Note reading this comment from @leebyron : graphql/graphql-js#210 (comment)

This is expected behavior for GraphQL. The library currently does not distinguish between undefined and null values for input. So both are replaced with a default value.

One nice thing about this from a server development point of view is that if you provide a default value for an argument, you can be guaranteed to always get a non-null value for that argument.

graphql-js seems to use the default value from the GraphQL schema when null is explicitly sent by client.

[jord1e]

Note reading this comment from @leebyron : graphql/graphql-js#210 (comment)

This is expected behavior for GraphQL. The library currently does not distinguish between undefined and null values for input. So both are replaced with a default value.

One nice thing about this from a server development point of view is that if you provide a default value for an argument, you can be guaranteed to always get a non-null value for that argument.

graphql-js seems to use the default value from the GraphQL schema when null is explicitly sent by client.

That comment is from 2015 when no null literals were present in GraphQL

See graphql/graphql-js#544 which negates the behavior you (and then Lee) are describing

This test case may also be of your interest:
https://github.com/graphql/graphql-js/blob/a91fdc600f2012a60e44356c373e51c5dd20ba81/src/execution/__tests__/variables-test.ts#L330

Logic for seperate undefined and null handling is located here: https://github.com/graphql/graphql-js/blob/a91fdc600f2012a60e44356c373e51c5dd20ba81/src/execution/values.ts#L97