Closed lindaandersson closed 11 months ago
Thanks for the report! graphql-java drastically changed how it handles default values and a lot of logic in SPQR had to be reimplemented. Somewhere in that migration your case fell through the cracks. I'll investigate what can be done.
Thank you for your quick reply! It would be really appreciated :)
Oof, this is tricky business... Here's the breakdown.
In earlier versions, graphql-java permitted any Java object as the default value, and it would then do its best to represent such value in the serialized schema. And what SPQR did was simply deserialize the provided default value string into an object of the corresponding class and had it over to graphql-java. In your case, {}
would deserialize to a Pagination
instance, which would then get serialized back into schema as {limit : 10, offset : 0}
.
Then, recent versions of graphql-java got rather strict and started only accepting values that can be unambiguously represented in the schema - in Java terms that would mean primitives, strings, lists and maps. No arbitrary Java objects allowed. So the way SPQR adapted was to deserialize the given default value to just the simple types mentioned previously. In your example, it means {}
gets deserialized to an empty map, with gets serialized back into the schema as nothing.
At runtime, in both cases, you receive an equal instance of Pagination
. This is because in the first case the default value is already a Pagination
that simply gets passed, and in the second the default value is a map that gets converted into Pagination
at each invocation. So the there should be no observable differences at runtime. But the printed schema is, as you've noticed, different.
I am not exactly sure yet how to approach this. It sounds possible to emulate the earlier behavior of graphql-java by deserializing the provided default value string to an instance of the corresponding class just as before (thus triggering the appropriate constructor and populating the fields), but then convert it into a map before handing it back to graphql-java. In your example, that would mean deserializing {}
to Pagination
first, just as before, and then converting that instance into a map {limit : 10, offset : 0}
. ~This also sounds more correct, as the default values will get captured as they really are in Java. But I have to investigate a bit more to see whether this is indeed possible and will not have any unwanted effects.~
This also sounds more correct, as the default values will get captured as they really are in Java. But I have to investigate a bit more to see whether this is indeed possible and will not have any unwanted effects.
On second though, this really isn't true, is it? After all, you're not surprised that you're seeing
input PaginationInput {
limit: Int
offset: Int
}
and not
input PaginationInput {
limit: Int = 10
offset: Int = 0
}
despite 10 and 0 being the default for the corresponding Java fields, right? To achieve the latter, you'd have to do something like:
public class Pagination {
@GraphQLInputField(defaultValue = "10")
public Integer limit = 10;
@GraphQLInputField(defaultValue = "0")
public Integer ffs = 0;
}
So why would you then be surprised to see
PaginationInput = {}
and not
PaginationInput = {limit : 10, offset : 0}
when {}
is what was specified and not {limit : 10, offset : 0}
? The above 2 cases are semantically equivalent, right? In both cases the GraphQL values and the Java values differ, and have to be brought in line explicitly.
Hmmm... I'm really unsure what to make of this yet 🫤
I think the current behavior is consistent, so I'm inclined to leave it as-is. But, here's how you can pretty easily get the behavior you're after by yourself:
public class JsonReserializer implements DefaultValueProvider {
private static final AnnotatedType OBJECT = TypeFactory.annotatedClass(Object.class, new Annotation[0]);
private final ValueMapper valueMapper;
public JsonReserializer() {
this.valueMapper = Defaults.valueMapperFactory().getValueMapper();
}
@Override
public DefaultValue getDefaultValue(AnnotatedElement targetElement, AnnotatedType type, DefaultValue initialValue) {
// This is just guarding against exotic edge-cases, it's not strictly necessary
if (initialValue.getValue() == null || initialValue.getValue().getClass() != String.class || type.getType() == String.class) {
return initialValue;
}
return initialValue
.map(v -> valueMapper.fromString((String) v, type)) //String -> Pagination
.map(v -> valueMapper.fromInput(v, OBJECT)); //Pagination -> Map (as graphql-java wants it)
}
}
You can then either register this as the default provider, or use it where applicable only:
@GraphQLQuery(name = "items")
public CompletableFuture<List<RelayTest.Book>> items(
@GraphQLArgument(
name = "pagination",
description = "Pagination of results.",
defaultValue = "{}",
defaultValueProvider = JsonReserializer.class)
final Pagination pagination) {
return ...;
}
Note: Make sure Pagination
has fields visible for serialization (e.g. make them public or have public getters). Previously Pagination
instances were only ever deserialized, now they're being serialized as well.
Do you find this solution acceptable?
Hi!
Thank you for all the context and advice. I didn't know about the way you could use @GraphQLInputField(defaultValue = "10")
in a class like that, that is really nice! And maybe one way I can go about defining the default values in the future.
I also tried out the JsonReserializer
solution which also worked great. That would make it possible to have the schema look the same as it used to and we could keep {}
as the default value, so that would also be an option.
The variables are public in the Pagination class, I just forgot to change it in my example, sorry about that. I am using Lombok and have the following annotations @Data @NoArgsConstructor @AllArgsConstructor @Builder
on the class.
However, the issue still remains :(
When I have defaultValue
set in the GraphQLArgument
in the java code, either as "{}"
or as
"{\"limit\":10, \"offset\":0}"
and I write my query like this with the separate fields defined:
query GetItems( $limit: Int, $offset: Int) {
items(
pagination: {limit: $limit, offset: $offset}
) {
name
color
}
}
With input:
{
"limit": 5,
"offset": 0
}
Then I still get the following error:
graphql.AssertException: Expected a value that can be converted to type 'Int' but it was a 'LinkedHashMap'
at graphql.Assert.assertNotNull(Assert.java:17)
at graphql.scalar.GraphqlIntCoercing.valueToLiteralImpl(GraphqlIntCoercing.java:94)
at graphql.scalar.GraphqlIntCoercing.valueToLiteral(GraphqlIntCoercing.java:140)
at graphql.execution.ValuesResolverConversion.externalValueToLiteralForScalar(ValuesResolverConversion.java:148)
at graphql.execution.ValuesResolverConversion.externalValueToLiteral(ValuesResolverConversion.java:132)
at graphql.execution.ValuesResolverConversion.valueToLiteralImpl(ValuesResolverConversion.java:77)
at graphql.execution.ValuesResolver.valueToLiteral(ValuesResolver.java:220)
at graphql.validation.rules.VariableTypesMatch.checkVariable(VariableTypesMatch.java:68)
at graphql.validation.RulesVisitor.lambda$checkVariable$12(RulesVisitor.java:154)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
at graphql.validation.RulesVisitor.checkVariable(RulesVisitor.java:154)
at graphql.validation.RulesVisitor.enter(RulesVisitor.java:79)
Maybe the error is strictly related to how the default value gets interpreted and parsed when generating the schema, but it is more something that fails later in run time, since the other formats of the query that I posted in my initial question still works. Maybe this is outside of the graphql-spqr scope?
Another question just because I got curious. With the new default value validation changes made in graphql-java, does this mean it is no longer recommended to send input values as objects? (And I do agree that it was a bit too loose validation before, so having it stricter sounds great! We found a couple of miss-matching types when upgrading as well.)
I managed to replicate this without SPQR, so it really is a bug in graphql-java. I opened an issue there: https://github.com/graphql-java/graphql-java/issues/3276
With the new default value validation changes made in graphql-java, does this mean it is no longer recommended to send input values as objects?
Nothing perceivable should have changed, and I don't see anything wrong with how you're using the variables. It's really just a bug that will likely get fixed quickly 🤞
I'll close this issue now as it seems the SPQR part has been addressed.
Thank you! I should have checked that before reporting it to you.
Looking forward to the new release with the fix!
I am trying to upgrade from 0.11 to 0.12 and I ran into an issue related to DefaultValues.
My query used to be defined like this:
This used to give me the following schema where the default values were presens.
With the new version I had to explicitly say how the default object should look, otherwise it would not show up in the schema. So now I define it like this:
The issue is that when I make a query towards a GraphQLQuery with a GraphQLArgument that has defaultValue set like this, and if I write my query like this (which used to work, and should work), then I get the following error:
Expected a value that can be converted to type 'Int' but it was a 'LinkedHashMap'
Input:
But setting the whole input object like this works:
Input:
Setting the values directly in the query also works.
It only fails in runtime so it is really hard to guard against as all these 3 queries are valid graphQL queries and I can't stop my users from writing their queries like that. The only work-around I found was to remove the default value completely from the GraphQLArgument and instead set the default if the argument is null. This comes with two downsides, defaultvalues will not show up in the schema definition and I have to add more code in my resolvers, so I would really appreciate another way of solving this.
Happy to provide more information if necessary and grateful for any help!