From 5613d0f2cb1e4c65ec6d13a9753dfab35c2a1d09 Mon Sep 17 00:00:00 2001 From: bbaker Date: Wed, 23 Jul 2025 20:07:42 +1000 Subject: [PATCH 1/5] MergedField single field support --- .../graphql/execution/ExecutionStrategy.java | 2 +- .../graphql/execution/FieldCollector.java | 16 +- .../java/graphql/execution/MergedField.java | 216 ++++++++++++++---- .../directives/QueryDirectivesImpl.java | 2 +- .../incremental/DeferredExecutionSupport.java | 2 +- .../graphql/execution/MergedFieldTest.groovy | 136 +++++++++++ 6 files changed, 322 insertions(+), 52 deletions(-) create mode 100644 src/test/groovy/graphql/execution/MergedFieldTest.groovy diff --git a/src/main/java/graphql/execution/ExecutionStrategy.java b/src/main/java/graphql/execution/ExecutionStrategy.java index a16f0f80f1..acb078249e 100644 --- a/src/main/java/graphql/execution/ExecutionStrategy.java +++ b/src/main/java/graphql/execution/ExecutionStrategy.java @@ -160,7 +160,7 @@ public static String mkNameForPath(Field currentField) { @Internal public static String mkNameForPath(MergedField mergedField) { - return mkNameForPath(mergedField.getFields()); + return mergedField.getResultKey(); } @Internal diff --git a/src/main/java/graphql/execution/FieldCollector.java b/src/main/java/graphql/execution/FieldCollector.java index 93d58f97ab..9a63e009b3 100644 --- a/src/main/java/graphql/execution/FieldCollector.java +++ b/src/main/java/graphql/execution/FieldCollector.java @@ -41,12 +41,11 @@ public MergedSelectionSet collectFields(FieldCollectorParameters parameters, Mer public MergedSelectionSet collectFields(FieldCollectorParameters parameters, MergedField mergedField, boolean incrementalSupport) { Map subFields = new LinkedHashMap<>(); Set visitedFragments = new LinkedHashSet<>(); - for (Field field : mergedField.getFields()) { - if (field.getSelectionSet() == null) { - continue; + mergedField.forEach(field -> { + if (field.getSelectionSet() != null) { + this.collectFields(parameters, field.getSelectionSet(), visitedFragments, subFields, null, incrementalSupport); } - this.collectFields(parameters, field.getSelectionSet(), visitedFragments, subFields, null, incrementalSupport); - } + }); return newMergedSelectionSet().subFields(subFields).build(); } @@ -142,11 +141,8 @@ private void collectField(FieldCollectorParameters parameters, Map builder - .addField(field) - .addDeferredExecution(deferredExecution)) - ); + MergedField currentMergedField = fields.get(name); + fields.put(name, currentMergedField.newMergedFieldWith(field,deferredExecution)); } else { fields.put(name, MergedField.newSingletonMergedField(field, deferredExecution)); } diff --git a/src/main/java/graphql/execution/MergedField.java b/src/main/java/graphql/execution/MergedField.java index 64eb43361c..5225cb9164 100644 --- a/src/main/java/graphql/execution/MergedField.java +++ b/src/main/java/graphql/execution/MergedField.java @@ -3,9 +3,11 @@ import com.google.common.collect.ImmutableList; import graphql.ExperimentalApi; import graphql.PublicApi; +import graphql.collect.ImmutableKit; import graphql.execution.incremental.DeferredExecution; import graphql.language.Argument; import graphql.language.Field; +import org.jspecify.annotations.NullMarked; import org.jspecify.annotations.Nullable; import java.util.List; @@ -13,11 +15,12 @@ import java.util.function.Consumer; import static graphql.Assert.assertNotEmpty; +import static graphql.Assert.assertNotNull; /** * This represents all Fields in a query which overlap and are merged into one. * This means they all represent the same field actually when the query is executed. - * + *

* Example query with more than one Field merged together: * * 

@@ -45,7 +48,7 @@
  *
* * Here the field is merged together including the sub selections. - * + *

* A third example with different directives: * 

  * {@code
@@ -56,13 +59,14 @@
  * }
  *
* These examples make clear that you need to consider all merged fields together to have the full picture. - * + *

* The actual logic when fields can be successfully merged together is implemented in {#graphql.validation.rules.OverlappingFieldsCanBeMerged} */ @PublicApi +@NullMarked public class MergedField { - private final ImmutableList fields; + private @Nullable ImmutableList fields; private final Field singleField; private final ImmutableList deferredExecutions; @@ -73,13 +77,18 @@ private MergedField(ImmutableList fields, ImmutableList deferredExecutions) { + // we make "this.fields" lazy because mostly we have single fields, and we avoid a + // list allocation if we can. This is a micro memory optimisation but for large + // operations this can add up. + this.fields = null; + this.singleField = field; + this.deferredExecutions = deferredExecutions; } /** * All merged fields have the same name. - * + *

* WARNING: This is not always the key in the execution result, because of possible aliases. See {@link #getResultKey()} * * @return the name of the merged fields. @@ -100,7 +109,7 @@ public String getResultKey() { /** * The first of the merged fields. - * + *

* Because all fields are almost identically * often only one of the merged fields are used. * @@ -126,9 +135,53 @@ public List getArguments() { * @return all merged fields */ public List getFields() { + List fields = this.fields; + if (fields == null) { + synchronized (this) { + if (this.fields == null) { + this.fields = ImmutableList.of(singleField); + } + fields = this.fields; + } + } return fields; } + /** + * @return how many fields are in this merged field + */ + public int getFieldsCount() { + if (this.fields == null) { + return 1; + } + return this.fields.size(); + } + + /** + * @return true if the field has a sub selection + */ + public boolean hasSubSelection() { + if (this.fields == null) { + return singleField.getSelectionSet() != null; + } + for (Field field : this.fields) { + if (field.getSelectionSet() != null) { + return true; + } + } + return false; + } + + /** + * @return true if this {@link MergedField} represents a single {@link Field} in the operation + */ + public boolean isSingleField() { + if (this.fields == null) { + return true; + } + return this.fields.size() == 1; + } + /** * Get a list of all {@link DeferredExecution}s that this field is part of * @@ -149,6 +202,35 @@ public boolean isDeferred() { return !deferredExecutions.isEmpty(); } + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + MergedField that = (MergedField) o; + if (fields != null) { + return fields.equals(that.fields); + } else { + return this.singleField.equals(that.singleField); + } + } + + @Override + public int hashCode() { + return Objects.hashCode(Objects.requireNonNullElse(fields, singleField)); + } + + @Override + public String toString() { + return "MergedField{" + + "field(s)=" + (fields != null ? fields : singleField) + + '}'; + } + + public static Builder newMergedField() { return new Builder(); } @@ -161,8 +243,40 @@ public static Builder newMergedField(List fields) { return new Builder().fields(fields); } - static MergedField newSingletonMergedField(Field field, DeferredExecution deferredExecution) { - return new MergedField(field, deferredExecution); + /** + * This is an important method because it creates a new MergedField from the existing one without using a builder + * to save memory. + * + * @param field the new field to add to the current merged field + * @param deferredExecution the deferred execution + * + * @return a new {@link MergedField} instance + */ + MergedField newMergedFieldWith(Field field, @Nullable DeferredExecution deferredExecution) { + ImmutableList deferredExecutions = this.deferredExecutions; + if (deferredExecution != null) { + deferredExecutions = ImmutableKit.addToList(deferredExecutions, deferredExecution); + } + ImmutableList fields = this.fields; + if (fields == null) { + fields = ImmutableList.of(singleField, field); + } else { + fields = ImmutableKit.addToList(fields, field); + } + return new MergedField(fields, deferredExecutions); + } + + /** + * This is an important method in that it creates a MergedField direct without the list and without a builder and hence + * saves some micro memory in not allocating a list of 1 + * + * @param field the field to wrap + * @param deferredExecution the deferred execution + * + * @return a new {@link MergedField} + */ + static MergedField newSingletonMergedField(Field field, @Nullable DeferredExecution deferredExecution) { + return new MergedField(field, deferredExecution == null ? ImmutableList.of() : ImmutableList.of(deferredExecution)); } public MergedField transform(Consumer builderConsumer) { @@ -171,25 +285,68 @@ public MergedField transform(Consumer builderConsumer) { return builder.build(); } + /** + * Runs a consumer for each field + * + * @param fieldConsumer the consumer to run + */ + public void forEach(Consumer fieldConsumer) { + if (fields == null) { + fieldConsumer.accept(singleField); + } else { + fields.forEach(fieldConsumer); + } + } + public static class Builder { - private final ImmutableList.Builder fields = new ImmutableList.Builder<>(); + /* + The builder logic is complicated by these dual singleton / list duality code, + but it prevents memory allocation curing field collection and every bit counts + when the CPU is running hot and an operation has lots of fields! + */ + private ImmutableList.@Nullable Builder fields; + private @Nullable Field singleField; private final ImmutableList.Builder deferredExecutions = new ImmutableList.Builder<>(); private Builder() { } private Builder(MergedField existing) { - fields.addAll(existing.getFields()); + if (existing.fields != null) { + this.singleField = null; + this.fields = new ImmutableList.Builder<>(); + this.fields.addAll(existing.getFields()); + } else { + this.singleField = existing.singleField; + } deferredExecutions.addAll(existing.deferredExecutions); } + private ImmutableList.Builder ensureFieldsListBuilder() { + if (this.fields == null) { + this.fields = new ImmutableList.Builder<>(); + if (this.singleField != null) { + this.fields.add(this.singleField); + this.singleField = null; + } + } + return this.fields; + } + public Builder fields(List fields) { + this.fields = ensureFieldsListBuilder(); this.fields.addAll(fields); return this; } public Builder addField(Field field) { + if (singleField == null && this.fields == null) { + singleField = field; + return this; + } else { + this.fields = ensureFieldsListBuilder(); + } this.fields.add(field); return this; } @@ -199,40 +356,21 @@ public Builder addDeferredExecutions(List deferredExecutions) return this; } + @SuppressWarnings("UnusedReturnValue") public Builder addDeferredExecution(@Nullable DeferredExecution deferredExecution) { - if(deferredExecution != null) { + if (deferredExecution != null) { this.deferredExecutions.add(deferredExecution); } return this; } public MergedField build() { - return new MergedField(fields.build(), deferredExecutions.build()); - } - - } - - @Override - public boolean equals(Object o) { - if (this == o) { - return true; - } - if (o == null || getClass() != o.getClass()) { - return false; + ImmutableList deferredExecutions = this.deferredExecutions.build(); + if (this.singleField != null && this.fields == null) { + return new MergedField(singleField, deferredExecutions); + } + ImmutableList fields = assertNotNull(this.fields, () -> "You MUST add at least one field via the builder").build(); + return new MergedField(fields, deferredExecutions); } - MergedField that = (MergedField) o; - return fields.equals(that.fields); - } - - @Override - public int hashCode() { - return Objects.hashCode(fields); - } - - @Override - public String toString() { - return "MergedField{" + - "fields=" + fields + - '}'; } } diff --git a/src/main/java/graphql/execution/directives/QueryDirectivesImpl.java b/src/main/java/graphql/execution/directives/QueryDirectivesImpl.java index 4b75df378e..87f00d6f97 100644 --- a/src/main/java/graphql/execution/directives/QueryDirectivesImpl.java +++ b/src/main/java/graphql/execution/directives/QueryDirectivesImpl.java @@ -69,7 +69,7 @@ private void computeValuesLazily() { BiMap directiveCounterParts = HashBiMap.create(); BiMap gqlDirectiveCounterParts = HashBiMap.create(); BiMap gqlDirectiveCounterPartsInverse = gqlDirectiveCounterParts.inverse(); - mergedField.getFields().forEach(field -> { + mergedField.forEach(field -> { List directives = field.getDirectives(); BiMap directivesMap = directivesResolver .resolveDirectives(directives, schema, coercedVariables, graphQLContext, locale); diff --git a/src/main/java/graphql/execution/incremental/DeferredExecutionSupport.java b/src/main/java/graphql/execution/incremental/DeferredExecutionSupport.java index 3055e14c48..9a46020d83 100644 --- a/src/main/java/graphql/execution/incremental/DeferredExecutionSupport.java +++ b/src/main/java/graphql/execution/incremental/DeferredExecutionSupport.java @@ -74,7 +74,7 @@ public DeferredExecutionSupportImpl( ImmutableList.Builder nonDeferredFieldNamesBuilder = ImmutableList.builder(); mergedSelectionSet.getSubFields().values().forEach(mergedField -> { - if (mergedField.getFields().size() > mergedField.getDeferredExecutions().size()) { + if (mergedField.getFieldsCount() > mergedField.getDeferredExecutions().size()) { nonDeferredFieldNamesBuilder.add(mergedField.getSingleField().getResultKey()); return; } diff --git a/src/test/groovy/graphql/execution/MergedFieldTest.groovy b/src/test/groovy/graphql/execution/MergedFieldTest.groovy new file mode 100644 index 0000000000..1e05d93d4d --- /dev/null +++ b/src/test/groovy/graphql/execution/MergedFieldTest.groovy @@ -0,0 +1,136 @@ +package graphql.execution + +import graphql.language.Field +import graphql.language.SelectionSet +import spock.lang.Specification + +class MergedFieldTest extends Specification { + + def fa_1 = Field.newField("fa").build() + def fa_2 = Field.newField("fa").build() + def fa_3 = Field.newField("fa").build() + def alias1 = Field.newField("a1").alias("alias1").build() + def ss = SelectionSet.newSelectionSet([fa_1, fa_2]).build() + def sub1 = Field.newField("s1").selectionSet(ss).build() + + def "can construct from a single field"() { + when: + def mergedField = MergedField.newSingletonMergedField(fa_1, null) + then: + mergedField.getName() == "fa" + mergedField.getResultKey() == "fa" + mergedField.getSingleField() == fa_1 + mergedField.isSingleField() + !mergedField.hasSubSelection() + // lazily make the list + mergedField.getFields() == [fa_1] + + // these should still work + mergedField.getName() == "fa" + mergedField.getResultKey() == "fa" + mergedField.getSingleField() == fa_1 + mergedField.isSingleField() + !mergedField.hasSubSelection() + } + + def "can construct from multiple fields"() { + when: + def mergedField = MergedField.newMergedField().addField(fa_1).addField(fa_2).build() + + then: + mergedField.getName() == "fa" + mergedField.getResultKey() == "fa" + mergedField.getSingleField() == fa_1 + !mergedField.isSingleField() + !mergedField.hasSubSelection() + mergedField.getFields() == [fa_1, fa_2] + } + + def "can have aliases"() { + when: + def mergedField = MergedField.newSingletonMergedField(alias1, null) + then: + mergedField.getName() == "a1" + mergedField.getResultKey() == "alias1" + mergedField.getSingleField() == alias1 + mergedField.isSingleField() + !mergedField.hasSubSelection() + } + + def "can have selection set on single field"() { + when: + def mergedField = MergedField.newSingletonMergedField(sub1, null) + then: + mergedField.getName() == "s1" + mergedField.getResultKey() == "s1" + mergedField.getSingleField() == sub1 + mergedField.isSingleField() + mergedField.hasSubSelection() + } + + def "builder can build a singleton via addField()"() { + when: + def mergedField = MergedField.newMergedField().addField(sub1).build() + then: + mergedField.getName() == "s1" + mergedField.getResultKey() == "s1" + mergedField.getSingleField() == sub1 + mergedField.isSingleField() + mergedField.hasSubSelection() + } + + def "builder can build a multi via addField() / addField() combo"() { + when: + def mergedField = MergedField.newMergedField().addField(fa_1).addField(fa_2).build() + then: + mergedField.getName() == "fa" + mergedField.getResultKey() == "fa" + mergedField.getSingleField() == fa_1 + !mergedField.isSingleField() + mergedField.getFields() == [fa_1, fa_2] + } + + def "builder can build a multi via addField() / fields() combo"() { + when: + def mergedField = MergedField.newMergedField().addField(fa_1).fields([fa_2, fa_3]).build() + then: + mergedField.getName() == "fa" + mergedField.getResultKey() == "fa" + mergedField.getSingleField() == fa_1 + !mergedField.isSingleField() + mergedField.getFields() == [fa_1, fa_2, fa_3] + } + + def "builder can build a multi via fields()"() { + when: + def mergedField = MergedField.newMergedField().fields([fa_1, fa_2, fa_3]).build() + then: + mergedField.getName() == "fa" + mergedField.getResultKey() == "fa" + mergedField.getSingleField() == fa_1 + !mergedField.isSingleField() + mergedField.getFields() == [fa_1, fa_2, fa_3] + } + + def "can add to an existing field"() { + when: + def mergedField = MergedField.newSingletonMergedField(fa_1, null) + then: + mergedField.getName() == "fa" + mergedField.getDeferredExecutions().isEmpty() + mergedField.getSingleField() == fa_1 + mergedField.getFields() == [fa_1] + + when: + def mergedField2 = mergedField.newMergedFieldWith(fa_2, null) + then: + mergedField2.getName() == "fa" + mergedField2.getDeferredExecutions().isEmpty() + mergedField2.getSingleField() == fa_1 + mergedField2.getFields() == [fa_1, fa_2] + + // its a new instance + !(mergedField2 === mergedField) + } + +} From dbd071145ed63a02ed3c6f3f092262b7f746fa15 Mon Sep 17 00:00:00 2001 From: bbaker Date: Wed, 23 Jul 2025 23:14:25 +1000 Subject: [PATCH 2/5] MergedField single field support - added a class instead of managing field state --- .../java/graphql/execution/MergedField.java | 155 +++++++++++------- 1 file changed, 94 insertions(+), 61 deletions(-) diff --git a/src/main/java/graphql/execution/MergedField.java b/src/main/java/graphql/execution/MergedField.java index 5225cb9164..ae6a031076 100644 --- a/src/main/java/graphql/execution/MergedField.java +++ b/src/main/java/graphql/execution/MergedField.java @@ -66,22 +66,10 @@ @NullMarked public class MergedField { - private @Nullable ImmutableList fields; private final Field singleField; private final ImmutableList deferredExecutions; - private MergedField(ImmutableList fields, ImmutableList deferredExecutions) { - assertNotEmpty(fields); - this.fields = fields; - this.singleField = fields.get(0); - this.deferredExecutions = deferredExecutions; - } - private MergedField(Field field, ImmutableList deferredExecutions) { - // we make "this.fields" lazy because mostly we have single fields, and we avoid a - // list allocation if we can. This is a micro memory optimisation but for large - // operations this can add up. - this.fields = null; this.singleField = field; this.deferredExecutions = deferredExecutions; } @@ -135,51 +123,28 @@ public List getArguments() { * @return all merged fields */ public List getFields() { - List fields = this.fields; - if (fields == null) { - synchronized (this) { - if (this.fields == null) { - this.fields = ImmutableList.of(singleField); - } - fields = this.fields; - } - } - return fields; + return ImmutableList.of(singleField); } /** * @return how many fields are in this merged field */ public int getFieldsCount() { - if (this.fields == null) { - return 1; - } - return this.fields.size(); + return 1; } /** * @return true if the field has a sub selection */ public boolean hasSubSelection() { - if (this.fields == null) { - return singleField.getSelectionSet() != null; - } - for (Field field : this.fields) { - if (field.getSelectionSet() != null) { - return true; - } - } - return false; + return singleField.getSelectionSet() != null; } /** * @return true if this {@link MergedField} represents a single {@link Field} in the operation */ public boolean isSingleField() { - if (this.fields == null) { - return true; - } - return this.fields.size() == 1; + return true; } /** @@ -211,25 +176,96 @@ public boolean equals(Object o) { return false; } MergedField that = (MergedField) o; - if (fields != null) { - return fields.equals(that.fields); - } else { - return this.singleField.equals(that.singleField); - } + return this.singleField.equals(that.singleField); } @Override public int hashCode() { - return Objects.hashCode(Objects.requireNonNullElse(fields, singleField)); + return Objects.hashCode(singleField); } @Override public String toString() { return "MergedField{" + - "field(s)=" + (fields != null ? fields : singleField) + + "field(s)=" + singleField + '}'; } + /** + * Most of the time we have a single field inside a MergedField but when we need more than one field + * represented then this {@link MultiMergedField} is used + */ + static final class MultiMergedField extends MergedField { + private final ImmutableList fields; + + MultiMergedField(ImmutableList fields, ImmutableList deferredExecutions) { + super(fields.get(0), deferredExecutions); + this.fields = fields; + } + + @Override + public List getFields() { + return fields; + } + + @Override + public boolean hasSubSelection() { + for (Field field : this.fields) { + if (field.getSelectionSet() != null) { + return true; + } + } + return false; + } + + @Override + public int getFieldsCount() { + return fields.size(); + } + + @Override + public boolean isSingleField() { + return fields.size() == 1; + } + + @Override + public boolean equals(@Nullable Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + MultiMergedField that = (MultiMergedField) o; + return fields.equals(that.fields); + } + + @Override + public int hashCode() { + return Objects.hashCode(fields); + } + + @Override + public String toString() { + return "MultiMergedField{" + + "field(s)=" + fields + + '}'; + } + + + @Override + public void forEach(Consumer fieldConsumer) { + fields.forEach(fieldConsumer); + } + + @Override + MergedField newMergedFieldWith(Field field, @Nullable DeferredExecution deferredExecution) { + ImmutableList deferredExecutions = mkDeferredExecutions(deferredExecution); + ImmutableList fields = ImmutableKit.addToList(this.fields, field); + return new MultiMergedField(fields, deferredExecutions); + } + } + public static Builder newMergedField() { return new Builder(); @@ -253,17 +289,17 @@ public static Builder newMergedField(List fields) { * @return a new {@link MergedField} instance */ MergedField newMergedFieldWith(Field field, @Nullable DeferredExecution deferredExecution) { + ImmutableList deferredExecutions = mkDeferredExecutions(deferredExecution); + ImmutableList fields = ImmutableList.of(singleField, field); + return new MultiMergedField(fields, deferredExecutions); + } + + ImmutableList mkDeferredExecutions(@Nullable DeferredExecution deferredExecution) { ImmutableList deferredExecutions = this.deferredExecutions; if (deferredExecution != null) { deferredExecutions = ImmutableKit.addToList(deferredExecutions, deferredExecution); } - ImmutableList fields = this.fields; - if (fields == null) { - fields = ImmutableList.of(singleField, field); - } else { - fields = ImmutableKit.addToList(fields, field); - } - return new MergedField(fields, deferredExecutions); + return deferredExecutions; } /** @@ -291,18 +327,14 @@ public MergedField transform(Consumer builderConsumer) { * @param fieldConsumer the consumer to run */ public void forEach(Consumer fieldConsumer) { - if (fields == null) { - fieldConsumer.accept(singleField); - } else { - fields.forEach(fieldConsumer); - } + fieldConsumer.accept(singleField); } public static class Builder { /* The builder logic is complicated by these dual singleton / list duality code, - but it prevents memory allocation curing field collection and every bit counts + but it prevents memory allocation and every bit counts when the CPU is running hot and an operation has lots of fields! */ private ImmutableList.@Nullable Builder fields; @@ -313,7 +345,7 @@ private Builder() { } private Builder(MergedField existing) { - if (existing.fields != null) { + if (existing instanceof MultiMergedField) { this.singleField = null; this.fields = new ImmutableList.Builder<>(); this.fields.addAll(existing.getFields()); @@ -370,7 +402,8 @@ public MergedField build() { return new MergedField(singleField, deferredExecutions); } ImmutableList fields = assertNotNull(this.fields, () -> "You MUST add at least one field via the builder").build(); - return new MergedField(fields, deferredExecutions); + assertNotEmpty(fields); + return new MultiMergedField(fields, deferredExecutions); } } } From 05ef8266b83c52b3550293788a2b56c4bfed8cd1 Mon Sep 17 00:00:00 2001 From: bbaker Date: Wed, 23 Jul 2025 23:20:58 +1000 Subject: [PATCH 3/5] MergedField single field support -code re-org --- .../java/graphql/execution/MergedField.java | 47 ++++++++++--------- 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/src/main/java/graphql/execution/MergedField.java b/src/main/java/graphql/execution/MergedField.java index ae6a031076..9a38bdea63 100644 --- a/src/main/java/graphql/execution/MergedField.java +++ b/src/main/java/graphql/execution/MergedField.java @@ -168,7 +168,7 @@ public boolean isDeferred() { } @Override - public boolean equals(Object o) { + public boolean equals(@Nullable Object o) { if (this == o) { return true; } @@ -191,6 +191,29 @@ public String toString() { '}'; } + /** + * This is an important method because it creates a new MergedField from the existing one without using a builder + * to save memory. + * + * @param field the new field to add to the current merged field + * @param deferredExecution the deferred execution + * + * @return a new {@link MergedField} instance + */ + MergedField newMergedFieldWith(Field field, @Nullable DeferredExecution deferredExecution) { + ImmutableList deferredExecutions = mkDeferredExecutions(deferredExecution); + ImmutableList fields = ImmutableList.of(singleField, field); + return new MultiMergedField(fields, deferredExecutions); + } + + ImmutableList mkDeferredExecutions(@Nullable DeferredExecution deferredExecution) { + ImmutableList deferredExecutions = this.deferredExecutions; + if (deferredExecution != null) { + deferredExecutions = ImmutableKit.addToList(deferredExecutions, deferredExecution); + } + return deferredExecutions; + } + /** * Most of the time we have a single field inside a MergedField but when we need more than one field * represented then this {@link MultiMergedField} is used @@ -279,28 +302,6 @@ public static Builder newMergedField(List fields) { return new Builder().fields(fields); } - /** - * This is an important method because it creates a new MergedField from the existing one without using a builder - * to save memory. - * - * @param field the new field to add to the current merged field - * @param deferredExecution the deferred execution - * - * @return a new {@link MergedField} instance - */ - MergedField newMergedFieldWith(Field field, @Nullable DeferredExecution deferredExecution) { - ImmutableList deferredExecutions = mkDeferredExecutions(deferredExecution); - ImmutableList fields = ImmutableList.of(singleField, field); - return new MultiMergedField(fields, deferredExecutions); - } - - ImmutableList mkDeferredExecutions(@Nullable DeferredExecution deferredExecution) { - ImmutableList deferredExecutions = this.deferredExecutions; - if (deferredExecution != null) { - deferredExecutions = ImmutableKit.addToList(deferredExecutions, deferredExecution); - } - return deferredExecutions; - } /** * This is an important method in that it creates a MergedField direct without the list and without a builder and hence From 8bd0257e09243511a577ed12df35732dfc4d63ea Mon Sep 17 00:00:00 2001 From: bbaker Date: Thu, 24 Jul 2025 17:55:04 +1000 Subject: [PATCH 4/5] MergedField single field support - made the builder even more lazy --- .../java/graphql/execution/MergedField.java | 37 +++++++++++++--- .../incremental/DeferredExecution.java | 16 +++++++ .../graphql/execution/MergedFieldTest.groovy | 43 +++++++++++++++++++ 3 files changed, 90 insertions(+), 6 deletions(-) diff --git a/src/main/java/graphql/execution/MergedField.java b/src/main/java/graphql/execution/MergedField.java index 9a38bdea63..4dce66aa4f 100644 --- a/src/main/java/graphql/execution/MergedField.java +++ b/src/main/java/graphql/execution/MergedField.java @@ -340,7 +340,7 @@ public static class Builder { */ private ImmutableList.@Nullable Builder fields; private @Nullable Field singleField; - private final ImmutableList.Builder deferredExecutions = new ImmutableList.Builder<>(); + private ImmutableList.@Nullable Builder deferredExecutions; private Builder() { } @@ -353,7 +353,17 @@ private Builder(MergedField existing) { } else { this.singleField = existing.singleField; } - deferredExecutions.addAll(existing.deferredExecutions); + if (!existing.deferredExecutions.isEmpty()) { + this.deferredExecutions = ensureDeferredExecutionsListBuilder(); + this.deferredExecutions.addAll(existing.deferredExecutions); + } + } + + private ImmutableList.Builder ensureDeferredExecutionsListBuilder() { + if (this.deferredExecutions == null) { + this.deferredExecutions = new ImmutableList.Builder<>(); + } + return this.deferredExecutions; } private ImmutableList.Builder ensureFieldsListBuilder() { @@ -368,8 +378,14 @@ private ImmutableList.Builder ensureFieldsListBuilder() { } public Builder fields(List fields) { - this.fields = ensureFieldsListBuilder(); - this.fields.addAll(fields); + if (singleField == null && this.fields == null && fields.size() == 1) { + // even if you present a list - if its a list of one, we dont allocate a list + singleField = fields.get(0); + return this; + } else { + this.fields = ensureFieldsListBuilder(); + this.fields.addAll(fields); + } return this; } @@ -385,20 +401,29 @@ public Builder addField(Field field) { } public Builder addDeferredExecutions(List deferredExecutions) { - this.deferredExecutions.addAll(deferredExecutions); + if (!deferredExecutions.isEmpty()) { + this.deferredExecutions = ensureDeferredExecutionsListBuilder(); + this.deferredExecutions.addAll(deferredExecutions); + } return this; } @SuppressWarnings("UnusedReturnValue") public Builder addDeferredExecution(@Nullable DeferredExecution deferredExecution) { if (deferredExecution != null) { + this.deferredExecutions = ensureDeferredExecutionsListBuilder(); this.deferredExecutions.add(deferredExecution); } return this; } public MergedField build() { - ImmutableList deferredExecutions = this.deferredExecutions.build(); + ImmutableList deferredExecutions; + if (this.deferredExecutions == null) { + deferredExecutions = ImmutableList.of(); + } else { + deferredExecutions = this.deferredExecutions.build(); + } if (this.singleField != null && this.fields == null) { return new MergedField(singleField, deferredExecutions); } diff --git a/src/main/java/graphql/execution/incremental/DeferredExecution.java b/src/main/java/graphql/execution/incremental/DeferredExecution.java index 321ecc1603..f03f20d2ed 100644 --- a/src/main/java/graphql/execution/incremental/DeferredExecution.java +++ b/src/main/java/graphql/execution/incremental/DeferredExecution.java @@ -4,6 +4,8 @@ import graphql.normalized.incremental.NormalizedDeferredExecution; import org.jspecify.annotations.Nullable; +import java.util.Objects; + /** * Represents details about the defer execution that can be associated with a {@link graphql.execution.MergedField}. *

@@ -22,4 +24,18 @@ public DeferredExecution(String label) { public String getLabel() { return label; } + + @Override + public boolean equals(Object o) { + if (o == null || getClass() != o.getClass()) { + return false; + } + DeferredExecution that = (DeferredExecution) o; + return Objects.equals(label, that.label); + } + + @Override + public int hashCode() { + return Objects.hashCode(label); + } } diff --git a/src/test/groovy/graphql/execution/MergedFieldTest.groovy b/src/test/groovy/graphql/execution/MergedFieldTest.groovy index 1e05d93d4d..f327339891 100644 --- a/src/test/groovy/graphql/execution/MergedFieldTest.groovy +++ b/src/test/groovy/graphql/execution/MergedFieldTest.groovy @@ -1,5 +1,6 @@ package graphql.execution +import graphql.execution.incremental.DeferredExecution import graphql.language.Field import graphql.language.SelectionSet import spock.lang.Specification @@ -12,6 +13,8 @@ class MergedFieldTest extends Specification { def alias1 = Field.newField("a1").alias("alias1").build() def ss = SelectionSet.newSelectionSet([fa_1, fa_2]).build() def sub1 = Field.newField("s1").selectionSet(ss).build() + def deferred1 = new DeferredExecution("defer1") + def deferred2 = new DeferredExecution("defer2") def "can construct from a single field"() { when: @@ -133,4 +136,44 @@ class MergedFieldTest extends Specification { !(mergedField2 === mergedField) } + def "builder can handle no deferred executions"() { + when: + def mergedField = MergedField.newMergedField().addField(fa_1) + .addDeferredExecutions([]).build() + then: + mergedField.getName() == "fa" + mergedField.getSingleField() == fa_1 + mergedField.getDeferredExecutions().isEmpty() + } + + def "builder can handle list of deferred executions"() { + when: + def mergedField = MergedField.newMergedField().addField(fa_1) + .addDeferredExecutions([deferred1, deferred2]).build() + then: + mergedField.getName() == "fa" + mergedField.getSingleField() == fa_1 + mergedField.getDeferredExecutions() == [deferred1, deferred2] + + } + + def "builder can handle a single deferred executions"() { + when: + def mergedField = MergedField.newMergedField().addField(fa_1) + .addDeferredExecution(deferred1).build() + then: + mergedField.getName() == "fa" + mergedField.getSingleField() == fa_1 + mergedField.getDeferredExecutions() == [deferred1] + } + + def "builder can handle a single deferred execution at a time"() { + when: + def mergedField = MergedField.newMergedField().addField(fa_1) + .addDeferredExecution(deferred1).addDeferredExecution(deferred2).build() + then: + mergedField.getName() == "fa" + mergedField.getSingleField() == fa_1 + mergedField.getDeferredExecutions() == [deferred1,deferred2] + } } From d98674b82f3602c9e2fa61c805a14257afe350c5 Mon Sep 17 00:00:00 2001 From: bbaker Date: Fri, 1 Aug 2025 11:00:42 +1000 Subject: [PATCH 5/5] removed .equals on DeferredExecution --- .../incremental/DeferredExecution.java | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/main/java/graphql/execution/incremental/DeferredExecution.java b/src/main/java/graphql/execution/incremental/DeferredExecution.java index f03f20d2ed..ae63808989 100644 --- a/src/main/java/graphql/execution/incremental/DeferredExecution.java +++ b/src/main/java/graphql/execution/incremental/DeferredExecution.java @@ -4,8 +4,6 @@ import graphql.normalized.incremental.NormalizedDeferredExecution; import org.jspecify.annotations.Nullable; -import java.util.Objects; - /** * Represents details about the defer execution that can be associated with a {@link graphql.execution.MergedField}. *

@@ -25,17 +23,17 @@ public String getLabel() { return label; } + // this class uses object identity - do not put .equals() / .hashCode() implementations on it + // otherwise it will break defer handling. I have put the code just to be explicit that object identity + // is needed + @Override - public boolean equals(Object o) { - if (o == null || getClass() != o.getClass()) { - return false; - } - DeferredExecution that = (DeferredExecution) o; - return Objects.equals(label, that.label); + public int hashCode() { + return super.hashCode(); } @Override - public int hashCode() { - return Objects.hashCode(label); + public boolean equals(Object obj) { + return super.equals(obj); } }