subtle breaking change for v22

[dfa1]

Describe the bug
In certain cases, the java.util.List objects returned by graphql-java are immutable. This is breaking our code that is assuming that every list to be mutable.

For non-empty lists with size > 1, the type is ArrayList.
For non-empty lists with size == 1, the type is Collections.singletonList
For empty lists, the type is Collections.emptyList.

The problematic case for us is the singleton-list.

The root cause could be the Async class (see below):

I simple fix could be to allocate an ArrayList with size 1 while keeping the immutable instance for the empty case.

To Reproduce

Check the return type of any list used in the graphql query.

Workaround

There is a rather ugly workaround via instrumentation:

    @Override
    protected FieldValueInfo completeValueForList(ExecutionContext executionContext, ExecutionStrategyParameters parameters, Object result) {
        FieldValueInfo fieldValueInfo = super.completeValueForList(executionContext, parameters, result);
        return makeListMutableWorkaround(fieldValueInfo);
    }

    @NotNull
    private static FieldValueInfo makeListMutableWorkaround(FieldValueInfo fieldValueInfo) {
        Object fieldValueObject = fieldValueInfo.getFieldValueObject();
        // in Java, it is not possible to know if the list is immutable or not without checking the actual class
        if (fieldValueObject instanceof List<?> list && list.size() == 1 && fieldValueObject.getClass().getSimpleName().equals("SingletonList")) {
            final List<Object> mutableList = new ArrayList<>(list);
            return new FieldValueInfo(fieldValueInfo.getCompleteValueType(), mutableList, fieldValueInfo.getFieldValueInfos());
        }
        return fieldValueInfo;
    }

[bbakerman]

In certain cases, the java.util.List objects returned by graphql-java are immutable.

Welcome to Java - where List has been problematic for 25 years. Is it mutable / immutable?

(And by the way... that noise... thats Kotlin people sniggering in the background)

I am loath the classify this as a bug for three reasons. One the return type is List and not ArrayList so its true - it is a list

Secondly the class is marked as an internal class - its not part of the API of graphql-java

@Internal
public class Async {

You can work around it of course via a

ArrayList mutable(List l) {
   if (l instanceof ArrayList) {
      return (ArrayList) l;
   else {
     return new ArrayList(l);
}

I would ask you a question in reply - what are you doing with the field return values such that they are mutable - you want to add more values into them beyond the data fetcher want some other step??

What are you trying to acheive

[dfa1]

In certain cases, the java.util.List objects returned by graphql-java are immutable.

Welcome to Java - where List has been problematic for 25 years. Is it mutable / immutable?

(And by the way... that noise... thats Kotlin people sniggering in the background)

ahahah yes :) I know, I know... that why there is the comment in the workaround.

I am loath the classify this as a bug for three reasons. One the return type is List and not ArrayList so its true - it is a list

Secondly the class is marked as an internal class - its not part of the API of graphql-java

@Internal
public class Async {

yes, I agree with you and it is not even promised in any javadoc the mutability/immutability of List. But now users of graphql-java can observe a change in the final result.

You can work around it of course via a

ArrayList mutable(List l) {
   if (l instanceof ArrayList) {
      return (ArrayList) l;
   else {
     return new ArrayList(l);
}

Yep, maybe this is a better workaround than mine! thanks!

I would ask you a question in reply - what are you doing with the field return values such that they are mutable - you want to add more values into them beyond the data fetcher want some other step??

What are you trying to acheive

In my group at SIX, graphql replaced an internal service with similar purpose (the client decides which field to fetch). During the migration (done between 2019 and 2021), the downstream services noticed some breaking changes around list with only null inside. So, in this case the instrumentation transforms "field": [ null ] into "field": null.

Feel free to close the bug if you feel that is not worth documenting the change. IMHO since ExecutionResult is always mutable (LinkedHashMap+ArrayList everywhere), people will take advantage of if (like us).

[bbakerman]

people will take advantage of if (like us).

Can I ask how you take advantage of it? Whats the use cases? I am trying to build unerstanding of why you need a mutable structure and what "adjustments" you are making to data. This helps us think about how we might to the same but without guaranteeing that things are really an immutable list

[dfa1]

@bbakerman see the comment above (it was hidden by the wrong formatting)

In my group at SIX, graphql replaced an internal service with similar purpose (the client decides which field to fetch). During the migration (done between 2019 and 2021), the downstream services noticed some breaking changes around list with only null inside. So, in this case the instrumentation transforms "field": [ null ] into "field": null.

[dfa1]

@bbakerman I guess we can close this, the implemented workaround is working as expected! thanks! :-)