Add test coverage and fix real bugs in grails-datamapping-core's services package - #16156
Open
borinquenkid wants to merge 12 commits into
Open
Add test coverage and fix real bugs in grails-datamapping-core's services package#16156borinquenkid wants to merge 12 commits into
borinquenkid wants to merge 12 commits into
Conversation
…vice Both classes had zero test coverage in org.grails.datastore.gorm.services. Mock-based Spock specs cover all public methods including the multi-tenancy-mode/datastore-capability branch checks, bringing each class to 100% line/branch/method coverage. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Covers DSL shapes with zero or partial coverage: UpdateOneImplementer, DeleteWhereImplementer, FindOneInterfaceProjectionWhereImplementer, FindAllPropertyProjectionImplementer, FindOnePropertyProjectionImplementer, the findById(id) shortcut in FindOneByImplementer, and the AbstractWriteOperationImplementer enhance() path for abstract-class services with pre-existing concrete write methods. Fixes a real bug found while writing the invalid-argument test for AbstractSaveImplementer: the error path referenced newMethodNode.declaringClass.module (null on the synthesized implementation method), crashing the compiler with a NullPointerException/ GroovyBugError instead of reporting a clean compile error. Changed to abstractMethodNode.declaringClass.module.context, matching the pattern used elsewhere in this package (e.g. FindAllByImplementer). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
domainClassNode is intentionally unused in the base implementation; the method is a protected extension point so subclasses can pick a different DetachedCriteria type per domain class. No current subclass overrides it, but making the method static or dropping the parameter would remove that polymorphic extension point. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…riteriaType Same shape as the AbstractDetachedCriteriaServiceImplementor fix: the unused domainClassNode parameter is a protected extension point for subclasses, not dead code. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
AbstractProjectionImplementer overrides lookupById() to return false, and doImplement() dispatches on it via virtual call. Making it static would silently break that override rather than just being unnecessary, so it's suppressed with an explanation instead of "fixed". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The parameter became dead after the earlier error-reporting fix (which now uses abstractMethodNode instead of the buggy newMethodNode-based lookup). No subclass overrides this method and it has only two call sites, both updated. Unlike the getDetachedCriteriaType/lookupById warnings, this parameter has no polymorphic purpose, so it's removed rather than suppressed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ForConnectionId None of these three protected helpers have any call site anywhere in the repo (core, rx, hibernate5/7, mongodb) and nothing overrides them, unlike the earlier getDetachedCriteriaType/lookupById extension points. findInstanceApiForConnectionId's live sibling, findStaticApiForConnectionId, is actively used by 5 implementers, but the instance-API equivalent was never wired up the same way -- every save/delete implementer builds that lookup by hand instead. Drops the now-unused TransactionService/ TransactionCapableDatastore imports too. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…Invoke Same shape as the earlier getDetachedCriteriaType/lookupById fixes: domainClassNode/newMethodNode are unused in the base 'find' return, but the method is a protected extension point on a class with real subclasses (FindAndDeleteImplementer, FindOneInterfaceProjectionImplementer), so the parameters are kept and the warning is suppressed rather than the signature changed. Also includes minor pre-existing IDE cleanups picked up in this pass: ClassHelper.VOID_TYPE.equals(x) -> == in DeleteImplementer/ DeleteWhereImplementer, and using the already-statically-imported AstUtils.error instead of the fully-qualified call in FindAllByImplementer. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Same shape as FindOneImplementer.findMethodToInvoke: classNode/methodNode are unused in the base implementation, but the method is a protected extension point and FindOneStringQueryImplementer has a real subclass (FindOneInterfaceProjectionStringQueryImplementer), so the parameters stay and the warning is suppressed instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
isInterfaceProjection()'s per-property compatibility loop was copy-pasted
almost verbatim between InterfaceProjectionBuilder and
IterableInterfaceProjectionBuilder (only the candidate type variable name
differed). Extracted into a shared hasCompatibleProperties(domainClass,
candidateType) method on the base trait; the iterable variant now just
resolves its generic type and delegates. Drops the now-unused
AstPropertyResolveUtils import from IterableInterfaceProjectionBuilder.
Also fixes a cast IntelliJ's stricter Groovy static checker rejects in
buildInterfaceImpl: casting a List<ConstantExpression> (from .collect{})
directly to List<Expression> is generics-invariant-illegal even though
groovyc accepts it. Uses the double-cast-through-raw-List trick already
established elsewhere in this codebase for the same class of warning.
Both isInterfaceProjection() variants and buildInterfaceImpl() are
already exercised by ServiceTransformSpec's existing interface/iterable
projection tests; full module suite + codeStyle pass unchanged.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nter parameters[1..-1] as Parameter[] relies on Groovy's range-slicing getAt on an array; IntelliJ's stricter @CompileStatic checker infers the 1..-1 literal as ObjectRange<Integer> and can't resolve a matching DefaultGroovyMethods.getAt overload for it, even though groovyc compiles it fine. Arrays.copyOfRange(parameters, 1, parameters.length) expresses the same "drop the first parameter" intent unambiguously for both compilers, with no cast needed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request improves correctness and confidence in grails-datamapping-core’s GORM services/AST implementer layer by adding targeted Spock coverage and addressing a compiler-crash bug in the save implementer error path.
Changes:
- Add new Spock specs covering
DefaultTenantService,DefaultTransactionService, and multiple service implementer edge cases viaGroovyClassLoader.parseClass(...)structural assertions. - Fix
AbstractSaveImplementerto report a clean compile error instead of throwing due to a null module context on synthesized methods. - Refactor/cleanup service implementer code (projection compatibility deduplication, dead-code removal, minor compatibility/suppression adjustments, and a
@CompileStatic-friendly parameter copy inUpdateOneImplementer).
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/services/DefaultTransactionServiceSpec.groovy | Adds unit coverage for DefaultTransactionService transaction/rollback entry points. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/services/DefaultTenantServiceSpec.groovy | Adds unit coverage for DefaultTenantService multi-tenancy behaviors and error modes. |
| grails-datamapping-core/src/test/groovy/grails/gorm/services/ServiceImplementerEdgeCaseSpec.groovy | Adds structural tests for several service implementer edge cases (update/delete/@where projections/findById shortcut/abstract-class enhancement). |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/UpdateOneImplementer.groovy | Replaces Groovy range slicing with Arrays.copyOfRange for @CompileStatic compatibility when selecting property parameters. |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/SaveImplementer.groovy | Updates call site to match the new bindParametersAndSave signature. |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/IterableInterfaceProjectionBuilder.groovy | Deduplicates interface-projection property compatibility logic by delegating to shared helper. |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/InterfaceProjectionBuilder.groovy | Extracts shared hasCompatibleProperties(...) helper and adjusts typing around getter-name list construction. |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/FindOneStringQueryImplementer.groovy | Adds suppression/comment clarifying unused params retained for polymorphic override points. |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/FindOneImplementer.groovy | Adds suppression/comment clarifying unused params retained for polymorphic override points. |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/FindAllByImplementer.groovy | Uses statically imported error(...) helper consistently for compile-time error reporting. |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/DeleteWhereImplementer.groovy | Minor return-type check simplification (== vs .equals) for VOID_TYPE. |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/DeleteImplementer.groovy | Minor return-type check simplification (== vs .equals) for VOID_TYPE. |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/AbstractWhereImplementer.groovy | Adds suppressions/comments to preserve override-friendly extension points. |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/AbstractServiceImplementer.groovy | Removes unused helper methods/imports and simplifies AST dispatch calls. |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/AbstractSaveImplementer.groovy | Fixes invalid-argument error reporting to use the abstract method’s module context and node, preventing compiler NPEs. |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/AbstractDetachedCriteriaServiceImplementor.groovy | Adds suppressions/comments to preserve override-friendly extension points. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 8.1.x #16156 +/- ##
==================================================
+ Coverage 53.1444% 53.2562% +0.1118%
- Complexity 19371 19420 +49
==================================================
Files 2080 2080
Lines 99000 98995 -5
Branches 17363 17362 -1
==================================================
+ Hits 52613 52721 +108
+ Misses 38828 38726 -102
+ Partials 7559 7548 -11
🚀 New features to boost your workflow:
|
DeleteWhereImplementer.isCompatibleReturnType and AbstractServiceImplementer.isValidParameter each had one branch missing on lines Codecov flagged. Added a compile-error test for a @where delete method with an incompatible (non-void, non-Number) return type, and a save-with-id-named-parameter test that exercises isValidParameter's GormProperties.IDENTITY branch directly (SaveImplementer, unlike UpdateOneImplementer, doesn't strip the id parameter before validating it). Both branches now fully covered. The third flagged line, FindAllByImplementer's matchSpec == null error path, was verified empirically rather than assumed unreachable: a probe test with an edge-case DSL input (findAllBy() with no property suffix) showed doesImplement() already computes the identical memoized matchSpec before selecting this implementer, so doImplement() can never observe a null matchSpec -- the method falls through to FindAllImplementer instead whenever matchSpec would be null. Genuinely unreachable through the public DSL, not a gap. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
✅ All tests passed ✅🏷️ Commit: 984fe36 Learn more about TestLens at testlens.app. |
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
DefaultTenantServiceandDefaultTransactionService(org.grails.datastore.gorm.services), both previously at 0% coverage. Pure Spock-mock specs, no live datastore needed.@ServiceAST-transform implementer classes (UpdateOneImplementer,DeleteWhereImplementer,FindOneInterfaceProjectionWhereImplementer,FindAllPropertyProjectionImplementer,FindOnePropertyProjectionImplementer, thefindById(id)shortcut inFindOneByImplementer, and theAbstractWriteOperationImplementer.enhance()path for abstract-class services with pre-existing concrete write methods), all via the existingGroovyClassLoader().parseClass(...)+@Implemented(by=...)structural-testing convention already used byServiceTransformSpec/WhereConnectionRoutingSpec.AbstractSaveImplementer's invalid-argument error path referencednewMethodNode.declaringClass.module(null on the synthesized implementation method), crashing the compiler with aNullPointerException/GroovyBugErrorinstead of reporting a clean compile error. Fixed to useabstractMethodNode.declaringClass.module.context, matching the pattern used elsewhere in this package (e.g.FindAllByImplementer).getDetachedCriteriaType/lookupById/findMethodToInvoke/getFindMethodToInvokewhere the flagged parameter/method is a legitimateprotectedpolymorphic extension point on a class with real subclasses — changing the signature or making itstaticwould silently break overriding.bindParametersAndSave's unusednewMethodNodeparameter (a side effect of theAbstractSaveImplementerfix above), and three unused helper methods onAbstractServiceImplementer(transactionalDatastore,transactionService,findInstanceApiForConnectionId) with zero call sites anywhere in the repo.InterfaceProjectionBuilderandIterableInterfaceProjectionBuilderinto a sharedhasCompatibleProperties(...)method.parameters[1..-1] as Parameter[]) withArrays.copyOfRange(...)inUpdateOneImplementer, since IntelliJ's stricter@CompileStaticchecker can't resolve thegetAtoverload for it even thoughgroovyccompiles it fine.Test plan
./gradlew :grails-datamapping-core:test— full module suite passes./gradlew :grails-datamapping-core:codeStyle— 0 Checkstyle/CodeNarc violations./gradlew :grails-datamapping-rx:compileGroovy— sibling module unaffected byAbstractServiceImplementercleanup🤖 Generated with Claude Code