[OPIK-7371] [BE] feat: auto-suffix duplicate automation rule names on create - #7568
Merged
Conversation
… create Rule names are not unique, so re-running an SDK script that creates the same rule produced multiple rules that look identical in the UI. On create, when a name already exists within the target project(s), the new rule is now stored as name-1, name-2, ... so rules stay distinguishable without a DB uniqueness constraint or data migration. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
java
Contributor
⏱️ pre-commit per-hook timing
⏭️ 40 skipped (no matching files changed)
|
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
…suffixing Address Baz review: collision comparison now strips diacritics (approximating utf8mb4_unicode_ci) so accented duplicates collide, and truncateToFit no longer splits a surrogate pair on non-BMP names. Split the blank-name parameterized test into explicit cases and added accent/surrogate coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address Baz review: stable literal prefix first, values grouped at the end. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ions - Collision query now also considers the legacy automation_rules.project_id column (LEFT JOIN + OR), so pre-junction rules on older installs are deduped. - canonicalKey strips trailing spaces to match MySQL PAD SPACE comparison. - Add unit (trailing space) and integration (multi-project shared collision, cross-evaluator-type collision) coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ng self) Final review flagged the uniqueness invariant as half-enforced: create() auto-suffixed but update() wrote the name verbatim, so a PATCH rename could re-introduce a collision. update() now resolves a unique name against the target project(s) excluding the rule itself (via findNamesByProjectsExcludingRule), so an unchanged name or a non-name edit is never spuriously suffixed. Adds an integration test for rename-collision and the self-exclusion no-op. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…r commit Baz review round 3: - update() only resolves a unique name when the name actually changes vs the stored name (findNameById); a non-name edit (sampling rate, enabled, filters) no longer silently renames a rule to name-1 when a same-named rule already exists in the project scope (e.g. legacy duplicates or a shared-project edit). - Move the "applied suffix" log out of the write transaction so a rolled-back write can't leave a misleading success line. - Add integration test: a non-name edit that introduces a scope collision keeps the name unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…missing 404 Self-review: findNameById returned a bare String, deviating from the codebase's universal Optional<...> lookup convention and relying on JDBI's ambiguous zero-row behavior. Updating a non-existent rule would call it before updateBaseRule; return Optional<String> so the missing row is handled cleanly and the update still 404s (not 500). Add a regression test for that path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Resolve conflict in AutomationRuleEvaluatorService.update(): keep the OPIK-7371 name dedup (pass resolved uniqueName to updateBaseRule) while adopting main's new trigger_scope parameter (OPIK-6741). save() and the DAO auto-merged (both name-suffix queries and trigger_scope coexist). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Backend Tests - Unit Tests2 867 tests 2 865 ✅ 1m 33s ⏱️ Results for commit fbcb493. ♻️ This comment has been updated with latest results. |
Contributor
Author
ℹ️ Note on the failing
|
andrescrz
requested changes
Jul 23, 2026
andrescrz
left a comment
andrescrz
left a comment
Member
There was a problem hiding this comment.
The PR has major issues that we should address before moving forward. I left guidance to fix them. Please take a look.
…ookup Address @andrescrz review: - Replace the fetch-all-names-in-workspace + Java-match approach with a bounded, index-backed prefix query (name LIKE 'base%'), so it no longer loads every rule name into memory. - Add index automation_rules(workspace_id, name) (migration 000091). - Scope collisions per project via the junction table only (authoritative after the AutomationRuleProjectMigration backfill); drop the legacy project_id OR branch. - Collapse the two near-identical DAO queries into one StringTemplate query with an optional <if(excludeRuleId)> clause. - Escape LIKE metacharacters (\ % _) in the prefix so free-form names match literally. - update(): return the resolved name from the transaction (drop the String[] holder); keep the non-name-edit short-circuit; log after commit, no longer dumping projectIds. Optimistic by design (no DB unique constraint; rare un-backfilled legacy rule or concurrent create may duplicate) per ticket scope. Tests: unit 15, integration 10. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… main merge main introduced its own 000091 (000091_add_filter_guardrail_type_config_type); renumber the automation-rules name index migration to 000092 to avoid a duplicate changeset number. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The query returns prefix-filtered candidates; the precise collision check happens in AutomationRuleNames.generateUniqueName. Name now says what it does. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
andrescrz
reviewed
Jul 27, 2026
andrescrz
left a comment
andrescrz
left a comment
Member
There was a problem hiding this comment.
This has addressed many of the previous comments.
Let's keep iterating on this.
Please make sure all Baz comments are either addressed or replied.
…E escaping, shared resolution Review fixes: - Reject names >150 chars with 422 at the API boundary (@SiZe on both the create and update models); previously an over-long name reached the insert and failed with a 500 (pre-existing, surfaced by review) - Declare the LIKE escape character explicitly and switch it from '\' to '!': the query passes through three escaping layers (Java text block -> StringTemplate -> MySQL), so ESCAPE '\' silently rendered as a syntax error; '!' is inert in all three - Fix duplicate suffixes for max-length names on re-run: suffixing a 150-char name truncates its base, so the stored "name-1" no longer shared the full search prefix and the next run regenerated it; the LIKE prefix is now capped at 138 chars (150 minus the worst-case suffix reserve) - Extract resolveUniqueName() shared by save() and update() so the suffixing rules cannot drift between them - Hoist the \p{M} diacritics Pattern to a static final; use CollectionUtils.isEmpty for the null-safe check - Rework the suffix log line: stable greppable prefix, all values trailing, no longer claims to print a "suffix" when it prints the full name - Correct the "index-backed" claim in findCandidateNames docs: measured on MySQL 8.4 with 50k rules, the (workspace_id, name) index from migration 000092 is never selected; the project filter is what bounds the query Tests: - DuplicateNameHandling: centralise helpers into two primitives with delegating overloads; add 422 length tests (create + update), wildcard '%'-named rule non-overmatch, SQL-payload literal round-trips, and the max-length re-run regression test - AutomationRuleNamesTest: parameterise the (requested, existing) -> expected cases into one @MethodSource test (18 named cases); keep truncation and likePrefix cases standalone; cover the new prefix cap incl. surrogate-pair boundary Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
Follow-up on my CI note above (23 Jul): obsolete — the Latest push (45f1981) addresses the remaining review round: name-length validation (422 instead of 500), explicit LIKE |
Two claims previously rested on unit tests or shared code paths alone: - rename-to-metacharacter-name: the update path shares resolveUniqueName with create, but escaping on rename had no end-to-end lock - accent-insensitive collision (Café/Cafe): the in-Java canonical-key fold was unit-tested, but nothing proved the LIKE prefix fetch (running under utf8mb4_unicode_ci) actually returns the accented stored name as a candidate Both are now integration-tested in DuplicateNameHandling (21 tests). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
andrescrz
approved these changes
Jul 28, 2026
andrescrz
left a comment
andrescrz
left a comment
Member
There was a problem hiding this comment.
Thanks for putting this together. LGTM.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Details
Automation / online-evaluation rule names have no uniqueness, so re-running an SDK script that creates the same rule produces multiple rules that look identical in the UI (the list surfaces the name; the UUID is the real identifier). This adds best-effort, project-scoped auto-suffixing on create and on actual rename: when a name already exists in the target project(s), the rule is stored as
name-1,name-2, … so rules stay distinguishable — without a DB uniqueness constraint or data migration (existing installs already contain colliding names, which the ticket rules out enforcing).How it works
AutomationRuleDAO.findCandidateNamesfetches existing names in the target project(s) sharing the requested name's prefix (name LIKE concat(?, '%') ESCAPE '!'), scoped via theautomation_rule_projectsjunction table (authoritative post-backfill; the legacyproject_idcolumn is intentionally not consulted). One query serves create and update — update passesexcludeRuleIdso a rule never collides with its own current name.AutomationRuleNames(new helper) resolves the final name over that candidate set: smallest-free-Nsuffix; comparison case-, accent- and trailing-space-insensitive to approximate MySQL'sutf8mb4_unicode_cicollation; suffixed candidates are truncated to the 150-char column on a code-point boundary. The LIKE prefix escapes metacharacters (!,%,_— escape char is!because the query crosses three backslash-eating layers: Java text block → StringTemplate → MySQL) and is capped at 138 chars so truncated candidates of max-length names still match on re-run.resolveUniqueName(service) is shared bysave()(all 6 evaluator types) andupdate(); a non-name edit (sampling rate, enabled, filters, projects) never renames the rule — the stored name is compared first viafindNameById.@Size(max = 150)on the create and update models: an over-long name is now rejected with 422 at the API boundary instead of failing the insert with a 500 (minor API contract change; OpenAPI spec sync is automated).000092adds a(workspace_id, name)index. Open question for reviewers: measured with EXPLAIN on MySQL 8.4 at 50k rules, the optimizer never selects it — the project filter (junction index) is what bounds the query. Keep (in case optimizer stats change) or drop (write amplification while unused)? Documented infindCandidateNamesJavadoc.Intentional scope / accepted limitations
Locationrather than assume the name is unchanged. This is the ticket's chosen direction (suffix, not hard-uniqueness/upsert).ß→ss) and non-default install collations (e.g.utf8mb4_0900_ai_ci) are not replicated exactly.Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
Testing
mvn test -Dtest=AutomationRuleNamesTest— 23/23 (parameterised resolve cases incl. case/accent/trailing-space folding, gap-fill, freed-base reuse, prefix lookalikes; likePrefix escaping + long-name cap incl. surrogate-pair boundaries; 150-char truncation and re-run distinctness).mvn test -Dtest='AutomationRuleEvaluatorsResourceTest$DuplicateNameHandling'— 21/21 end-to-end (MySQL/ClickHouse/Redis testcontainers): same-project suffixing, cross-project isolation, multi-project shared-project collision, cross-evaluator-type collision, rename-to-existing with self-exclusion, non-name edits never rename, 422 for >150-char names (create + update), LIKE-metacharacter names (create + rename), a rule literally named%not over-matching, SQL-payload names stored literally, max-length names staying distinct across re-runs, accent-insensitive collision end-to-end.-1,-2) and confirmed cross-project isolation. GIFs attached below.Documentation
No user-facing docs change. Behavior is transparent (rule names simply gain a
-Nsuffix on collision). Note the 422-for-over-long-names contract change above.