marta-jankovics commented on code in PR #3555:
URL: https://github.com/apache/fineract/pull/3555#discussion_r1392352138
##########
fineract-core/src/main/java/org/apache/fineract/commands/service/SynchronousCommandProcessingService.java:
##########
@@ -139,7 +139,8 @@ public CommandProcessingResult executeCommand(final
CommandWrapper wrapper, fina
commandSource.setCommandJson(toApiJsonSerializer.serializeResult(result.getChanges()));
}
- commandSource =
commandSourceService.saveResultSameTransaction(commandSource);
+ commandSource = sameTransaction ?
commandSourceService.saveResultSameTransaction(commandSource)
+ : commandSourceService.saveResultNewTransaction(commandSource);
Review Comment:
"When enclosingTransaction=true, if there is at least one unsuccessful
sub-request, then both: before and after my changes, no sub-request is saved in
the audit."
Exactly this is the problem to be fixed.
"Due to the fact that the saveInitialSameTransaction was called instead of
saveInitialNewTransaction, unsuccessful sub-requests were not logged to the
audit trail (we are talking about the case when enclosingTransaction=false when
calling BatchAPI, since this is exactly the case described in the task, as far
as I understand)."
The task is about both with- and without enclosing transaction use-cases so
we should fix both.
SaveInitial just saves a skeleton, that we cannot leave in the database as
such (maybe we can completely remove this step), but we should extend this -
for both failed and successful cases - with the error code or response and we
should make sure that this extension is also saved. Without this, saving the
skeleton is not a solution.
"a new problem arose: after executing saveInitialNewTransaction, the
database already contained a transaction with a unique key binding
(action_name, entity_name, idempotency_key) in table
"m_portfolio_command_source""
This cannot happen, only if you create new instance and try to save in new
transaction, which is a bug. That is why we check if the command already exists
for retry.
If someone called the same request with the same idempotency key, that
should fail and return the appropriate error code.
"You can't use saveInitialNewTransaction first and saveResultSameTransaction
below on the same entity".
Why not? Either every sub-requests are successful and then will be saved
with the same transaction, or when a sub-request fails, you should go and
update all the already ran sub requests as failed in the catch branch. With
your solution we might save sub-request commands as successful, although these
were rolled back with a later failed sub-request.
"I also tried to do as you described ("Тhe code that you should change is in
the catch (Throwable t) branch: instead of saveResultSameTransaction for
'sameTransaction' we should call 'saveResultNewTransaction' each time."). But
with this approach, unsuccessful sub-requests are not logged to the audit trail
:("
I guess the actual sub request was saved as failed, but the previous
sub-requests were not saved. This is because you should go back to the already
ran sub requests and mark them as failed in a new transaction. Even if we do
not save these earlier sub-requests, it is better than save them as success.
Maybe we should introduce an idempotency key for the batch itself and not
for the sub-requests.
This was not designed yet.
##########
fineract-core/src/main/java/org/apache/fineract/commands/service/SynchronousCommandProcessingService.java:
##########
@@ -139,7 +139,8 @@ public CommandProcessingResult executeCommand(final
CommandWrapper wrapper, fina
commandSource.setCommandJson(toApiJsonSerializer.serializeResult(result.getChanges()));
}
- commandSource =
commandSourceService.saveResultSameTransaction(commandSource);
+ commandSource = sameTransaction ?
commandSourceService.saveResultSameTransaction(commandSource)
+ : commandSourceService.saveResultNewTransaction(commandSource);
Review Comment:
"When enclosingTransaction=true, if there is at least one unsuccessful
sub-request, then both: before and after my changes, no sub-request is saved in
the audit."
Exactly this is the problem to be fixed.
"Due to the fact that the saveInitialSameTransaction was called instead of
saveInitialNewTransaction, unsuccessful sub-requests were not logged to the
audit trail (we are talking about the case when enclosingTransaction=false when
calling BatchAPI, since this is exactly the case described in the task, as far
as I understand)."
The task is about both with- and without enclosing transaction use-cases so
we should fix both.
SaveInitial just saves a skeleton, that we cannot leave in the database as
such (maybe we can completely remove this step), but we should extend this -
for both failed and successful cases - with the error code or response and we
should make sure that this extension is also saved. Without this, saving the
skeleton is not a solution.
"a new problem arose: after executing saveInitialNewTransaction, the
database already contained a transaction with a unique key binding
(action_name, entity_name, idempotency_key) in table
"m_portfolio_command_source""
This cannot happen, only if you create new instance and try to save in new
transaction, which is a bug. That is why we check if the command already exists
for retry.
If someone called the same request with the same idempotency key, that
should fail and return the appropriate error code.
"You can't use saveInitialNewTransaction first and saveResultSameTransaction
below on the same entity".
Why not? Either every sub-requests are successful and then will be saved
with the same transaction, or when a sub-request fails, you should go and
update all the already ran sub requests as failed in the catch branch. With
your solution we might save sub-request commands as successful, although these
were rolled back with a later failed sub-request.
"I also tried to do as you described ("Тhe code that you should change is in
the catch (Throwable t) branch: instead of saveResultSameTransaction for
'sameTransaction' we should call 'saveResultNewTransaction' each time."). But
with this approach, unsuccessful sub-requests are not logged to the audit trail
:("
I guess the actual sub request was saved as failed, but the previous
sub-requests were not saved. This is because you should go back to the already
ran sub requests and mark them as failed in a new transaction. Even if we do
not save these earlier sub-requests, it is better than save them as success.
Maybe we should introduce an idempotency key for the batch itself and not
for the sub-requests.
This was not designed yet.
--
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]
