marta-jankovics commented on code in PR #3555:
URL: https://github.com/apache/fineract/pull/3555#discussion_r1399309361
##########
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:
Hi Maria, appreciate your detailed description, here is my answer.
We have three different cases for command processing:
1. Independent API calls
There is no _BatchRequestContextHolder.enclosingTransaction_ set.
It is working as expected, both successful and failed commands are saved, no
issue here.
2. Batch request without enclosing transaction
_BatchRequestContextHolder. batchAttributes_ is set – so we know that we are
inside a batch.
_BatchRequestContextHolder.enclosingTransaction_ was set for each
sub-request separately to a new transaction (which was different for each
sub-request).
You changed this to keep _BatchRequestContextHolder.enclosingTransaction_
empty , which might be ok, I just asked one thing to check:
**When the parent request fails then all the referencing child requests
should also fail, in method parentRequestFailedRecursive we should mark the
child transactions to be rolled back. Is this working fine after your change?**
Another minor change: **there is already a parameter, called boolean
enclosingTransaction. Please name your parameter the same way**.
After your change, this will behave the same way as the independent
transactions, see point 1., so **I do not think, that we need to change
anything** in _SynchronousCommandProcessingService_.
3. Batch request with enclosing transaction
_BatchRequestContextHolder. batchAttributes_ is set – so we know that we are
inside a batch.
_BatchRequestContextHolder.enclosingTransaction_ is set to one common
transaction for all the sub-requests.
**Actually, the story was created to solve the problem of this use-case,
because the failed requests here are not saved at all, and it is not correct
from audit perspective.**
With your change, you eliminated the problem for the previous batch type by
changing the _enclosingTransaction_ state, but the problem itself was not
solved.
The solution needs to be designed first and shared with the community,
before we change anything.
Probably the best solution would be to add an idempotency key to the batch
request itself, and save only one command (with the whole request body as
command json), and nothing for the sub-requests.
Please note that there are some changes coming for both
_SynchronousCommandProcessingService_ and _BatchApiServiceImpl_ with this PR:
https://github.com/apache/fineract/pull/3589
##########
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:
Hi Maria, appreciate your detailed description, here is my answer.
We have three different cases for command processing:
1. Independent API calls
There is no _BatchRequestContextHolder.enclosingTransaction_ set.
It is working as expected, both successful and failed commands are saved, no
issue here.
2. Batch request without enclosing transaction
_BatchRequestContextHolder. batchAttributes_ is set – so we know that we are
inside a batch.
_BatchRequestContextHolder.enclosingTransaction_ was set for each
sub-request separately to a new transaction (which was different for each
sub-request).
You changed this to keep _BatchRequestContextHolder.enclosingTransaction_
empty , which might be ok, I just asked one thing to check:
**When the parent request fails then all the referencing child requests
should also fail, in method parentRequestFailedRecursive we should mark the
child transactions to be rolled back. Is this working fine after your change?**
Another minor change: **there is already a parameter, called boolean
enclosingTransaction. Please name your parameter the same way**.
After your change, this will behave the same way as the independent
transactions, see point 1., so **I do not think, that we need to change
anything** in _SynchronousCommandProcessingService_.
3. Batch request with enclosing transaction
_BatchRequestContextHolder. batchAttributes_ is set – so we know that we are
inside a batch.
_BatchRequestContextHolder.enclosingTransaction_ is set to one common
transaction for all the sub-requests.
**Actually, the story was created to solve the problem of this use-case,
because the failed requests here are not saved at all, and it is not correct
from audit perspective.**
With your change, you eliminated the problem for the previous batch type by
changing the _enclosingTransaction_ state, but the problem itself was not
solved.
The solution needs to be designed first and shared with the community,
before we change anything.
Probably the best solution would be to add an idempotency key to the batch
request itself, and save only one command (with the whole request body as
command json), and nothing for the sub-requests.
Please note that there are some changes coming for both
_SynchronousCommandProcessingService_ and _BatchApiServiceImpl_ with this PR:
https://github.com/apache/fineract/pull/3589
--
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]
