From 8e8302606ec38f3729f3597418c86159db9c8824 Mon Sep 17 00:00:00 2001 From: Uday Chandra Date: Mon, 5 Oct 2026 05:49:22 +0000 Subject: [PATCH 1/5] Bring the README in line with what the library does Every example was run against the library and corrected: - The dependency snippet names 1.2.0. - Clauses carry the prefix as a table alias (p.user_name) and parameter keys are not prefixed (:userName1). The README showed the opposite, and claimed the prefix made it safe to use several parsed filters in one query. Keys are unique only within one parsed filter. - The JSON example was not a valid filter. Quotes inside a JSON value are escaped in the filter text. - An attribute can name its own table alias (u.userName). Dotted paths are not kept as they are. - Parameters are collected by toClause(), so it must run before indexedParams() or isValid(). - The query analysis sample shows the records and the normalized SQL that are actually returned. A Known Limitations section records behaviour a caller can trip over: path segments after alias.attribute are ignored, Context.isValid sees nothing before rendering and never sees an attribute used with pr, and eq null renders as = :param. --- README.md | 71 +++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 50 insertions(+), 21 deletions(-) diff --git a/README.md b/README.md index e698a00..9f65cd3 100644 --- a/README.md +++ b/README.md @@ -15,7 +15,7 @@ Add the dependency: ai.singlr scim-sql - 1.0.0 + 1.2.0 ``` @@ -26,16 +26,22 @@ var engine = new ScimEngine(); var filter = engine.parseFilter("userName eq \"john\"", "p", null); filter.toClause(); -// → "user_name = :p_userName1" +// → "p.user_name = :userName1" filter.context().indexedParams(); -// → {p_userName1=john} +// → {userName1=john} ``` -The `prefix` parameter namespaces all generated parameter keys, making it safe to combine multiple parsed filters in a single query. +The `prefix` is the table alias put in front of every attribute that does not name its own. Pass an empty string for no alias: `user_name = :userName1`. + +Parameter keys are the attribute name plus a counter (`userName1`, `userName2`). They are unique within one parsed filter and are not prefixed, so two separately parsed filters can produce the same key. To use two filters in one query, combine them into one expression first (see [Scoping a Client Filter](#scoping-a-client-filter)) and parse that. + +Call `toClause()` before reading `indexedParams()`. The parameters are collected while the clause is rendered. ## Supported Operators +The examples use an empty prefix. + | SCIM Operator | SQL Output | Example | |--------------|------------|---------| | `eq` | `=` | `name eq "John"` → `name = :name1` | @@ -50,19 +56,21 @@ The `prefix` parameter namespaces all generated parameter keys, making it safe t | `pr` | `IS NOT NULL` | `name pr` → `name IS NOT NULL` | | `in` | `IN (…)` | `status in ["active", "pending"]` → `status IN (:status1, :status2)` | +Values are strings in double quotes, whole numbers, decimals, `true`, `false` and `null`. + ## Logical Operators Combine filters with `and`, `or`, `not`, and parentheses: ```java engine.parseFilter("name eq \"John\" and age gt 21", "p", null).toClause(); -// → "name = :p_name1 AND age > :p_age1" +// → "p.name = :name1 AND p.age > :age1" engine.parseFilter("not (active eq true)", "p", null).toClause(); -// → "NOT (active = :p_active1)" +// → "NOT (p.active = :active1)" engine.parseFilter("(a eq 1 or b eq 2) and c eq 3", "p", null).toClause(); -// → "(a = :p_a1 OR b = :p_b1) AND c = :p_c1" +// → "(p.a = :a1 OR p.b = :b1) AND p.c = :c1" ``` `and` binds tighter than `or`, as in SQL. Use parentheses to group. @@ -120,16 +128,21 @@ Values can carry type hints that produce SQL `CAST` expressions. Prefix the valu |--------|------|----------|---------------| | `#` | UUID | `CAST(… AS UUID)` | `"#550e8400-e29b-41d4-a716-446655440000"` | | `@` | Timestamp | `CAST(… AS timestamptz)` | `"@2026-01-15T10:30:00Z"` | -| `$` | JSON | `CAST(… AS jsonb)` | `"${"key":"value"}"` | +| `$` | JSON | `CAST(… AS jsonb)` | `"${\"key\":\"value\"}"` | ```java engine.parseFilter("id eq \"#550e8400-e29b-41d4-a716-446655440000\"", "p", null).toClause(); -// → "id = CAST(:p_id1 AS UUID)" +// → "p.id = CAST(:id1 AS UUID)" -engine.parseFilter("metadata eq \"${\"role\":\"admin\"}\"", "p", null).toClause(); -// → "metadata @> CAST(:p_metadata1 AS jsonb)" +engine.parseFilter("createdAt gt \"@2026-01-15T10:30:00Z\"", "p", null).toClause(); +// → "p.created_at > CAST(:createdAt1 AS timestamptz)" + +engine.parseFilter("metadata eq \"${\\\"role\\\":\\\"admin\\\"}\"", "p", null).toClause(); +// → "p.metadata @> CAST(:metadata1 AS jsonb)" ``` +Quotes inside a JSON value are escaped with a backslash in the filter text: `metadata eq "${\"role\":\"admin\"}"`. + JSON equality uses PostgreSQL's `@>` (contains) operator instead of `=`. ## Attribute Name Conversion @@ -139,11 +152,18 @@ CamelCase attribute names are automatically converted to snake_case column names - `userName` → `user_name` - `createdAtUtc` → `created_at_utc` -Dotted paths (e.g. `name.familyName`) are preserved as-is in the clause and flattened to underscores in parameter keys. +An attribute can name its own table alias, which replaces the prefix for that attribute: + +```java +engine.parseFilter("u.userName eq \"john\" and active eq true", "p", null).toClause(); +// → "u.user_name = :u_userName1 AND p.active = :active1" +``` + +The dot becomes an underscore in the parameter key. Only `alias.attribute` is supported; see [Known Limitations](#known-limitations). ## Parameter Binding -`Context` collects all parameter bindings as the filter tree is evaluated: +`Context` collects the parameter bindings while the clause is rendered: ```java var filter = engine.parseFilter("name eq \"John\" and age gt 21", "p", null); @@ -151,11 +171,11 @@ var clause = filter.toClause(); var params = filter.context().indexedParams(); // Use with JDBC named parameters, JOOQ, or any query builder: -// clause = "name = :p_name1 AND age > :p_age1" -// params = {p_name1=John, p_age1=21} +// clause = "p.name = :name1 AND p.age > :age1" +// params = {name1=John, age1=21} ``` -Use `context().isValid(Set.of("name", "age"))` to allowlist which attributes callers are permitted to filter on. +Use `context().isValid(Set.of("name", "age"))` to allowlist which attributes callers are permitted to filter on. An attribute with its own alias is checked by its full path (`u.userName`). Call it after `toClause()`, and read [Known Limitations](#known-limitations) before relying on it alone. ## Custom Filter Builders @@ -165,11 +185,18 @@ Override SQL generation for specific comparisons by passing a `compareFilterBuil var filter = engine.parseFilter( "tags eq \"admin\"", "p", - cf -> new ComparisonFilter.ListFilter(cf) // wraps as ListFilter + cf -> new ComparisonFilter.ListFilter(cf) ); ``` -This lets you intercept `ComparisonFilter` instances and return a subclass with custom `toClause()` or `paramKey()` behavior. +This lets you intercept `ComparisonFilter` instances and return a subclass with custom `toClause()` or `paramKey()` behavior. `ComparisonFilter.ListFilter` is a ready-made subclass that renders like the default and marks the comparison as coming from a list query. + +## Known Limitations + +- **Attribute paths deeper than `alias.attribute`.** Segments after the second are ignored: `emails.work.value co "x"` renders as `LOWER(emails.work) LIKE …`. Do not rely on deeper paths. +- **`Context.isValid` before rendering.** The context is filled by `toClause()`. Called earlier, `isValid` has nothing to check and returns `true`. +- **`Context.isValid` and `pr`.** A presence check binds no parameter, so its attribute is not seen: `secret pr` passes an allowlist that does not contain `secret`. To allowlist strictly, also walk the filter tree and check every `PresentFilter`. +- **`eq null` and `ne null`.** They render as `= :param` and `!= :param` with a null value, which SQL never matches. Use `pr` and `not (… pr)` to test for a value. ## PostgreSQL Query Analysis @@ -182,13 +209,15 @@ var analysis = PostgresQueryAnalyzer.analyze( analysis.statementKind(); // SELECT analysis.statementCount(); // 1 analysis.relations(); // [RelationReference[schema=null, name=users, alias=u, kind=PHYSICAL]] -analysis.columns(); // [u.id, u.created_at, u.id] -analysis.functions(); // [count at 1:17] +analysis.columns(); // [ColumnReference[qualifier=u, name=id], ColumnReference[qualifier=u, name=created_at], …] +analysis.functions(); // [FunctionReference[schema=null, name=count, line=1, column=13]] analysis.parameters(); // [start_at] analysis.features(); // [] -analysis.normalizedSql(); // deterministic form for hashing/audit +analysis.normalizedSql(); // "select u . id , count ( * ) from users u where u . created_at >= :start_at group by u . id" ``` +`normalizedSql()` is a deterministic form for hashing and audit, not SQL meant to be read. + Key properties: - **Named parameters** (`:start_at`, `:user_id`) are first-class expression values via a deliberate, documented grammar extension. Names are reported exactly; values are never bound or substituted. From 457831f6e98194bbd9933a815708d69a11805375 Mon Sep 17 00:00:00 2001 From: Uday Chandra Date: Mon, 5 Oct 2026 06:05:12 +0000 Subject: [PATCH 2/5] Make the allowlist see every attribute a filter uses Context.isValid checked the parameters a filter had bound, which left two holes: - A presence check binds no parameter, so its attribute was never seen. `secret pr` passed an allowlist that did not contain `secret`. - Parameters are bound while the clause is rendered. Called before toClause(), isValid had nothing to check and returned true for any filter. The evaluator now records the attribute of every comparison, list check and presence check it builds, and binding a parameter records its attribute too. isValid requires every recorded attribute to be listed. For a comparison that a custom builder replaces, the attribute of the replacement is recorded, so a builder that renames an attribute is checked by the new name. --- src/main/java/ai/singlr/scimsql/Context.java | 29 +++- .../java/ai/singlr/scimsql/ScimEvaluator.java | 8 +- .../java/ai/singlr/scimsql/ContextTest.java | 42 ++++++ .../ai/singlr/scimsql/ScimEngineTest.java | 126 ++++++++++++++++++ 4 files changed, 200 insertions(+), 5 deletions(-) diff --git a/src/main/java/ai/singlr/scimsql/Context.java b/src/main/java/ai/singlr/scimsql/Context.java index efeceed..31d47ea 100644 --- a/src/main/java/ai/singlr/scimsql/Context.java +++ b/src/main/java/ai/singlr/scimsql/Context.java @@ -7,6 +7,7 @@ import java.util.ArrayList; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -15,13 +16,24 @@ public class Context { private final Map> params; private final Map indexedParams; + private final Set attributes; public Context() { this.params = new HashMap<>(); this.indexedParams = new HashMap<>(); + this.attributes = new HashSet<>(); + } + + /** + * Records that the filter uses an attribute, so the allowlist sees it before the clause is + * rendered and whether or not it binds a parameter. + */ + void reference(Filter attribute) { + attributes.add(attribute.toString()); } public String process(Filter attribute, Filter value, Function keyMapper) { + reference(attribute); var rawValue = value instanceof ValueFilter ? ((ValueFilter) value).value() : null; var key = attribute.toString(); @@ -37,6 +49,7 @@ public String process(Filter attribute, Filter value, Function k public String processArray( Filter attribute, List valueFilters, Function keyMapper) { + reference(attribute); var key = attribute.toString(); List values = params.computeIfAbsent(key, k -> new ArrayList<>()); var builder = new StringBuilder(); @@ -70,13 +83,23 @@ public String processArray( return builder.toString(); } + /** + * Checks the filter against an allowlist of attributes. + * + *

Every attribute the filter uses must be listed: those in comparisons, in list checks and in + * presence checks ({@code pr}). An attribute with its own alias is listed by its full path + * ({@code u.userName}). For a filter parsed by {@link ScimEngine} the answer does not depend on + * whether the clause has been rendered. + * + * @param validParamKeys the attributes callers may filter on + * @return {@code true} if the filter uses only listed attributes + */ public boolean isValid(Set validParamKeys) { - for (String key : params.keySet()) { - if (!validParamKeys.contains(key)) { + for (String attribute : attributes) { + if (!validParamKeys.contains(attribute)) { return false; } } - return true; } diff --git a/src/main/java/ai/singlr/scimsql/ScimEvaluator.java b/src/main/java/ai/singlr/scimsql/ScimEvaluator.java index 33bf748..503c658 100644 --- a/src/main/java/ai/singlr/scimsql/ScimEvaluator.java +++ b/src/main/java/ai/singlr/scimsql/ScimEvaluator.java @@ -58,6 +58,7 @@ public Filter visitLogicalExp(ScimParser.LogicalExpContext ctx) { @Override public Filter visitPresentExp(ScimParser.PresentExpContext ctx) { Filter attributePath = visitAttrPath(ctx.attrPath()); + context.reference(attributePath); return new PresentFilter(attributePath); } @@ -65,6 +66,7 @@ public Filter visitPresentExp(ScimParser.PresentExpContext ctx) { public Filter visitInExp(ScimParser.InExpContext ctx) { Filter attributePath = visitAttrPath(ctx.attrPath()); Filter values = visitArrayValue(ctx.arrayValue()); + context.reference(attributePath); return new InFilter(attributePath, (ArrayValueFilter) values, context); } @@ -74,8 +76,10 @@ public Filter visitCompareExp(ScimParser.CompareExpContext ctx) { String operator = ctx.op.getText(); Filter value = visit(ctx.value()); - return compareFilterBuilder.apply( - new ComparisonFilter(attributePath, operator, value, context)); + var comparison = + compareFilterBuilder.apply(new ComparisonFilter(attributePath, operator, value, context)); + context.reference(comparison.attribute()); + return comparison; } @Override diff --git a/src/test/java/ai/singlr/scimsql/ContextTest.java b/src/test/java/ai/singlr/scimsql/ContextTest.java index a7bc9e6..73d5178 100644 --- a/src/test/java/ai/singlr/scimsql/ContextTest.java +++ b/src/test/java/ai/singlr/scimsql/ContextTest.java @@ -144,4 +144,46 @@ void shouldReturnTrueForEmptyParams() { var context = new Context(); assertTrue(context.isValid(Set.of())); } + + @Test + @DisplayName("isValid sees an attribute processed as a list") + void shouldSeeAnAttributeProcessedAsAList() { + var context = new Context(); + var attr = new AttributeFilter("id", null, "t", context); + context.processArray(attr, List.of(new ValueFilter(1L, context)), k -> ":" + k); + + assertFalse(context.isValid(Set.of("name"))); + assertTrue(context.isValid(Set.of("id"))); + } + + @Test + @DisplayName("isValid sees a recorded attribute that binds no parameter") + void shouldSeeARecordedAttributeThatBindsNoParameter() { + var context = new Context(); + var sub = new AttributeFilter("userName", null, "", context); + context.reference(new AttributeFilter("u", sub, "t", context)); + + assertFalse(context.isValid(Set.of("userName"))); + assertTrue(context.isValid(Set.of("u.userName"))); + assertTrue(context.indexedParams().isEmpty()); + } + + @Test + @DisplayName("Building a filter by hand records nothing") + void shouldRecordNothingWhenAFilterIsOnlyBuilt() { + var context = new Context(); + var attr = new AttributeFilter("scratch", null, "t", context); + new PresentFilter(attr); + new ComparisonFilter(attr, "eq", new ValueFilter("x", context), context); + + assertTrue(context.isValid(Set.of())); + } + + @Test + @DisplayName("A presence check needs no context to render") + void shouldRenderAPresenceCheckWithoutAContext() { + var filter = new PresentFilter(new AttributeFilter("a", null, "t", null)); + + assertEquals("t.a IS NOT NULL", filter.toClause()); + } } diff --git a/src/test/java/ai/singlr/scimsql/ScimEngineTest.java b/src/test/java/ai/singlr/scimsql/ScimEngineTest.java index 16b00e4..f985840 100644 --- a/src/test/java/ai/singlr/scimsql/ScimEngineTest.java +++ b/src/test/java/ai/singlr/scimsql/ScimEngineTest.java @@ -6,6 +6,7 @@ package ai.singlr.scimsql; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -13,6 +14,7 @@ import java.util.List; import java.util.Random; +import java.util.Set; import org.antlr.v4.runtime.CharStreams; import org.antlr.v4.runtime.CommonTokenStream; import org.junit.jupiter.api.BeforeEach; @@ -979,6 +981,130 @@ void shouldRenderMixedOperatorsWithSqlPrecedence() { } } + @Nested + @DisplayName("Allowlist") + class AllowlistTests { + + @Test + @DisplayName("A presence check on an attribute outside the allowlist is invalid") + void shouldRejectAPresenceCheckOutsideTheAllowlist() { + Filter result = engine.parseFilter("secret pr", "t", null); + assertFalse(result.context().isValid(Set.of("name"))); + + result.toClause(); + assertFalse(result.context().isValid(Set.of("name"))); + assertTrue(result.context().isValid(Set.of("secret"))); + } + + @Test + @DisplayName("A presence check hidden among allowed comparisons is invalid") + void shouldRejectAPresenceCheckAmongAllowedComparisons() { + Filter result = engine.parseFilter("name eq \"John\" and secret pr and age gt 21", "t", null); + result.toClause(); + + assertFalse(result.context().isValid(Set.of("name", "age"))); + assertTrue(result.context().isValid(Set.of("name", "age", "secret"))); + } + + @ParameterizedTest(name = "is invalid [{0}]") + @ValueSource( + strings = { + "not (secret pr)", + "name eq \"John\" or (age gt 21 and (secret pr))", + "not (name eq \"John\" and not (secret pr))" + }) + @DisplayName("A presence check is seen wherever it is nested") + void shouldSeeAPresenceCheckWhereverItIsNested(String filter) { + Filter result = engine.parseFilter(filter, "t", null); + assertFalse(result.context().isValid(Set.of("name", "age"))); + } + + @Test + @DisplayName("The allowlist applies before the clause is rendered") + void shouldApplyTheAllowlistBeforeRendering() { + Filter result = engine.parseFilter("name eq \"John\" and age gt 21", "t", null); + + assertFalse(result.context().isValid(Set.of("name"))); + assertTrue(result.context().isValid(Set.of("name", "age"))); + } + + @Test + @DisplayName("The allowlist applies to a list check before the clause is rendered") + void shouldApplyTheAllowlistToAListCheckBeforeRendering() { + Filter result = engine.parseFilter("id in [1, 2]", "t", null); + + assertFalse(result.context().isValid(Set.of("name"))); + assertTrue(result.context().isValid(Set.of("id"))); + } + + @Test + @DisplayName("An aliased attribute is checked by its full path") + void shouldCheckAnAliasedAttributeByItsFullPath() { + Filter result = engine.parseFilter("u.userName pr and u.email eq \"a@b.c\"", "t", null); + + assertFalse(result.context().isValid(Set.of("userName", "email"))); + assertTrue(result.context().isValid(Set.of("u.userName", "u.email"))); + } + + @Test + @DisplayName("A custom comparison builder does not hide the attribute") + void shouldSeeTheAttributeThroughACustomComparisonBuilder() { + Filter result = + engine.parseFilter("tags eq \"admin\"", "t", ComparisonFilter.ListFilter::new); + + assertFalse(result.context().isValid(Set.of("name"))); + assertTrue(result.context().isValid(Set.of("tags"))); + } + + @Test + @DisplayName("Rendering does not change the verdict") + void shouldGiveTheSameVerdictBeforeAndAfterRendering() { + Filter result = + engine.parseFilter("name eq \"John\" and age gt 21 and id in [1, 2]", "t", null); + var before = result.context().isValid(Set.of("name", "age", "id")); + result.toClause(); + result.toClause(); + + assertTrue(before); + assertTrue(result.context().isValid(Set.of("name", "age", "id"))); + assertFalse(result.context().isValid(Set.of("name", "age"))); + } + + @Test + @DisplayName("A builder that renames the attribute is checked by the new name only") + void shouldCheckARenamedAttributeByItsNewName() { + Filter result = + engine.parseFilter( + "email eq \"a@b.c\"", + "t", + cf -> + new ComparisonFilter( + new AttributeFilter("emailAddress", null, "u", cf.context()), + cf.operator(), + cf.value(), + cf.context())); + + assertTrue(result.context().isValid(Set.of("emailAddress"))); + assertFalse(result.context().isValid(Set.of("email"))); + assertEquals("u.email_address = :emailAddress1", result.toClause()); + assertTrue(result.context().isValid(Set.of("emailAddress"))); + } + + @Test + @DisplayName("A client filter can be checked on its own before it is scoped") + void shouldCheckAClientFilterBeforeScoping() { + var clientFilter = "name eq \"John\""; + var clientIsValid = + engine.parseFilter(clientFilter, "t", null).context().isValid(Set.of("name")); + var scoped = engine.scopeFilter("not (deletedAt pr)", clientFilter); + + assertTrue(clientIsValid); + assertFalse(engine.parseFilter(scoped, "t", null).context().isValid(Set.of("name"))); + assertTrue( + engine.parseFilter(scoped, "t", null).context().isValid(Set.of("name", "deletedAt"))); + } + } + @Nested @DisplayName("Scope Filter") class ScopeFilterTests { From 6347345c0860bc23666e9681e8c965ceed839881 Mon Sep 17 00:00:00 2001 From: Uday Chandra Date: Mon, 5 Oct 2026 06:06:04 +0000 Subject: [PATCH 3/5] Reject attribute paths deeper than alias.attribute Only the first two segments of an attribute path were ever used: emails.work.value rendered as emails.work, so a filter silently applied to a different column than the one named. RFC 7644 allows one sub-attribute, and that is what the evaluator supports. The lexer, which already bounds what the parser may see, now rejects a third segment with a FilterSyntaxException that says only alias.attribute is supported and points at the segment. The grammar and the generated parser are unchanged. --- .../ai/singlr/scimsql/BoundedScimLexer.java | 18 +++---- .../java/ai/singlr/scimsql/ScimEngine.java | 4 +- .../ai/singlr/scimsql/ScimEngineTest.java | 48 +++++++++++++------ 3 files changed, 46 insertions(+), 24 deletions(-) diff --git a/src/main/java/ai/singlr/scimsql/BoundedScimLexer.java b/src/main/java/ai/singlr/scimsql/BoundedScimLexer.java index 8389bee..1b24709 100644 --- a/src/main/java/ai/singlr/scimsql/BoundedScimLexer.java +++ b/src/main/java/ai/singlr/scimsql/BoundedScimLexer.java @@ -9,19 +9,21 @@ import org.antlr.v4.runtime.Token; /** - * A lexer that rejects a filter before it grows past what the parser and the filter tree can - * handle. + * A lexer that rejects a filter the parser and the evaluator cannot handle faithfully. * - *

Parsing and evaluating a filter recurses once per level of parentheses, once per logical - * operator and once per attribute path segment. Without a bound, a filter built for the purpose - * exhausts the thread's stack. The limits are far above any filter written for a real query. A list - * of values ({@code in [...]}) does not recurse and is not limited. + *

Parsing and evaluating a filter recurses once per level of parentheses and once per logical + * operator. Without a bound, a filter built for the purpose exhausts the thread's stack. The limits + * are far above any filter written for a real query. A list of values ({@code in [...]}) does not + * recurse and is not limited. + * + *

An attribute path is {@code alias.attribute} at most. That is all the evaluator uses, and a + * deeper path would otherwise be cut short without notice. */ final class BoundedScimLexer extends ScimLexer { static final int MAX_NESTING_DEPTH = 50; static final int MAX_LOGICAL_OPERATORS = 500; - static final int MAX_PATH_SEGMENTS = 10; + static final int MAX_PATH_SEGMENTS = 2; private int nestingDepth; private int logicalOperators; @@ -48,7 +50,7 @@ public Token nextToken() { } case DOT -> { if (++pathSegments > MAX_PATH_SEGMENTS) { - reject(token, "attribute path longer than " + MAX_PATH_SEGMENTS + " segments"); + reject(token, "attribute path deeper than alias.attribute"); } } case ATTRNAME -> {} diff --git a/src/main/java/ai/singlr/scimsql/ScimEngine.java b/src/main/java/ai/singlr/scimsql/ScimEngine.java index a476d91..091ed0c 100644 --- a/src/main/java/ai/singlr/scimsql/ScimEngine.java +++ b/src/main/java/ai/singlr/scimsql/ScimEngine.java @@ -32,8 +32,8 @@ public class ScimEngine { * @param compareFilterBuilder customises how comparisons render, or {@code null} for the default * @return the parsed filter * @throws FilterSyntaxException if the expression is not one complete filter, holds a whole - * number that does not fit, or exceeds a limit: 50 levels of parentheses, 500 logical - * operators, or 10 segments in an attribute path + * number that does not fit, names an attribute path deeper than {@code alias.attribute}, or + * exceeds a limit: 50 levels of parentheses or 500 logical operators * @throws IllegalArgumentException if the expression or the prefix is missing; that is a mistake * in the calling code, never in a client's filter */ diff --git a/src/test/java/ai/singlr/scimsql/ScimEngineTest.java b/src/test/java/ai/singlr/scimsql/ScimEngineTest.java index f985840..aa60264 100644 --- a/src/test/java/ai/singlr/scimsql/ScimEngineTest.java +++ b/src/test/java/ai/singlr/scimsql/ScimEngineTest.java @@ -159,7 +159,7 @@ void shouldHandleInOperator() { void shouldHandleComplexNestedOperators() { Filter result = engine.parseFilter( - "emails.work.value co \"@example.com\" and (age gt 25 or active eq true)", "t", null); + "emails.value co \"@example.com\" and (age gt 25 or active eq true)", "t", null); result.toClause(); var map = result.context().params(); assertEquals(3, map.size()); @@ -834,27 +834,47 @@ void shouldRejectLogicalOperatorsPastTheLimit() { assertTrue(ex.getMessage().endsWith("more than 500 logical operators"), ex.getMessage()); } - @Test - @DisplayName("Attribute path up to the limit") - void shouldAcceptAnAttributePathUpToTheLimit() { - var filter = "a" + ".b".repeat(9) + " pr"; - assertNotNull(engine.parseFilter(filter, "t", null).toClause()); + @ParameterizedTest(name = "rejects [{0}]") + @ValueSource( + strings = { + "emails.work.value co \"x\"", + "a.b.c pr", + "a.b.c.d eq 1", + "u.name.x in [1, 2]", + "name pr and a.b.c pr", + "a..b pr", + "a. pr", + "a.b. pr" + }) + @DisplayName("An attribute path deeper than alias.attribute") + void shouldRejectAnAttributePathDeeperThanAliasAndAttribute(String filter) { + assertThrows(FilterSyntaxException.class, () -> engine.parseFilter(filter, "t", null)); } @Test - @DisplayName("Attribute path past the limit") - void shouldRejectAnAttributePathPastTheLimit() { - var filter = "a" + ".b".repeat(10) + " pr"; + @DisplayName("The error points at the segment that is not supported") + void shouldPointAtTheUnsupportedPathSegment() { + var filter = "emails.work.value co \"x\""; var ex = assertThrows(FilterSyntaxException.class, () -> engine.parseFilter(filter, "t", null)); - assertTrue( - ex.getMessage().endsWith("attribute path longer than 10 segments"), ex.getMessage()); + assertEquals( + "Failed to parse filter: Invalid filter syntax at position " + + "emails.work".length() + + ": attribute path deeper than alias.attribute", + ex.getMessage()); + } + + @Test + @DisplayName("An aliased attribute is kept whole") + void shouldKeepAnAliasedAttributeWhole() { + Filter result = engine.parseFilter("u.userName eq \"john\" and active pr", "t", null); + assertEquals("u.user_name = :u_userName1 AND t.active IS NOT NULL", result.toClause()); } @Test - @DisplayName("Attribute paths do not add up across the filter") - void shouldCountPathSegmentsPerAttribute() { - var filter = "a.b.c pr" + " and a.b.c pr".repeat(50); + @DisplayName("Aliased attributes do not add up across the filter") + void shouldAcceptManyAliasedAttributes() { + var filter = "a.b pr" + " and a.b pr".repeat(50); assertNotNull(engine.parseFilter(filter, "t", null).toClause()); } From fbf83fc9ac162da653e958816f1409d9287c315c Mon Sep 17 00:00:00 2001 From: Uday Chandra Date: Mon, 5 Oct 2026 06:06:57 +0000 Subject: [PATCH 4/5] Keep parameter keys apart for attributes that flatten to the same name A parameter key is the attribute path with its dot flattened to an underscore plus a counter, and the counter ran per attribute path. `u.name` and `u_name` both started at `u_name1`, so the second value overwrote the first and a filter using both bound one comparison to the other's value. The counter now runs per flattened name. Keys are unchanged for every filter that did not collide. --- src/main/java/ai/singlr/scimsql/Context.java | 15 ++++++- .../ai/singlr/scimsql/ScimEngineTest.java | 44 +++++++++++++++++++ 2 files changed, 57 insertions(+), 2 deletions(-) diff --git a/src/main/java/ai/singlr/scimsql/Context.java b/src/main/java/ai/singlr/scimsql/Context.java index 31d47ea..2d117d8 100644 --- a/src/main/java/ai/singlr/scimsql/Context.java +++ b/src/main/java/ai/singlr/scimsql/Context.java @@ -17,11 +17,13 @@ public class Context { private final Map> params; private final Map indexedParams; private final Set attributes; + private final Map keyCounts; public Context() { this.params = new HashMap<>(); this.indexedParams = new HashMap<>(); this.attributes = new HashSet<>(); + this.keyCounts = new HashMap<>(); } /** @@ -41,7 +43,7 @@ public String process(Filter attribute, Filter value, Function k List values = params.computeIfAbsent(key, k -> new ArrayList<>()); values.add(rawValue); - var indexedKey = key.replace(".", "_") + values.size(); + var indexedKey = nextIndexedKey(key); indexedParams.put(indexedKey, rawValue); return keyMapper.apply(indexedKey); @@ -66,7 +68,7 @@ public String processArray( values.add(rawValue); - var indexedKey = key.replace(".", "_") + values.size(); + var indexedKey = nextIndexedKey(key); indexedParams.put(indexedKey, rawValue); var paramKey = keyMapper.apply(indexedKey); @@ -83,6 +85,15 @@ public String processArray( return builder.toString(); } + /** + * A parameter key is the attribute path with its dot flattened to an underscore, plus a counter. + * The counter runs per flattened name, so {@code u.name} and {@code u_name} never share a key. + */ + private String nextIndexedKey(String attributeKey) { + var flattened = attributeKey.replace(".", "_"); + return flattened + keyCounts.merge(flattened, 1, Integer::sum); + } + /** * Checks the filter against an allowlist of attributes. * diff --git a/src/test/java/ai/singlr/scimsql/ScimEngineTest.java b/src/test/java/ai/singlr/scimsql/ScimEngineTest.java index aa60264..2ad636c 100644 --- a/src/test/java/ai/singlr/scimsql/ScimEngineTest.java +++ b/src/test/java/ai/singlr/scimsql/ScimEngineTest.java @@ -13,6 +13,7 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.List; +import java.util.Map; import java.util.Random; import java.util.Set; import org.antlr.v4.runtime.CharStreams; @@ -1001,6 +1002,49 @@ void shouldRenderMixedOperatorsWithSqlPrecedence() { } } + @Nested + @DisplayName("Parameter Keys") + class ParameterKeyTests { + + @Test + @DisplayName("An aliased attribute and an underscore attribute keep separate parameters") + void shouldKeepSeparateParametersForNamesThatFlattenAlike() { + Filter result = engine.parseFilter("u.name eq \"alice\" and u_name eq \"bob\"", "t", null); + + assertEquals("u.name = :u_name1 AND t.u_name = :u_name2", result.toClause()); + assertEquals(Map.of("u_name1", "alice", "u_name2", "bob"), result.context().indexedParams()); + } + + @Test + @DisplayName("List checks on names that flatten alike keep separate parameters") + void shouldKeepSeparateListParametersForNamesThatFlattenAlike() { + Filter result = engine.parseFilter("u.id in [1, 2] and u_id in [3]", "t", null); + + assertEquals("u.id IN (:u_id1, :u_id2) AND t.u_id IN (:u_id3)", result.toClause()); + assertEquals(Map.of("u_id1", 1L, "u_id2", 2L, "u_id3", 3L), result.context().indexedParams()); + } + + @Test + @DisplayName( + "A comparison and a list check on names that flatten alike keep separate parameters") + void shouldKeepSeparateParametersAcrossComparisonsAndListChecks() { + Filter result = engine.parseFilter("u_id eq 9 and u.id in [1, 2]", "t", null); + + assertEquals("t.u_id = :u_id1 AND u.id IN (:u_id2, :u_id3)", result.toClause()); + assertEquals(Map.of("u_id1", 9L, "u_id2", 1L, "u_id3", 2L), result.context().indexedParams()); + } + + @Test + @DisplayName("Repeated use of one attribute still counts from one") + void shouldCountRepeatedUseOfOneAttributeFromOne() { + Filter result = engine.parseFilter("name eq \"a\" or name eq \"b\" or age gt 1", "t", null); + + assertEquals("t.name = :name1 OR t.name = :name2 OR t.age > :age1", result.toClause()); + assertEquals( + Map.of("name", List.of("a", "b"), "age", List.of(1L)), result.context().params()); + } + } + @Nested @DisplayName("Allowlist") class AllowlistTests { From a698a7b9558f252d8c508ace5d649dd96ece4c33 Mon Sep 17 00:00:00 2001 From: Uday Chandra Date: Mon, 5 Oct 2026 06:07:29 +0000 Subject: [PATCH 5/5] Describe the allowlist, attribute paths and release step as they now work The README explains that the allowlist covers every attribute a filter uses, including presence checks, that a path deeper than alias.attribute is rejected, and how the allowlist relates to a scoped filter. The limitations that are fixed are removed from Known Limitations. RELEASE.md sets the README's dependency version as part of the release commit, so the README stops naming an older release. --- README.md | 15 +++++++++------ RELEASE.md | 4 +++- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 9f65cd3..d07d7e2 100644 --- a/README.md +++ b/README.md @@ -95,7 +95,6 @@ A filter is also rejected with `FilterSyntaxException` when it exceeds one of th |-------|-------| | Levels of nested parentheses | 50 | | Logical operators (`and`, `or`) | 500 | -| Segments in one attribute path | 10 | A list of values (`in [...]`) is not limited. A whole number that does not fit in a `long` is rejected the same way. @@ -120,6 +119,8 @@ The result is itself a filter expression, so it fits wherever a single filter is Do not join filter text by hand. `scope + " and " + clientFilter` renders as `owner AND status OR status`, and a client filter containing `or` then matches rows outside the scope. +The allowlist sees the scope's attributes too. Either check the client filter against its allowlist before scoping it, or include the scope's attributes when checking the scoped filter. A client that names a scope attribute can only narrow the result within the scope. + ## Typed Value Prefixes Values can carry type hints that produce SQL `CAST` expressions. Prefix the value inside the quotes: @@ -159,7 +160,7 @@ engine.parseFilter("u.userName eq \"john\" and active eq true", "p", null).toCla // → "u.user_name = :u_userName1 AND p.active = :active1" ``` -The dot becomes an underscore in the parameter key. Only `alias.attribute` is supported; see [Known Limitations](#known-limitations). +The dot becomes an underscore in the parameter key. Only `alias.attribute` is supported. A deeper path such as `emails.work.value` is rejected with `FilterSyntaxException`. ## Parameter Binding @@ -175,7 +176,12 @@ var params = filter.context().indexedParams(); // params = {name1=John, age1=21} ``` -Use `context().isValid(Set.of("name", "age"))` to allowlist which attributes callers are permitted to filter on. An attribute with its own alias is checked by its full path (`u.userName`). Call it after `toClause()`, and read [Known Limitations](#known-limitations) before relying on it alone. +Use `context().isValid(Set.of("name", "age"))` to allowlist which attributes callers are permitted to filter on. Every attribute the filter uses must be listed, whether it appears in a comparison, a list check or a presence check (`pr`). An attribute with its own alias is listed by its full path (`u.userName`). The answer is the same before and after `toClause()`. + +```java +engine.parseFilter("name eq \"John\" and secret pr", "p", null).context().isValid(Set.of("name")); +// → false +``` ## Custom Filter Builders @@ -193,9 +199,6 @@ This lets you intercept `ComparisonFilter` instances and return a subclass with ## Known Limitations -- **Attribute paths deeper than `alias.attribute`.** Segments after the second are ignored: `emails.work.value co "x"` renders as `LOWER(emails.work) LIKE …`. Do not rely on deeper paths. -- **`Context.isValid` before rendering.** The context is filled by `toClause()`. Called earlier, `isValid` has nothing to check and returns `true`. -- **`Context.isValid` and `pr`.** A presence check binds no parameter, so its attribute is not seen: `secret pr` passes an allowlist that does not contain `secret`. To allowlist strictly, also walk the filter tree and check every `PresentFilter`. - **`eq null` and `ne null`.** They render as `= :param` and `!= :param` with a null value, which SQL never matches. Use `pr` and `not (… pr)` to test for a value. ## PostgreSQL Query Analysis diff --git a/RELEASE.md b/RELEASE.md index 06aca29..b1883ac 100644 --- a/RELEASE.md +++ b/RELEASE.md @@ -99,10 +99,12 @@ git checkout -b release/1.0.0 ### 3. Set the release version +Set the same version in the dependency snippet in `README.md` first, so the README never names an older release. + ```bash mvn versions:set -DnewVersion=1.0.0 mvn versions:commit -git add pom.xml +git add pom.xml README.md git commit -m "Release 1.0.0" ```