refactor(grails-data-graphql): clean up the entity package - #16202
Open
borinquenkid wants to merge 8 commits into
Open
refactor(grails-data-graphql): clean up the entity package#16202borinquenkid wants to merge 8 commits into
borinquenkid wants to merge 8 commits into
Conversation
- Mark the four instance fields final; each is assigned exactly once,
in the constructor.
- Fix Javadoc grammar ("need prepended" -> "need to be prepended").
- Use pattern-matching instanceof in isForeignKeyInChild() and the
single-selection branch of handleField(), replacing the redundant
cast.
- Use List#getFirst() instead of get(0) (SequencedCollection, JDK 21).
- Tighten the raw Map<String, Map> return/local types on the
getFetchArgument() overloads to Map<String, Map<String, String>>,
matching the actual join-map shape they build.
EntityFetchOptions is documented public API (grails-data-graphql docs
point users at it directly for custom data fetchers) but had zero
direct test coverage - only indirect coverage through
DefaultGormDataFetcherSpec. Added EntityFetchOptionsSpec covering
construction (including the null-entity guard), getAssociations(),
getFetchArgument()'s map shape, and isForeignKeyInChild() for
ToMany/hasOne/plain-toOne associations.
Left the single-arg EntityFetchOptions(PersistentEntity) constructor
in place despite it having no internal callers - it's the constructor
external consumers are documented to use directly.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ions Association<T> and ToOne<T> are generic (T extends Property); every field, parameter, and local variable using them raw is now Association<?> /ToOne<?>. Same for the Selection<T> pattern variable introduced by the earlier instanceof cleanup. The one exception is the local variable holding SelectionSet#getSelections()'s result: graphql-java itself declares that method to return a raw List<Selection>, so there's no parameterized type to assign it to without an unchecked cast. Left that one raw with a comment and a scoped @SuppressWarnings("rawtypes"), since the raw type there is coming from the library's own API, not a gap in ours. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…r.buildFetchOptions
A prior commit in this cleanup ("Parameterize raw
Association/ToOne/Selection usages") accidentally dropped the
null-guard around environment.getMergedField() in
EntityFetchOptions.getJoinProperties(DataFetchingEnvironment, boolean)
while editing nearby code, turning a graceful "no merged field ->
empty fields list" fallback into a NullPointerException. This broke
6+ existing specs whose DataFetchingEnvironment mocks don't stub
getMergedField() (so it returns null, as real callers can also see).
Restored the guard as a single-assignment if/else.
Also adds test coverage for ClosureDataFetcher.buildFetchOptions(),
which had none: null domain type, non-GORM domain type, a real GORM
entity domain type, and that the built EntityFetchOptions is cached
across calls. Converts ClosureDataFetcherSpec to HibernateSpec to
back the GORM-entity case.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…assignment The final 'order' field was conditionally assigned from up to four different branches spread across two separate if-blocks in the constructor. That shape is genuinely ambiguous for definite-assignment analysis of a blank final field (assigned once on some paths, not assigned at the point it gets re-checked and possibly assigned again on others) even though groovyc accepted it. Extracted the whole decision tree into resolveOrder(), assigned to this.order exactly once. Behavior is unchanged - confirmed by the existing 9-case "test graphQL order for #name" spec, which still covers every branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…resolution The local 'entity' var followed the same shape as the order field fix: declared blank, conditionally assigned in an Association check, then conditionally reassigned again from a null-guard - flagged as unused assignments since the write is only reachable through a read that guards a second write to the same variable. Replaced with a single elvis-operator assignment (association?.associatedEntity ?: fallback) and a single boolean expression for 'embedded'. Same runtime behavior - verified against the existing toMany/toOne/embedded specs, all of which exercise this method. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ue calls graphql-java 25 deprecates Builder#defaultValue(Object) in favor of defaultValueLiteral(Value) or defaultValueProgrammatic(Object). CustomArgument and ComplexTyped supply a plain, uncoerced Java value from the DSL, so defaultValueProgrammatic is the correct replacement. Switching unconditionally surfaced a real bug: the old deprecated call always marked the default value as "set" (even when the DSL user never configured one), which the old INTERNAL_VALUE state silently exempted from schema validation. defaultValueProgrammatic's EXTERNAL_VALUE state is validated, and a null default on a non-null argument/field type failed schema build. Both call sites now only apply a default value when one was actually configured via the DSL, leaving it unset otherwise - matching the DSL's actual intent and passing validation. Also documents (without changing) the unqualified withDelegate() calls in Arguable and ComplexTyped: explicitly qualifying the inherited ExecutesClosures static trait method there does not resolve under @CompileStatic, so the plain call must stay despite IDE inspections suggesting otherwise. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
args was typed as plain Object, so indexing it (args[0]) had no statically resolvable getAt(Object, Integer) overload, forcing @CompileDynamic and triggering IDE warnings on every index access. The Groovy methodMissing hook still requires the (String, Object) signature to be recognized as the protocol method, but the runtime value is always an Object[], so it's now cast to Object[] once and indexed from there - letting the method compile statically like the rest of the class, with identical runtime behavior. Adds coverage for the two methodMissing failure branches (no arguments, unsupported argument type) that had no tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
GraphQLFieldDefinition.Builder#dataFetcher(DataFetcher) has been deprecated since graphql-java 12; data fetchers are wired through GraphQLCodeRegistry instead. CustomOperation.createField() now takes the parent type name (Query or Mutation) and registers the built data fetcher with the type manager's code registry, matching the pattern already used everywhere else in Schema.groovy. Adds assertions to GraphQLMappingSpec verifying the data fetcher for each custom query/mutation operation is actually wired into the code registry, which the previous test never checked. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 task
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## test/grails-data-graphql-coverage #16202 +/- ##
===========================================================================
- Coverage 53.4149% 53.4048% -0.0101%
+ Complexity 19459 19457 -2
===========================================================================
Files 2081 2081
Lines 98993 98993
Branches 17361 17361
===========================================================================
- Hits 52877 52867 -10
- Misses 38566 38579 +13
+ Partials 7550 7547 -3 🚀 New features to boost your workflow:
|
✅ All tests passed ✅🏷️ Commit: 1654c6c Learn more about TestLens at testlens.app/docs. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stacked on #16201. Fixes a series of IntelliJ-flagged issues in
org.grails.gorm.graphql.entity.*:EntityFetchOptions.java: added first direct unit coverage, parameterized rawAssociation/ToOne/Selectionusages, and restored a null-safegetMergedField()guard that had been accidentally dropped mid-refactor (covered byClosureDataFetcher.buildFetchOptionstests).PersistentGraphQLProperty: consolidated two multi-branch reassignment patterns (orderresolution,getGraphQLType's entity/embedded resolution) into single-assignment forms.CustomArgument/Arguable/ComplexTyped: replaced deprecatedGraphQLArgument/GraphQLInputObjectField#defaultValuecalls with the non-deprecateddefaultValueProgrammatic, guarded against nulls to avoid breaking schema validation.GraphQLMapping.methodMissing: removed the@CompileDynamicescape hatch while preserving the exact(String, Object)signature Groovy'smethodMissingprotocol requires.CustomOperation/Schema: migratedGraphQLFieldDefinition.Builder#dataFetcher(deprecated) to registration viaGraphQLCodeRegistry.Builder.Test plan
./gradlew :grails-data-graphql-core:test :grails-data-graphql:test :grails-data-graphql-core:codeStyle :grails-data-graphql:codeStylepasses🤖 Generated with Claude Code