jamesfredley commented on code in PR #15395:
URL: https://github.com/apache/grails-core/pull/15395#discussion_r2829299195
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/SaveImplementer.groovy:
##########
@@ -63,9 +65,18 @@ class SaveImplementer extends AbstractSaveImplementer
implements SingleResultSer
Parameter[] parameters = newMethodNode.parameters
int parameterCount = parameters.length
if (parameterCount == 1 && AstUtils.isDomainClass(parameters[0].type))
{
- body.addStatement(
- returnS(callX(varX(parameters[0]), 'save',
namedArgs(failOnError: ConstantExpression.TRUE)))
- )
+ Expression connectionId = findConnectionId(abstractMethodNode)
+ if (connectionId != null) {
+ // Route save through the instance API for the specified
connection
+ body.addStatement(
+ returnS(callX(buildInstanceApiLookup(domainClassNode,
connectionId), 'save', args(varX(parameters[0]), namedArgs(failOnError:
ConstantExpression.TRUE))))
+ )
+ }
+ else {
+ body.addStatement(
+ returnS(callX(varX(parameters[0]), 'save',
namedArgs(failOnError: ConstantExpression.TRUE)))
+ )
+ }
Review Comment:
This suggestion was incorrect and introduced the CI failure. The assumption
that `@Transactional(connection=...)` gets "copied onto the generated
implementation class" is wrong - the generated `$ServiceImplementation` class
does NOT carry the annotation.
`findConnectionId` resolves the annotation via
`TransactionalTransform.findTransactionalAnnotation(methodNode)`, which falls
back to `methodNode.getDeclaringClass()`. With `abstractMethodNode`, the
declaring class is the original interface/abstract class (which has the
annotation). With `newMethodNode`, the declaring class is the generated impl
class (which does not). So switching to `newMethodNode` caused
`findConnectionId` to return null, silently routing all operations to the
default datasource.
The fix is to keep using `abstractMethodNode` for the `findConnectionId`
lookup while using `newMethodNode` for everything else (error reporting, return
type checks, etc.).
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/DeleteImplementer.groovy:
##########
@@ -80,7 +81,15 @@ class DeleteImplementer extends
AbstractDetachedCriteriaServiceImplementor imple
void implementById(ClassNode domainClassNode, MethodNode
abstractMethodNode, MethodNode newMethodNode, ClassNode targetClassNode,
BlockStatement body, Expression byIdLookup) {
boolean isVoidReturnType =
ClassHelper.VOID_TYPE.equals(newMethodNode.returnType)
VariableExpression obj = varX('$obj')
- Statement deleteStatement = stmt(callX(obj, 'delete'))
+ Expression connectionId = findConnectionId(abstractMethodNode)
+ Statement deleteStatement
+ if (connectionId != null) {
+ // Route delete through the instance API for the specified
connection
+ deleteStatement =
stmt(callX(buildInstanceApiLookup(domainClassNode, connectionId), 'delete',
args(obj)))
+ }
+ else {
+ deleteStatement = stmt(callX(obj, 'delete'))
+ }
Review Comment:
Same issue as the SaveImplementer comment - this suggestion was incorrect.
`@Transactional(connection=...)` is not copied onto the generated
implementation class. Using `findConnectionId(newMethodNode)` here causes it to
return null because `newMethodNode.getDeclaringClass()` is the generated impl
class which has no such annotation. Reverted to
`findConnectionId(abstractMethodNode)`.
--
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]
