Add test coverage and fix lint for GORM domain event listeners - #16151
Open
borinquenkid wants to merge 9 commits into
Open
Add test coverage and fix lint for GORM domain event listeners#16151borinquenkid wants to merge 9 commits into
borinquenkid wants to merge 9 commits into
Conversation
DomainEventListener, DefaultApplicationEventPublisher, and ConfigurableApplicationContextEventPublisher had zero test coverage. Adds mock-based Spock specs driving each class through its public constructor/method/onApplicationEvent surface only. Coverage: DomainEventListener 0% -> 92% lines / 91% branches / 96% methods; DefaultApplicationEventPublisher 0% -> 95% / 81% / 100%; ConfigurableApplicationContextEventPublisher 0% -> 100% / n/a / 100%. Three branches are documented as intentionally left uncovered in DomainEventListenerSpec: invokeEvent's ea==null path is unreachable via the public API, and its eventMethod.getParameterTypes().length==1 path is confirmed (via decompiling spring-core) structurally dead — findAndCacheEvent only ever caches zero-argument hook methods, so event-argument-accepting hooks can never be invoked.
The existing AutoTimestampEventListenerSpec only drove the suppression logic and beforeInsert/beforeUpdate directly via a test subclass that bypassed initForMappingContext entirely, leaving construction, real entity/property scanning (storeDateCreatedAndLastUpdatedInfo, including @CreatedDate/@LastModifiedDate/@CreatedBy/@LastModifiedBy annotation detection), the deferred-initialization path, setApplicationContext, supportsEventType, and onApplicationEvent dispatch untested. Coverage: 84% -> 98% lines, 61% -> 83% branches, 76% -> 96% methods.
invokeEvent's eventMethod.getParameterTypes().length == 1 branch could never be taken: findAndCacheEvent caches hooks via Spring's ReflectionUtils.findMethod(Class, String), which only ever matches zero-argument methods (confirmed by decompiling spring-core), so a cached eventMethod can never have one parameter. Dropped the branch and the now-unused ApplicationEvent parameter it required, along with the argument at all 8 call sites. No behavior change; full module suite, jacoco, and codeStyle all clean.
- entityEvents field made final (never reassigned) - Removed ZERO_PARAMS constant (zero references anywhere in the repo) - Parameterized raw ConnectionSourcesProvider/Class usages - supportsEventType now null-safe: Spring 7's SmartApplicationListener.supportsEventType declares its parameter @nullable, and this override would NPE via Class.isAssignableFrom(null) - invokeEvent's ea.refresh() call is now guarded by the same ea != null check already used earlier in the method, closing a latent NPE path - Removed the two switch branches in onPersistenceEvent (SaveOrUpdate, Validation) that were duplicates of the default branch - Reordered each before/after method pair so the 2-arg overload holds the real logic and is canonical; the 3-arg overload (whose event parameter has been unused since the dead 1-arg-hook branch was removed) is now @deprecated, forwards to the 2-arg overload, and is scheduled for removal in 9.0. onPersistenceEvent now calls the 2-arg overloads directly instead of the newly-deprecated 3-arg ones. Added tests for the deprecated overloads' delegation and the null eventType case. Full suite, jacoco, and codeStyle all clean.
Rather than defensively guarding against null, declare the contract explicitly via jspecify's @nonnull and let a null argument fail fast with an NPE. Updated the corresponding spec to assert the NPE instead of a graceful false return.
- supportsEventType is now null-safe, matching Spring 7's SmartApplicationListener.supportsEventType @nullable contract - Parameterized every raw Class/List<Class> usage across the withoutLastUpdated/withoutDateCreated/withoutTimestamps overloads and their shared runWithDisabled helper - Replaced disabled.entityNames.removeAll(added) (a HashSet.removeAll of a List, which can fall into an O(n*m) path depending on relative collection sizes) with a direct per-element remove, guaranteeing O(1) removals regardless of size getTimestampProvider() was also flagged as unused but left as-is: it's the getter half of a real getter/setter bean-property pair (the setter is the intended extension point for injecting a custom TimestampProvider), not dead code. Full suite, jacoco, and codeStyle all clean.
publishEvent(ApplicationEvent) and publishEvent(Object) both iterated applicationListeners and applied the same SmartApplicationListener event/source-type filtering before dispatching; only the event-wrapping step differed. Extracted the shared iterate-filter-dispatch logic into a private dispatch(ApplicationEvent) method both overloads now call. Also narrows ConfigurableApplicationEventPublisher.addApplicationListener's parameter to ApplicationListener<? extends ApplicationEvent>, matching ConfigurableApplicationContextEventPublisher's already-narrower signature. No behavior change; existing DefaultApplicationEventPublisherSpec coverage (95%/81%) verifies both overloads unchanged.
Renamed the local RxDatastoreClient variable in onApplicationEvent from datastoreClient to sourceClient, since it shadowed the class's own datastoreClient field (the source of the "might not be assigned" confusion) and made the two equals() calls read ambiguously. Both this.datastoreClient.equals(datastoreClient) calls are now datastoreClient == sourceClient, Groovy's idiomatic null-safe equals. No behavior change; full suite, jacoco, and codeStyle clean.
…tListener Same idiomatic Groovy null-safe equals fix already applied to MultiTenantEventListener. No behavior change; full suite, jacoco, and codeStyle clean.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR strengthens and modernizes the GORM domain event listener infrastructure by adding substantial Spock test coverage, removing a confirmed-dead reflective invocation path, and making small API and lint/IDE-warning cleanups across the org.grails.datastore.gorm.events package (plus corresponding Rx listener adjustments).
Changes:
- Adds new mock-based Spock specs covering
DomainEventListener,AutoTimestampEventListener, and the application event publisher implementations. - Simplifies domain hook invocation by removing a dead “event-argument” hook path and deprecating the now-vestigial 3-arg hook overloads (event parameter).
- Refactors event publishing/listener handling and updates Rx listeners to align with the core changes and reduce IDE/lint warnings.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| grails-datamapping-rx/src/main/groovy/org/grails/gorm/rx/events/MultiTenantEventListener.groovy | Replaces explicit .equals() usage and clarifies source client naming in event handling. |
| grails-datamapping-rx/src/main/groovy/org/grails/gorm/rx/events/DomainEventListener.groovy | Uses Groovy == for datastore client comparisons in source validation. |
| grails-datamapping-rx/src/main/groovy/org/grails/gorm/rx/events/AutoTimestampEventListener.groovy | Uses Groovy == for datastore client comparisons in source validation. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/events/DomainEventListenerSpec.groovy | Adds comprehensive unit coverage for domain event listener behavior and hook dispatch. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/events/DefaultApplicationEventPublisherSpec.groovy | Adds tests for listener dispatch and SmartApplicationListener filtering behavior. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/events/ConfigurableApplicationContextEventPublisherSpec.groovy | Adds delegation tests for the application-context-backed event publisher. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/events/AutoTimestampEventListenerConstructionSpec.groovy | Adds construction + scanning behavior tests, plus event dispatch coverage for AutoTimestamp listener. |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/events/DomainEventListener.java | Removes dead reflective hook path, updates dispatch to call canonical 2-arg hooks, and deprecates 3-arg overloads. |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/events/DefaultApplicationEventPublisher.groovy | Deduplicates publish paths into a shared dispatch method and tightens listener API generics. |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/events/ConfigurableApplicationEventPublisher.groovy | Narrows listener API type bound to ApplicationEvent. |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/events/AutoTimestampEventListener.java | Tightens generics, adjusts disabled-timestamps cleanup, and updates supportsEventType signature/formatting. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+296
to
298
| public boolean supportsEventType(@NonNull Class<? extends ApplicationEvent> eventType) { | ||
| return AbstractPersistenceEvent.class.isAssignableFrom(eventType); | ||
| } |
Comment on lines
+129
to
131
| public boolean supportsEventType(@NonNull Class<? extends ApplicationEvent> eventType) { | ||
| return PreInsertEvent.class.isAssignableFrom(eventType) || PreUpdateEvent.class.isAssignableFrom(eventType); | ||
| } |
Comment on lines
+126
to
+135
| void "supportsEventType throws on a null event type, per its @NonNull contract"() { | ||
| given: | ||
| DomainEventListener listener = new DomainEventListener(plainDatastore(Stub(MappingContext) { getPersistentEntities() >> [] })) | ||
|
|
||
| when: | ||
| listener.supportsEventType(null) | ||
|
|
||
| then: | ||
| thrown(NullPointerException) | ||
| } |
Comment on lines
52
to
55
| if (listener instanceof SmartApplicationListener) { | ||
| SmartApplicationListener smartApplicationListener = (SmartApplicationListener) listener | ||
| if (!smartApplicationListener.supportsEventType((Class<ApplicationEvent>) eventObject.getClass())) { | ||
| if (!smartApplicationListener.supportsEventType((Class<ApplicationEvent>) event.getClass())) { | ||
| continue |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 8.1.x #16151 +/- ##
==================================================
+ Coverage 52.8254% 52.8711% +0.0457%
- Complexity 18871 18895 +24
==================================================
Files 2079 2079
Lines 97207 97193 -14
Branches 16873 16868 -5
==================================================
+ Hits 51350 51387 +37
+ Misses 38446 38405 -41
+ Partials 7411 7401 -10
🚀 New features to boost your workflow:
|
✅ All tests passed ✅🏷️ Commit: 339a861 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
org.grails.datastore.gorm.events(core), which had little to no coverage:DomainEventListener0%→93% lines/93% branches,AutoTimestampEventListener84%/61%→98%/83%,DefaultApplicationEventPublisher0%→95%/81%,ConfigurableApplicationContextEventPublisher0%→100%DomainEventListener.invokeEvent: the event-argument-accepting hook branch could never execute sincefindAndCacheEventonly ever caches zero-argument methods (confirmed by decompiling spring-core'sReflectionUtils.findMethod)eventparameter on the 8 public 3-argbefore*/after*overloads (scheduled for removal in 9.0); the 2-arg overloads are now canonical andonPersistenceEventcalls them directly.equals()→==, aSet.removeAll(List)performance smell, duplicatedpublishEventoverload bodies deduplicated into a shared method, and a null-safety fix onsupportsEventType(Spring 7'sSmartApplicationListener.supportsEventTypeparameter is@Nullable)Test plan
:grails-datamapping-core:testfull suite green:grails-datamapping-rx:testfull suite green (verifies the rx subclasses still compile/pass against the changed core signatures):grails-datamapping-core:codeStyle/:grails-datamapping-rx:codeStyle— zero Checkstyle/CodeNarc violations