codeconsole commented on code in PR #15944:
URL: https://github.com/apache/grails-core/pull/15944#discussion_r3555299128
##########
grails-forge/grails-forge-api/src/test/groovy/org/grails/forge/api/FeatureControllerSpec.groovy:
##########
@@ -21,7 +21,9 @@ package org.grails.forge.api
import com.fasterxml.jackson.databind.ObjectMapper
import io.micronaut.http.HttpRequest
+import io.micronaut.http.HttpStatus
Review Comment:
`HttpStatus` (line 24) and `HttpClientResponseException` (line 26) are
imported but never used — the new test only asserts success responses. Can be
dropped.
##########
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(GormFeature.class) &&
grailsDataHibernate5 != null) {
Review Comment:
Changing the guard from `isPresent(HibernateGorm.class)` to
`isPresent(GormFeature.class)` also changes behavior for the MongoDB case:
since `GrailsDataMongoDB extends GormOneOfFeature extends GormFeature` (and
`isPresent` uses `isAssignableFrom`), selecting e.g. `postgres` +
`gorm-mongodb` previously auto-added Hibernate 5 and now doesn't.
That's needed for the Hibernate 7 case and arguably more correct, but the
generated app now has a JDBC driver with no SQL data implementation and no
`dataSource` config (that config is only applied from the Hibernate features'
`apply`). Is that intended? If so, could you add a test locking the mongo +
SQL-driver output so this doesn't regress silently?
##########
grails-forge/grails-forge-core/src/main/java/org/grails/forge/options/GormImplTypeConverter.java:
##########
@@ -0,0 +1,48 @@
+/*
+ * 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 io.micronaut.core.convert.exceptions.ConversionErrorException;
+import io.micronaut.core.type.Argument;
+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) {
+ if (object == null) {
+ return Optional.empty();
+ }
+ GormImpl gormImpl = GormImpl.parse(object.toString());
+ if (gormImpl == null) {
+ throw new ConversionErrorException(Argument.of(GormImpl.class),
Review Comment:
Minor/optional: Micronaut's `TypeConverter` contract prefers
`context.reject(object, e); return Optional.empty();` over throwing. Throwing
`ConversionErrorException` happens to produce the behavior the tests lock in
(400 on `/create`, fallback-to-default on the bean-bound feature filter), but
it relies on which binder catches the exception. If `reject` + empty still
yields the 400 on `/create`, it would be the more robust form — fine as a
follow-up.
--
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]
