Re: (tomcat) branch main updated: Fixes from code review

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