Fix UniqueKeyPolicy enforcement for nested paths (Issues #10, #13)#27
Draft
McNultyyy wants to merge 7 commits into
Draft
Fix UniqueKeyPolicy enforcement for nested paths (Issues #10, #13)#27McNultyyy wants to merge 7 commits into
McNultyyy wants to merge 7 commits into
Conversation
The SQL parser was routing legacy functions (CONTAINS, STARTSWITH, ENDSWITH, etc.) through the string-based FunctionCondition path even when their arguments contained nested function calls like LOWER() or UPPER(). The string-based ResolveValue method cannot parse function call syntax, causing a JsonException. Added a check for nested function calls in arguments, routing them through the modern SqlExpressionCondition path which recursively evaluates expressions. Bumps version to 2.0.184.
13 additional test cases covering: - 3-level deep nesting: CONTAINS(LOWER(TRIM(...))) - Nested inside nested: STARTSWITH(CONCAT(LOWER(...), ...), ...) - Both args as functions: CONTAINS(LOWER(...), LOWER(...)) - 3-arg CONTAINS with nested function + case-insensitive flag - IS_DEFINED/IS_NULL with nested function args - ARRAY_CONTAINS with nested function in search value - NOT combined with nested function predicate - AND with multiple nested function predicates - Undefined property through nested function - Null value flowing through nested function - Nested function in second argument only - Type conversion via ToString() in nested function
IS_DEFINED() in EvaluateSqlFunction had a fallthrough that returned true for UndefinedValue.Instance (args[0] != null). This was exposed by routing nested-function legacy calls through the modern path. Fixed to check: args[0] is not null and not UndefinedValue. This also fixes the 8 pre-existing GitHubIssueReproTests failures. Added 4 edge case tests: - IS_DEFINED(LOWER(c.nonexistent)) → false - IS_NULL(LOWER(c.nonexistent)) → false - CONTAINS(LOWER(c.numericField), 'x') → no match - ARRAY_CONTAINS + nested function combined with AND
Red-team edge case testing found 3 additional bugs in the nested function call fix: 1. ContainsFunctionCall only checked for nested function calls but missed CoalesceExpression (??) and arithmetic BinaryExpression in legacy function arguments. These still routed through the string- based FunctionCondition path, causing JsonException crashes. Added ContainsArithmetic and ContainsComplexExpression checks. 2. ARRAY_CONTAINS with UndefinedValue search value (from nested function on missing property) caused NullReferenceException. UndefinedValue.ToString() returns null, and searchStr.StartsWith throws NRE on object-type array elements. Fixed in both legacy and modern evaluation paths. Added 20 edge case tests covering coalesce, arithmetic, undefined propagation, deep nesting, type-checking, NOT, OR, and projection. Bumps version to 2.0.185.
The ValidateUniqueKeys method used p.TrimStart('/') to convert Cosmos DB
paths (e.g. /value/code) for JObject.SelectToken, yielding 'value/code'.
Newtonsoft.Json's SelectToken uses dot notation ('value.code'), not
forward slashes, so nested paths always resolved to null — causing false
positive conflicts (every second insert conflicted regardless of values).
Fix: add CosmosPathToSelectTokenPath() helper that splits on '/' and
delegates to the existing BuildSelectTokenPath() which produces correct
JSONPath notation (e.g. 'value.code', 'items[0].name').
Added 11 tests covering:
- Nested paths (/value/code) — conflict detection and non-conflict
- Deeply nested paths (/a/b/c)
- Cross-partition isolation with nested paths
- Upsert and Replace with nested paths
- Container creation via Database.CreateContainerAsync
- Container creation via Database.CreateContainerIfNotExistsAsync
All 7762 existing tests continue to pass.
Fixes #10
Fixes #13
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Null/missing unique key values: Allow multiple items with null/missing unique key properties (matching real Cosmos DB behavior). Previously, two nulls compared equal causing false positive conflicts. - Type coercion: Use JToken type-aware comparison instead of .ToString(). Integer 42 vs string '42' no longer falsely conflict. Numeric types (Integer/Float) with equal values (e.g. 1 vs 1.0) correctly conflict. - TTL expired items: Skip expired-but-not-evicted items during unique key validation so they don't block reuse of their unique key values. - Special character property names: Use bracket notation (['my field']) in SelectToken paths for properties containing spaces, dots, or other special characters. - Add 41 comprehensive edge case tests covering null/missing values, type coercion, empty strings, nested objects, arrays, deeply nested paths, unicode, case sensitivity, special chars, TTL, concurrency, batch operations, patch operations, replace/upsert, and more. - Add BuildSelectTokenPath tests for bracket notation with special chars. Bump version to 2.0.187. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add UniqueKeyPolicy property to IContainerTestSetup interface - Implement UniqueKeyPolicy getter/setter in InMemoryContainer - Update InMemoryTestFixture.ApplyContainerProperties to copy UniqueKeyPolicy - Add 32 integration tests covering: nested paths (issue #10), database creation (issue #13), null/missing values, type coercion, composite keys, multiple independent keys, string variants, TTL interaction, upsert self - Bump version to 4.0.6 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
McNultyyy
force-pushed
the
agents/issue-10-13-review-and-fix
branch
from
aaa1b6d to
e0a789f
Compare
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
1 participant
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.
Fix: UniqueKeyPolicy enforcement (Issues #10 & #13)
Problem
/value/code) was not enforced.ValidateUniqueKeysusedTrimStart('/')producingvalue/code, butJObject.SelectTokenuses dot notation (value.code). This caused SelectToken to return null for ALL items → false positive 409 Conflict on every second insert.database.CreateContainerAsync(containerProperties).Root Cause
SelectToken("value/code")treats/as a literal character, not a path separator. The method returns null, andnull == nullcauses false-positive conflict detection.Fix
CosmosPathToSelectTokenPath()— splits Cosmos/a/b/cpaths and delegates toBuildSelectTokenPath()for proper dot/bracket notationUniqueKeyTokensMatch()— type-aware comparison: different JSON types don't match (int42≠ string"42"), numeric types unified (int1= float1.0)IsExpired()checkBuildSelectTokenPathnow uses bracket notation['my field']for properties with spaces, dots, brackets, or quotesIContainerTestSetup.UniqueKeyPolicy— added soInMemoryTestFixturecorrectly applies UniqueKeyPolicy through the test fixture pipelineTest Coverage
ITestContainerFixtureInMemoryContainertesting for internal behaviorBuildSelectTokenPathbracket notation testsUniqueKeyPolicyTestsVersion
Bumped to 4.0.6 (centralized in
src/Directory.Build.props)Closes #10, Closes #13