fix(unique): include language in value identity - #9820
Conversation
| require.Equal(t, hex.EncodeToString(h.Sum(nil)), getHash(10, 20)) | ||
| } | ||
|
|
||
| func TestVerifyUniqueWithinMutationLanguageIdentity(t *testing.T) { |
There was a problem hiding this comment.
@gooohgb Thanks for the fix.
One ask: three of the important cases are only covered by unit tests right now. Unit tests check the new comparison logic in isolation, but they don't prove the whole flow (mutation → uniqueness query → verify) behaves correctly against a real cluster. Could you add integration tests for these three?
1. Tagged + untagged value in separate mutations (both orders)
First commit _:a <name> "x" (no language), then in a new mutation try _:b <name@en> "x" —and also the other way around. Your unit test says these two are different values and both should be accepted. But whether the database-side check agrees depends on how eq(name, "x") behaves when the value is stored under a language tag and that's never actually tested. If the DB side sees them as the same value, we end up with the same "works in separate mutations, fails in one mutation" inconsistency this PR is fixing just for the tagged/untagged pair instead of en/fr.
2. Swap across two languages (must now FAIL)
Node X already has "v"@en. One mutation gives "v"@en to a new node and at the same time changes X's @fr value. Before this PR that slipped a real duplicate past @unique; with your fix it must be rejected. That's the most valuable behavior change in the PR, and it currently has no end-to-end test.
3. Swap within the same language (must still SUCCEED)
Node X changes its "v"@en to "w"@en while a new node takes "v"@en the classic legal swap, just on a @lang predicate. The existing swap tests only use a predicate without @lang.
Two small side notes:
- Now that language is part of the value's identity, the duplicate error could say which one:
predicate [gxid@en]instead ofpredicate [gxid]. Cheap to add and makes the error self-explanatory. - The PR says "Fixes fix(unique): @unique and @lang disagree on whether language is part of value identity #9815", which will auto-close the issue but the issue's minor second point (the
__dgraph_uniquecheck_N__variable names leaking into unrelated error messages) isn't addressed here. Maybe split that into its own issue so it doesn't get closed along with this.
|
Hi @shiva-istari, thank you for the review. I’ve addressed the requested cases by adding end-to-end integration coverage for: Tagged and untagged values in separate mutations, in both orders. A swap across different languages, which must fail. A legal swap within the same language, which must succeed. Duplicate-value errors now include the language-qualified predicate, such as name@en. I verified the changes against a temporary Zero/Alpha cluster. go test ./edgraph -count=1 and all TestUnique* integration tests passed. |
26fc117 to
2c3f827
Compare
2 participants
Description
Fixes #9815.
For predicates using both
@uniqueand@lang, equal values under differentlanguage tags were handled inconsistently.
The database-level uniqueness query generated by
addQueryIfUniquealreadyscopes values by language, such as
gxid@enandgxid@fr. However,verifyUniqueWithinMutationcompared only the predicate and value, causingequal values under different languages in the same mutation to be incorrectly
rejected as duplicates.
This change includes the language tag when checking value identity within a
mutation. It also applies the same rule to
isSwap, preventing a mutation ofone language from being mistaken for releasing a unique value stored under
another language.
Predicates without language tags retain their existing behavior because both
language values are empty.
Regression coverage includes:
TestUniqueForLangDirective.Checklist
Conventional Commits syntax, leading
with
fix:,feat:,chore:,ci:, etc.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.