Copilot commented on code in PR #15774:
URL: https://github.com/apache/grails-core/pull/15774#discussion_r3484192678
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/HibernateSession.java:
##########
@@ -386,21 +440,43 @@ public long updateAll(final QueryableCriteria criteria,
final Map<String, Object
});
}
- @Override
- @SuppressWarnings({"PMD.DataflowAnomalyAnalysis"})
- //TODO Cleanup
+ private static Object coerceId(Object key, Class<?> idType) {
+ if (key == null || idType == null || idType.isInstance(key)) {
+ return key;
+ }
+ if (key instanceof String s) {
+ if (idType == Long.class) return Long.parseLong(s);
+ if (idType == Integer.class) return Integer.parseInt(s);
+ }
+ return key;
+ }
+
public List retrieveAll(final Class type, final Iterable keys) {
final GrailsHibernatePersistentEntity persistentEntity =
(GrailsHibernatePersistentEntity)
getMappingContext().getPersistentEntity(type.getName());
final String entityName = persistentEntity.getName();
final String idName = persistentEntity.getIdentity().getName();
+ final Class<?> idType = persistentEntity.getIdentity().getType();
+
+ // Collect input keys preserving order, coercing to the entity's ID
type
+ List<Object> inputKeys = new ArrayList<>();
+ for (Object k : keys) {
+ inputKeys.add(coerceId(k, idType));
+ }
+ if (inputKeys.isEmpty()) {
+ return Collections.emptyList();
+ }
+ // Determine the unique set of keys for the HQL IN query
+ Collection<Object> uniqueKeys = new LinkedHashMap<Object, Object>() {{
+ for (Object k : inputKeys) { put(k, k); }
+ }}.keySet();
Review Comment:
This uses double-brace initialization to build a LinkedHashMap just to
obtain an ordered unique key set. Double-brace init creates an extra anonymous
class and can retain outer references unnecessarily; a LinkedHashSet achieves
the same result more cheaply and clearly.
##########
grails-test-examples/hibernate7/grails-database-per-tenant/grails-app/conf/logback.xml:
##########
@@ -31,7 +31,11 @@
</encoder>
</appender>
- <root level="error">
+ <logger name="org.grails.orm.hibernate" level="debug" />
+ <logger name="grails.gorm.transactions" level="debug" />
+ <logger name="org.springframework.transaction" level="debug" />
+
+ <root level="debug">
<appender-ref ref="STDOUT" />
Review Comment:
Setting the root logger to DEBUG in this example app will produce extremely
verbose output (including Spring transaction and Hibernate debug logs) and can
make integration-test runs noisy and slower by default. Prefer keeping the root
level at ERROR/INFO and enabling DEBUG only for specific loggers when actively
debugging.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/HibernateSession.java:
##########
@@ -103,13 +116,16 @@ public Serializable insert(Object o) {
}
@Override
- public boolean isConnected() {
- return connected;
+ public void disconnect() {
+ connected = false;
+ if (nativeSession != null && nativeSession.isOpen()) {
+ nativeSession.close();
+ }
Review Comment:
HibernateCriteriaBuilder.closeSession() always calls session.disconnect(),
even when it is participating in an existing transaction. Closing the
underlying nativeSession here can inadvertently close a thread-bound
transactional session and break the outer transaction/session lifecycle.
disconnect() should mark the wrapper disconnected but leave native session
lifecycle management to Spring/Hibernate (or only close sessions it explicitly
opened).
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/HibernateSession.java:
##########
@@ -386,21 +440,43 @@ public long updateAll(final QueryableCriteria criteria,
final Map<String, Object
});
}
- @Override
- @SuppressWarnings({"PMD.DataflowAnomalyAnalysis"})
- //TODO Cleanup
+ private static Object coerceId(Object key, Class<?> idType) {
+ if (key == null || idType == null || idType.isInstance(key)) {
+ return key;
+ }
+ if (key instanceof String s) {
+ if (idType == Long.class) return Long.parseLong(s);
+ if (idType == Integer.class) return Integer.parseInt(s);
+ }
+ return key;
+ }
Review Comment:
coerceId() is intended to coerce String IDs to the entity's declared
identifier type, but it currently only handles Long and Integer. For entities
with other identifier types (e.g. UUID), String keys will remain unconverted
and lookups via retrieveAll() will still miss. Consider using the
MappingContext ConversionService here (similar to retrieve()) so coercion
matches the declared identity type generically.
--
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]
