Copilot commented on code in PR #15944:
URL: https://github.com/apache/grails-core/pull/15944#discussion_r3553705560
##########
grails-forge/grails-forge-core/src/main/java/org/grails/forge/feature/database/DatabaseDriverFeature.java:
##########
@@ -56,8 +56,8 @@ public void processSelectedFeatures(FeatureContext
featureContext) {
if (!featureContext.isPresent(TestContainers.class) && testContainers
!= null) {
featureContext.addFeature(testContainers);
}
- if (!featureContext.isPresent(HibernateGorm.class) && hibernateGorm !=
null) {
- featureContext.addFeature(hibernateGorm);
+ if (!featureContext.isPresent(GrailsDataHibernate5.class) &&
grailsDataHibernate5 != null) {
+ featureContext.addFeature(grailsDataHibernate5);
}
Review Comment:
DatabaseDriverFeature always auto-adds GrailsDataHibernate5 when it is not
present. If a user selects Hibernate 7 (gorm-hibernate7 / --data hibernate7)
and also selects a database driver feature (e.g. postgres), this will add
Hibernate 5 as well, resulting in both Hibernate implementations being applied
and reintroducing the kind of dependency conflicts this PR is trying to prevent.
##########
grails-forge/grails-forge-core/src/main/java/org/grails/forge/options/GormImplTypeConverter.java:
##########
@@ -0,0 +1,38 @@
+/*
+ * 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.forge.options;
+
+import java.util.Optional;
+
+import io.micronaut.core.convert.ConversionContext;
+import io.micronaut.core.convert.TypeConverter;
+import jakarta.inject.Singleton;
+
+/**
+ * Converts user-supplied selection values (including the legacy {@code
hibernate}
+ * value) to {@link GormImpl} for HTTP parameter binding.
+ */
+@Singleton
+public class GormImplTypeConverter implements TypeConverter<CharSequence,
GormImpl> {
+
+ @Override
+ public Optional<GormImpl> convert(CharSequence object, Class<GormImpl>
targetType, ConversionContext context) {
+ return Optional.ofNullable(GormImpl.parse(object.toString()));
+ }
Review Comment:
GormImplTypeConverter.convert calls object.toString() without a null check.
Micronaut TypeConverter implementations should be null-safe; if a null value is
passed through conversion, this will throw a NullPointerException during
request binding.
##########
grails-forge/grails-forge-cli/src/test/groovy/org/grails/forge/cli/command/CreateAppCommandSpec.groovy:
##########
@@ -64,7 +64,72 @@ class CreateAppCommandSpec extends CommandSpec implements
CommandFixture {
then:
noExceptionThrown()
- baos.toString().contains("Invalid GORM implementation selection: xyz")
+ baos.toString().contains("Invalid Grails Data implementation
selection: xyz")
Review Comment:
This test redirects System.err but never restores it. Because other tests in
the suite don't necessarily reset System.err, this can leak across specs and
hide/alter subsequent test output.
##########
grails-forge/grails-forge-core/src/main/java/org/grails/forge/feature/database/GrailsDataHibernateValidator.java:
##########
@@ -0,0 +1,45 @@
+/*
+ * 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.forge.feature.database;
+
+import java.util.Set;
+
+import jakarta.inject.Singleton;
+
+import org.grails.forge.application.ApplicationType;
+import org.grails.forge.feature.Feature;
+import org.grails.forge.feature.validation.FeatureValidator;
+import org.grails.forge.options.Options;
+
+@Singleton
+public class GrailsDataHibernateValidator implements FeatureValidator {
+
+ @Override
+ public void validatePreProcessing(Options options, ApplicationType
applicationType, Set<Feature> features) {
+ if (features.stream().anyMatch(GrailsDataHibernate5.class::isInstance)
+ &&
features.stream().anyMatch(GrailsDataHibernate7.class::isInstance)) {
+ throw new IllegalArgumentException("Only one Grails Data for
Hibernate implementation can be selected: gorm-hibernate5 or gorm-hibernate7");
+ }
+ }
+
+ @Override
+ public void validatePostProcessing(Options options, ApplicationType
applicationType, Set<Feature> features) {
+
+ }
Review Comment:
GrailsDataHibernateValidator only validates in validatePreProcessing, but
conflicting Hibernate features can be introduced during
FeatureContext.processSelectedFeatures() (e.g. via other features adding
defaults). Leaving validatePostProcessing empty makes it easy for conflicts to
slip through.
--
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]
