refactor(grails-data-graphql): clean up the fetcher package - #16203
Open
borinquenkid wants to merge 6 commits into
Open
refactor(grails-data-graphql): clean up the fetcher package#16203borinquenkid wants to merge 6 commits into
borinquenkid wants to merge 6 commits into
Conversation
'success' and 'exception' were declared blank and conditionally assigned across the try/catch, the same shape flagged elsewhere in this cleanup: 'success = false' is only meaningfully read on the exception path, and 'exception' is read unassigned (implicitly null) on the success path. Build and return the response directly in each branch instead - same two call shapes (createResponse(env, true, null) / createResponse(env, false, e)), still verified by the existing "test get"/"test get invalid" specs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tcher buildCriteria(environment).list(queryArgs) returns a raw List, which was returned directly from a method declared to return T. Compiles under CompileStatic only because T is unbounded (erases to Object), but the assignment isn't actually type-checked - the same shape PaginatedEntityDataFetcher and every other DefaultGormDataFetcher subclass already guards against with an explicit (T) cast at the return site. Add the same cast here for consistency and to make the intent explicit. Covered by the existing EntityDataFetcherSpec "test get"/"test pagination" specs, which assert on the returned list. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…kers CustomInterceptorInvoker#invoke and ProvidedInterceptorInvoker#invoke were identical 9-line fragments: resolve the operation name, run the subclass-specific invocation, log when it returns false, return the result. Extract the shared name/log/return steps into a logIfPrevented(name, result) template method on the common InterceptorInvoker base (which now carries the @Slf4j/@CompileStatic annotations instead of each sibling declaring its own logger), and have both invoke() overrides call it. Behavior is unchanged; the existing Query/Mutation/CustomQuery/CustomMutation InterceptorInvoker specs already cover both the allowed and prevented-by-interceptor branches. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…Invoker getFields() has been deprecated in graphql-java since 2018-12-20 in favor of getMergedField(). InterceptorInvoker#getName() used environment.fields[0].name (with an empty-list guard) purely to grab the name of the current field; MergedField#getName() returns exactly the same value (the single field's name), so swap to getMergedField() with a null guard instead. Updated QueryInterceptorInvokerSpec/MutationInterceptorInvokerSpec and InterceptingDataFetcherSpec, which stubbed the now-unused getFields(), to stub getMergedField() instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- associations was declared Map<String, Association> (raw) but assigned from EntityFetchOptions#getAssociations(), which returns Map<String, Association<?>> since the earlier raw-type cleanup. Parameterize the field to match. - initializeEntity(PersistentEntity) never used its parameter - the entity it needs is already captured on `this.entity` by the time the constructor calls it. Drop the redundant parameter. - withTransaction declared a blank `datastore` local reassigned across an if/else, the same "assign-in-every-branch" shape flagged elsewhere in this cleanup. Extract the branch into a resolveDatastore() helper that returns directly from each branch. Behavior is unchanged; covered by the existing DefaultGormDataFetcherSpec join-property tests plus every subclass's "test get" spec, which all exercise withTransaction/resolveDatastore. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ildcard IntelliJ flags Map<String, Association<?>> with "Type parameter '?' is not in its bound; should extend Property" since Association<T extends Property>. Writing the bound out explicitly (Association<? extends Property>) looks like the obvious fix, but Groovy's static type checker - unlike javac - treats the bounded and unbounded wildcard forms as distinct types, so initializeEntity()'s `this.associations = this.entityFetchOptions.associations` (which returns Map<String, Association<?>>) fails to compile against it. Keep the unbounded form and record why, the same treatment given to EntityFetchOptions' raw List<Selection> earlier in this cleanup. 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 #16203 +/- ##
======================================================================
Coverage ? 53.4199%
Complexity ? 19462
======================================================================
Files ? 2081
Lines ? 98993
Branches ? 17361
======================================================================
Hits ? 52882
Misses ? 38563
Partials ? 7548 🚀 New features to boost your workflow:
|
✅ All tests passed ✅🏷️ Commit: 2865d10 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.fetcher.*:DeleteEntityDataFetcher/EntityDataFetcher: simplified branch-per-response construction and added a missing(T)cast onexecuteQuery'sDetachedCriteria#listresult, matching the cast convention every sibling fetcher already uses.CustomInterceptorInvoker/ProvidedInterceptorInvoker: deduplicated an identical 9-line "resolve name → invoke → log if prevented → return" fragment into a sharedlogIfPreventedmethod onInterceptorInvoker.InterceptorInvoker: replaced deprecatedDataFetchingEnvironment#getFields()withgetMergedField().DefaultGormDataFetcher: fixed a type mismatch (Map<String, Association>→Map<String, Association<?>>... with a documented reason it can't be fully parameterized under Groovy's static type checker), dropped an unused constructor parameter, and extractedresolveDatastore()to eliminate an ambiguous "might not be assigned" local variable.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