Repository navigation
fix: Throw the EntityManager's own exception from the kernel DAOs' proxy - EXO-91299 - #1344
Conversation
…oxy - EXO-91299 Symptom: a database error raised inside an EntityManager call of a kernel DAO (find, persist, merge, remove, refresh...) skipped every catch naming that exception. In video calls, such a failure while saving a call, such as reading the call or a participant before writing it, left the service as an UndeclaredThrowableException instead of the service's own call error (a StorageException naming the step, wrapped in an InvalidCallException when starting, joining, leaving, stopping or removing a call), and the code that tolerates a storage error, such as removing a call whose owner no longer exists while reading it, failed the whole read instead. A failure at commit was not affected: the commit does not go through the proxy. Cause: the EntityManagerHolder proxy invoked the thread's EntityManager by reflection without unwrapping the InvocationTargetException, which the JDK proxy then wrapped in an UndeclaredThrowableException: a catch of a JPA exception, an IllegalArgumentException or an IllegalStateException does not recognise it. Fix: the proxy rethrows the exception the EntityManager threw. Impact: every caller of a kernel DAO (a GenericDAOJPAImpl subclass, in every addon) now receives the exception an EntityManager method called on the proxy throws (find, persist, merge, remove, refresh, flush, getReference...); the execution of a query never went through the proxy and is unchanged. Only a catch narrower than Exception or RuntimeException behaves differently for an exception: - a catch of the EntityManager's exception, or of IllegalArgumentException or IllegalStateException, now fires. Hibernate has by then marked the transaction rollback-only for most of them (not for NoResultException, NonUniqueResultException and the lock and query timeouts): inside an outer @ExoTransactional method, such a catch that does not rethrow lets the aspect roll the transaction back without an error, where the failure used to reach the caller; - a catch of UndeclaredThrowableException around a kernel DAO call no longer matches, and an Error the EntityManager throws now passes a catch of Exception or RuntimeException, as an Error does anywhere else. A sweep of the direct callers in the org's repositories found the catches that now fire only in web-conferencing, none of them inside an outer transaction, and no catch of UndeclaredThrowableException around a kernel DAO call. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Self-review close-out — EXO-91299 (head
|
| # | Source | Sev | Finding | Status |
|---|---|---|---|---|
| 1 | delivery checks | 🟡 | Symptom claimed write-time failures, but those run at flush or commit, outside the proxy | ✅ Symptom scoped to failures inside a proxied EntityManager call |
| 2 | delivery checks | 🟢 | Cause said "a type no caller catches", but catch (Exception) callers do |
✅ reworded |
| 3 | delivery checks | 🟡 | Claimed a change in the error the user sees, but the CometD handlers answer the same server error either way | ✅ claim removed |
| 4 | delivery checks | 🟡 | "the logged failure did not say what failed": the CometD log names the call and the request | ✅ clause removed |
| 5 | delivery checks | 🟢 | Start/join/leave/stop/remove leave as InvalidCallException, not StorageException |
✅ Symptom names both |
| 6 | round 1 | 🟡 | Body silent on the blast radius of an N2 shared-component change | ✅ Impact paragraph (refute pass CONFIRMED the silent rollback on hibernate-core 7.4.5) |
| 7 | round 1 | 🟢 | Code comment overclaimed "no caller's catch recognises" | ✅ scoped (EntityManagerHolder.java:69-72) |
| 8 | round 1 | 🟡 | A corpus entry describing the old behaviour turns false once this merges | ✅ knowledge update, see the Knowledge: line |
| 9 | round 1 | 🟢 | Tribe task wording claimed a user-visible change | ✅ task description updated |
| — | round 2 (fresh) | — | — | CLEAN |
Verified conform: the handler rethrows InvocationTargetException#getTargetException() and keeps closing the EntityManager it opened when no transaction is active (EntityManagerHolder.java:67-79). EntityManagerHolderTest covers find(null), the persist of a detached entity, the refresh of a removed row, and the closing after a failed call outside a request. Mutation check: the old handler fails 4 of the 5 tests, and removing the close fails 1. A sweep of every kernel DAO caller's catches in the org's repositories found the catches that now fire only in web-conferencing, none inside an outer transaction, and no catch of UndeclaredThrowableException around a kernel DAO call.
Tests at ffe62c50f6: mvn -B -ntp clean verify javadoc:javadoc -Pcoverage, full reactor: BUILD SUCCESS, 1653 tests, 0 failures, 0 errors, coverage checks met in all 14 tested modules, javadoc OK. CI is not consulted (org billing).
Classification: N2 at ffe62c50f6, a shared persistence component with no socle trigger. Its approver must be an Archi/Dev who knows it is N2, not an approval on AI review alone.
|
ahamdi
left a comment
There was a problem hiding this comment.
AI review — Round #1 (final)
Reviewed at ffe62c50f6 (one commit on feature/maintenance @ 15e587bb1b), whole diff read. No finding: nothing to fix from the AI review side.
Verified conform
- The fix (
EntityManagerHolder.java:68-73):InvocationTargetException#getTargetException()is rethrown, and thefinallystill closes the EntityManager the proxy opened outside a request. EntityManager methods declare no checked exception, so nothing new can surface asUndeclaredThrowableException. - Scope claims in the body:
- The commit is not affected:
ExoTransactionalAspectcommits throughEntityManagerService, not the proxy. - Query execution is not affected: a
Queryreturned bycreateQueryis the engine's own, sogetResultList/getSingleResultnever go through the proxy. - The silent rollback the Impact paragraph describes is real: the aspect rolls a rollback-only transaction back without throwing.
- Hibernate 7.4.5
ExceptionConverterImplmarks rollback-only except forNoResultException,NonUniqueResultExceptionand the lock/query timeouts, as stated.
- The commit is not affected:
- The sweep claim, re-checked in web-conferencing (
feature/maintenance@2ab6d8c5,WebConferencingService): theIllegalArgumentException | IllegalStateException | PersistenceExceptioncatches (e.g.deleteCall,deleteAllUserCalls,updateCallAndParticipants) sit in non-transactional wrappers around the@ExoTransactionaltx*methods, never inside one. - Tests:
EntityManagerHolderTestdrives each case through a real EntityManager over the test database:find(null), persist of a detached entity, refresh of a removed row, and the close after a failed call outside a request. It follows its siblings'BaseTestCase/@Teststyle (SettingsDAOTest), and eachassertThrowswould receive anUndeclaredThrowableExceptionon the old handler. Knowledge:Meeds-io/eng-standards#560 is an open draft. It amendsa-kernel-dao-entitymanager-call-throws-its-jpa-exception-wrapped-in-undeclaredthrowableexceptionand adds the silent-rollback pitfall, as expected for code not yet ondevelop.
What the PR does well: a one-line root-cause fix at the shared seam, not catch-site workarounds in each addon. The Impact paragraph names the exact behaviour change, including the one that cuts the other way (a now-firing catch inside an @ExoTransactional that swallows the failure). The claim is backed by an org-wide sweep of the catch sites.
All findings are resolved (none were raised); nothing is outstanding from the AI review side.
Classification: this change is classified N2: a shared persistence component used by every addon's kernel DAOs, with no socle trigger. It must be acknowledged by an Architect/Developer who knows it is N2, not auto-merged on AI review alone.
🤖 Generated with Claude Code
…oxy - EXO-91299 (#1344) **Symptom**: a database error raised inside an EntityManager call of a kernel DAO (find, persist, merge, remove, refresh...) skipped every catch naming that exception. In video calls, such a failure while saving a call, such as reading the call or a participant before writing it, left the service as an UndeclaredThrowableException instead of the service's own call error (a StorageException naming the step, wrapped in an InvalidCallException when starting, joining, leaving, stopping or removing a call), and the code that tolerates a storage error, such as removing a call whose owner no longer exists while reading it, failed the whole read instead. A failure at commit was not affected: the commit does not go through the proxy. **Cause**: the `EntityManagerHolder` proxy invoked the thread's EntityManager by reflection without unwrapping the `InvocationTargetException`, which the JDK proxy then wrapped in an `UndeclaredThrowableException`: a catch of a JPA exception, an `IllegalArgumentException` or an `IllegalStateException` does not recognise it. **Fix**: the proxy rethrows the exception the EntityManager threw. `EntityManagerHolderTest` pins it on a real EntityManager: a find with a null id, a persist of a detached entity and a refresh of a removed row each reach the DAO's caller as the engine's own exception, and the EntityManager the proxy opened outside a request is closed after a failed call. **Impact**: every caller of a kernel DAO (a `GenericDAOJPAImpl` subclass, in every addon) now receives the exception an EntityManager method called on the proxy throws (find, persist, merge, remove, refresh, flush, getReference...); the execution of a query never went through the proxy and is unchanged. Only a catch narrower than `Exception` or `RuntimeException` behaves differently for an exception: - a catch of the EntityManager's exception, or of `IllegalArgumentException` or `IllegalStateException`, now fires. Hibernate has by then marked the transaction rollback-only for most of them (not for `NoResultException`, `NonUniqueResultException` and the lock and query timeouts): inside an outer `@ExoTransactional` method, such a catch that does not rethrow lets the aspect roll the transaction back without an error, where the failure used to reach the caller; - a catch of `UndeclaredThrowableException` around a kernel DAO call no longer matches, and an `Error` the EntityManager throws now passes a catch of `Exception` or `RuntimeException`, as an `Error` does anywhere else. A sweep of the direct callers in the org's repositories found the catches that now fire only in web-conferencing, none of them inside an outer transaction, and no catch of `UndeclaredThrowableException` around a kernel DAO call. exoplatform/web-conferencing#421 pins the video-call errors on this fix; its build passes once this fix is published. Knowledge: Meeds-io/eng-standards#560 This change is classified N2 (at `ffe62c50f6`) — its approver must be an Archi/Dev who knows it is N2, not an approval on AI review alone. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…XO-91299 (#421) **Symptom**: when the database failed inside an EntityManager call made while saving a video call, the failure left the service as an `UndeclaredThrowableException` instead of the service's own call error. That error depends on the operation. Starting, stopping, removing, joining or leaving a call reports an `InvalidCallException` wrapping the `StorageException` of the failed step, whether that step is the read of the call they start with or a later read, such as the participant row of a space member. Updating a call, adding a guest and updating the invitees report a failed read of the call the same way, and a later failure as a `StorageException` naming the step. Adding a participant and creating a call report a failed read of a participant, and updating the participants a failed read of the call, as a `StorageException` naming the step. Creating a call also checks that no call already has its id: a failed read there is logged as a warning and the creation goes on, as that check intends, where it failed the creation before. A failure at commit was already reported as a call storage error. **Cause**: the kernel DAOs' EntityManager proxy delivered the EntityManager's exception wrapped in an `UndeclaredThrowableException`, which none of the service's storage catches recognises. It is fixed in portal's `EntityManagerHolder`, Meeds-io/portal#1344. **Fix**: `WebConferencingServiceStorageErrorTest` runs each of those operations through the real DAOs, the EntityManager proxy and the `ExoTransactional` aspect. The container, the `EntityManagerService`, the EntityManager and its transaction are mocked. The service is a spy whose `userInfo`, `spaceInfo` and `getProvider` return a user (none for an external guest), a space or a provider without reading them, and whose `getCall`, in the guest and invitation tests, returns a call without reading it. Each failure but the commit's is raised on an EntityManager call that reaches the database within a request: the read of the call an operation starts with, unless the request has read the call before, the check that no call has the id of the one being created, the read of a participant the request has not loaded yet (such as the row of a member who joined a space after its call started), or the persist of an invitation that flushes. The test checks that each failure is reported as the operation's error, that the creation of a call goes on after a failed check, and that a failure at commit, when adding a participant, is still reported as a storage error. **Depends on** Meeds-io/portal#1344. `WebConferencingServiceStorageErrorTest` at `a0c905d1`: - with #1344's `component/common` classes on the test classpath: `Tests run: 14, Failures: 0, Errors: 0, Skipped: 0`; - with a `portal.component.common` 7.3.x-maintenance-SNAPSHOT built before #1344: `Tests run: 14, Failures: 12, Errors: 1, Skipped: 0`. Each of the 13 fails on the stubbed failure wrapped in an `UndeclaredThrowableException`; only the commit test passes, its failure being raised outside the proxy. So the build of this PR passes once #1344 is published, and it merges after it. Knowledge: Meeds-io/eng-standards#560 This change is classified N3 (at `a0c905d1`): no REST/DAO/schema/ACL surface touched, a test only (`classification.md` §3). 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…XO-91299 (#435) **Symptom**: when the database failed inside an EntityManager call made while saving a video call, the failure left the service as an `UndeclaredThrowableException` instead of the service's own call error. That error depends on the operation. Starting, stopping, removing, joining or leaving a call reports an `InvalidCallException` wrapping the `StorageException` of the failed step, whether that step is the read of the call they start with or a later read, such as the participant row of a space member. Updating a call, adding a guest and updating the invitees report a failed read of the call the same way, and a later failure as a `StorageException` naming the step. Adding a participant and creating a call report a failed read of a participant, and updating the participants a failed read of the call, as a `StorageException` naming the step. Creating a call also checks that no call already has its id: a failed read there is logged as a warning and the creation goes on, as that check intends, where it failed the creation before. A failure at commit was already reported as a call storage error. **Cause**: the kernel DAOs' EntityManager proxy delivered the EntityManager's exception wrapped in an `UndeclaredThrowableException`, which none of the service's storage catches recognises. It is fixed in portal's `EntityManagerHolder`, Meeds-io/portal#1344. **Fix**: `WebConferencingServiceStorageErrorTest` runs each of those operations through the real DAOs, the EntityManager proxy and the `ExoTransactional` aspect. The container, the `EntityManagerService`, the EntityManager and its transaction are mocked. The service is a spy whose `userInfo`, `spaceInfo` and `getProvider` return a user (none for an external guest), a space or a provider without reading them, and whose `getCall`, in the guest and invitation tests, returns a call without reading it. Each failure but the commit's is raised on an EntityManager call that reaches the database within a request: the read of the call an operation starts with, unless the request has read the call before, the check that no call has the id of the one being created, the read of a participant the request has not loaded yet (such as the row of a member who joined a space after its call started), or the persist of an invitation that flushes. The test checks that each failure is reported as the operation's error, that the creation of a call goes on after a failed check, and that a failure at commit, when adding a participant, is still reported as a storage error. **Depends on** Meeds-io/portal#1344. `WebConferencingServiceStorageErrorTest` at `a0c905d1`: - with #1344's `component/common` classes on the test classpath: `Tests run: 14, Failures: 0, Errors: 0, Skipped: 0`; - with a `portal.component.common` 7.3.x-maintenance-SNAPSHOT built before #1344: `Tests run: 14, Failures: 12, Errors: 1, Skipped: 0`. Each of the 13 fails on the stubbed failure wrapped in an `UndeclaredThrowableException`; only the commit test passes, its failure being raised outside the proxy. So the build of this PR passes once #1344 is published, and it merges after it. Knowledge: Meeds-io/eng-standards#560 This change is classified N3 (at `a0c905d1`): no REST/DAO/schema/ACL surface touched, a test only (`classification.md` §3). 🤖 Generated with [Claude Code](https://claude.com/claude-code)



Symptom: a database error raised inside an EntityManager call of a kernel DAO (find, persist, merge, remove, refresh...) skipped every catch naming that exception. In video calls, such a failure while saving a call, such as reading the call or a participant before writing it, left the service as an UndeclaredThrowableException instead of the service's own call error (a StorageException naming the step, wrapped in an InvalidCallException when starting, joining, leaving, stopping or removing a call), and the code that tolerates a storage error, such as removing a call whose owner no longer exists while reading it, failed the whole read instead. A failure at commit was not affected: the commit does not go through the proxy.
Cause: the
EntityManagerHolderproxy invoked the thread's EntityManager by reflection without unwrapping theInvocationTargetException, which the JDK proxy then wrapped in anUndeclaredThrowableException: a catch of a JPA exception, anIllegalArgumentExceptionor anIllegalStateExceptiondoes not recognise it.Fix: the proxy rethrows the exception the EntityManager threw.
EntityManagerHolderTestpins it on a real EntityManager: a find with a null id, a persist of a detached entity and a refresh of a removed row each reach the DAO's caller as the engine's own exception, and the EntityManager the proxy opened outside a request is closed after a failed call.Impact: every caller of a kernel DAO (a
GenericDAOJPAImplsubclass, in every addon) now receives the exception an EntityManager method called on the proxy throws (find, persist, merge, remove, refresh, flush, getReference...); the execution of a query never went through the proxy and is unchanged. Only a catch narrower thanExceptionorRuntimeExceptionbehaves differently for an exception:IllegalArgumentExceptionorIllegalStateException, now fires. Hibernate has by then marked the transaction rollback-only for most of them (not forNoResultException,NonUniqueResultExceptionand the lock and query timeouts): inside an outer@ExoTransactionalmethod, such a catch that does not rethrow lets the aspect roll the transaction back without an error, where the failure used to reach the caller;UndeclaredThrowableExceptionaround a kernel DAO call no longer matches, and anErrorthe EntityManager throws now passes a catch ofExceptionorRuntimeException, as anErrordoes anywhere else.A sweep of the direct callers in the org's repositories found the catches that now fire only in web-conferencing, none of them inside an outer transaction, and no catch of
UndeclaredThrowableExceptionaround a kernel DAO call. exoplatform/web-conferencing#421 pins the video-call errors on this fix; its build passes once this fix is published.Knowledge: Meeds-io/eng-standards#560
This change is classified N2 (at
ffe62c50f6) — its approver must be an Archi/Dev who knows it is N2, not an approval on AI review alone.🤖 Generated with Claude Code