Fixes #31568: read logical-suite revisions back through the DAO that wrote them - #31569
Conversation
prepareLogicalSuiteRelationshipChange writes the test case and test suite revision rows through the repository's own daoCollection, then reads them back through the static helpers, which resolve Entity.getCollectionDAO() instead. That global is mutable: OpenMetadataApplication and the integration-test bootstrap each set it to an on-demand DAO over their own Jdbi, while a repository captures daoCollection once at construction. The two can therefore name different instances, and the write lands on one connection while the read-back runs on another. MySQL pins a REPEATABLE READ snapshot at the transaction's first read, so it cannot see the other connection's later commit at all and the rows just written come back missing -- reported as "Failed to persist every test suite relationship revision" and "Failed to persist the test suite tests revision". Postgres re-snapshots per statement, which is why only the MySQL lanes ever failed. Give both helpers an overload that takes the caller's DAO and use it from the write path, so the read-back always shares the writer's connection. The four call sites that read outside a transaction -- reindexing, the retry worker and the two index builders -- keep resolving the global. Also report the counts and the suite id in the two messages: as written they named a cause that was never checked, which sent the investigation at the persistence layer rather than at which connection was being read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
✅ PR checks passedThe linked issue has a description and all required Shipping project fields set. Thanks! |
github-actions
Bot
added
backend
safe to test
There was a problem hiding this comment.
Pull request overview
This PR fixes intermittent MySQL failures when adding test cases to a logical test suite by ensuring “write then read-back” of revision rows happens through the same CollectionDAO (and therefore the same Jdbi/connection scope) that performed the write, rather than via the mutable global Entity.getCollectionDAO().
Changes:
- Added
CollectionDAO-accepting overloads for revision read helpers inTestCaseRepositoryandTestSuiteRepository, and updated the transactional write-path to use them. - Improved failure messages to report what was actually observed (counts + suite id) instead of asserting an unverified “failed to persist” cause.
- Added unit tests that pin
Entity.getCollectionDAO()to a different DAO and assert the caller DAO is used (and the global DAO is never touched).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java | Uses caller daoCollection for revision read-back and improves related error messages; adds DAO overload for revision reads. |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestSuiteRepository.java | Adds DAO overload for reading tests relationship revisions while keeping the existing global-resolving overload for non-transactional callers. |
| openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/TestCaseRepositoryTest.java | Adds a unit test asserting revision reads use the caller DAO and do not interact with the global DAO. |
| openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/TestSuiteRepositoryTest.java | Adds a unit test asserting revision reads use the caller DAO and do not interact with the global DAO. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
✅ Playwright Results — workflow succeededValidated commit ✅ 771 passed · ❌ 0 failed · 🟡 1 flaky · ⏭️ 0 skipped · 🧰 0 lifecycle flaky PerformanceBlocking targets: ✅ met · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 48m 5s ⏱️ Max setup 3m 0s · max shard execution 20m 7s · max shard-job elapsed before upload 23m 53s · reporting 5s 🌐 214.30 requests/attempt · 2.64 app boots/UI scenario · 13.01% common-shard skew Optimization targets still in progress:
🟡 1 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
|
Failed to cherry-pick changes to the 1.13 branch. |
|
Changes have been cherry-picked to the 2.0 branch. |
…1569) prepareLogicalSuiteRelationshipChange writes the test case and test suite revision rows through the repository's own daoCollection, then reads them back through the static helpers, which resolve Entity.getCollectionDAO() instead. That global is mutable: OpenMetadataApplication and the integration-test bootstrap each set it to an on-demand DAO over their own Jdbi, while a repository captures daoCollection once at construction. The two can therefore name different instances, and the write lands on one connection while the read-back runs on another. MySQL pins a REPEATABLE READ snapshot at the transaction's first read, so it cannot see the other connection's later commit at all and the rows just written come back missing -- reported as "Failed to persist every test suite relationship revision" and "Failed to persist the test suite tests revision". Postgres re-snapshots per statement, which is why only the MySQL lanes ever failed. Give both helpers an overload that takes the caller's DAO and use it from the write path, so the read-back always shares the writer's connection. The four call sites that read outside a transaction -- reindexing, the retry worker and the two index builders -- keep resolving the global. Also report the counts and the suite id in the two messages: as written they named a cause that was never checked, which sent the investigation at the persistence layer rather than at which connection was being read. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 3588eb3)
Code Review ✅ ApprovedUpdates logical-suite revision read-backs to use the DAO that wrote them instead of the global collection DAO, preventing transaction snapshot visibility issues on MySQL. No issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
Describe your changes:
Fixes #31568
TestCaseRepository.prepareLogicalSuiteRelationshipChangewrites the test case and test suite revision rows through the repository's owndaoCollection, then reads them back through the static helpers, which resolveEntity.getCollectionDAO()instead.That global is mutable:
OpenMetadataApplication.run()and the integration-testTestSuiteBootstrapeach set it to an on-demand DAO over their ownJdbi, while a repository capturesdaoCollectiononce at construction (EntityRepository.java:666). The two can therefore name different instances, and the write lands on one connection while the read-back runs on another. MySQL pins a REPEATABLE READ snapshot at the transaction's first read, so it cannot see the other connection's later commit at all and the rows just written come back missing. Postgres re-snapshots per statement — which is precisely why only the MySQL lanes ever failed.I gave both helpers an overload that takes the caller's DAO and used it from the write path, so a read-back always shares the writer's connection. The four call sites that read outside a transaction —
ReindexingUtil,SearchIndexRetryWorker,TestCaseIndexandTestSuiteIndex— keep resolving the global, which is correct for them.I also put the counts and the suite id into the two messages. As written they asserted a cause that was never checked ("failed to persist"), which points an investigation at the persistence layer rather than at which connection is being read.
Type of change:
High-level design:
N/A — small change.
Tests:
Use cases covered
Entity.getCollectionDAO()global does not match the repository's captureddaoCollectionUnit tests
readsRevisionsThroughTheCallersDaoRatherThanTheGlobalOneto bothTestCaseRepositoryTestandTestSuiteRepositoryTest. Each points the global at a different DAO and asserts the revisions still come from the caller's DAO and that the global is never touched — so a revert to the global fails both the value assertion andverifyNoInteractions.Local run —
mvn -pl openmetadata-service test -Dtest='*TestCase*Test,*TestSuite*Test,*DataQuality*Test':That set includes
TestCaseIndexTestandTestSuiteIndexTest, which exercise the unchanged single-argument overload.Integration tests
The existing
TestCaseResourceITandTestSuiteResourceITlogical-suite tests are the real regression coverage — they are the ones failing today on the MySQL lanes. No new IT is added, because reproducing the divergence deterministically needs a secondJdbimid-startup, which the unit tests model directly.UI Screen Recording:
N/A — no UI change.