Integ test cases for field-level security #5008
Conversation
Signed-off-by: Simeon Widdis <[email protected]>
Signed-off-by: Simeon Widdis <[email protected]>
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds SecurityTestBase utilities for test-auth, a new FGACIndexScanningIT integration test covering index/field/row-level FGAC with background scanning, refactors PPLPermissionsIT to use the new base class, and updates test dependencies to include JUnit parameterized support. Changes
Sequence Diagram(s)sequenceDiagram
actor Test
participant Base as SecurityTestBase
participant OpenSearch
participant Auth as Auth/Authorization
rect rgb(230,240,255)
Note over Test,Base: Setup Phase
Test->>Base: createRoleWithIndexAccess(role, indexPattern)
Base->>OpenSearch: HTTP role creation (index/DLS/FLS/permissions)
OpenSearch-->>Base: 200 OK
Test->>Base: createUser(username, role)
Base->>OpenSearch: HTTP user creation & role-mapping
OpenSearch-->>Base: 200 OK
end
rect rgb(240,255,230)
Note over Test,Base: Bulk Load Phase
Test->>Base: performBulkInsert(bulkBody)
Base->>OpenSearch: Bulk API request (with refresh)
OpenSearch-->>Base: Bulk response
end
rect rgb(255,240,230)
Note over Test,Base: Authenticated Query Phase
Test->>Base: executeQueryAsUser(query, username)
Base->>Base: createBasicAuthHeader(username, password)
Base->>OpenSearch: PPL query + Basic Auth header
OpenSearch->>Auth: Validate credentials & evaluate roles
Auth-->>OpenSearch: Authorization decision
OpenSearch-->>Base: Query results (filtered by FGAC)
Base-->>Test: JSONObject response
end
rect rgb(245,235,255)
Note over Test: Verification
Test->>Test: Assert results match expected FGAC behavior
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
Signed-off-by: Simeon Widdis <[email protected]>
Swiddis
force-pushed
the
fix/background-io-thread-context
branch
from
1b1ca7b to
ded19e9
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java (2)
53-64: Static initialization flag may cause issues with test isolation.The
static boolean initializedflag combined with@BeforeEachcould cause issues if tests run in parallel or if the test class is instantiated multiple times. Consider using@BeforeAllwith a static method instead, which is the idiomatic JUnit 5 pattern for one-time setup.🔎 Proposed refactor to use @BeforeAll
- private static boolean initialized = false; - - @SneakyThrows - @BeforeEach - public void initialize() { - if (!initialized) { - setUpIndices(); // Initialize client if needed - setupTestIndices(); - createSecurityRolesAndUsers(); - initialized = true; - } - } + @SneakyThrows + @BeforeAll + public static void initializeOnce() { + // Note: This requires making the setup methods static or + // using a different initialization approach + }Alternatively, if
@BeforeAllisn't compatible with the parent class, document why the static flag pattern is necessary.
74-85: Consider adding test cleanup for created indices.The test creates multiple indices (
public_logs_fgac,sensitive_logs_fgac,secure_logs_fgac,employee_records_fgac) but there's no@AfterAllor@AfterEachcleanup method. While using unique_fgacsuffix provides isolation, these indices will persist after tests complete.As per coding guidelines, integration tests should validate that tests clean up resources after execution.
🔎 Consider adding cleanup
@AfterAll public static void cleanup() throws IOException { // Delete test indices String[] indices = {PUBLIC_LOGS, SENSITIVE_LOGS, SECURE_LOGS, EMPLOYEE_RECORDS}; for (String index : indices) { try { client().performRequest(new Request("DELETE", "/" + index)); } catch (Exception e) { // Index may not exist, ignore } } }integ-test/src/test/java/org/opensearch/sql/security/SecurityTestBase.java (2)
56-98: Consider using JSONArray for permission arrays to avoid manual string building.The manual StringBuilder approach for building JSON arrays works but is error-prone. Using
JSONArraywould be cleaner and safer.🔎 Optional refactor using JSONArray
import org.json.JSONArray; // In createRoleWithPermissions: JSONArray clusterPermsArray = new JSONArray(clusterPermissions); JSONArray indexPermsArray = new JSONArray(indexPermissions); // Then use clusterPermsArray.toString() and indexPermsArray.toString() in the template
341-353: Remove unusedBulkDocumentBuilderhelper class or document its intended purpose.The
BulkDocumentBuilderis defined but never instantiated or used anywhere in the codebase. Either remove it if not needed for this PR, or add a comment documenting its intended usage in future tests.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
integ-test/build.gradleinteg-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.javainteg-test/src/test/java/org/opensearch/sql/security/PPLPermissionsIT.javainteg-test/src/test/java/org/opensearch/sql/security/SecurityTestBase.java
🧰 Additional context used
📓 Path-based instructions (4)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
integ-test/src/test/java/org/opensearch/sql/security/PPLPermissionsIT.javainteg-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.javainteg-test/src/test/java/org/opensearch/sql/security/SecurityTestBase.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
integ-test/src/test/java/org/opensearch/sql/security/PPLPermissionsIT.javainteg-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.javainteg-test/src/test/java/org/opensearch/sql/security/SecurityTestBase.java
integ-test/**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
End-to-end scenarios need integration tests in
integ-test/module
Files:
integ-test/src/test/java/org/opensearch/sql/security/PPLPermissionsIT.javainteg-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
⚙️ CodeRabbit configuration file
integ-test/**/*IT.java: - Integration tests MUST use valid test data from resources
- Verify test data files exist in integ-test/src/test/resources/
- Check test assertions are meaningful and specific
- Validate tests clean up resources after execution
- Ensure tests are independent and can run in any order
- Flag tests that reference non-existent indices (e.g., EMP)
- Verify integration tests are in correct module (integ-test/)
- Check tests can be run with ./gradlew :integ-test:integTest
- Ensure proper test data setup and teardown
- Validate end-to-end scenario coverage
Files:
integ-test/src/test/java/org/opensearch/sql/security/PPLPermissionsIT.javainteg-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
Name integration tests with
*IT.javasuffix in OpenSearch SQL
Files:
integ-test/src/test/java/org/opensearch/sql/security/PPLPermissionsIT.javainteg-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify NULL input tests for all new functions
- Check boundary condition tests (min/max values, empty inputs)
- Validate error condition tests (invalid inputs, exceptions)
- Ensure multi-document tests for per-document operations
- Flag smoke tests without meaningful assertions
- Check test naming follows pattern: test
- Verify test data is realistic and covers edge cases
- Verify test coverage for new business logic
- Ensure tests are independent and don't rely on execution order
- Validate meaningful test data that reflects real-world scenarios
- Check for proper cleanup of test resources
Files:
integ-test/src/test/java/org/opensearch/sql/security/PPLPermissionsIT.javainteg-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.javainteg-test/src/test/java/org/opensearch/sql/security/SecurityTestBase.java
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*IT.java : Name integration tests with `*IT.java` suffix in OpenSearch SQL
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Verify changes with `./gradlew :integ-test:integTest` before merge
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Verify changes with `./gradlew :integ-test:integTest` before merge
Applied to files:
integ-test/build.gradle
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to integ-test/**/*IT.java : End-to-end scenarios need integration tests in `integ-test/` module
Applied to files:
integ-test/build.gradle
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Use `./gradlew :integ-test:integTest` for integration testing in OpenSearch SQL
Applied to files:
integ-test/build.gradle
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*IT.java : Name integration tests with `*IT.java` suffix in OpenSearch SQL
Applied to files:
integ-test/build.gradleinteg-test/src/test/java/org/opensearch/sql/security/PPLPermissionsIT.javainteg-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.javainteg-test/src/test/java/org/opensearch/sql/security/SecurityTestBase.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Development requires JDK 21 for the OpenSearch SQL project
Applied to files:
integ-test/build.gradle
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*Test.java : Name unit tests with `*Test.java` suffix in OpenSearch SQL
Applied to files:
integ-test/build.gradleinteg-test/src/test/java/org/opensearch/sql/security/SecurityTestBase.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
🧬 Code graph analysis (1)
integ-test/src/test/java/org/opensearch/sql/security/SecurityTestBase.java (1)
integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java (1)
PPLIntegTestCase(36-415)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (25, unit)
- GitHub Check: build-linux (25, integration)
- GitHub Check: build-linux (21, unit)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: CodeQL-Scan (java)
🔇 Additional comments (12)
integ-test/build.gradle (2)
211-212: LGTM - Dependency addition for parameterized tests.The
junit-jupiter-paramsdependency aligns with the existing JUnit 5 version (5.9.3) used in the project.
449-453: LGTM - Test filter inclusion follows existing pattern.The
FGACIndexScanningITtest is correctly added to the security test filter alongside existing security integration tests. Based on learnings, the*IT.javanaming convention is properly followed.integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java (4)
197-223: LGTM - Bulk insert implementation follows best practices.The method correctly uses
StringBuilderfor string concatenation in loops (as per coding guidelines), sets the properContent-Type: application/x-ndjsonheader, and verifies the response status code.
363-405: LGTM - Index-level security test is comprehensive.The test properly validates both positive cases (user can access their index) and negative cases (user cannot access unauthorized index) with meaningful assertions. The exception handling verifies appropriate error messages.
407-499: LGTM - Column-level security test covers FLS comprehensively.The test validates:
- HR user can see all fields including sensitive
ssn- Manager user cannot see
ssnfield- Manager cannot even query
ssnfield- FLS is enforced even with large datasets (background scanning)
Error messages clearly indicate security violations with actionable context.
501-628: LGTM - Row-level security tests cover both V2 and V3 engines.The tests appropriately:
- Test V2 (legacy) engine by explicitly calling
disableCalcite()- Test V3 (Calcite) engine using the default enabled state
- Verify DLS filtering by counting documents per security level
- Include detailed error messages explaining ThreadContext implications
integ-test/src/test/java/org/opensearch/sql/security/SecurityTestBase.java (3)
19-26: LGTM - Well-structured base class with clear documentation.The abstract class properly inherits from
PPLIntegTestCaseand provides reusable utilities for security integration tests, reducing code duplication across test classes.
198-248: LGTM - User creation with role mapping is convenient for test setup.The method appropriately combines user creation and role mapping, which is a common pattern for test utilities. Both operations verify success via status codes.
250-277: LGTM - Clean implementation for authenticated query execution.The method properly constructs the request with Basic Auth headers and validates the response status code before parsing.
integ-test/src/test/java/org/opensearch/sql/security/PPLPermissionsIT.java (3)
31-31: LGTM - Good refactoring to extend SecurityTestBase.The class now properly inherits from
SecurityTestBase, eliminating duplicate code for role/user creation and authenticated query execution.
83-102: LGTM - Clean usage of inherited SecurityTestBase methods.The refactored setup properly delegates to the base class methods for creating roles and users, following DRY principles.
104-194: LGTM - Comprehensive negative test setup with inherited utilities.The helper methods properly use
createRoleWithPermissionsfromSecurityTestBaseto create roles with specific missing permissions, enabling thorough negative testing of permission enforcement.
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
Show resolved
Hide resolved
Signed-off-by: Simeon Widdis <[email protected]>
Swiddis
requested review from
GumpacG,
LantaoJin,
MaxKsyunz,
YANG-DB,
Yury-Fridlyand,
acarbonetto,
anirudha,
dai-chen,
derek-ho,
forestmvey,
joshuali925,
kavithacm,
mengweieric,
noCharger,
penghuo,
ps48,
seankao-az,
vamsimanohar and
ykmr1224
as code owners
Swiddis
requested review from
RyanL1997,
qianheng-aws and
yuancu
as code owners
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java (3)
74-195: Consider adding test cleanup to remove indices, roles, and users.Integration tests should clean up resources after execution. Currently, there's no
@AfterEachor@AfterAllmethod to delete the created indices (public_logs_fgac,sensitive_logs_fgac,secure_logs_fgac,employee_records_fgac) and security artifacts (roles/users).🔎 Example cleanup approach
@AfterAll public static void cleanup() throws IOException { // Delete test indices deleteIndexIfExists(PUBLIC_LOGS); deleteIndexIfExists(SENSITIVE_LOGS); deleteIndexIfExists(SECURE_LOGS); deleteIndexIfExists(EMPLOYEE_RECORDS); // Delete test users and roles deleteUser(PUBLIC_USER); deleteUser(SENSITIVE_USER); deleteUser(LIMITED_USER); deleteUser(MANAGER_USER); deleteUser(HR_USER); deleteRole(PUBLIC_ROLE); deleteRole(SENSITIVE_ROLE); deleteRole(LIMITED_ROLE); deleteRole(MANAGER_ROLE); deleteRole(HR_ROLE); }Check if
SecurityTestBaseprovides cleanup utilities.As per coding guidelines, integration tests should ensure proper cleanup of test resources.
404-496: Consider splitting this complex test into smaller, focused test methods.This method is 92 lines and tests 4 distinct scenarios. Per coding guidelines, methods should be under 50 lines with single responsibility.
🔎 Suggested refactoring
Split into focused test methods:
@Test public void testHRUserCanAccessAllFieldsIncludingSSN() throws IOException { // Current Test 1 logic (lines 408-429) } @Test public void testManagerUserCannotAccessSSNField() throws IOException { // Current Test 2 logic (lines 431-458) } @Test public void testManagerUserCannotQuerySSNField() throws IOException { // Current Test 3 logic (lines 460-474) } @Test public void testFLSEnforcedWithBackgroundScanning() throws IOException { // Current Test 4 logic (lines 476-495) }This improves readability and makes test failures easier to diagnose.
As per coding guidelines, flag methods >50 lines as potentially too complex and suggest refactoring.
498-625: Significant code duplication between testRowLevelSecurityV2 and testRowLevelSecurity.These two methods have nearly identical logic (lines 498-559 vs 561-625), differing only in:
- V2 calls
disableCalcite()while V3 uses Calcite by default- Error message prefixes ("[V2]" vs "[V3]")
Both methods exceed 50 lines. Consider extracting the common validation logic to reduce duplication and improve maintainability.
🔎 Suggested refactoring
@Test public void testRowLevelSecurityV2() throws IOException { disableCalcite(); verifyRowLevelSecurityEnforcement("V2"); } @Test public void testRowLevelSecurity() throws IOException { // Calcite enabled by default in init() verifyRowLevelSecurityEnforcement("V3"); } private void verifyRowLevelSecurityEnforcement(String engineVersion) throws IOException { String query = String.format( "search source=%s | fields security_level, message | stats count() by security_level", SECURE_LOGS); JSONObject result = executeQueryAsUser(query, LIMITED_USER); var datarows = result.getJSONArray("datarows"); int totalDocs = 0; boolean sawConfidential = false; boolean sawInternal = false; int publicDocs = 0; for (int i = 0; i < datarows.length(); i++) { var row = datarows.getJSONArray(i); int count = row.getInt(0); String securityLevel = row.getString(1); totalDocs += count; if ("confidential".equals(securityLevel)) sawConfidential = true; else if ("internal".equals(securityLevel)) sawInternal = true; else if ("public".equals(securityLevel)) publicDocs = count; } assertFalse( String.format("[%s] SECURITY VIOLATION: limited_user should NOT see 'confidential' documents...", engineVersion), sawConfidential); assertFalse( String.format("[%s] SECURITY VIOLATION: limited_user should NOT see 'internal' documents...", engineVersion), sawInternal); assertEquals( String.format("[%s] limited_user should ONLY see 'public' documents (~1000)...", engineVersion), 1000, publicDocs); assertEquals( String.format("[%s] Total visible documents should be ~1000 (only public)...", engineVersion), 1000, totalDocs); }As per coding guidelines, identify code reuse opportunities across similar implementations and flag methods >50 lines as potentially too complex.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
🧰 Additional context used
📓 Path-based instructions (4)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
integ-test/**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
End-to-end scenarios need integration tests in
integ-test/module
Files:
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
⚙️ CodeRabbit configuration file
integ-test/**/*IT.java: - Integration tests MUST use valid test data from resources
- Verify test data files exist in integ-test/src/test/resources/
- Check test assertions are meaningful and specific
- Validate tests clean up resources after execution
- Ensure tests are independent and can run in any order
- Flag tests that reference non-existent indices (e.g., EMP)
- Verify integration tests are in correct module (integ-test/)
- Check tests can be run with ./gradlew :integ-test:integTest
- Ensure proper test data setup and teardown
- Validate end-to-end scenario coverage
Files:
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
Name integration tests with
*IT.javasuffix in OpenSearch SQL
Files:
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify NULL input tests for all new functions
- Check boundary condition tests (min/max values, empty inputs)
- Validate error condition tests (invalid inputs, exceptions)
- Ensure multi-document tests for per-document operations
- Flag smoke tests without meaningful assertions
- Check test naming follows pattern: test
- Verify test data is realistic and covers edge cases
- Verify test coverage for new business logic
- Ensure tests are independent and don't rely on execution order
- Validate meaningful test data that reflects real-world scenarios
- Check for proper cleanup of test resources
Files:
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
🧠 Learnings (3)
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*IT.java : Name integration tests with `*IT.java` suffix in OpenSearch SQL
Applied to files:
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to integ-test/**/*IT.java : End-to-end scenarios need integration tests in `integ-test/` module
Applied to files:
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (21, unit)
- GitHub Check: build-linux (25, doc)
- GitHub Check: build-linux (25, unit)
- GitHub Check: build-linux (21, doc)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: build-linux (25, integration)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: security-it-linux (21)
- GitHub Check: security-it-linux (25)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: test-sql-cli-integration (21)
- GitHub Check: CodeQL-Scan (java)
🔇 Additional comments (1)
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java (1)
331-334: Previous review concern appears resolved.The past review flagged redundant role creation for
LIMITED_ROLE, but the current code only creates the role once viacreateRoleWithDocumentLevelSecurity()at line 333. The issue has been addressed.
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Simeon Widdis <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java (4)
198-198: Remove redundant class name qualification.Within the class, constants can be referenced directly without the
FGACIndexScanningIT.prefix. This applies toLARGE_DATASET_SIZE,EMPLOYEE_RECORDS, andSECURE_LOGSat lines 198, 253, 277, 290, and 303.🔎 Proposed simplification
- for (int i = 0; i < FGACIndexScanningIT.LARGE_DATASET_SIZE; i++) { + for (int i = 0; i < LARGE_DATASET_SIZE; i++) {Apply similar changes at lines 253, 277, 290, and 303 to remove the
FGACIndexScanningIT.prefix.Also applies to: 253-253, 277-277, 290-290, 303-303
402-494: Consider splitting this test method for better clarity.At 92 lines,
testColumnLevelSecuritycontains four distinct test scenarios that could be separated into individual test methods: (1) HR user sees all fields, (2) Manager user sees restricted fields, (3) Manager cannot query SSN directly, and (4) FLS enforcement with large datasets. Splitting would improve test isolation and make failures easier to diagnose.💡 Suggested approach
Consider creating separate test methods:
testHRUserSeesAllFields()testManagerUserSensitiveFieldsHidden()testManagerUserCannotQuerySensitiveField()testFieldLevelSecurityWithLargeDataset()This would make each test focused on a single concern and improve maintainability.
As per coding guidelines, methods over 50 lines should be reviewed for complexity.
496-623: Consider refactoring duplicated row-level security test logic.The methods
testRowLevelSecurityV2andtestRowLevelSecuritycontain nearly identical logic (validation code at lines 514-556 and 579-622), differing only in the engine configuration. This duplication could be reduced using JUnit 5's parameterized tests or a shared helper method.💡 Suggested approach using parameterized tests
@ParameterizedTest @ValueSource(booleans = {false, true}) // false = V2, true = V3/Calcite public void testRowLevelSecurity(boolean useCalcite) throws IOException { if (!useCalcite) { disableCalcite(); } String engineLabel = useCalcite ? "V3" : "V2"; // Execute query as limited_user String query = String.format( "search source=%s | fields security_level, message | stats count() by security_level", SECURE_LOGS); JSONObject result = executeQueryAsUser(query, LIMITED_USER); // Extract and validate datarows (shared logic) var datarows = result.getJSONArray("datarows"); int totalDocs = 0; boolean sawConfidential = false; boolean sawInternal = false; int publicDocs = 0; for (int i = 0; i < datarows.length(); i++) { var row = datarows.getJSONArray(i); int count = row.getInt(0); String securityLevel = row.getString(1); totalDocs += count; if ("confidential".equals(securityLevel)) sawConfidential = true; else if ("internal".equals(securityLevel)) sawInternal = true; else if ("public".equals(securityLevel)) publicDocs = count; } assertFalse( String.format("[%s] SECURITY VIOLATION: limited_user should NOT see 'confidential' documents...", engineLabel), sawConfidential); assertFalse( String.format("[%s] SECURITY VIOLATION: limited_user should NOT see 'internal' documents...", engineLabel), sawInternal); assertEquals(1000, publicDocs, String.format("[%s] limited_user should ONLY see 'public' documents (~1000)...", engineLabel)); assertEquals(1000, totalDocs, String.format("[%s] Total visible documents should be ~1000 (only public)...", engineLabel)); }Note: The test dependencies already include
junit-jupiter-params(from build.gradle changes in this PR).
56-62: Consider adding explicit test resource cleanup.While the test indices use the
_fgacsuffix for isolation, consider adding an@AfterAllmethod to explicitly clean up test indices, roles, and users. This ensures a clean state even if tests fail and improves test isolation.💡 Suggested cleanup approach
@SneakyThrows @AfterAll public void cleanup() { // Delete test indices deleteIndex(PUBLIC_LOGS); deleteIndex(SENSITIVE_LOGS); deleteIndex(SECURE_LOGS); deleteIndex(EMPLOYEE_RECORDS); // Delete test users and roles (if helper methods exist in SecurityTestBase) deleteUser(PUBLIC_USER); deleteUser(SENSITIVE_USER); deleteUser(LIMITED_USER); deleteUser(MANAGER_USER); deleteUser(HR_USER); deleteRole(PUBLIC_ROLE); deleteRole(SENSITIVE_ROLE); deleteRole(LIMITED_ROLE); deleteRole(MANAGER_ROLE); deleteRole(HR_ROLE); }As per coding guidelines, integration tests should properly clean up resources after execution.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
🧰 Additional context used
📓 Path-based instructions (4)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
integ-test/**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
End-to-end scenarios need integration tests in
integ-test/module
Files:
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
⚙️ CodeRabbit configuration file
integ-test/**/*IT.java: - Integration tests MUST use valid test data from resources
- Verify test data files exist in integ-test/src/test/resources/
- Check test assertions are meaningful and specific
- Validate tests clean up resources after execution
- Ensure tests are independent and can run in any order
- Flag tests that reference non-existent indices (e.g., EMP)
- Verify integration tests are in correct module (integ-test/)
- Check tests can be run with ./gradlew :integ-test:integTest
- Ensure proper test data setup and teardown
- Validate end-to-end scenario coverage
Files:
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
Name integration tests with
*IT.javasuffix in OpenSearch SQL
Files:
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify NULL input tests for all new functions
- Check boundary condition tests (min/max values, empty inputs)
- Validate error condition tests (invalid inputs, exceptions)
- Ensure multi-document tests for per-document operations
- Flag smoke tests without meaningful assertions
- Check test naming follows pattern: test
- Verify test data is realistic and covers edge cases
- Verify test coverage for new business logic
- Ensure tests are independent and don't rely on execution order
- Validate meaningful test data that reflects real-world scenarios
- Check for proper cleanup of test resources
Files:
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
🧠 Learnings (4)
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*IT.java : Name integration tests with `*IT.java` suffix in OpenSearch SQL
Applied to files:
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to integ-test/**/*IT.java : End-to-end scenarios need integration tests in `integ-test/` module
Applied to files:
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Verify changes with `./gradlew :integ-test:integTest` before merge
Applied to files:
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: build-linux (25, unit)
- GitHub Check: build-linux (21, doc)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (25, doc)
- GitHub Check: build-linux (25, integration)
- GitHub Check: build-linux (21, unit)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: security-it-linux (21)
- GitHub Check: security-it-linux (25)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: test-sql-cli-integration (21)
- GitHub Check: CodeQL-Scan (java)
🔇 Additional comments (3)
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java (3)
25-55: LGTM! Class structure and constants are well-organized.The use of
@TestInstance(Lifecycle.PER_CLASS)with@BeforeAllproperly addresses the previous concern about static mutable test state. Constants follow proper naming conventions and are clearly documented.
56-83: LGTM! Initialization is properly structured.The test setup correctly uses
@BeforeAllwith instance-level lifecycle management, enabling proper one-time initialization without static state issues.
319-356: LGTM! Security roles and users are correctly configured.The setup properly creates roles with appropriate access controls for testing index-level, field-level, and document-level security. The previous issue with redundant role creation has been resolved.
Signed-off-by: Simeon Widdis <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java (4)
56-62: Consider adding cleanup to ensure test isolation.Integration tests should clean up created resources (indices, users, roles) after execution to maintain test independence and avoid side effects in CI environments.
💡 Consider adding an @afterall cleanup method
+ @SneakyThrows + @AfterAll + public void cleanup() { + // Delete test indices + deleteIndex(PUBLIC_LOGS); + deleteIndex(SENSITIVE_LOGS); + deleteIndex(SECURE_LOGS); + deleteIndex(EMPLOYEE_RECORDS); + + // Delete test users and roles + deleteUser(PUBLIC_USER); + deleteUser(SENSITIVE_USER); + deleteUser(LIMITED_USER); + deleteUser(MANAGER_USER); + deleteUser(HR_USER); + + deleteRole(PUBLIC_ROLE); + deleteRole(SENSITIVE_ROLE); + deleteRole(LIMITED_ROLE); + deleteRole(MANAGER_ROLE); + deleteRole(HR_ROLE); + }As per coding guidelines, integration tests should clean up resources after execution and be independent.
254-307: Consider refactoring to reduce method length and repetition.This method exceeds 50 lines and contains three similar loops that differ only by security level, count range, and message prefix. Extracting a helper method would improve maintainability.
💡 Proposed refactoring to reduce duplication
+ private void appendSecurityLevelDocs(StringBuilder bulk, String level, int start, int end) { + for (int i = start; i < end; i++) { + bulk.append( + String.format( + Locale.ROOT, + """ + { "index": { "_index": "%s" } } + { "message": "%s message %d", "security_level": "%s", "timestamp": "2025-01-01T00:00:00Z" } + """, + SECURE_LOGS, + level, + i, + level)); + } + } + private void bulkInsertDocsWithSecurityLevel() throws IOException { StringBuilder bulk = new StringBuilder(); - - // 1000 public documents - for (int i = 0; i < 1000; i++) { - bulk.append(...); - } - - // 500 internal documents - for (int i = 1000; i < 1500; i++) { - bulk.append(...); - } - - // 500 confidential documents - for (int i = 1500; i < 2000; i++) { - bulk.append(...); - } + appendSecurityLevelDocs(bulk, "public", 0, 1000); + appendSecurityLevelDocs(bulk, "internal", 1000, 1500); + appendSecurityLevelDocs(bulk, "confidential", 1500, 2000); Request request = new Request("POST", "/_bulk"); // ... rest of method }As per coding guidelines, methods over 50 lines should be reviewed for complexity.
499-625: Eliminate duplication with parameterized tests.Both row-level security test methods (
testRowLevelSecurityV2andtestRowLevelSecurity) exceed 50 lines and share nearly identical logic, differing only in the engine toggle and assertion message prefixes. Sincejunit-jupiter-paramswas added to dependencies, consider using@ParameterizedTestto eliminate this duplication.💡 Proposed parameterized test approach
+ @ParameterizedTest + @CsvSource({ + "V2, true", + "V3, false" + }) + public void testRowLevelSecurity(String engineVersion, boolean shouldDisableCalcite) throws IOException { + if (shouldDisableCalcite) { + disableCalcite(); + } + + String query = + String.format( + "search source=%s | fields security_level, message | stats count() by security_level", + SECURE_LOGS); + JSONObject result = executeQueryAsUser(query, LIMITED_USER); + + var datarows = result.getJSONArray("datarows"); + int totalDocs = 0; + boolean sawConfidential = false; + boolean sawInternal = false; + int publicDocs = 0; + + for (int i = 0; i < datarows.length(); i++) { + var row = datarows.getJSONArray(i); + int count = row.getInt(0); + String securityLevel = row.getString(1); + totalDocs += count; + + if ("confidential".equals(securityLevel)) { + sawConfidential = true; + } else if ("internal".equals(securityLevel)) { + sawInternal = true; + } else if ("public".equals(securityLevel)) { + publicDocs = count; + } + } + + assertFalse( + String.format("[%s] SECURITY VIOLATION: limited_user should NOT see 'confidential' documents. " + + "This indicates ThreadContext is not being properly copied to async worker threads, " + + "causing queries to run with admin permissions and bypass row-level security.", engineVersion), + sawConfidential); + + assertFalse( + String.format("[%s] SECURITY VIOLATION: limited_user should NOT see 'internal' documents. " + + "This indicates ThreadContext is not being properly copied to async worker threads, " + + "causing queries to run with admin permissions and bypass row-level security.", engineVersion), + sawInternal); + + assertEquals( + 1000, + publicDocs, + String.format("[%s] limited_user should ONLY see 'public' documents (~1000). " + + "Seeing more indicates row-level security is being bypassed.", engineVersion)); + + assertEquals( + 1000, + totalDocs, + String.format("[%s] Total visible documents should be ~1000 (only public). " + + "Seeing 2000 documents indicates row-level security is completely bypassed.", engineVersion)); + } - - @Test - public void testRowLevelSecurityV2() throws IOException { - // ... 60 lines of duplicated code - } - - @Test - public void testRowLevelSecurity() throws IOException { - // ... 63 lines of duplicated code - }As per coding guidelines, methods over 50 lines should be reviewed for complexity, and code reuse opportunities should be identified.
400-496: Consider extracting field-checking logic into a helper method.Multiple test methods use the same pattern to check for field presence in the schema (iterating through schema array and setting boolean flags). Extracting this into a reusable helper would reduce repetition.
💡 Example helper method
private Set<String> extractFieldNamesFromSchema(JSONObject result) { Set<String> fieldNames = new HashSet<>(); var schema = result.getJSONArray("schema"); for (int i = 0; i < schema.length(); i++) { fieldNames.add(schema.getJSONObject(i).getString("name")); } return fieldNames; }Usage:
Set<String> fields = extractFieldNamesFromSchema(hrResult); assertTrue("hr_user should see 'name' field", fields.contains("name")); assertTrue("hr_user should see 'ssn' field", fields.contains("ssn")); // etc.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
🧰 Additional context used
📓 Path-based instructions (4)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
integ-test/**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
End-to-end scenarios need integration tests in
integ-test/module
Files:
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
⚙️ CodeRabbit configuration file
integ-test/**/*IT.java: - Integration tests MUST use valid test data from resources
- Verify test data files exist in integ-test/src/test/resources/
- Check test assertions are meaningful and specific
- Validate tests clean up resources after execution
- Ensure tests are independent and can run in any order
- Flag tests that reference non-existent indices (e.g., EMP)
- Verify integration tests are in correct module (integ-test/)
- Check tests can be run with ./gradlew :integ-test:integTest
- Ensure proper test data setup and teardown
- Validate end-to-end scenario coverage
Files:
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
Name integration tests with
*IT.javasuffix in OpenSearch SQL
Files:
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify NULL input tests for all new functions
- Check boundary condition tests (min/max values, empty inputs)
- Validate error condition tests (invalid inputs, exceptions)
- Ensure multi-document tests for per-document operations
- Flag smoke tests without meaningful assertions
- Check test naming follows pattern: test
- Verify test data is realistic and covers edge cases
- Verify test coverage for new business logic
- Ensure tests are independent and don't rely on execution order
- Validate meaningful test data that reflects real-world scenarios
- Check for proper cleanup of test resources
Files:
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
🧠 Learnings (4)
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*IT.java : Name integration tests with `*IT.java` suffix in OpenSearch SQL
Applied to files:
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to integ-test/**/*IT.java : End-to-end scenarios need integration tests in `integ-test/` module
Applied to files:
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Verify changes with `./gradlew :integ-test:integTest` before merge
Applied to files:
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: build-linux (25, integration)
- GitHub Check: build-linux (21, doc)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (25, doc)
- GitHub Check: build-linux (21, unit)
- GitHub Check: build-linux (25, unit)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: security-it-linux (21)
- GitHub Check: security-it-linux (25)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: test-sql-cli-integration (21)
- GitHub Check: CodeQL-Scan (java)
🔇 Additional comments (4)
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java (4)
25-55: Well-structured test class with clear security testing scope.The class-level JavaDoc clearly documents the three levels of FGAC being tested (index, field, row), and constants are properly defined. The use of
@TestInstance(Lifecycle.PER_CLASS)appropriately supports the@BeforeAllsetup pattern.
56-62: Good setup pattern with @BeforeAll.The initialization properly leverages
@TestInstance(Lifecycle.PER_CLASS)and the previous issue with the staticinitializedflag has been resolved.
348-397: Index-level security tests are well-structured.These tests properly validate index-level access control with clear assertions and appropriate error handling for forbidden access scenarios. Test naming follows best practices.
309-332: Security role setup is clean and properly structured.The role and user creation logic is well-organized. The previous issue with redundant
LIMITED_ROLEcreation has been successfully resolved.
| } | ||
|
|
||
| @Test | ||
| public void testRowLevelSecurity() throws IOException { |
There was a problem hiding this comment.
- testRowLevelSecurity and testRowLevelSecurityV2 can be merge to testRowLevelSecurity
- all tests should run with calcite enable and disable.
| /** Bulk inserts documents to trigger background scanning. */ | ||
| private void bulkInsertDocs(String indexName, String prefix) throws IOException { | ||
| StringBuilder bulk = new StringBuilder(); | ||
| for (int i = 0; i < FGACIndexScanningIT.LARGE_DATASET_SIZE; i++) { |
There was a problem hiding this comment.
Why we need large test dataset to verify the issue? Is it an overhead for both CI and troubleshoot?
3 participants
Description
Adds some tests to validate various scenarios for document-level security (DLS), since there was an internal scare for a bit on if DLS is working with background IO. Turns out we're protected implicitly by ContextPreservingRunnable. Just pushing some work from that to keep verifying this in the future.
Related Issues
N/A
Check List
--signoffor-s.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.