jdaugherty commented on code in PR #16572:
URL: https://github.com/apache/grails-core/pull/16572#discussion_r4361364345
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/HibernateSession.groovy:
##########
@@ -76,8 +67,10 @@
* @author Graeme Rocher
* @since 1.0
*/
-@SuppressWarnings({"rawtypes", "PMD.DataflowAnomalyAnalysis",
"PMD.AvoidDuplicateLiterals"})
-public class HibernateSession extends AbstractAttributeStoringSession
implements QueryAliasAwareSession {
+// TODO(groovyism-cleanup): semicolons + double-quoted strings kept to
preserve rename-similarity against the original .java (see matrei's review on
#16576). Removed by the stacked groovyism-cleanup PR.
Review Comment:
This comment, and the `@SuppressWarnings('UnnecessarySemicolon')` on the
package statement above, are in 93 files, and the reason they give is gone: the
move commit preserves the history now, not the similarity of this diff. What is
left is two styles across the 324 converted files, chosen by a git heuristic:
136 files keep 5,431 lines ending in a semicolon (1,423 imports and 119 package
statements among them), 47 class-level suppressions hide `UnnecessaryGString`
over 607 double-quoted strings without interpolation, and 188 files are already
clean. Please strip the semicolons and the plain double quotes in the convert
commit and delete the suppressions and these comments, so CodeNarc enforces it
from here. The semicolons are a `sed -E 's/;[[:space:]]*$//'` over the
converted files (or the semicolon step of the `codenarcFix` task; its GString
step also rewrites quotes inside comments and the license headers, so that
output can't be taken as is). `codenarcTest` is skipped in this b
uild, so the 43 test and fixture files in `dbmigration-core` that have
semicolons and no suppression need the same pass by hand.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/util/OrderByClauseBuilder.groovy:
##########
@@ -16,110 +16,120 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.grails.orm.hibernate.cfg.domainbinding.util;
+package org.grails.orm.hibernate.cfg.domainbinding.util
-import java.util.ArrayList;
-import java.util.List;
-import java.util.Locale;
-import java.util.stream.Collectors;
+import groovy.transform.CompileStatic
+import groovy.transform.PackageScope
+import org.hibernate.boot.model.internal.BinderHelper
+import org.hibernate.mapping.PersistentClass
+import org.hibernate.mapping.Property
+import org.hibernate.mapping.SingleTableSubclass
+import org.hibernate.mapping.Selectable
-import org.hibernate.boot.model.internal.BinderHelper;
-import org.hibernate.mapping.PersistentClass;
-import org.hibernate.mapping.Property;
-import org.hibernate.mapping.SingleTableSubclass;
-
-import org.grails.datastore.mapping.model.DatastoreConfigurationException;
+import org.grails.datastore.mapping.model.DatastoreConfigurationException
/** Utility class to build SQL order by clauses from HQL-style order by
strings. */
-@SuppressWarnings("PMD.DataflowAnomalyAnalysis")
-public class OrderByClauseBuilder {
+@CompileStatic
+@SuppressWarnings('PMD.DataflowAnomalyAnalysis')
+class OrderByClauseBuilder {
- public String buildOrderByClause(
+ String buildOrderByClause(
String hqlOrderBy, PersistentClass associatedClass, String role,
String defaultOrder) {
if (hqlOrderBy == null) {
- return null;
+ return null
}
if (hqlOrderBy.isEmpty()) {
- return associatedClass.getIdentifier().getSelectables().stream()
- .map(selectable -> selectable.getText() + " asc")
- .collect(Collectors.joining(", "));
+ List<String> parts = []
+ for (Selectable selectable :
associatedClass.identifier.selectables) {
+ parts.add(selectable.text + ' asc')
+ }
+ return parts.join(', ')
}
- List<SortEntry> entries = parseSortEntries(hqlOrderBy, role,
defaultOrder);
+ List<SortEntry> entries = parseSortEntries(hqlOrderBy, role,
defaultOrder)
- return entries.stream()
- .map(entry -> buildPropertyOrderBy(entry, associatedClass))
- .collect(Collectors.joining(", "));
+ List<String> parts = []
+ for (SortEntry entry : entries) {
+ parts.add(buildPropertyOrderBy(entry, associatedClass))
+ }
+ return parts.join(', ')
}
private List<SortEntry> parseSortEntries(String hqlOrderBy, String role,
String defaultOrder) {
- String[] tokens = hqlOrderBy.split("[ ,]+");
- List<SortEntry> entries = new ArrayList<>();
- SortEntry currentEntry = null;
+ String[] tokens = hqlOrderBy.split('[ ,]+')
+ List<SortEntry> entries = new ArrayList<>()
+ SortEntry currentEntry = null
for (String token : tokens) {
- if (token.isEmpty()) continue;
+ if (token.isEmpty()) {
+ continue
+ }
if (isDirectionToken(token)) {
if (currentEntry == null || currentEntry.direction != null) {
throw new DatastoreConfigurationException(
- "Error while parsing sort clause: " + hqlOrderBy +
" (" + role + ")");
+ "Error while parsing sort clause: ${hqlOrderBy}
(${role})".toString())
}
- currentEntry.direction = token.toLowerCase(Locale.ROOT);
- } else {
+ currentEntry.direction = token.toLowerCase(Locale.ROOT)
Review Comment:
`javap` shows this write, and the two below it, compile to
`ScriptBytecodeAdapter.setGroovyObjectProperty`: `direction` is a
`@PackageScope` field of a private nested class, written from the outer class.
It isn't in the description's list of remaining dynamic sites.
`currentEntry.@direction = …` (or a setter on `SortEntry`) makes it a field
write.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/PersistentEntityNamingStrategy.groovy:
##########
@@ -16,26 +16,27 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.grails.orm.hibernate.cfg;
+package org.grails.orm.hibernate.cfg
-import
org.grails.orm.hibernate.cfg.domainbinding.hibernate.GrailsHibernatePersistentEntity;
-import
org.grails.orm.hibernate.cfg.domainbinding.hibernate.HibernatePersistentProperty;
+import
org.grails.orm.hibernate.cfg.domainbinding.hibernate.GrailsHibernatePersistentEntity
+import
org.grails.orm.hibernate.cfg.domainbinding.hibernate.HibernatePersistentProperty
/**
* Allows plugging into to custom naming strategies
*
* @author Graeme Rocher
* @since 5.0
*/
-public interface PersistentEntityNamingStrategy {
+interface PersistentEntityNamingStrategy {
Review Comment:
Default methods are code, so this interface needs `@CompileStatic` like
every other converted type; without it their bodies compile dynamically. `grep
-L '@CompileStatic'` over the moved files finds this one plus 14 converted
tests and fixtures in `dbmigration-core` that have none either:
`ColumnRemarksIntegrationTest`, `HibernateIndexDifferenceSuppressionTest`,
`CustomIdGeneratorTest`, `IdentifierGeneratorSnapshotTest`,
`NativeGeneratorAutoIncrementTest`, `NativeGeneratorSequenceTest`,
`SequenceGeneratorTest`, `columnremarks/Item`, `CustomIdEntity`,
`NativeGenEntity`, `SnowflakeId`, `SnowflakeIdGenerator`, `ForcedTableEntity`,
`QualifiedSequenceEntity` (the batch #16497 added; the older fixtures have it).
The remaining hits are pure interfaces and `ManagedEntity`, which need nothing.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/HqlQueryContext.groovy:
##########
@@ -52,83 +43,102 @@
*
* <p>Use {@link #prepare} to build an instance from raw inputs.
*/
-@SuppressWarnings({
+// TODO(groovyism-cleanup): semicolons + double-quoted strings kept to
preserve rename-similarity against the original .java (see matrei's review on
#16576). Removed by the stacked groovyism-cleanup PR.
+@CompileStatic
+@SuppressWarnings([
"PMD.AvoidDuplicateLiterals",
"PMD.DataflowAnomalyAnalysis",
"PMD.AvoidLiteralsInIfCondition",
- "PMD.UseLocaleWithCaseConversions"
-})
-public record HqlQueryContext(
+ 'PMD.UseLocaleWithCaseConversions',
+ 'ClassStartsWithBlankLine',
+ 'Indentation',
+ 'UnnecessarySemicolon',
+ 'UnnecessaryGString'
+])
+@RecordOptions(getAt = false, toList = false, toMap = false, size = false)
+@TupleConstructor(defaults = false)
Review Comment:
The public `Map` constructor is still generated: `javap` shows
`HqlQueryContext(java.util.Map)`, and those constructors are the only place in
the converted classes where `InvokerHelper.createAssertError` appears (9 sites
here, 10 in `HibernateQueryExecutor`, 4 each in `HibernateAlias` and
`PropertyArithmetic`). `@TupleConstructor(defaults = false, namedVariant =
false)` removes it.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/GrailsHibernateTemplate.groovy:
##########
@@ -134,111 +127,113 @@ public class GrailsHibernateTemplate implements
IHibernateTemplate {
*
* @see #setFlushMode
*/
- public static final int FLUSH_ALWAYS = 4;
-
- private static final Logger LOG =
LoggerFactory.getLogger(GrailsHibernateTemplate.class);
- protected boolean exposeNativeSession = true;
- protected boolean cacheQueries = false;
- protected SessionFactory sessionFactory;
- protected DataSource dataSource = null;
- protected SQLExceptionTranslator jdbcExceptionTranslator;
- protected int flushMode = FLUSH_AUTO;
- private boolean osivReadOnly;
- private boolean passReadOnlyToHibernate = false;
- private boolean applyFlushModeOnlyToNonExistingTransactions = false;
- protected TransactionResources txResources = new
DefaultTransactionResources();
+ public static final int FLUSH_ALWAYS = 4
+
+ private static final Logger LOG =
LoggerFactory.getLogger(GrailsHibernateTemplate)
+ protected boolean exposeNativeSession = true
+ protected boolean cacheQueries = false
+ protected SessionFactory sessionFactory
+ protected DataSource dataSource = null
+ protected SQLExceptionTranslator jdbcExceptionTranslator
+ protected int flushMode = FLUSH_AUTO
+ private boolean osivReadOnly
+ private boolean passReadOnlyToHibernate = false
+ private boolean applyFlushModeOnlyToNonExistingTransactions = false
+ protected TransactionResources txResources = new
DefaultTransactionResources()
protected GrailsHibernateTemplate() {
// for testing
}
- public GrailsHibernateTemplate(SessionFactory sessionFactory) {
- Assert.notNull(sessionFactory, "Property 'sessionFactory' is
required");
- this.sessionFactory = sessionFactory;
+ GrailsHibernateTemplate(SessionFactory sessionFactory) {
+ Assert.notNull(sessionFactory, "Property 'sessionFactory' is required")
+ this.sessionFactory = sessionFactory
ConnectionProvider connectionProvider = ((SessionFactoryImplementor)
sessionFactory)
- .getServiceRegistry()
- .getService(ConnectionProvider.class);
- this.dataSource = connectionProvider != null ?
connectionProvider.unwrap(DataSource.class) : null;
+ .serviceRegistry
+ .getService(ConnectionProvider)
+ this.dataSource = connectionProvider != null ?
connectionProvider.unwrap(DataSource) : null
if (this.dataSource != null) {
if (this.dataSource instanceof TransactionAwareDataSourceProxy) {
- DataSource target = ((TransactionAwareDataSourceProxy)
this.dataSource).getTargetDataSource();
+ DataSource target = ((TransactionAwareDataSourceProxy)
this.dataSource).targetDataSource
if (target != null) {
- this.dataSource = target;
+ this.dataSource = target
}
}
- jdbcExceptionTranslator = new
SQLErrorCodeSQLExceptionTranslator(this.dataSource);
+ jdbcExceptionTranslator = new
SQLErrorCodeSQLExceptionTranslator(this.dataSource)
} else {
// must be in unit test mode, setup default translator
SQLErrorCodeSQLExceptionTranslator
sqlErrorCodeSQLExceptionTranslator =
- new SQLErrorCodeSQLExceptionTranslator();
- sqlErrorCodeSQLExceptionTranslator.setDatabaseProductName("H2");
- jdbcExceptionTranslator = sqlErrorCodeSQLExceptionTranslator;
+ new SQLErrorCodeSQLExceptionTranslator()
+ sqlErrorCodeSQLExceptionTranslator.databaseProductName = 'H2'
+ jdbcExceptionTranslator = sqlErrorCodeSQLExceptionTranslator
}
}
- public GrailsHibernateTemplate(SessionFactory sessionFactory,
HibernateDatastore datastore) {
- this(sessionFactory);
+ GrailsHibernateTemplate(SessionFactory sessionFactory, HibernateDatastore
datastore) {
+ this(sessionFactory)
if (datastore != null) {
- cacheQueries = datastore.isCacheQueries();
- this.osivReadOnly = datastore.isOsivReadOnly();
- this.passReadOnlyToHibernate =
datastore.isPassReadOnlyToHibernate();
- this.flushMode =
hibernateFlushModeToConstant(datastore.getDefaultFlushMode());
+ cacheQueries = datastore.isCacheQueries()
+ this.osivReadOnly = datastore.isOsivReadOnly()
+ this.passReadOnlyToHibernate =
datastore.isPassReadOnlyToHibernate()
+ this.flushMode =
hibernateFlushModeToConstant(datastore.defaultFlushMode)
}
}
- public GrailsHibernateTemplate(SessionFactory sessionFactory,
HibernateDatastore datastore, int defaultFlushMode) {
- this(sessionFactory);
+ GrailsHibernateTemplate(SessionFactory sessionFactory, HibernateDatastore
datastore, int defaultFlushMode) {
+ this(sessionFactory)
if (datastore != null) {
- cacheQueries = datastore.isCacheQueries();
- this.osivReadOnly = datastore.isOsivReadOnly();
- this.passReadOnlyToHibernate =
datastore.isPassReadOnlyToHibernate();
+ cacheQueries = datastore.isCacheQueries()
+ this.osivReadOnly = datastore.isOsivReadOnly()
+ this.passReadOnlyToHibernate =
datastore.isPassReadOnlyToHibernate()
}
- this.flushMode = defaultFlushMode;
+ this.flushMode = defaultFlushMode
}
/** Maps a Hibernate {@link FlushMode} to one of the {@code FLUSH_*}
constants of this class. */
- static int hibernateFlushModeToConstant(FlushMode mode) {
- return switch (mode) {
- case MANUAL -> FLUSH_NEVER;
- case COMMIT -> FLUSH_COMMIT;
- case ALWAYS -> FLUSH_ALWAYS;
- default -> FLUSH_AUTO;
- };
+ private static int hibernateFlushModeToConstant(FlushMode mode) {
Review Comment:
`private` here, package-private in Java, and `GrailsHibernateTemplateSpec`
still calls it; that only works because the spec is dynamic Groovy.
`@PackageScope` keeps the Java visibility and keeps the spec on a real API.
##########
grails-data-hibernate7/core/src/main/groovy/grails/orm/HibernateCriteriaBuilder.groovy:
##########
@@ -1219,37 +1215,37 @@ public ProjectionList sqlGroupProjection(String sql,
String groupBy, List<String
*/
private <T> T closingSessionOnError(Supplier<T> projections) {
try {
- return projections.get();
+ return projections.get()
} catch (IllegalArgumentException e) {
- closeSessionFollowingException();
- throw e;
+ closeSessionFollowingException()
+ throw e
}
}
/**
* Restricts the results with a native SQL condition. {@code {alias}} in
the SQL stands for the table alias of
* the queried entity.
*
- * @param sqlRestriction the SQL condition
+ * @param sql the SQL condition
* @return this criteria
*/
- public org.grails.datastore.mapping.query.api.Criteria
sqlRestriction(String sqlRestriction) {
- return sqlRestriction(sqlRestriction, Collections.emptyList());
+ org.grails.datastore.mapping.query.api.Criteria sqlRestriction(String sql)
{
Review Comment:
The parameter was `sqlRestriction` in Java (here, in the two-argument
overload and in `HibernateDetachedCriteriaExtension`), and the Javadoc `@param`
moved with it. Callers don't notice, but the conversion shouldn't rename public
parameters; please keep the Java names.
##########
grails-data-hibernate7/dbmigration-core/src/test/groovy/liquibase/ext/hibernate/database/HibernateDatabasePojoTest.groovy:
##########
@@ -0,0 +1,22 @@
+package liquibase.ext.hibernate.database
+
+import org.junit.Test
+
+import static org.junit.Assert.assertFalse
+
+/**
+ * {@link HibernateDatabase} has a public {@code getProperty(String)} that
returns a connection property.
+ * A Groovy class would take it for the {@code GroovyObject} hook behind every
dynamic property access,
+ * so the whole family is compiled without {@code GroovyObject}.
+ */
+class HibernateDatabasePojoTest {
Review Comment:
New Grails test, not fork code, so it needs the ASF header. RAT doesn't see
it because `database/**` is excluded wholesale.
##########
grails-data-hibernate7/dbmigration-core/src/test/groovy/liquibase/ext/hibernate/HibernateIntegrationTest.groovy:
##########
@@ -1,81 +1,79 @@
-package liquibase.ext.hibernate;
+package liquibase.ext.hibernate
-import liquibase.Liquibase;
-import liquibase.Scope;
-import liquibase.database.Database;
-import liquibase.database.core.H2Database;
-import liquibase.database.jvm.JdbcConnection;
-import liquibase.diff.DiffResult;
-import liquibase.diff.Difference;
-import liquibase.diff.ObjectDifferences;
-import liquibase.diff.compare.CompareControl;
-import liquibase.diff.output.DiffOutputControl;
-import liquibase.diff.output.changelog.DiffToChangeLog;
-import liquibase.diff.output.report.DiffToReport;
-import liquibase.ext.hibernate.database.HibernateClassicDatabase;
-import liquibase.ext.hibernate.database.connection.HibernateConnection;
-import liquibase.resource.ClassLoaderResourceAccessor;
-import liquibase.resource.DirectoryResourceAccessor;
-import liquibase.structure.DatabaseObject;
-import liquibase.structure.core.*;
-import org.junit.After;
-import org.junit.Before;
-import org.junit.Test;
+import java.io.*
Review Comment:
Wildcard imports, here and in `SpringPackageScanningIntegrationTest`; the
repository rule is explicit imports, fork test or not.
##########
grails-doc/src/en/guide/upgrading/upgrading80x.adoc:
##########
@@ -1449,6 +1449,12 @@ class SportsCar extends Car {
}
----
+===== 26.11 Custom User Types
Review Comment:
This section is #16695's (see the comment on `CompositeUserTypeBinder`); it
goes with the rest of that PR.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/SqlRestriction.groovy:
##########
@@ -34,100 +33,106 @@
* @param values the values of the {@code ?} placeholders, in order
* @since 8.0.0
*/
-public record SqlRestriction(String sql, List<?> values) implements
Query.Criterion {
+@CompileStatic
+record SqlRestriction(String sql, List<?> values) implements Query.Criterion {
Review Comment:
Public API (`@since 8.0.0`) and still without the record options the other
records got: `SqlRestriction()`, `SqlRestriction(String)`,
`SqlRestriction(Map)`, `getAt`, `toList`, `toMap` and `size` are all generated.
Same for `GroovyProxyInterceptorLogic.InterceptorState`.
--
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]
