chore(feature): explain the cause when a variation cannot be deleted - #2790
Merged
Conversation
…error occurs
Deleting a variation that is still referenced returned a generic error that
did not explain the cause. References inside the flag surfaced as
"InvalidArgumentNotMatchFormatError" with field "variation", and references
from other flags surfaced as a bare "FailedPreconditionError". Neither
carried the variation or the flag holding the reference, and no server-side
log was written at all.
Split the single ErrVariationInUse sentinel into six errors, one per cause,
each with its own message key so the console can explain what to fix:
- VariationInUseByOffVariationError
- VariationInUseByDefaultStrategyError
- VariationInUseByTargetingRuleError
- VariationInUseByIndividualTargetingError
- VariationInUseByPrerequisiteError
- VariationInUseByFeatureFlagRuleError
The cross-flag errors embed the referencing flag id and name so the console
can name it, and their message spells out the whole relationship for the
logs ("variation X of feature A is used as a prerequisite by feature B").
Also add logVariationInUseError so the rejected update is recorded with the
environment, the flag, the variations being deleted and the reference.
Validation failures were previously not logged anywhere.
Note that the gRPC code for same-flag references changes from
InvalidArgument to FailedPrecondition, matching the code already used for
cross-flag references.
Closes #1794
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kimurakazuhiro-c
marked this pull request as ready for review
August 27, 2026 00:59
kimurakazuhiro-c
requested review from
cre8ivejp,
hvn2k1 and
t-kikuc
as code owners
August 27, 2026 00:59
There was a problem hiding this comment.
Pull request overview
Introduces cause-specific variation deletion errors so the dashboard and server logs explain blocking references.
Changes:
- Adds six structured error types with gRPC mappings and localization.
- Logs rejected variation deletions with dependency metadata.
- Expands domain, API, and E2E coverage.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
ui/dashboard/src/@locales/ja/backend.json |
Adds Japanese error messages. |
ui/dashboard/src/@locales/en/backend.json |
Adds English error messages. |
test/e2e/feature/feature_test.go |
Tests rejected deletions end-to-end. |
pkg/feature/domain/feature.go |
Produces cause-specific errors. |
pkg/feature/domain/feature_update.go |
Updates the primary update path. |
pkg/feature/domain/feature_update_test.go |
Updates expected errors. |
pkg/feature/domain/feature_test.go |
Tests structured dependency errors. |
pkg/feature/api/validation.go |
Preserves domain error details. |
pkg/feature/api/validation_test.go |
Verifies cross-flag error types. |
pkg/feature/api/feature.go |
Logs variation-use failures. |
pkg/feature/api/error.go |
Removes the generic error status. |
pkg/error/error.go |
Defines structured variation-use errors. |
pkg/error/error_test.go |
Tests construction and recognition. |
pkg/api/api/grpc_status.go |
Maps new errors to gRPC statuses. |
pkg/api/api/grpc_status_test.go |
Tests gRPC mappings and metadata. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Address the Copilot review on #2790. The two newly distinct same-flag causes had no assertions: `makeFeature` gives every target users, so the existing tests always returned the individual-targeting error before reaching the strategy checks. Add cases with the target users cleared that assert the default-strategy and targeting-rule errors in both removal paths (`RemoveVariation` and `updateRemoveVariation`), since each path has its own validation. Also fix `TestNewGRPCStatus` for the variation-in-use errors: the metadata assertions ran inside the `st.Details()` loop, so the test passed when no `ErrorInfo` detail existed at all. Extract the detail first and require it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
Fixes #1794
What this PR does
Splits the single
ErrVariationInUsesentinel into six cause-specific errors so the console can tell the user why a variation cannot be deleted, and adds a server-side log for the rejected update.Background / Why this PR is needed
Deleting a variation that is still referenced returned an error that did not explain the cause:
InvalidArgumentNotMatchFormatErrorwith fieldvariation, rendering as "variation format is invalid".FailedPreconditionError, rendering as "The operation failed due to unmet preconditions".Neither carried the variation or the flag holding the reference, so the user could not tell what to fix. On the server side nothing was logged at all: validation failures returned early without touching the logger, so there was no way to diagnose a rejected update after the fact.
#1795 (merged into #1794) asked for logs that spell out the relationship, e.g. "Variation B of flag A is used as a prerequisite for flag C".
Points
Six error types, one per cause.
ErrorTypedoubles as the frontend message key, so a distinct message per cause needs a distinct type. This follows the existingDifferentVariationsSizeErrorprecedent rather than introducing a second mechanism for overriding the message key.Only the cross-flag errors carry embedded values.
VariationInUseByPrerequisiteErrorandVariationInUseByFeatureFlagRuleErrorembed the referencing flag's id and name so the console can name it. The four same-flag causes need no extra data — the user is already looking at that flag.The gRPC code for same-flag references changes from
InvalidArgumenttoFailedPrecondition, matching the code cross-flag references already used. All six are now consistent.Error message vs. embedded values serve different readers. The message spells out the whole relationship for the logs (
variation X of feature A is used as a prerequisite by feature B (name)); the embedded key values feed the localized console message.E2E coverage is new for the error path. The existing e2e tests only covered successful deletion.