Rémy,
On 7/10/26 11:45 AM, Rémy Maucherat wrote:
On Fri, Jul 10, 2026 at 5:13 PM Mark Thomas <[email protected]> wrote:
On 10/07/2026 13:17, [email protected] wrote:
This is an automated email from the ASF dual-hosted git repository.
rmaucher pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new 8120a12a0d Fixes from code review
8120a12a0d is described below
commit 8120a12a0dbbaafc104fa0d61a319d480b7ff92c
Author: remm <[email protected]>
AuthorDate: Fri Jul 10 14:17:44 2026 +0200
Fixes from code review
Take advantage of rollback to improve save reliability.
Clear out transient memory changes only if save is successful,
Line 1253: There is no rollback if the commit fails
I don't see the point. What should I do then if rollback fails. And so
on. LLMs often go on a "what if it fails" loop and there's no good
solution.
There is missing rollback for all kinds of things. Any unchecked
exception, for example. I know that was already a problem, but it should
probably be corrected.
Also... saveInternal() creates an exception object as a return value?
WTH? Why not throw an exception on error like normal Java code?
Line 1255-67: Clearing out of transient changes can still fail. Better
to wrapp all the changes in a single transaction that either suceeds or
fails.
The idea is to clear out the transient changes if the commit fails.
Since there's only one connection to a datasource, I don't see the
point of using a transaction, I either commit at the end or rollback.
Lines 1288-1717: Will any of these excetions be thrown no everything is
in a transaction? Assuming the code is correct, I suspect not. This code
could be a lot cleaner.
It looks ok to me. The code review indicated that all the errors were
logged and everything was going on as is. So now all the exceptions
are aggregated and presumably there's a rollback at the end. It's also
possible to try to exit early (simply return a single exception ?).
Line 1731: Some of these fields were optional (and are still marked as
such in the documentation). That migth be a breaking change for some odd
configurations. No objections to the change if a) the breaking change is
documented in the migration guide and b) the docs are updated
Ok, I see userRoleTable and roleNameCol should be semi optional (so
users without roles). I will make some adjustments.
-chri
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
