getMaximumPropertyLength should degrade gracefully instead of throwing#6328
getMaximumPropertyLength should degrade gracefully instead of throwing#6328dkayiwa wants to merge 1 commit into
Conversation
HibernateAdministrationDAO.getMaximumPropertyLength threw an APIException when metadata.getEntityBinding(...) returned null. It looks the class up in a boot Metadata snapshot captured once at context init, so if that snapshot is ever consulted in an incomplete state the method throws. This is called from validate() on every save, and validate() already skips the length check when the value is negative (maxLength >= 0), and the method itself already returns -1 when it cannot determine a column's length. So the hard throw on a missing binding is an inconsistent outlier that can fail a save. It surfaced as an intermittent test failure: getMaximumPropertyLength_* errored with "Couldn't find a class in the hibernate configuration named: ..." in some full-suite orderings while passing in isolation. Return -1 (unknown length) instead of throwing, consistent with the existing sentinel and validate()'s guard, and add a regression test asserting an unmapped class yields -1 rather than an exception. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6328 +/- ##
============================================
- Coverage 59.47% 59.43% -0.04%
+ Complexity 9541 9538 -3
============================================
Files 731 731
Lines 38323 38325 +2
Branches 5587 5587
============================================
- Hits 22794 22780 -14
- Misses 13489 13516 +27
+ Partials 2040 2029 -11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| // every save, and validate() already skips the length check when it returns a negative value. | ||
| log.debug("Could not find a hibernate entity binding for {}; treating maximum length as unknown", | ||
| aClass.getName()); | ||
| return -1; |
There was a problem hiding this comment.
Since this changes what AdministrationService#getMaximumPropertyLength observably does for a class with no entity binding (it used to throw APIException, now it returns -1), it would be worth updating the @return javadoc on the service interface to document the sentinel, something like @return the max field length of a property, or -1 if it cannot be determined. Both in-core consumers already key off it (validate() checks maxLength >= 0 and ValidateUtil.validateFieldLengths checks length == -1), but a module author calling the service has no way to discover it without reading this DAO.
| // The boot Metadata is captured once at startup, so it can fail to resolve an entity if it is | ||
| // ever consulted in an incomplete state (for example after a session factory rebuild). Treat | ||
| // the length as unknown (-1) instead of throwing: this method is called from validate() on | ||
| // every save, and validate() already skips the length check when it returns a negative value. |
There was a problem hiding this comment.
Isn't this comment somewhat inaccurate? The metadata should be captured at session start time, but that's not the same as "boot" time (e.g., when a module refresh happens. I also don't think it adds much value that can't be gleaned from the log statement.
3 participants
Problem
HibernateAdministrationDAO.getMaximumPropertyLength(aClass, fieldName)throws anAPIExceptionwhenmetadata.getEntityBinding(aClass.getName())returns null. Thatmetadatais a boot-time HibernateMetadatasnapshot captured once insetApplicationContext, so if it is ever consulted in an incomplete state the method throws hard.This method is called by
validate()on everyOpenmrsObjectsave. Two things show the hard throw is an inconsistent outlier:validate()already guards every use withif (maxLength >= 0 && ...), i.e. a negative return means "length unknown, skip the check".-1when it cannot determine a column's length (thecatchblock).So a transient inability to resolve an entity should behave the same way,
-1, not throw and fail the save.How it surfaced
Intermittent CI failure:
HibernateAdministrationDAOTest.getMaximumPropertyLength_*errored withCouldn't find a class in the hibernate configuration named: org.openmrs.event.outbox.OutboxEventin some full-suite orderings, while passing in isolation. The throw is the sharp edge; the underlying "the boot Metadata snapshot sometimes can't resolve an annotation-scanned entity" is what varies by test order.Change
Return
-1(unknown length) instead of throwing when the entity binding cannot be resolved, consistent with the existing sentinel and withvalidate()'smaxLength >= 0guard. Adds a regression test asserting an unmapped class yields-1rather than an exception (it errors on the pre-change code with the sameAPIException, and passes after).Scope / what this does and doesn't do
This hardens the production
validate()path so a transient metadata gap can no longer fail a save, and removes the exception that was erroring the test. It does not, on its own, make assertions that require a specific entity's column lengths deterministic; a test that asserts exact lengths still needs that entity registered. Verified: the class passes (5/5) with the change and spotless is clean; the new test errors on the pre-change code and passes after.