Skip to content

Support ArgumentValue as payload attribute in GraphQlClient #1166

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

Closed
bclozel opened this issue Mar 31, 2025 · 5 comments
Closed

Support ArgumentValue as payload attribute in GraphQlClient #1166

bclozel opened this issue Mar 31, 2025 · 5 comments
Labels
status: superseded Issue is superseded by another type: enhancement A general enhancement

Comments

@bclozel
Copy link
Member

bclozel commented Mar 31, 2025

As seen in this StackOverflow question, developers are expecting to use ArgumentValue<T> as a way of expressing a T value being present/absent/null, not only in GraphQL controllers but also on the client side with GraphQlClient and GraphQlTester.

Currently, if we try to use ArgumentValue<T> on the client side for an input type like this:

record UpdateImageInput (Long id, int version, ArgumentValue<String> title) {
}

This will result in Jackson serializing ArgumentValue itself with its value and ommitted properties.
We should look into ways of supporting this use case.

@bclozel bclozel self-assigned this Mar 31, 2025
@bclozel bclozel added type: enhancement A general enhancement status: waiting-for-triage An issue we've not yet triaged labels Mar 31, 2025
@bclozel
Copy link
Member Author

bclozel commented Mar 31, 2025

As explained in our reference documentation:

GraphQL Java, server applications use Jackson only for serialization to and from maps of data. Client input is parsed into a map. Server output is assembled into a map based on the field selection set. This means you can’t rely on Jackson serialization/deserialization annotations. Instead, you can use custom scalar types.

On the server side, Spring for GraphQL is handling the ArgumentValue<T> case by looking up the map key (presence of value) or then wrapping the existing map value. On the client side, we must rely on Jackson serializing the payload to a Map before it's send over the configured transport.

There are several options we can explore.

  1. Write a custom Jackson Serializer for the UpdateImageInput type. This requires a lot of efforts in all applications.
  2. A combination of Optional<T> + @JsonInclude doesn't seem to be possible, as Optional<T> does not support holding a null value in the first place: it is either empty, or holds a non-null value
  3. A built-in solution for ArgumentValue support in Spring for GraphQL

The third option doesn't seem straightforward at all, because it needs to involve several features and still requires developer interaction. First, we would need to contribute a new Jackson Module that teaches Jackson to serialize ArgumentValue<T> types. This involves both a ReferenceTypeSerializer and a TypeModifier: with that, Jackson knows ArgumentValue as a reference type and knows whether a value is present or not, and how to fetch it. Developers would still need to opt-in and signal whether they want to include empty values during the serialization process.

A complete example looks like this:

class GraphQlJacksonModuleTests {

	private final ObjectMapper objectMapper = new ObjectMapper().registerModule(new GraphQlJacksonModule());

	@Test
	void serializeArgumentValue() throws Exception {
		Project project = new Project("spring-graphql", ArgumentValue.omitted());
		objectMapper.writeValue(System.out, project);

		project = new Project("spring-graphql", ArgumentValue.ofNullable(null));
		objectMapper.writeValue(System.out, project);

		project = new Project("spring-graphql", ArgumentValue.ofNullable("Spring for GraphQL"));
		objectMapper.writeValue(System.out, project);
	}

	// Note, the JsonInclude annotation can be set on the "description" attribute directly
	@JsonInclude(Include.NON_EMPTY)
	record Project(String name, ArgumentValue<String> description) {
	}

}

This results in:

{"name":"spring-graphql"}
{"name":"spring-graphql","description":null}
{"name":"spring-graphql","description":"Spring for GraphQL"}

As a next step, we would need to figure out the following:

  • are there easier ways to support this?
  • would developers be happy with this experience?
  • where and how should we register this Jackson module? Should we delegate to developers or manually register it on the JSON codec in clients?

@JBodkin-Amphora
Copy link

JBodkin-Amphora commented Apr 3, 2025

I've written a Jackson module that can be used to serialize/deserialize the ArgumentValue, which is based upon the implementation that exists for Optional.

The branch for the changes is available here (commit), it has a dependency on the changes in this pull request. If that pull request can be merged, I can rebase and create a pull request for the jackson module.

@bclozel
Copy link
Member Author

bclozel commented Apr 3, 2025

@JBodkin-Amphora Thanks a lot for this proposal. I have very similar changes already stashed, but yours include documentation and tests so we'll proceed with your changes.

Before submitting the PR, here is some feedback that I would provide on the current changes:

  • I would the new types in a org.springframework.graphql.data.json package
  • Add the missing package-info file
  • Instead of documenting this feature in the controllers chapter, I'd add a new "ArgumentValue" section in the client chapter right after Interception
  • Maybe rename the ArgumentValueModule to GraphQlModule so we can gather future features there as well
  • Can we get away with making all types package protected and only leave GraphQlModule as public? This would limit our API exposure if it's not strictly necessary

Please proceed with the PR, with or without making additional changes regarding my feedback, I can take care of those if you don't have time right now. I'll close this issue as superseded once your PR is here. Thanks!

@JBodkin-Amphora
Copy link

JBodkin-Amphora commented Apr 3, 2025

I've made the changes as requested and raised a pull request. Please let me know if there is anything else

@bclozel
Copy link
Member Author

bclozel commented Apr 3, 2025

Superseded by #1174.

@bclozel bclozel closed this as not planned Won't fix, can't repro, duplicate, stale Apr 3, 2025
@bclozel bclozel added status: superseded Issue is superseded by another and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 3, 2025
@bclozel bclozel removed their assignment Apr 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: superseded Issue is superseded by another type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

2 participants