Copilot commented on code in PR #16151: URL: https://github.com/apache/grails-core/pull/16151#discussion_r3788268256
########## 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: This spec currently asserts supportsEventType(null) throws NPE. The PR description calls out a null-safety fix for supportsEventType; if the intent is to be null-tolerant (as Spring’s SmartApplicationListener contract allows), this should instead assert it returns false and does not throw. ########## 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: supportsEventType is described in the PR as needing a null-safety fix (Spring’s SmartApplicationListener contract allows a nullable eventType), but this implementation will still throw NullPointerException if eventType is null (Class#isAssignableFrom(null)). Consider guarding against null and returning false instead, to match the intended behavior and avoid potential NPEs. ########## 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: supportsEventType is described in the PR as requiring null-safety, but this will throw NullPointerException when eventType is null (Class#isAssignableFrom(null)). Consider treating null as “unsupported” and returning false. ########## grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/events/DefaultApplicationEventPublisher.groovy: ########## @@ -36,42 +36,34 @@ import org.springframework.context.event.SmartApplicationListener class DefaultApplicationEventPublisher implements ConfigurableApplicationEventPublisher { private List<ApplicationListener> applicationListeners = [] + @Override void publishEvent(ApplicationEvent event) { - for (listener in applicationListeners) { - if (listener instanceof SmartApplicationListener) { - SmartApplicationListener smartApplicationListener = (SmartApplicationListener) listener - if (!smartApplicationListener.supportsEventType((Class<ApplicationEvent>) event.getClass())) { - continue - } - else if (!smartApplicationListener.supportsSourceType(event.source.getClass())) { - continue - } - } - listener.onApplicationEvent(event) - } + dispatch(event) } @Override void publishEvent(Object event) { + dispatch(new PayloadApplicationEvent<Object>(this, event)) + } + + private void dispatch(ApplicationEvent event) { for (listener in applicationListeners) { - def eventObject = new PayloadApplicationEvent<Object>(this, event) if (listener instanceof SmartApplicationListener) { SmartApplicationListener smartApplicationListener = (SmartApplicationListener) listener - if (!smartApplicationListener.supportsEventType((Class<ApplicationEvent>) eventObject.getClass())) { + if (!smartApplicationListener.supportsEventType((Class<ApplicationEvent>) event.getClass())) { continue Review Comment: The cast to (Class<ApplicationEvent>) is unnecessary (and can be an inconvertible/unchecked cast) since event is already typed as ApplicationEvent and event.getClass() is a Class<? extends ApplicationEvent>. Removing the cast avoids static type checking warnings under @CompileStatic. -- 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]
