borinquenkid commented on code in PR #16151:
URL: https://github.com/apache/grails-core/pull/16151#discussion_r3792118204
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/events/DomainEventListener.java:
##########
@@ -270,11 +293,11 @@ public void persistentEntityAdded(PersistentEntity
entity) {
* @see
org.springframework.context.event.SmartApplicationListener#supportsEventType(
* java.lang.Class)
*/
- public boolean supportsEventType(Class<? extends ApplicationEvent>
eventType) {
+ public boolean supportsEventType(@NonNull Class<? extends
ApplicationEvent> eventType) {
return AbstractPersistenceEvent.class.isAssignableFrom(eventType);
}
Review Comment:
Checked this against Spring 7.0.8's actual `SmartApplicationListener`
source: `supportsEventType`'s `eventType` parameter is documented `never null`,
not `@Nullable` — it's the sibling `supportsSourceType(sourceType)` method
whose parameter is `@Nullable`. I'd mixed the two up in the PR description (now
corrected). Since nothing in the real dispatch path ever calls
`supportsEventType(null)` (Spring's multicaster and this PR's own
`DefaultApplicationEventPublisher.dispatch` always pass `event.getClass()`),
the `@NonNull` annotation + fail-fast NPE here is the correct contract,
consistent with this PR's theme of removing defensive code for guarantees the
framework already provides. Leaving as-is.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/events/AutoTimestampEventListener.java:
##########
@@ -124,9 +126,8 @@ protected void onPersistenceEvent(final
AbstractPersistenceEvent event) {
}
}
- public boolean supportsEventType(Class<? extends ApplicationEvent>
eventType) {
- return PreInsertEvent.class.isAssignableFrom(eventType) ||
- PreUpdateEvent.class.isAssignableFrom(eventType);
+ public boolean supportsEventType(@NonNull Class<? extends
ApplicationEvent> eventType) {
+ return PreInsertEvent.class.isAssignableFrom(eventType) ||
PreUpdateEvent.class.isAssignableFrom(eventType);
}
Review Comment:
Same finding as on the `DomainEventListener` copy of this method: Spring 7's
`SmartApplicationListener.supportsEventType`'s `eventType` is documented `never
null` (it's `supportsSourceType`'s `sourceType` that's `@Nullable`, not this
one). The PR description was wrong about which parameter is nullable — fixed
now. `@NonNull` + fail-fast here is intentional and correct; no code change
needed.
##########
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/events/DomainEventListenerSpec.groovy:
##########
@@ -0,0 +1,595 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.grails.datastore.gorm.events
+
+import java.sql.Timestamp
+
+import spock.lang.Specification
+import spock.lang.Unroll
+
+import org.springframework.beans.factory.config.AutowireCapableBeanFactory
+import org.springframework.context.ApplicationEvent
+import org.springframework.context.ConfigurableApplicationContext
+import org.springframework.context.PayloadApplicationEvent
+
+import org.grails.datastore.mapping.config.Entity
+import org.grails.datastore.mapping.core.Datastore
+import org.grails.datastore.mapping.core.connections.ConnectionSource
+import org.grails.datastore.mapping.core.connections.ConnectionSourceSettings
+import org.grails.datastore.mapping.core.connections.ConnectionSources
+import org.grails.datastore.mapping.core.connections.ConnectionSourcesProvider
+import org.grails.datastore.mapping.dirty.checking.DirtyCheckable
+import org.grails.datastore.mapping.engine.EntityAccess
+import org.grails.datastore.mapping.engine.event.MergeEvent
+import org.grails.datastore.mapping.engine.event.PersistEvent
+import org.grails.datastore.mapping.engine.event.PostDeleteEvent
+import org.grails.datastore.mapping.engine.event.PostInsertEvent
+import org.grails.datastore.mapping.engine.event.PostLoadEvent
+import org.grails.datastore.mapping.engine.event.PostUpdateEvent
+import org.grails.datastore.mapping.engine.event.PreDeleteEvent
+import org.grails.datastore.mapping.engine.event.PreInsertEvent
+import org.grails.datastore.mapping.engine.event.PreLoadEvent
+import org.grails.datastore.mapping.engine.event.PreUpdateEvent
+import org.grails.datastore.mapping.engine.event.SaveOrUpdateEvent
+import org.grails.datastore.mapping.engine.event.ValidationEvent
+import org.grails.datastore.mapping.model.ClassMapping
+import org.grails.datastore.mapping.model.MappingContext
+import org.grails.datastore.mapping.model.PersistentEntity
+import org.grails.datastore.mapping.model.PersistentProperty
+import org.grails.datastore.mapping.model.config.GormProperties
+
+/**
+ * Note on coverage gaps left deliberately untested:
+ * - {@code invokeEvent}'s {@code ea != null} branch is always true through
every public before-
+ * and after-hook method, which never passes a null {@code EntityAccess};
the {@code ea == null}
+ * path is unreachable via the public API.
+ * - The protected {@code DomainEventListener(ConnectionSourcesProvider,
MappingContext)}
+ * constructor exists solely for subclassing (e.g. {@code
grails.gorm.rx.events.DomainEventListener}),
+ * which is covered by its own module's spec; exercising it here would
duplicate that coverage.
+ *
+ * {@code invokeEvent} previously also branched on {@code
eventMethod.getParameterTypes().length == 1}
+ * to invoke a hook with the triggering event as an argument. That branch was
confirmed dead (via
+ * decompiling spring-core's {@code ReflectionUtils.findMethod(Class,
String)}, which only ever
+ * matches zero-argument methods) and removed.
+ */
+class DomainEventListenerSpec extends Specification {
+
+ void "registers itself as a mapping context listener and creates event
caches for entities present at construction time"() {
+ given:
+ RecordingDomain domain = new RecordingDomain()
+ PersistentEntity entity = entityFor(RecordingDomain)
+ MappingContext mappingContext = Mock(MappingContext) {
+ getPersistentEntities() >> [entity]
+ }
+ Datastore datastore = plainDatastore(mappingContext)
+
+ when:
+ DomainEventListener listener = new DomainEventListener(datastore)
+
+ then:
+ 1 * mappingContext.addMappingContextListener(_)
+
+ when: 'the pre-existing entity\'s hook is invoked'
+ EntityAccess ea = Stub(EntityAccess) { getEntity() >> domain }
+ listener.beforeInsert(entity, ea)
+
+ then: 'it fires immediately, proving the cache was created eagerly at
construction time'
+ domain.invoked == ['beforeInsert']
+ }
+
+ void "persistentEntityAdded creates event caches for a newly discovered
entity"() {
+ given:
+ RecordingDomain domain = new RecordingDomain()
+ PersistentEntity entity = entityFor(RecordingDomain)
+ Datastore datastore = plainDatastore(Stub(MappingContext) {
getPersistentEntities() >> [] })
+ DomainEventListener listener = new DomainEventListener(datastore)
+ EntityAccess ea = Stub(EntityAccess) { getEntity() >> domain }
+
+ expect: 'the hook is not yet wired up before the entity is added'
+ listener.beforeInsert(entity, ea)
+ domain.invoked.isEmpty()
+
+ when:
+ listener.persistentEntityAdded(entity)
+ listener.beforeInsert(entity, ea)
+
+ then:
+ domain.invoked == ['beforeInsert']
+ }
+
+ void "supportsEventType accepts AbstractPersistenceEvent subtypes and
rejects unrelated ApplicationEvents"() {
+ given:
+ DomainEventListener listener = new
DomainEventListener(plainDatastore(Stub(MappingContext) {
getPersistentEntities() >> [] }))
+
+ expect:
+ listener.supportsEventType(PreInsertEvent)
+ !listener.supportsEventType(PayloadApplicationEvent)
+ }
+
+ 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)
+ }
Review Comment:
Same root cause as the two `supportsEventType` comments above: I
mischaracterized the Spring contract in the PR description.
`SmartApplicationListener.supportsEventType`'s `eventType` parameter is
documented `never null` in Spring 7.0.8 — it's `supportsSourceType`'s
`sourceType` param that's actually `@Nullable`. So this spec correctly asserts
the fail-fast NPE, matching the real (non-nullable) contract rather than a
nullable one. Description corrected; test left as-is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
