feat(1.12.9): backfill task domains + stamp domains on approval task creation#28286
feat(1.12.9): backfill task domains + stamp domains on approval task creation#28286yan-3005 wants to merge 1 commit into
Conversation
…n approval tasks Ports the task-domain backfill work from the main-branch PR (#28013) onto the 1.12.9 release branch. Direct cherry-pick is not viable because main has moved to the new Task entity (CreateTask), while 1.12.9 still uses the Thread-based CreateApprovalTaskImpl. 1. v1129 MigrationUtil now also runs migrateTaskDomains alongside the existing policy migrations. The thread_entity path patches \$.domains based on the linked entity. The task_entity path is guarded by tableExists() so it is a no-op on this release branch but ports cleanly forward. 2. Both Migration.java runners wrap the policy and task-domain calls in their own try/catch so a backfill failure no longer blocks server startup. The prior @SneakyThrows is removed. 3. CreateApprovalTaskImpl now stamps Thread.domains from entity.getDomains() at creation time so newly-created approval tasks carry the correct domain from the start (no further backfill required for new rows).
yan-3005
added
the
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| + "LIMIT " | ||
| + BATCH_SIZE | ||
| + " ON CONFLICT (fromId, toId, relation) DO NOTHING"; |
There was a problem hiding this comment.
💡 Edge Case: PostgreSQL LIMIT in INSERT...SELECT before ON CONFLICT
In buildPostgresInsertTaskDomainSql(), the LIMIT clause is placed outside parentheses directly before ON CONFLICT DO NOTHING. While this is syntactically valid in modern PostgreSQL (LIMIT is parsed as part of the SELECT, ON CONFLICT as part of the INSERT), it is an unusual pattern that can confuse readers and has historically caused issues with some PostgreSQL parser edge cases. Wrapping the SELECT in parentheses makes the intent unambiguous and is the commonly recommended pattern for INSERT ... (SELECT ... LIMIT n) ON CONFLICT ....
The migration is wrapped in try/catch so a failure wouldn't block startup, but it would silently skip the task_entity domain backfill on PostgreSQL deployments.
Wrap the SELECT in parentheses so LIMIT is unambiguously part of the subquery. Also add an opening paren after the column list: change the first line to "INSERT INTO entity_relationship " + " (fromId, toId, fromEntity, toEntity, relation) " + "(SELECT ...".:
+ " ) "
+ "LIMIT "
+ BATCH_SIZE
+ ") ON CONFLICT (fromId, toId, relation) DO NOTHING";
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
Code Review 👍 Approved with suggestions 0 resolved / 1 findingsBackfills domains on existing approval tasks and updates creation logic to stamp domains inline for the 1.12.9 release. Ensure the PostgreSQL LIMIT clause in the migration script is correctly positioned to avoid syntax issues with ON CONFLICT. 💡 Edge Case: PostgreSQL LIMIT in INSERT...SELECT before ON CONFLICTIn The migration is wrapped in try/catch so a failure wouldn't block startup, but it would silently skip the task_entity domain backfill on PostgreSQL deployments. Wrap the SELECT in parentheses so LIMIT is unambiguously part of the subquery. Also add an opening paren after the column list: change the first line to `"INSERT INTO entity_relationship " + " (fromId, toId, fromEntity, toEntity, relation) " + "(SELECT ..."`.🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
🟡 Playwright Results — all passed (16 flaky)✅ 2087 passed · ❌ 0 failed · 🟡 16 flaky · ⏭️ 52 skipped
🟡 16 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 |
1 participant
Summary
Ports the task-domain backfill from PR #28013 (main) onto the 1.12.9 release branch. Direct cherry-pick was not viable because main has moved to the new Task entity (
CreateTask), while 1.12.9 still uses the Thread-basedCreateApprovalTaskImpl.1. v1129 migration: backfill domains on existing approval tasks
MigrationUtil(v1129) now runsmigrateTaskDomains()alongside the existing policy migrations.thread_entitypath (1.12.x): batches rows whereJSON_EXTRACT(json,'$.domains') IS NULL/json->'domains' IS NULL(Postgres), resolves domains from the linked entity viaMessageParser.EntityLink, and patches$.domainsviaJSON_SET/jsonb_set. Caches(entityType, entityFQN) -> domainIdsso each unique target entity is resolved only once.task_entitypath (2.x): guarded bytableExists()so it is a no-op on this release branch but ports cleanly forward.$.domains = [](and counted inmarkedDoneOnError) so the batch always advances — no more infinite loop on persistent failures.2. Migration runners: non-fatal policy + task backfill
mysql/v1129/Migration.javaandpostgres/v1129/Migration.javawrap the policy migrations and the task-domain backfill in separate try/catch blocks so a failure in either step no longer blocks server startup.@SneakyThrowswas removed.3. CreateApprovalTaskImpl: stamp domains at creation
CreateApprovalTaskImplnow setsThread.domainsfromentity.getDomains()(mapped to UUIDs) at thread-creation time, so newly-created approval tasks carry the correct domain inline — no further backfill needed for new rows.Test plan
MigrationUtilTest(v1129) — 17 unit tests covering both DB dialects,deleted = FALSEfilters, JSON-NULL WHERE-clause shape, loop termination, malformed-JSON-marked-migrated, empty-batch termination, INSERT IGNORE vs ON CONFLICT shape, full-batch loop continuation. All passing.Thread.domainsis set to the entity's domains in the persisted row.Related: main-branch PR #28013.