Skip to content

Prohibit Map.of and provide alternative methods#4086

Merged
dondonz merged 3 commits intomasterfrom
ban-map-of
Aug 16, 2025
Merged

Prohibit Map.of and provide alternative methods#4086
dondonz merged 3 commits intomasterfrom
ban-map-of

Conversation

@dondonz
Copy link
Member

@dondonz dondonz commented Aug 16, 2025 •
edited
Loading

Fixes #3989

Turns out Map.of doesn't guarantee consistent order when serialized, which is easy to forget. This PR adds an ArchUnit rule to ban it.

Additionally I created LinkedHashMap equivalents of Map.of to make things easier. It is verbose but intended to be a copy of the original Map methods.

Sample error message for ArchUnit test

Map.of() usage detected. Please use LinkedHashMap instead for consistent serialization order:
- Method <graphql.normalized.ExecutableNormalizedOperationToAstCompiler.compileToDocument(graphql.schema.GraphQLSchema, graphql.language.OperationDefinition$Operation, java.lang.String, java.util.List, graphql.normalized.VariablePredicate)> calls method <java.util.Map.of()> in (ExecutableNormalizedOperationToAstCompiler.java:103)
- Method <graphql.normalized.ExecutableNormalizedOperationToAstCompiler.compileToDocumentWithDeferSupport(graphql.schema.GraphQLSchema, graphql.language.OperationDefinition$Operation, java.lang.String, java.util.List, graphql.normalized.VariablePredicate)> calls method <java.util.Map.of()> in (ExecutableNormalizedOperationToAstCompiler.java:154)

dondonz
dondonz commented Aug 16, 2025
View reviewed changes
src/main/java/graphql/util/LinkedHashMapFactory.java
* instances that preserve insertion order, which is important for consistent serialization.
*/
@Internal
public final class LinkedHashMapFactory {
Copy link
Member Author

@dondonz dondonz Aug 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is verbose. Originally I was only going to have one method (the last one) with variable length parameters, but saw the original Map.java had all of these extra methods for performance.

I don't think the difference is massive for our purposes, but I thought I'd follow the original

@dondonz dondonz added this to the 25.x breaking changes milestone Aug 16, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Aug 16, 2025

Test Results

  326 files   -    649    326 suites   - 649   3m 31s ⏱️ - 7m 22s
5 022 tests  -    345  5 016 ✅  -    346  6 💤 + 1  0 ❌ ±0 
5 111 runs   - 10 219  5 105 ✅  - 10 208  6 💤  - 11  0 ❌ ±0 

Results for commit 11d1357. ± Comparison against base commit 9c92725.

This pull request removes 558 and adds 189 tests. Note that renamed tests count towards both. 
	?

	, expected: combo-\"\\\b\f\n\r\t, #4]
                __schema { types { fields { args { type { name fields { name }}}}}}
                __schema { types { fields { type { name fields { name }}}}}
                __schema { types { inputFields { type { inputFields { name }}}}}
                __schema { types { interfaces { fields { type { interfaces { name } } } } } }
                __schema { types { name} }
                __type(name : "t") { name }
                a1: __schema { types { name} }
                a1: __type(name : "t") { name }
…

graphql.AssertTest ‑ assertFalse with different number of error args but false does not throw assertions [toRun: <graphql.AssertTest$__spock_feature_0_21prov0_closure23@2a49fe delegate=graphql.AssertTest@67a056f1 owner=graphql.AssertTest@67a056f1 thisObject=graphql.AssertTest@67a056f1 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1, #0]
graphql.AssertTest ‑ assertFalse with different number of error args but false does not throw assertions [toRun: <graphql.AssertTest$__spock_feature_0_21prov0_closure24@66596a88 delegate=graphql.AssertTest@67a056f1 owner=graphql.AssertTest@67a056f1 thisObject=graphql.AssertTest@67a056f1 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1 arg2, #1]
graphql.AssertTest ‑ assertFalse with different number of error args but false does not throw assertions [toRun: <graphql.AssertTest$__spock_feature_0_21prov0_closure25@5aae8eb5 delegate=graphql.AssertTest@67a056f1 owner=graphql.AssertTest@67a056f1 thisObject=graphql.AssertTest@67a056f1 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1 arg2 arg3, #2]
graphql.AssertTest ‑ assertFalse with different number of error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_20prov0_closure20@198ef2ce delegate=graphql.AssertTest@67a056f1 owner=graphql.AssertTest@67a056f1 thisObject=graphql.AssertTest@67a056f1 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1, #0]
graphql.AssertTest ‑ assertFalse with different number of error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_20prov0_closure21@4cbd03e7 delegate=graphql.AssertTest@67a056f1 owner=graphql.AssertTest@67a056f1 thisObject=graphql.AssertTest@67a056f1 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1 arg2, #1]
graphql.AssertTest ‑ assertFalse with different number of error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_20prov0_closure22@52fc5eb1 delegate=graphql.AssertTest@67a056f1 owner=graphql.AssertTest@67a056f1 thisObject=graphql.AssertTest@67a056f1 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1 arg2 arg3, #2]
graphql.AssertTest ‑ assertNotNull with different number of  error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_5prov0_closure4@488b50ec delegate=graphql.AssertTest@67a056f1 owner=graphql.AssertTest@67a056f1 thisObject=graphql.AssertTest@67a056f1 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1 arg2, #1]
graphql.AssertTest ‑ assertNotNull with different number of  error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_5prov0_closure5@54da32dc delegate=graphql.AssertTest@67a056f1 owner=graphql.AssertTest@67a056f1 thisObject=graphql.AssertTest@67a056f1 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1 arg2 arg3, #2]
graphql.AssertTest ‑ assertNotNull with different number of error args with non null does not throw assertions [toRun: <graphql.AssertTest$__spock_feature_0_6prov0_closure6@783ec989 delegate=graphql.AssertTest@67a056f1 owner=graphql.AssertTest@67a056f1 thisObject=graphql.AssertTest@67a056f1 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1, #0]
graphql.AssertTest ‑ assertNotNull with different number of error args with non null does not throw assertions [toRun: <graphql.AssertTest$__spock_feature_0_6prov0_closure7@4d33940d delegate=graphql.AssertTest@67a056f1 owner=graphql.AssertTest@67a056f1 thisObject=graphql.AssertTest@67a056f1 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1 arg2, #1]
…
This pull request skips 1 test. 
graphql.schema.fetching.LambdaFetchingSupportTest ‑ different class loaders induce certain behaviours

@dondonz dondonz merged commit 17dffdb into master Aug 16, 2025
2 checks passed
@dondonz dondonz deleted the ban-map-of branch August 16, 2025 07:08
@dondonz dondonz restored the ban-map-of branch August 16, 2025 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bbakerman bbakerman bbakerman approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

25.x breaking changes

Development

Successfully merging this pull request may close these issues.

Ban Map.of

2 participants

@dondonz @bbakerman