jdaugherty commented on code in PR #15118: URL: https://github.com/apache/grails-core/pull/15118#discussion_r2484070777
########## grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/timestamp/AuditorAware.java: ########## @@ -0,0 +1,58 @@ +/* + * 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.timestamp; + +import java.util.Optional; + +/** + * Interface for components that are aware of the application's current auditor. + * This will be used to populate @CreatedBy and @LastModifiedBy annotated fields + * in domain objects. + * + * <p>Implementations should be registered as Spring beans. The type parameter + * should match the type of the auditor field in your domain classes (e.g., User, + * Long, String, etc.).</p> + * + * <p>Example implementation:</p> + * <pre> + * @Component Review Comment: You can remove the html escaping if you wrap the code with `{@code` and `}`. Unfortunately, you still need the pre to preserve line breaks. ########## grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/model/AutoTimestampUtils.java: ########## @@ -0,0 +1,202 @@ +/* + * 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.mapping.model; + +import java.lang.reflect.Field; + +import org.springframework.util.ReflectionUtils; + +import grails.util.Environment; +import org.grails.datastore.mapping.config.Property; +import org.grails.datastore.mapping.config.Property.AutoTimestampType; + +/** + * Utility class for detecting and caching auto-timestamp and auditing annotations on domain properties. + * This avoids repeated reflection calls by storing the annotation type in the Property metadata. + * + * <p>Supports the following annotations (both GORM and Spring Data variants):</p> + * <ul> + * <li>@CreatedDate / @grails.gorm.annotation.CreatedDate - automatically set on insert</li> + * <li>@LastModifiedDate / @grails.gorm.annotation.LastModifiedDate - automatically set on insert and update</li> + * <li>@CreatedBy / @grails.gorm.annotation.CreatedBy - automatically populated with current auditor on insert</li> + * <li>@LastModifiedBy / @grails.gorm.annotation.LastModifiedBy - automatically populated with current auditor on insert and update</li> + * <li>@AutoTimestamp - GORM-specific annotation for backwards compatibility</li> + * </ul> + * + * <p>Caching is automatically disabled in development mode ({@link Environment#isDevelopmentMode()}) + * to ensure annotation changes are picked up during class reloading.</p> + * + * @author Scott Murphy Heiberg + * @since 7.0 + */ +public class AutoTimestampUtils { + + private static final String CREATED_DATE_ANNOTATION = "grails.gorm.annotation.CreatedDate"; + private static final String LAST_MODIFIED_DATE_ANNOTATION = "grails.gorm.annotation.LastModifiedDate"; + private static final String AUTO_TIMESTAMP_ANNOTATION = "grails.gorm.annotation.AutoTimestamp"; + private static final String CREATED_BY_ANNOTATION = "grails.gorm.annotation.CreatedBy"; + private static final String LAST_MODIFIED_BY_ANNOTATION = "grails.gorm.annotation.LastModifiedBy"; + + private static final String CREATED_DATE_SPRING_ANNOTATION = "org.springframework.data.annotation.CreatedDate"; + private static final String LAST_MODIFIED_DATE_SPRING_ANNOTATION = "org.springframework.data.annotation.LastModifiedDate"; + private static final String CREATED_BY_SPRING_ANNOTATION = "org.springframework.data.annotation.CreatedBy"; + private static final String LAST_MODIFIED_BY_SPRING_ANNOTATION = "org.springframework.data.annotation.LastModifiedBy"; + + /** + * Gets the auto-timestamp type for a persistent property, using cached metadata when not in development mode. + * + * <p>In development mode, this method will always perform reflection to detect the current + * annotation state, ensuring that annotation changes during class reloading are immediately + * recognized. In production, the result is cached to avoid repeated reflection calls.</p> + * + * @param persistentProperty The persistent property to check + * @return The auto-timestamp type (CREATED, UPDATED, or NONE) + */ + public static AutoTimestampType getAutoTimestampType(PersistentProperty<?> persistentProperty) { + Property mappedForm = persistentProperty.getMapping().getMappedForm(); + + // In development mode, always detect fresh to support class reloading + if (Environment.isDevelopmentMode()) { Review Comment: We need another way to enable/disable caching. GORM was meant to work without grails, and this breaks for non-Grails. ########## grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/core/GrailsGradlePlugin.groovy: ########## @@ -226,17 +226,50 @@ class GrailsGradlePlugin extends GroovyPlugin { protected Closure<String> getGroovyCompilerScript(GroovyCompile compile, Project project) { GrailsExtension grails = project.extensions.findByType(GrailsExtension) - if (!grails.importJavaTime) { + + List<String> starImports = [] + + // Add java.time if enabled + if (grails.importJavaTime) { + starImports.add('java.time') + } + + // Add Grails annotation packages and common validation annotations if enabled + if (grails.importGrailsCommonAnnotations) { Review Comment: Instead of hard coding the default imports, can we store these defaults as a list on the GrailsExtension and then use them here if this is set? This makes it clear what's be included to the end user. ########## grails-datamapping-core/src/main/groovy/grails/gorm/annotation/CreatedDate.java: ########## @@ -0,0 +1,36 @@ +/* + * 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 grails.gorm.annotation; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * A property annotation used to apply auto-timestamping on a field + * upon gorm insert events. This is an alias for @AutoTimestamp(EventType.CREATED). Review Comment: Examples exist for the other annotations, why not add one for this one? ########## grails-datamapping-core/src/main/groovy/grails/gorm/annotation/LastModifiedBy.java: ########## @@ -0,0 +1,56 @@ +/* + * 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 grails.gorm.annotation; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * A property annotation used to automatically populate a field with the current auditor + * upon GORM insert and update events. The current auditor is retrieved from an + * {@link org.grails.datastore.gorm.timestamp.AuditorAware} bean registered in the Spring application context. + * + * <p>Example usage:</p> + * <pre> + * class Book { Review Comment: See my other comment about {@code so we can remove the escaping. ########## grails-datamapping-validation/src/main/groovy/org/grails/datastore/gorm/validation/constraints/eval/DefaultConstraintEvaluator.java: ########## @@ -305,6 +306,11 @@ protected boolean isConstrainableProperty(PersistentProperty persistentProperty, return NameUtils.isNotConfigurational(propertyName); } else { + // Check if property has @CreatedDate or @LastModifiedDate annotations + if (AutoTimestampUtils.hasAutoTimestampAnnotation(persistentProperty)) { Review Comment: Why can't constraints be added for the auditor type? It's perfectly reasonable if the auditor is a String, to set a max string (i.e. if you wanted a value greater than 255) ########## grails-datamapping-core/src/main/groovy/grails/gorm/annotation/CreatedBy.java: ########## @@ -0,0 +1,56 @@ +/* + * 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 grails.gorm.annotation; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * A property annotation used to automatically populate a field with the current auditor + * upon GORM insert events. The current auditor is retrieved from an {@link org.grails.datastore.gorm.timestamp.AuditorAware} + * bean registered in the Spring application context. + * + * <p>Example usage:</p> + * <pre> + * class Book { Review Comment: See my other comment about `{@code` so we can remove the escaping. ########## grails-datastore-core/build.gradle: ########## @@ -45,6 +45,9 @@ dependencies { implementation platform(project(':grails-bom')) api project(':grails-common') + api 'org.apache.grails.gradle:grails-gradle-model', { Review Comment: 1. grails-gradle-model includes groovy3, etc and must be excluded. 2. Environment is a grails concept, but gorm can be used in non-grails applications. I think we need another solution here. This is likely a breaking change. ########## grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/core/GrailsGradlePlugin.groovy: ########## @@ -226,17 +226,50 @@ class GrailsGradlePlugin extends GroovyPlugin { protected Closure<String> getGroovyCompilerScript(GroovyCompile compile, Project project) { GrailsExtension grails = project.extensions.findByType(GrailsExtension) - if (!grails.importJavaTime) { + + List<String> starImports = [] + + // Add java.time if enabled + if (grails.importJavaTime) { + starImports.add('java.time') + } + + // Add Grails annotation packages and common validation annotations if enabled + if (grails.importGrailsCommonAnnotations) { + // Always add jakarta.validation.constraints + starImports.add('jakarta.validation.constraints') + + // Check for grails-datamapping-core (grails.gorm.annotation.*) + def datamappingCoreDep = project.configurations.getByName('compileClasspath').dependencies.find { Dependency d -> Review Comment: This is going to run every time and for an app with lots of dependencies this could be a slow down. Seems like the list based approach may be better and then people can opt in or out of them? ########## grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/model/AutoTimestampUtils.java: ########## @@ -0,0 +1,202 @@ +/* + * 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.mapping.model; + +import java.lang.reflect.Field; + +import org.springframework.util.ReflectionUtils; + +import grails.util.Environment; +import org.grails.datastore.mapping.config.Property; +import org.grails.datastore.mapping.config.Property.AutoTimestampType; + +/** + * Utility class for detecting and caching auto-timestamp and auditing annotations on domain properties. + * This avoids repeated reflection calls by storing the annotation type in the Property metadata. + * + * <p>Supports the following annotations (both GORM and Spring Data variants):</p> + * <ul> + * <li>@CreatedDate / @grails.gorm.annotation.CreatedDate - automatically set on insert</li> + * <li>@LastModifiedDate / @grails.gorm.annotation.LastModifiedDate - automatically set on insert and update</li> + * <li>@CreatedBy / @grails.gorm.annotation.CreatedBy - automatically populated with current auditor on insert</li> + * <li>@LastModifiedBy / @grails.gorm.annotation.LastModifiedBy - automatically populated with current auditor on insert and update</li> + * <li>@AutoTimestamp - GORM-specific annotation for backwards compatibility</li> + * </ul> + * + * <p>Caching is automatically disabled in development mode ({@link Environment#isDevelopmentMode()}) Review Comment: In large applications this will significantly slow down development mode. Is this to support reloading? It would be better to add that method on the grails plugin for gorm that triggers on reload/refresh and then clear the cache correctly there. ########## grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/config/Property.groovy: ########## @@ -137,6 +137,36 @@ class Property implements Cloneable { private List<String> uniquenessGroup = new ArrayList<String>() private String propertyName private EnumType enumType + /** + * The auto-timestamp type for this property, cached to avoid repeated reflection calls + */ + AutoTimestampType autoTimestampType + + /** + * Enum representing the type of auto-timestamp annotation on a property + */ + static enum AutoTimestampType { Review Comment: 1. Can we pull this into it's own file? 2. Is "AutoTimestamp" really a good name for createdBy / updatedBy? Would Tracked or TrackedEntityType be a better name? ########## grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/core/GrailsGradlePlugin.groovy: ########## @@ -226,17 +226,50 @@ class GrailsGradlePlugin extends GroovyPlugin { protected Closure<String> getGroovyCompilerScript(GroovyCompile compile, Project project) { GrailsExtension grails = project.extensions.findByType(GrailsExtension) - if (!grails.importJavaTime) { + + List<String> starImports = [] + + // Add java.time if enabled + if (grails.importJavaTime) { + starImports.add('java.time') + } + + // Add Grails annotation packages and common validation annotations if enabled + if (grails.importGrailsCommonAnnotations) { + // Always add jakarta.validation.constraints + starImports.add('jakarta.validation.constraints') + + // Check for grails-datamapping-core (grails.gorm.annotation.*) + def datamappingCoreDep = project.configurations.getByName('compileClasspath').dependencies.find { Dependency d -> + d.group == 'org.apache.grails.data' && d.name == 'grails-datamapping-core' + } + if (datamappingCoreDep) { + starImports.add('grails.gorm.annotation') + } + + // Check for grails-scaffolding (grails.plugin.scaffolding.annotation.*) + def scaffoldingDep = project.configurations.getByName('compileClasspath').dependencies.find { Dependency d -> Review Comment: I wouldn't consider scaffolding a default. I think this needs a separate setting. My app doesn't actually use scaffolding so this is a problem for me. -- 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]
