jerryshao commented on code in PR #13478: URL: https://github.com/apache/gravitino/pull/13478#discussion_r4245442165
########## core/src/main/java/org/apache/gravitino/policy/rego/CanonicalExpression.java: ########## @@ -0,0 +1,685 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.policy.rego; + +import com.google.common.base.Preconditions; +import java.math.BigDecimal; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Objects; + +/** + * A node in an unresolved {@code restricted-rego-v1} expression tree. + * + * <p>The tree contains only the source constructs allowlisted by the read-restriction contract. + * Column and context references remain symbolic until the effective-policy resolver binds them to a + * table schema and request context. + */ +public interface CanonicalExpression { + + /** + * Validates this expression node and all of its children. + * + * @throws IllegalArgumentException if the expression is malformed or outside the allowlisted + * source profile + */ + void validate() throws IllegalArgumentException; + + /** + * Returns the operation depth defined by {@code restricted-rego-v1}. + * + * <p>Value leaves such as a column, string, or number have depth 0. Atomic predicates such as a + * comparison, Boolean literal, or group-membership check have depth 1. Negation and a logical + * expression add one to their deepest operand. A consecutive n-ary logical chain counts as one + * operation, so {@code true and false and true} has depth 2. + * + * @return expression operation depth + */ + int depth(); + + /** Comparison operators supported by {@code restricted-rego-v1}. */ + enum ComparisonOperator { + /** Equality comparison. */ + EQ("=="), + /** Inequality comparison. */ + NEQ("!="), + /** Less-than comparison. */ + LT("<"), + /** Less-than-or-equal comparison. */ + LTE("<="), + /** Greater-than comparison. */ + GT(">"), + /** Greater-than-or-equal comparison. */ + GTE(">="), + /** Literal-array membership comparison. */ + IN("in"); + + private final String sourceToken; + + ComparisonOperator(String sourceToken) { + this.sourceToken = sourceToken; + } + + /** + * Parses an operator token from restricted Rego source. + * + * @param token source operator token + * @return parsed operator + */ + public static ComparisonOperator fromSourceToken(String token) { + Preconditions.checkArgument(token != null && !token.isEmpty(), "operator cannot be empty"); + switch (token) { + case "==": + return EQ; + case "!=": + return NEQ; + case "<": + return LT; + case "<=": + return LTE; + case ">": + return GT; + case ">=": + return GTE; + case "in": + return IN; + default: + throw new IllegalArgumentException("Unsupported restricted-rego-v1 operator: " + token); + } + } + + /** + * Returns the token used in restricted Rego source. + * + * @return source token + */ + public String sourceToken() { + return sourceToken; + } + } + + /** N-ary logical operators supported by {@code restricted-rego-v1}. */ + enum LogicalOperator { + /** Boolean conjunction. */ + AND("and"), + /** Boolean disjunction. */ + OR("or"); + + private final String sourceToken; + + LogicalOperator(String sourceToken) { + this.sourceToken = sourceToken; + } + + /** + * Returns the token used in restricted Rego source. + * + * @return source token + */ + public String sourceToken() { + return sourceToken; + } + } + + /** Source literal types supported by {@code restricted-rego-v1}. */ + enum LiteralType { + /** String literal. */ + STRING, + /** Exact base-10 numeric literal. */ + NUMBER, + /** Boolean literal. */ + BOOLEAN, + /** Null literal. */ + NULL + } + + /** A binary comparison operation. */ + final class Comparison implements CanonicalExpression { + private final ComparisonOperator operator; + private final CanonicalExpression left; + private final CanonicalExpression right; + + Comparison(ComparisonOperator operator, CanonicalExpression left, CanonicalExpression right) { + this.operator = operator; + this.left = left; + this.right = right; + } + + /** + * Returns the comparison operator. + * + * @return comparison operator + */ + public ComparisonOperator operator() { + return operator; + } + + /** + * Returns the left operand. + * + * @return left operand + */ + public CanonicalExpression left() { + return left; + } + + /** + * Returns the right operand. + * + * @return right operand + */ + public CanonicalExpression right() { + return right; + } + + @Override + public void validate() throws IllegalArgumentException { + Preconditions.checkArgument(operator != null, "comparison operator cannot be null"); + Preconditions.checkArgument( + left != null && right != null, "comparison operands cannot be null"); + left.validate(); + right.validate(); + try { + ExpressionValidator.validateComparison(operator, left, right); + } catch (IllegalArgumentException exception) { + throw new IllegalArgumentException( + exception.getMessage() + "; offending comparison: " + this, exception); Review Comment: [Nit] Semantic errors embed the whole offending sub-expression, so a small invalid source becomes a very large 400 body. `this` is the full `Comparison`, and its `toString()` expands every literal - each array element prints as `Literal{literalType=NUMBER, value=N, negativeZero=false}`, roughly 55 characters of wrapper per element. Measured: `filter := session_user() in [0..255]` is a 1198-byte source and yields a 15,422-character message; `filter := [256 strings] in col("x")` is 11,944 bytes and yields 25,652. `PolicyExceptionHandler` maps `IllegalArgumentException` to 400, so that whole string is returned to the caller and written to the server log on every rejected create or alter. The amplification is bounded - the source is capped at 16 KiB, so the worst case is a few tens of KB - so this is not a denial-of-service concern, just a lot of response and log volume for a typo. Truncating the appended sub-expression, or printing only the operator and the operand kinds (which is the part that actually helps the author), would keep the message useful and short. Verified by: compiled the four files under `core/src/main/java/org/apache/gravitino/policy/rego` standalone (stubbing `Preconditions`, `StringUtils`, `ReadRestrictionContent.MAX_SOURCE_LENGTH_BYTES`) and measured `getMessage().length()` for both inputs above. ########## core/src/main/java/org/apache/gravitino/policy/rego/RestrictedRegoProgram.java: ########## @@ -0,0 +1,417 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.policy.rego; + +import com.google.common.base.Preconditions; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Objects; +import org.apache.gravitino.policy.rego.CanonicalExpression.LogicalExpression; +import org.apache.gravitino.policy.rego.CanonicalExpression.LogicalOperator; +import org.apache.gravitino.policy.rego.CanonicalExpression.Not; + +/** A parsed, unresolved program in the {@code restricted-rego-v1} source dialect. */ +public interface RestrictedRegoProgram { + + /** + * Returns the rule type declared by this program. + * + * @return rule type + */ + RuleType ruleType(); + + /** + * Validates the rule and every expression it contains. + * + * @throws IllegalArgumentException if the rule is malformed + */ + void validate() throws IllegalArgumentException; + + /** Rule heads supported by {@code restricted-rego-v1}. */ + enum RuleType { + /** A row-filter rule. */ + FILTER, + /** A column-mask rule. */ + MASK + } + + /** Logical column-mask actions supported by the first restricted profile. */ + enum MaskAction { + /** Mask alphanumeric characters. */ + MASK_ALPHANUM("mask-alphanum"), + /** Replace the value with the action's fixed value. */ + MASK_TO_FIXED_VALUE("mask-to-fixed-value"), + /** Replace the value with null. */ + REPLACE_WITH_NULL("replace-with-null"), + /** Preserve only the first four characters. */ + SHOW_FIRST_4("show-first-4"), + /** Preserve only the last four characters. */ + SHOW_LAST_4("show-last-4"), + /** Truncate a temporal value to its year. */ + TRUNCATE_TO_YEAR("truncate-to-year"), + /** Truncate a temporal value to its month. */ + TRUNCATE_TO_MONTH("truncate-to-month"), + /** Apply a global SHA-256 mask. */ + SHA_256_GLOBAL("sha-256-global"), + /** Apply a query-local SHA-256 mask. */ + SHA_256_QUERY_LOCAL("sha-256-query-local"); + + private final String value; + + MaskAction(String value) { + this.value = value; + } + + /** + * Parses a mask-action value. + * + * @param value source action value + * @return parsed action + * @throws IllegalArgumentException if the action is not allowlisted + */ + public static MaskAction fromValue(String value) { + Preconditions.checkArgument(value != null && !value.isEmpty(), "mask action cannot be empty"); + for (MaskAction action : values()) { + if (action.value.equals(value)) { + return action; + } + } + throw new IllegalArgumentException("Unsupported restricted-rego-v1 mask action: " + value); + } + + /** + * Returns the source action value. + * + * @return source action value + */ + public String value() { + return value; + } + } + + /** One guarded result in a conditional row-filter rule. */ + final class FilterBranch { + private final CanonicalExpression result; + private final CanonicalExpression condition; + + FilterBranch(CanonicalExpression result, CanonicalExpression condition) { + this.result = result; + this.condition = condition; + } + + /** + * Returns the Boolean result selected by this branch. + * + * @return unresolved Boolean result + */ + public CanonicalExpression result() { + return result; + } + + /** + * Returns this branch's Boolean condition. + * + * @return unresolved Boolean condition + */ + public CanonicalExpression condition() { + return condition; + } + + /** + * Validates the branch. + * + * @throws IllegalArgumentException if its result or condition is not Boolean + */ + public void validate() throws IllegalArgumentException { + Preconditions.checkArgument(result != null, "filter branch result cannot be null"); + Preconditions.checkArgument(condition != null, "filter branch condition cannot be null"); + result.validate(); + condition.validate(); + Preconditions.checkArgument( + ExpressionValidator.isPredicate(result), "filter branch result must be Boolean"); + Preconditions.checkArgument( + ExpressionValidator.isPredicate(condition), "filter branch condition must be Boolean"); + } + + @Override + public boolean equals(Object other) { + if (!(other instanceof FilterBranch)) { + return false; + } + FilterBranch that = (FilterBranch) other; + return Objects.equals(result, that.result) && Objects.equals(condition, that.condition); + } + + @Override + public int hashCode() { + return Objects.hash(result, condition); + } + + @Override + public String toString() { + return "FilterBranch{" + "result=" + result + ", condition=" + condition + '}'; + } + } + + /** A complete {@code filter := ...} row-filter rule. */ + final class RowFilter implements RestrictedRegoProgram { + private final List<FilterBranch> branches; + private final CanonicalExpression fallback; + + RowFilter(List<FilterBranch> branches, CanonicalExpression fallback) { + this.branches = + branches == null ? null : Collections.unmodifiableList(new ArrayList<>(branches)); + this.fallback = fallback; + } + + @Override + public RuleType ruleType() { + return RuleType.FILTER; + } + + /** + * Returns the guarded results in source order. + * + * @return immutable branches, or an empty list for an unconditional rule + */ + public List<FilterBranch> branches() { + return branches; + } + + /** + * Returns the unconditional result or final {@code else} result. + * + * @return unresolved Boolean fallback + */ + public CanonicalExpression fallback() { + return fallback; + } + + /** + * Returns whether this rule contains at least one conditional branch. + * + * @return {@code true} for a conditional rule + */ + public boolean isConditional() { + return branches != null && !branches.isEmpty(); + } + + /** + * Lowers the ordered branches to one Boolean predicate with first-match semantics. + * + * <p>The returned predicate remains unresolved. Context and column binding can be applied to it + * by a later resolver. + * + * @return lowered unresolved predicate + */ + public CanonicalExpression lower() { + validate(); + if (branches.isEmpty()) { + return fallback; + } + + List<CanonicalExpression> disjuncts = new ArrayList<>(); + List<CanonicalExpression> precedingBranchMisses = new ArrayList<>(); + for (FilterBranch branch : branches) { + List<CanonicalExpression> selected = new ArrayList<>(precedingBranchMisses); + selected.add(branch.condition()); + selected.add(branch.result()); + disjuncts.add(LogicalExpression.of(LogicalOperator.AND, selected)); + precedingBranchMisses.add(new Not(branch.condition())); Review Comment: [Nit] `new Not(...)` is unfolded, so a negated branch condition becomes `not(not(c))` in the lowered predicate. When a branch condition is itself a `Not`, this wraps it again instead of folding it away. `filter := true if not is_group_member("g") else := false` lowers to a disjunct containing `Not{operand=Not{operand=GroupMembership{group='g'}}}`, and the lowered depth is 5 where the same filter with an un-negated condition is 4. The cost grows with branch count: k negated conditions put k(k+1)/2 redundant `Not` nodes into the lowered tree, all charged against the depth-8 and 256-node caps that save-time validation now enforces - so such a filter is rejected earlier than the documented limits would suggest. The redundant node also survives into the resolved predicate: the canonicalization step described in design-docs/tag-based-read-restrictions-for-iceberg-rest.md:504 folds Boolean constants and dedups commutative children, but says nothing about double negation. A small factory that returns `((Not) e).operand()` when `e` is already a `Not` - the same shape as `LogicalExpression.of` - would fix both the budget and the wire predicate. Verified by: lowered the three filters above with the standalone build and printed `depth()` plus the tree; the `Not{operand=Not{...}}` nodes are in the output, two of them for the two-branch case. ########## core/src/main/java/org/apache/gravitino/policy/rego/RestrictedRegoExpressionParserFacade.java: ########## @@ -0,0 +1,819 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.policy.rego; + +import com.google.common.base.Preconditions; +import java.math.BigDecimal; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.List; +import org.apache.commons.lang3.StringUtils; +import org.apache.gravitino.policy.ReadRestrictionContent; +import org.apache.gravitino.policy.rego.CanonicalExpression.Comparison; +import org.apache.gravitino.policy.rego.CanonicalExpression.ComparisonOperator; +import org.apache.gravitino.policy.rego.CanonicalExpression.GroupMembership; +import org.apache.gravitino.policy.rego.CanonicalExpression.Literal; +import org.apache.gravitino.policy.rego.CanonicalExpression.LiteralArray; +import org.apache.gravitino.policy.rego.CanonicalExpression.LiteralType; +import org.apache.gravitino.policy.rego.CanonicalExpression.LogicalExpression; +import org.apache.gravitino.policy.rego.CanonicalExpression.LogicalOperator; +import org.apache.gravitino.policy.rego.CanonicalExpression.Not; +import org.apache.gravitino.policy.rego.CanonicalExpression.SessionUser; +import org.apache.gravitino.policy.rego.RestrictedRegoProgram.ColumnMask; +import org.apache.gravitino.policy.rego.RestrictedRegoProgram.FilterBranch; +import org.apache.gravitino.policy.rego.RestrictedRegoProgram.MaskAction; +import org.apache.gravitino.policy.rego.RestrictedRegoProgram.MaskBranch; +import org.apache.gravitino.policy.rego.RestrictedRegoProgram.RowFilter; + +/** Parses and validates complete programs in the {@code restricted-rego-v1} source dialect. */ +public final class RestrictedRegoExpressionParserFacade { + + private static final int MAX_SOURCE_DEPTH = 8; + private static final int MAX_AST_NODES = 256; + private static final int MAX_STRING_BYTES = 4 * 1024; + private static final int MAX_ARRAY_ELEMENTS = 256; + private static final int MAX_TOTAL_ARRAY_ELEMENTS = 256; Review Comment: [Question] With both array limits set to 256, which budget can an author actually rely on? `MAX_ARRAY_ELEMENTS` (per array) and `MAX_TOTAL_ARRAY_ELEMENTS` (per program) are the same number, so the per-array limit is only reachable by a filter with no `if` branch. `filter := col("x") in [256 elements]` is accepted, but as soon as one conditional branch exists `lower()` duplicates the condition and the usable budget halves - the PR's own boundary test at TestRestrictedRegoExpressionParserFacade.java:409-421 pins 128 passing and 129 failing - and it keeps shrinking with branch count: a two-branch filter whose first condition carries an 86-element array is already rejected (86 x 3 = 258). docs/policies.md:113-114 lists the two numbers as if they were independent budgets (`256 elements in a literal array` and `256 array elements in total`), which gives an author no way to predict the 128 or the 86. Was 256 the intended total, or should the total be a multiple of the per-array limit? Either way, one sentence in the Limits section saying that conditional lowering repeats each branch condition, so the usable source budget shrinks as branches are added, would stop this being discovered through a 400. Verified by: ran all of these through the standalone build - single 256-element array accepted; `128 or 128` accepted; `128 or 129` rejected with `restricted-rego-v1 literal arrays must not contain more than 256 total elements`; the one-branch 129 and the two-branch 86 cases rejected with `lowered row-filter literal arrays must not contain more than 256 total elements`. ########## core/src/test/java/org/apache/gravitino/policy/rego/TestRestrictedRegoExpressionParserFacade.java: ########## @@ -0,0 +1,688 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.policy.rego; + +import java.io.IOException; +import java.io.InputStream; +import java.math.BigDecimal; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; +import org.apache.gravitino.policy.rego.CanonicalExpression.Column; +import org.apache.gravitino.policy.rego.CanonicalExpression.Comparison; +import org.apache.gravitino.policy.rego.CanonicalExpression.ComparisonOperator; +import org.apache.gravitino.policy.rego.CanonicalExpression.GroupMembership; +import org.apache.gravitino.policy.rego.CanonicalExpression.Literal; +import org.apache.gravitino.policy.rego.CanonicalExpression.LiteralArray; +import org.apache.gravitino.policy.rego.CanonicalExpression.LiteralType; +import org.apache.gravitino.policy.rego.CanonicalExpression.LogicalExpression; +import org.apache.gravitino.policy.rego.CanonicalExpression.LogicalOperator; +import org.apache.gravitino.policy.rego.CanonicalExpression.Not; +import org.apache.gravitino.policy.rego.CanonicalExpression.SessionUser; +import org.apache.gravitino.policy.rego.RestrictedRegoProgram.ColumnMask; +import org.apache.gravitino.policy.rego.RestrictedRegoProgram.MaskAction; +import org.apache.gravitino.policy.rego.RestrictedRegoProgram.RowFilter; +import org.apache.gravitino.policy.rego.RestrictedRegoProgram.RuleType; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +/** Tests for {@link RestrictedRegoExpressionParserFacade}. */ +public class TestRestrictedRegoExpressionParserFacade { + + private static final String GOLDEN_CASES_RESOURCE = "/restricted-rego-v1-cases.txt"; + private static final String GOLDEN_FILE = + "core/src/test/resources/restricted-rego-v1-cases.golden"; + private static final String UPDATE_GOLDEN_ENV = "GRAVITINO_UPDATE_RESTRICTED_REGO_GOLDEN"; + private static final String GOLDEN_LICENSE_HEADER = + "# Licensed to the Apache Software Foundation (ASF) under one or more\n" + + "# contributor license agreements. See the NOTICE file distributed with\n" + + "# this work for additional information regarding copyright ownership.\n" + + "# The ASF licenses this file to You under the Apache License, Version 2.0\n" + + "# (the \"License\"); you may not use this file except in compliance with\n" + + "# the License. You may obtain a copy of the License at\n" + + "#\n" + + "# http://www.apache.org/licenses/LICENSE-2.0\n"; + + "#\n" + + "# Unless required by applicable law or agreed to in writing, software\n" + + "# distributed under the License is distributed on an \"AS IS\" BASIS,\n" + + "# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.\n" + + "# See the License for the specific language governing permissions and\n" + + "# limitations under the License.\n"; + + @Test + void testParsesTypedRules() { + RowFilter filter = + RestrictedRegoExpressionParserFacade.parseRowFilter( + "filter := col(\"owner\") == session_user()"); + Assertions.assertEquals(RuleType.FILTER, filter.ruleType()); + Assertions.assertFalse(filter.isConditional()); + Assertions.assertInstanceOf(Comparison.class, filter.fallback()); + + ColumnMask mask = + RestrictedRegoExpressionParserFacade.parseColumnMask("mask := action(\"show-last-4\")"); + Assertions.assertEquals(RuleType.MASK, mask.ruleType()); + Assertions.assertFalse(mask.isConditional()); + Assertions.assertEquals(MaskAction.SHOW_LAST_4, mask.fallback()); + + Assertions.assertInstanceOf( + RowFilter.class, RestrictedRegoExpressionParserFacade.parse("filter := true")); + Assertions.assertInstanceOf( + ColumnMask.class, + RestrictedRegoExpressionParserFacade.parse("mask := action(\"mask-alphanum\")")); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + RestrictedRegoExpressionParserFacade.parseRowFilter("mask := action(\"show-last-4\")")); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> RestrictedRegoExpressionParserFacade.parseColumnMask("filter := true")); + } + + @Test + void testParsesContextReferences() { + Comparison comparison = (Comparison) parseFilterExpression("col(\"owner\") == session_user()"); + + Assertions.assertEquals(ComparisonOperator.EQ, comparison.operator()); + Assertions.assertEquals("owner", ((Column) comparison.left()).name()); + Assertions.assertInstanceOf(SessionUser.class, comparison.right()); + + GroupMembership membership = + (GroupMembership) parseFilterExpression("is_group_member(\"auditors\")"); + Assertions.assertEquals("auditors", membership.group()); + } + + @Test + void testParsesPrecedenceAndLiteralArray() { + LogicalExpression or = + (LogicalExpression) + parseFilterExpression( + "col(\"region\") in [\"US\", \"CA\"] or " + + "col(\"level\") >= 3 and not col(\"deleted\") == true"); + + Assertions.assertEquals(LogicalOperator.OR, or.operator()); + Assertions.assertEquals(2, or.operands().size()); + + Comparison membership = (Comparison) or.operands().get(0); + Assertions.assertEquals(ComparisonOperator.IN, membership.operator()); + LiteralArray values = (LiteralArray) membership.right(); + Assertions.assertEquals(LiteralType.STRING, values.elementType()); + Assertions.assertEquals( + List.of("US", "CA"), + List.of(values.values().get(0).value(), values.values().get(1).value())); + + LogicalExpression and = (LogicalExpression) or.operands().get(1); + Assertions.assertEquals(LogicalOperator.AND, and.operator()); + Assertions.assertEquals( + ComparisonOperator.GTE, ((Comparison) and.operands().get(0)).operator()); + Not not = (Not) and.operands().get(1); + Assertions.assertEquals(ComparisonOperator.EQ, ((Comparison) not.operand()).operator()); + } + + @Test + void testParsesConditionalRowFilterAndLowersFirstMatch() { + RowFilter filter = + RestrictedRegoExpressionParserFacade.parseRowFilter( + "filter := col(\"tenant\") == session_user() if is_group_member(\"member\") " + + "else := col(\"public\") == true if session_user() == \"guest\" " + + "else := false"); + + Assertions.assertTrue(filter.isConditional()); + Assertions.assertEquals(2, filter.branches().size()); + Assertions.assertInstanceOf(GroupMembership.class, filter.branches().get(0).condition()); + Assertions.assertEquals( + ComparisonOperator.EQ, ((Comparison) filter.branches().get(1).condition()).operator()); + Assertions.assertEquals(false, ((Literal) filter.fallback()).value()); + Assertions.assertThrows(UnsupportedOperationException.class, () -> filter.branches().add(null)); + + CanonicalExpression expected = + parseFilterExpression( + "(is_group_member(\"member\") and col(\"tenant\") == session_user()) or " + + "(not is_group_member(\"member\") and session_user() == \"guest\" and " + + "col(\"public\") == true) or " + + "(not is_group_member(\"member\") and " + + "not session_user() == \"guest\" and false)"); + Assertions.assertEquals(expected, filter.lower()); + Assertions.assertEquals(4, filter.lower().depth()); + } + + @Test + void testParsesConditionalColumnMask() { + ColumnMask mask = + RestrictedRegoExpressionParserFacade.parseColumnMask( + "mask := action(\"show-last-4\") if is_group_member(\"support\") " + + "else := action(\"sha-256-query-local\") if session_user() == \"service\" " + + "else := action(\"replace-with-null\")"); + + Assertions.assertTrue(mask.isConditional()); + Assertions.assertEquals(2, mask.branches().size()); + Assertions.assertEquals(MaskAction.SHOW_LAST_4, mask.branches().get(0).action()); + Assertions.assertEquals(MaskAction.SHA_256_QUERY_LOCAL, mask.branches().get(1).action()); + Assertions.assertEquals(MaskAction.REPLACE_WITH_NULL, mask.fallback()); + Assertions.assertInstanceOf(GroupMembership.class, mask.branches().get(0).condition()); + } + + @ParameterizedTest + @ValueSource( + strings = { + "mask-alphanum", + "mask-to-fixed-value", + "replace-with-null", + "show-first-4", + "show-last-4", + "truncate-to-year", + "truncate-to-month", + "sha-256-global", + "sha-256-query-local" + }) + void testAcceptsAllMaskActions(String action) { + ColumnMask mask = + RestrictedRegoExpressionParserFacade.parseColumnMask("mask := action(\"" + action + "\")"); + Assertions.assertEquals(action, mask.fallback().value()); + } + + @Test + void testRetainsExactNumbersAndNegativeZero() { + Comparison comparison = (Comparison) parseFilterExpression("col(\"ratio\") == -0.00"); + Literal literal = (Literal) comparison.right(); + + Assertions.assertEquals(new BigDecimal("0.00"), literal.value()); + Assertions.assertTrue(literal.negativeZero()); + + Comparison positiveZero = (Comparison) parseFilterExpression("col(\"ratio\") == 0.00"); + Assertions.assertNotEquals(literal, positiveZero.right()); + + Comparison one = (Comparison) parseFilterExpression("col(\"x\") == 1"); + Comparison onePointZero = (Comparison) parseFilterExpression("col(\"x\") == 1.00"); + Assertions.assertEquals(one.right(), onePointZero.right()); + } + + @Test + void testDecodesJsonStringsWithoutNormalizing() { + Comparison comparison = (Comparison) parseFilterExpression("col(\"a\\\"b.c\") == \"a'b\\n\""); + + Assertions.assertEquals("a\"b.c", ((Column) comparison.left()).name()); + Assertions.assertEquals("a'b\n", ((Literal) comparison.right()).value()); + + String escapedPair = "\\" + "uD83D" + "\\" + "uDE00"; + Comparison unicode = + (Comparison) parseFilterExpression("col(\"emoji\") == \"" + escapedPair + "\""); + Assertions.assertEquals("😀", ((Literal) unicode.right()).value()); + } + + @Test + void testSourceDepthLimit() { + CanonicalExpression depthEight = + parseFilterExpression("not not not not not not not col(\"active\") == true"); + Assertions.assertEquals(8, depthEight.depth()); + + Assertions.assertThrows( + IllegalArgumentException.class, + () -> parseFilterExpression("not not not not not not not not col(\"active\") == true")); + } + + @Test + void testFlattensAssociativeBooleanChains() { + LogicalExpression and = + (LogicalExpression) + parseFilterExpression( + "true and true and true and true and true and true and true and true and true"); + Assertions.assertEquals(LogicalOperator.AND, and.operator()); + Assertions.assertEquals(9, and.operands().size()); + Assertions.assertEquals(2, and.depth()); + } + + @Test + void testChecksLoweredRowFilterDepthAtSaveTime() { + Assertions.assertDoesNotThrow( + () -> + RestrictedRegoExpressionParserFacade.parseRowFilter( + "filter := true if not not not not col(\"active\") == true else := false")); + + IllegalArgumentException error = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + RestrictedRegoExpressionParserFacade.parseRowFilter( + "filter := true if not not not not not col(\"active\") == true " + + "else := false")); + Assertions.assertTrue(error.getMessage().contains("lowered row-filter depth")); + } + + @Test + void testFlattensLoweredLogicalOperands() { + RowFilter filter = + RestrictedRegoExpressionParserFacade.parseRowFilter( + "filter := true if is_group_member(\"g\") else := " + + "true and (true or (true and (true or (true and (true or true)))))"); + + Assertions.assertEquals(8, filter.lower().depth()); + LogicalExpression lowered = (LogicalExpression) filter.lower(); + LogicalExpression fallback = (LogicalExpression) lowered.operands().get(1); + Assertions.assertEquals(3, fallback.operands().size()); + } + + @Test + void testChecksLoweredRowFilterNodeCountAtSaveTime() { + Assertions.assertDoesNotThrow( + () -> + RestrictedRegoExpressionParserFacade.parseRowFilter(conditionalFilterWithBranches(9))); + + IllegalArgumentException error = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + RestrictedRegoExpressionParserFacade.parseRowFilter( + conditionalFilterWithBranches(10))); + Assertions.assertTrue(error.getMessage().contains("lowered row-filter AST")); + } + + @Test + void testSemanticErrorsIncludeOffendingSubExpression() { + IllegalArgumentException error = + Assertions.assertThrows( + IllegalArgumentException.class, () -> parseFilterExpression("col(\"b\") < true")); + + Assertions.assertTrue(error.getMessage().contains("boolean literal supports only == and !=")); + Assertions.assertTrue(error.getMessage().contains("offending comparison")); + Assertions.assertTrue(error.getMessage().contains("name='b'")); + } + + @Test + void testRejectsInvalidUnicodeScalarsAndNul() { + String isolatedSurrogate = "\\" + "uD800"; + String nul = "\\" + "u0000"; + Assertions.assertThrows( + IllegalArgumentException.class, + () -> parseFilterExpression("col(\"" + isolatedSurrogate + "\") == \"x\"")); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> parseFilterExpression("col(\"x\") == \"" + isolatedSurrogate + "\"")); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> parseFilterExpression("col(\"" + nul + "\") == \"x\"")); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> parseFilterExpression("is_group_member(\"" + nul + "\")")); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> parseFilterExpression("col(\"x\") == \"" + nul + "\"")); + } + + @Test + void testRejectsNonAsciiUnicodeEscapeDigits() { + List<String> invalidEscapes = + List.of( + unicodeEscape(0x0660, 0x0660, 0x0664, 0x0661), + unicodeEscape(0xFF10, 0xFF10, 0xFF14, 0xFF11), + unicodeEscape(0x0966, 0x0966, 0x096A, 0x0967), + unicodeEscape('0', '0', 0x0664, 0x0661)); + + for (String invalidEscape : invalidEscapes) { + IllegalArgumentException error = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> parseFilterExpression("col(\"" + invalidEscape + "\") == \"x\"")); + Assertions.assertTrue(error.getMessage().contains("invalid Unicode escape")); + } + } + + @Test + void testResourceLimits() { + String sourcePrefix = "filter := true"; + String maximumSource = + sourcePrefix + " ".repeat(16 * 1024 - sourcePrefix.getBytes(StandardCharsets.UTF_8).length); + Assertions.assertDoesNotThrow( + () -> RestrictedRegoExpressionParserFacade.parseRowFilter(maximumSource)); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> RestrictedRegoExpressionParserFacade.parseRowFilter(maximumSource + " ")); + + String maximumString = "a".repeat(4 * 1024); + Assertions.assertDoesNotThrow( + () -> parseFilterExpression("col(\"value\") == \"" + maximumString + "\"")); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> parseFilterExpression("col(\"value\") == \"" + maximumString + "a\"")); + + String maximumUnicodeString = "😀".repeat(1024); + Assertions.assertDoesNotThrow( + () -> parseFilterExpression("col(\"value\") == \"" + maximumUnicodeString + "\"")); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> parseFilterExpression("col(\"value\") == \"" + maximumUnicodeString + "😀\"")); + + Assertions.assertDoesNotThrow( + () -> parseFilterExpression("col(\"value\") in " + literalArray(256))); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> parseFilterExpression("col(\"value\") in " + literalArray(257))); + + Assertions.assertDoesNotThrow( + () -> + parseFilterExpression( + "col(\"left\") in " + + literalArray(128) + + " or col(\"right\") in " + + literalArray(128))); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + parseFilterExpression( + "col(\"left\") in " + + literalArray(128) + + " or col(\"right\") in " + + literalArray(129))); + + String maximumNumber = "1" + "0".repeat(255); + Assertions.assertDoesNotThrow( + () -> parseFilterExpression("col(\"value\") == " + maximumNumber)); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> parseFilterExpression("col(\"value\") == " + maximumNumber + "0")); + + Assertions.assertDoesNotThrow(() -> parseFilterExpression(balancedAndExpression(0, 85))); + Assertions.assertThrows( + IllegalArgumentException.class, () -> parseFilterExpression(balancedAndExpression(0, 86))); + } + + @Test + void testChecksLoweredRowFilterArrayElementCountAtSaveTime() { + Assertions.assertDoesNotThrow( + () -> + RestrictedRegoExpressionParserFacade.parseRowFilter( + "filter := true if col(\"value\") in " + literalArray(128) + " else := false")); + + IllegalArgumentException error = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + RestrictedRegoExpressionParserFacade.parseRowFilter( + "filter := true if col(\"value\") in " + literalArray(129) + " else := false")); + Assertions.assertTrue(error.getMessage().contains("lowered row-filter literal arrays")); + } + + @Test + void testRestrictedRegoGoldenFile() throws IOException { + String actual = renderGoldenResults(); + Path goldenFile = repositoryRoot().resolve(GOLDEN_FILE); + if (Boolean.parseBoolean(System.getenv(UPDATE_GOLDEN_ENV))) { + Files.writeString(goldenFile, actual, StandardCharsets.UTF_8); + } + + String expected = Files.readString(goldenFile, StandardCharsets.UTF_8); + Assertions.assertEquals( + expected, + actual, + "Golden output changed. Review the parser behavior and regenerate with " + + UPDATE_GOLDEN_ENV + + "=true if the change is intentional."); + } + + @Test + void testParsesDocumentedPrograms() throws IOException { + List<String> programs = documentedPrograms(repositoryRoot().resolve("docs/policies.md")); + + Assertions.assertEquals(2, programs.size()); Review Comment: [Nit] This guard against documentation drift does not run on a docs-only change, and the fixed count of 2 turns a new example into a failure. `docs/policies.md` is read from the working tree, but it is not an input of the `test` task: neither `core/build.gradle.kts:106-113` nor the root `tasks.configureEach<Test>` block (`build.gradle.kts:976`) declares it, sets a `workingDir`, or disables up-to-date checks. So editing only the docs leaves `:core:test` UP-TO-DATE and this test is skipped - which is exactly the workflow where drift happens. Declaring the input, for example `tasks.test { inputs.file(rootProject.file("docs/policies.md")) }`, closes it. The golden file happens to be covered already, but only incidentally: it sits in `src/test/resources`, so it reaches the task through the runtime classpath even though the test reads it from the source tree. Two smaller points on the same helper. `Assertions.assertEquals(2, programs.size())` means adding a third documented program fails the test rather than being parsed; asserting the list is non-empty and parsing every entry would age better. And `repositoryRoot()` (line 613) re-derives the root by walking parents looking for `docs/policies.md`, while `core/build.gradle.kts:106-113` already passes `GRAVITINO_HOME` (the repo root in the default embedded mode) - a declared input or an explicit system property would be shorter and independent of the test working directory. Verified by: read both build files looking for a `workingDir`, `inputs` or `upToDateWhen` declaration on the `test` task - there is none - and read the helper at lines 613-647. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
