Skip to content

fix(ebean-dao): honor isTestMode in batchUpsert and create paths#624

Merged
jphui merged 1 commit into
masterfrom
jhui/batch-upsert-test-table-fix
May 16, 2026
Merged

fix(ebean-dao): honor isTestMode in batchUpsert and create paths#624
jphui merged 1 commit into
masterfrom
jhui/batch-upsert-test-table-fix

Conversation

@jphui
Copy link
Copy Markdown
Contributor

@jphui jphui commented May 15, 2026

Problem & Solution Overview

EbeanLocalAccess.prepareMultiColumnInsert hardcoded getTableName(urn) and ignored the isTestMode flag both callers (batchUpsert and create) passed in. As a result the test-table dual-write feature in metadata-graph-assets (which calls dao.batchUpsert(...) with IngestionParams.setTestMode(true)) silently wrote to the production table; the test table was never populated.

This PR threads isTestMode into prepareMultiColumnInsert and routes between getTableName / getTestTableName at the single SQL build site (line 945), matching the pattern already used by the per-aspect add path and by every helper in SQLStatementUtils (e.g. createAspectUpsertSql, createReadDeletionInfoByUrnsSql).

Testing Done

  • Added testBatchUpsertWithTestModeWritesToTestTable in EbeanLocalAccessTest — exercises batchUpsert(..., isTestMode=true) and verifies the row lands in the test table via batchGetUnion(..., isTestMode=true).
  • Added the missing metadata_entity_foo_test schema rows (table + a_urn + a_aspectfoo columns) to both ebean-local-access-create-all.sql fixture files so the test fixture mirrors the prod table for the columns the upsert touches.
  • Full EbeanLocalAccessTest suite passes (no regression from the schema additions).

Notes for Reviewers

The equivalent fix for the add and addWithOptimisticLocking paths is already correct (line 193 calls SQLStatementUtils.createAspectUpsertSql(urn, aspectClass, urnExtraction, isTestMode)). This bug was specific to the multi-column insert helper used only by batchUpsert and create.

The ON DUPLICATE KEY UPDATE clause builders (buildOnDuplicateKeyForUpsert, buildOnDuplicateKeyForCreate) reference column names only, not the table name, so they did not need to change.

🤖 Generated with Claude Code

prepareMultiColumnInsert hardcoded getTableName(urn), silently dropping the
isTestMode flag both callers passed. Both batchUpsert and create therefore
always wrote to the production table even when the caller requested test
mode, which made test-table dual-write features (e.g. the shadow write in
metadata-graph-assets) effectively double-write to production.

Thread isTestMode into prepareMultiColumnInsert and route between
getTableName / getTestTableName at the SQL build site, matching the pattern
already used by the per-aspect add path and the helpers in SQLStatementUtils.

Adds a regression test for the batchUpsert + isTestMode=true case and the
missing metadata_entity_foo_test schema rows in both test SQL fixtures.
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.83%. Comparing base (b025046) to head (556b8fd).

Additional details and impacted files
@@            Coverage Diff            @@
##             master     #624   +/-   ##
=========================================
  Coverage     66.82%   66.83%           
- Complexity     1880     1881    +1     
=========================================
  Files           148      148           
  Lines          7262     7263    +1     
  Branches        879      880    +1     
=========================================
+ Hits           4853     4854    +1     
  Misses         2024     2024           
  Partials        385      385           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jphui jphui merged commit 7df4703 into master May 16, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants