Remove all AccessController refs #4924
Conversation
Signed-off-by: Simeon Widdis <[email protected]>
Signed-off-by: Simeon Widdis <[email protected]>
Swiddis
requested review from
GumpacG,
LantaoJin,
MaxKsyunz,
RyanL1997,
YANG-DB,
Yury-Fridlyand,
acarbonetto,
anirudha,
dai-chen,
derek-ho,
forestmvey,
joshuali925,
kavithacm,
mengweieric,
noCharger,
penghuo,
ps48,
qianheng-aws,
seankao-az,
vamsimanohar,
ykmr1224 and
yuancu
as code owners
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughRemoved java.security AccessController.doPrivileged / PrivilegedAction wrappers across many modules, replacing them with direct method calls and standard try-catch error handling while keeping public signatures, logging, metrics, and exception mapping intact. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
🧰 Additional context used📓 Path-based instructions (6)**/*.java📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
Files:
⚙️ CodeRabbit configuration file
Files:
**/test/**/*.java⚙️ CodeRabbit configuration file
Files:
**/calcite/**/*.java⚙️ CodeRabbit configuration file
Files:
integ-test/**/*IT.java📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
Files:
⚙️ CodeRabbit configuration file
Files:
**/*IT.java📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
Files:
**/ppl/**/*.java⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (4)📚 Learning: 2025-12-02T17:27:55.938ZApplied to files:
📚 Learning: 2025-12-02T17:27:55.938ZApplied to files:
📚 Learning: 2025-12-02T17:27:55.938ZApplied to files:
📚 Learning: 2025-12-02T17:27:55.938ZApplied to files:
⏰ 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). (29)
🔇 Additional comments (4)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
async-query/src/main/java/org/opensearch/sql/spark/config/SparkExecutionEngineConfigClusterSettingLoader.java (1)
15-15: Update the JavaDoc to reflect removal of privilege checks.The class-level JavaDoc mentions "with privilege check", but the AccessController wrapper has been removed in this PR. Update the documentation to accurately describe the current behavior.
Apply this diff to correct the JavaDoc:
-/** Load SparkExecutionEngineConfigClusterSetting from settings with privilege check. */ +/** Load SparkExecutionEngineConfigClusterSetting from settings. */
🧹 Nitpick comments (11)
protocol/src/main/java/org/opensearch/sql/protocol/response/format/JsonResponseFormatter.java (1)
40-43: Direct dispatch informat(Throwable)looks correct and matches prior behaviorUsing the ternary on
styleto selectprettyFormatvscompactFormatis straightforward and keeps behavior aligned with the previous privileged block, just withoutAccessController. This cleanly supports the PR goal of dropping SecurityManager-related code, with no change to the public API surface.Optionally, consider adding a brief JavaDoc to this public overload (and the other
formatmethod) in a follow-up to align with the project guideline that public methods have JavaDoc, but that’s not blocking here.prometheus/src/main/java/org/opensearch/sql/prometheus/functions/scan/QueryRangeFunctionTableScanOperator.java (1)
38-51: Preserve exception cause and log full stack trace for Prometheus query failuresThe behavior change (removing
AccessController) looks fine, but the currentIOExceptionhandling loses useful debugging information:
LOG.error(e.getMessage());logs only the message, not the stack trace.- The new
RuntimeExceptionis created with a formatted message but without the original cause, so upstream handlers can’t see the underlyingIOException.To improve observability and align with the guideline of meaningful error handling, consider preserving the cause and logging the full exception:
- try { - JSONObject responseObject = - prometheusClient.queryRange( - request.getPromQl(), - request.getStartTime(), - request.getEndTime(), - request.getStep()); - this.prometheusResponseHandle = new QueryRangeFunctionResponseHandle(responseObject); - } catch (IOException e) { - LOG.error(e.getMessage()); - throw new RuntimeException( - String.format( - "Error fetching data from prometheus server: %s", e.getMessage())); - } + try { + JSONObject responseObject = + prometheusClient.queryRange( + request.getPromQl(), + request.getStartTime(), + request.getEndTime(), + request.getStep()); + this.prometheusResponseHandle = new QueryRangeFunctionResponseHandle(responseObject); + } catch (IOException e) { + LOG.error("Error fetching data from Prometheus server", e); + throw new RuntimeException("Error fetching data from Prometheus server", e); + }This keeps the behavior (still a
RuntimeException) while making failures much easier to diagnose.opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/core/ExpressionScript.java (1)
62-68: LGTM: AccessController removal in execute method.The direct calls to
buildValueEnvandevaluator.applyare correct. Removing the privileged execution wrapper is appropriate for these operations.Optional: Simplify return statement.
The intermediate
resultvariable can be eliminated:- Environment<Expression, ExprValue> valueEnv = buildValueEnv(fields, valueFactory, docProvider); - ExprValue result = evaluator.apply(expression, valueEnv); - return result; + Environment<Expression, ExprValue> valueEnv = buildValueEnv(fields, valueFactory, docProvider); + return evaluator.apply(expression, valueEnv);direct-query/src/main/java/org/opensearch/sql/directquery/transport/model/ExecuteDirectQueryActionResponse.java (1)
142-151: Correct error handling approach, but inconsistent with other methods.This implementation correctly lets
IOExceptionpropagate directly fromOBJECT_MAPPER.readValuewithout wrapping, which properly matches the method'sthrows IOExceptiondeclaration. This is the preferred approach.However, note the inconsistency: the constructor (lines 69-73) and
writeTo(lines 108-112) wrapIOExceptioninRuntimeException, while this method doesn't. Standardizing on this direct-propagation approach across all three locations would improve consistency.Also observe that the
RuntimeExceptioncatch block at lines 154-162 was designed to unwrapIOExceptionfrom the oldAccessControllerpattern. With the new direct approach, this catch block will only handle unexpectedRuntimeExceptions (not from ObjectMapper), making it potentially dead code or at least confusing.core/src/main/java/org/opensearch/sql/executor/QueryService.java (1)
140-151: Avoid redundant nestedCalcitePlanContext.runinexplainWithCalciteYou’re already inside an outer
CalcitePlanContext.run(…, settings)at Line 137; the innercontext.run(…, settings)re-applies the same thread-local setting and then removes it, which is redundant and slightly obscures the flow.You can simplify by dropping the inner
runand mirroring the structure used inexecuteWithCalcite:- CalcitePlanContext context = - CalcitePlanContext.create( - buildFrameworkConfig(), SysLimit.fromSettings(settings), queryType); - context.run( - () -> { - RelNode relNode = analyze(plan, context); - relNode = mergeAdjacentFilters(relNode); - RelNode optimized = optimize(relNode, context); - RelNode calcitePlan = convertToCalcitePlan(optimized); - executionEngine.explain(calcitePlan, format, context, listener); - }, - settings); + CalcitePlanContext context = + CalcitePlanContext.create( + buildFrameworkConfig(), SysLimit.fromSettings(settings), queryType); + RelNode relNode = analyze(plan, context); + relNode = mergeAdjacentFilters(relNode); + RelNode optimized = optimize(relNode, context); + RelNode calcitePlan = convertToCalcitePlan(optimized); + executionEngine.explain(calcitePlan, format, context, listener);This keeps the
legacyPreferredhandling in the outerrunand removes an unnecessary nesting level.prometheus/src/main/java/org/opensearch/sql/prometheus/storage/PrometheusMetricScan.java (1)
54-67: Improve logging to include stack trace when Prometheus query failsCore logic of calling
queryRangeand wrapping the response is fine, but logging onlye.getMessage()loses stack information.Consider logging the exception itself:
- } catch (IOException e) { - LOG.error(e.getMessage()); - throw new RuntimeException( - "Error fetching data from prometheus server. " + e.getMessage()); - } + } catch (IOException e) { + LOG.error("Error fetching data from prometheus server", e); + throw new RuntimeException( + "Error fetching data from prometheus server. " + e.getMessage(), e); + }This keeps the external message but improves diagnostics.
prometheus/src/main/java/org/opensearch/sql/prometheus/request/system/PrometheusDescribeMetricRequest.java (1)
70-83: Consider preserving the cause when wrappingIOExceptionLabel fetching and mapping into
fieldTypeslooks correct, and the error message is clear. For easier debugging, you might want to keep the originalIOExceptionas a cause and log the full stack trace:- } catch (IOException e) { - LOG.error( - "Error while fetching labels for {} from prometheus: {}", - metricName, - e.getMessage()); - throw new PrometheusClientException( - String.format( - "Error while fetching labels " + "for %s from prometheus: %s", - metricName, e.getMessage())); - } + } catch (IOException e) { + LOG.error( + "Error while fetching labels for {} from prometheus", + metricName, + e); + throw new PrometheusClientException( + String.format( + "Error while fetching labels for %s from prometheus: %s", + metricName, e.getMessage()), + e); + }Assuming
PrometheusClientExceptionhas an overload accepting a cause, this improves observability without changing behavior.prometheus/src/main/java/org/opensearch/sql/prometheus/functions/scan/QueryExemplarsFunctionTableScanOperator.java (1)
38-48: Enhance error logging aroundqueryExemplarsfailuresDirect call to
prometheusClient.queryExemplarsand initialization ofQueryExemplarsFunctionResponseHandlelooks correct. As with the metric scan, only logginge.getMessage()drops stack information.You can improve diagnostics by logging the exception and optionally chaining it:
- } catch (IOException e) { - LOG.error(e.getMessage()); - throw new RuntimeException( - String.format( - "Error fetching data from prometheus server: %s", e.getMessage())); - } + } catch (IOException e) { + LOG.error("Error fetching exemplars from prometheus server", e); + throw new RuntimeException( + String.format( + "Error fetching data from prometheus server: %s", e.getMessage()), + e); + }prometheus/src/main/java/org/opensearch/sql/prometheus/request/system/PrometheusListMetricsRequest.java (1)
37-57: Preserve IOException cause and log full stack when wrapping metric retrieval failuresThe behavior after removing
AccessControllerlooks equivalent, but the current error handling drops useful context:
LOG.error(..., e.getMessage())only logs the message, not the stack trace.- The new
RuntimeExceptionis constructed with just the message; the originalIOExceptionis not set as the cause.To ease debugging while keeping the same control flow, consider:
- } catch (IOException e) { - LOG.error( - "Error while fetching metric list for from prometheus: {}", e.getMessage()); - throw new RuntimeException( - String.format( - "Error while fetching metric list " + "for from prometheus: %s", - e.getMessage())); - } + } catch (IOException e) { + LOG.error("Error while fetching metric list from prometheus", e); + throw new RuntimeException( + String.format("Error while fetching metric list from prometheus: %s", e.getMessage()), + e); + }This keeps the same observable behavior while aligning better with the “specific exception types with meaningful messages” and diagnosability goals.
direct-query-core/src/main/java/org/opensearch/sql/prometheus/query/PrometheusQueryHandler.java (2)
50-106: Clarify executeQuery error handling and future-proof enum switchThe new direct try/catch in
executeQuerylooks reasonable and removes the privileged wrapper cleanly, but a couple of details are worth tightening:
Contract vs throws declaration: The method still declares
throws IOException, but allIOExceptions are now caught and converted to JSON error responses. That’s fine if callers only ever inspect the returned JSON, but it means thethrowsclause is effectively vestigial. Please confirm that nothing upstream relies on catchingIOExceptionfrom this method.Enum default behavior:
switch (queryType) { case RANGE: ... case INSTANT: default: ... }With this pattern, any future
PrometheusQueryTypevalue will silently take the INSTANT path (usingoptions.getTime()), which may be null/invalid for a new type. Consider making the switch exhaustive and treating unknown values as an explicit error instead of falling through to INSTANT, e.g.:switch (queryType) {
case RANGE:...case INSTANT:default:...}
- switch (queryType) {
case RANGE:...case INSTANT:...default:return createErrorJson("Unsupported Prometheus query type: " + queryType);- }
This keeps the non-privileged behavior while making failures more predictable if the enum expands later. --- `115-211`: **Improve exception wrapping and error messages in getResources/writeResources** The removal of privileged blocks in `getResources` and `writeResources` looks good; both now use straightforward switches and a single `IOException` catch. A few small improvements would make error handling clearer and more informative: 1. **Preserve the original cause in PrometheusClientException** In both methods you currently have: ```java } catch (IOException e) { LOG.error("Error getting resources", e); throw new PrometheusClientException( String.format("Error while getting resources for %s: %s", request.getResourceType(), e.getMessage())); }This logs the stack trace but discards the cause from the thrown exception. Prefer:
- } catch (IOException e) { - LOG.error("Error getting resources", e); - throw new PrometheusClientException( - String.format( - "Error while getting resources for %s: %s", - request.getResourceType(), e.getMessage())); - } + } catch (IOException e) { + LOG.error("Error getting resources", e); + throw new PrometheusClientException( + String.format( + "Error while getting resources for %s: %s", + request.getResourceType(), e.getMessage()), + e); + }(And analogously for
writeResources), so callers can inspect the root cause.
Message wording in writeResources
In
writeResources, the log and exception messages still talk about “getting resources” even though this is a write operation:LOG.error("Error getting resources", e); throw new PrometheusClientException("Error while getting resources for %s: %s", ...);For clarity (and for future debugging), it would be better to use “writing resources” here.
These changes keep semantics the same while improving diagnosability and aligning with the guideline of meaningful exception messages.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
async-query-core/src/main/java/org/opensearch/sql/spark/client/EMRServerlessClientFactoryImpl.java(1 hunks)async-query-core/src/main/java/org/opensearch/sql/spark/client/EmrServerlessClientImpl.java(3 hunks)async-query/src/main/java/org/opensearch/sql/spark/config/SparkExecutionEngineConfigClusterSettingLoader.java(1 hunks)core/src/main/java/org/opensearch/sql/executor/QueryService.java(2 hunks)direct-query-core/src/main/java/org/opensearch/sql/prometheus/query/PrometheusQueryHandler.java(2 hunks)direct-query-core/src/main/java/org/opensearch/sql/prometheus/utils/PrometheusClientUtils.java(1 hunks)direct-query/src/main/java/org/opensearch/sql/directquery/transport/model/ExecuteDirectQueryActionResponse.java(3 hunks)legacy/src/main/java/org/opensearch/sql/legacy/cursor/DefaultCursor.java(2 hunks)opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java(2 hunks)opensearch/src/main/java/org/opensearch/sql/opensearch/security/SecurityAccess.java(1 hunks)opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/core/ExpressionScript.java(2 hunks)prometheus/src/main/java/org/opensearch/sql/prometheus/functions/scan/QueryExemplarsFunctionTableScanOperator.java(1 hunks)prometheus/src/main/java/org/opensearch/sql/prometheus/functions/scan/QueryRangeFunctionTableScanOperator.java(1 hunks)prometheus/src/main/java/org/opensearch/sql/prometheus/request/system/PrometheusDescribeMetricRequest.java(1 hunks)prometheus/src/main/java/org/opensearch/sql/prometheus/request/system/PrometheusListMetricsRequest.java(1 hunks)prometheus/src/main/java/org/opensearch/sql/prometheus/storage/PrometheusMetricScan.java(1 hunks)prometheus/src/main/java/org/opensearch/sql/prometheus/storage/PrometheusStorageFactory.java(1 hunks)prometheus/src/test/java/org/opensearch/sql/prometheus/storage/PrometheusStorageFactoryTest.java(2 hunks)protocol/src/main/java/org/opensearch/sql/protocol/response/format/ErrorFormatter.java(2 hunks)protocol/src/main/java/org/opensearch/sql/protocol/response/format/JsonResponseFormatter.java(2 hunks)protocol/src/main/java/org/opensearch/sql/protocol/response/format/YamlResponseFormatter.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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:
prometheus/src/test/java/org/opensearch/sql/prometheus/storage/PrometheusStorageFactoryTest.javaprometheus/src/main/java/org/opensearch/sql/prometheus/storage/PrometheusMetricScan.javaprotocol/src/main/java/org/opensearch/sql/protocol/response/format/JsonResponseFormatter.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javaprometheus/src/main/java/org/opensearch/sql/prometheus/functions/scan/QueryExemplarsFunctionTableScanOperator.javaprometheus/src/main/java/org/opensearch/sql/prometheus/request/system/PrometheusListMetricsRequest.javaasync-query-core/src/main/java/org/opensearch/sql/spark/client/EMRServerlessClientFactoryImpl.javalegacy/src/main/java/org/opensearch/sql/legacy/cursor/DefaultCursor.javaopensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/core/ExpressionScript.javadirect-query-core/src/main/java/org/opensearch/sql/prometheus/utils/PrometheusClientUtils.javaasync-query-core/src/main/java/org/opensearch/sql/spark/client/EmrServerlessClientImpl.javaprometheus/src/main/java/org/opensearch/sql/prometheus/request/system/PrometheusDescribeMetricRequest.javaprometheus/src/main/java/org/opensearch/sql/prometheus/functions/scan/QueryRangeFunctionTableScanOperator.javadirect-query-core/src/main/java/org/opensearch/sql/prometheus/query/PrometheusQueryHandler.javaprometheus/src/main/java/org/opensearch/sql/prometheus/storage/PrometheusStorageFactory.javaprotocol/src/main/java/org/opensearch/sql/protocol/response/format/ErrorFormatter.javaopensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.javaprotocol/src/main/java/org/opensearch/sql/protocol/response/format/YamlResponseFormatter.javadirect-query/src/main/java/org/opensearch/sql/directquery/transport/model/ExecuteDirectQueryActionResponse.javaasync-query/src/main/java/org/opensearch/sql/spark/config/SparkExecutionEngineConfigClusterSettingLoader.javaopensearch/src/main/java/org/opensearch/sql/opensearch/security/SecurityAccess.java
⚙️ CodeRabbit configuration file
**/*.java: - 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 methods are under 20 lines with single responsibility
- Verify 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:
prometheus/src/test/java/org/opensearch/sql/prometheus/storage/PrometheusStorageFactoryTest.javaprometheus/src/main/java/org/opensearch/sql/prometheus/storage/PrometheusMetricScan.javaprotocol/src/main/java/org/opensearch/sql/protocol/response/format/JsonResponseFormatter.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javaprometheus/src/main/java/org/opensearch/sql/prometheus/functions/scan/QueryExemplarsFunctionTableScanOperator.javaprometheus/src/main/java/org/opensearch/sql/prometheus/request/system/PrometheusListMetricsRequest.javaasync-query-core/src/main/java/org/opensearch/sql/spark/client/EMRServerlessClientFactoryImpl.javalegacy/src/main/java/org/opensearch/sql/legacy/cursor/DefaultCursor.javaopensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/core/ExpressionScript.javadirect-query-core/src/main/java/org/opensearch/sql/prometheus/utils/PrometheusClientUtils.javaasync-query-core/src/main/java/org/opensearch/sql/spark/client/EmrServerlessClientImpl.javaprometheus/src/main/java/org/opensearch/sql/prometheus/request/system/PrometheusDescribeMetricRequest.javaprometheus/src/main/java/org/opensearch/sql/prometheus/functions/scan/QueryRangeFunctionTableScanOperator.javadirect-query-core/src/main/java/org/opensearch/sql/prometheus/query/PrometheusQueryHandler.javaprometheus/src/main/java/org/opensearch/sql/prometheus/storage/PrometheusStorageFactory.javaprotocol/src/main/java/org/opensearch/sql/protocol/response/format/ErrorFormatter.javaopensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.javaprotocol/src/main/java/org/opensearch/sql/protocol/response/format/YamlResponseFormatter.javadirect-query/src/main/java/org/opensearch/sql/directquery/transport/model/ExecuteDirectQueryActionResponse.javaasync-query/src/main/java/org/opensearch/sql/spark/config/SparkExecutionEngineConfigClusterSettingLoader.javaopensearch/src/main/java/org/opensearch/sql/opensearch/security/SecurityAccess.java
**/*Test.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*Test.java: All new business logic requires unit tests
Name unit tests with*Test.javasuffix in OpenSearch SQL
Files:
prometheus/src/test/java/org/opensearch/sql/prometheus/storage/PrometheusStorageFactoryTest.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify test coverage for new business logic
- Check test naming follows conventions (*Test.java for unit, *IT.java for integration)
- 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:
prometheus/src/test/java/org/opensearch/sql/prometheus/storage/PrometheusStorageFactoryTest.java
🧠 Learnings (2)
📚 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:
core/src/main/java/org/opensearch/sql/executor/QueryService.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:
opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/core/ExpressionScript.java
🧬 Code graph analysis (5)
core/src/main/java/org/opensearch/sql/executor/QueryService.java (1)
core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java (1)
CalcitePlanContext(30-137)
direct-query-core/src/main/java/org/opensearch/sql/prometheus/utils/PrometheusClientUtils.java (1)
common/src/main/java/org/opensearch/sql/common/setting/Settings.java (1)
Settings(17-105)
prometheus/src/main/java/org/opensearch/sql/prometheus/storage/PrometheusStorageFactory.java (1)
direct-query-core/src/main/java/org/opensearch/sql/prometheus/utils/PrometheusClientUtils.java (1)
PrometheusClientUtils(29-166)
opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java (1)
core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java (1)
OpenSearchRelRunners(337-366)
protocol/src/main/java/org/opensearch/sql/protocol/response/format/YamlResponseFormatter.java (1)
core/src/main/java/org/opensearch/sql/utils/YamlFormatter.java (1)
YamlFormatter(19-51)
⏰ 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: bwc-tests-rolling-upgrade (25)
- GitHub Check: build-linux (25, doc)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: build-linux (25, unit)
- GitHub Check: build-linux (25, integration)
- GitHub Check: build-linux (21, doc)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: build-linux (21, unit)
- GitHub Check: build-linux (21, integration)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: security-it-linux (25)
- GitHub Check: security-it-linux (21)
- 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 (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- 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 (macos-14, 21, unit)
- 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: CodeQL-Scan (java)
- GitHub Check: test-sql-cli-integration (21)
🔇 Additional comments (16)
protocol/src/main/java/org/opensearch/sql/protocol/response/format/JsonResponseFormatter.java (1)
57-59:jsonifystyle switch is concise and consistent with error formattingThe ternary on
styleto pickprettyJsonifyvscompactJsonifymirrors the throwable formatting path and keeps the behavior clear and deterministic without any privileged wrapper. No issues from a correctness, performance, or style standpoint.protocol/src/main/java/org/opensearch/sql/protocol/response/format/YamlResponseFormatter.java (1)
43-45: LGTM! Clean removal of AccessController wrapper.The direct call to
YamlFormatter.formatToYaml()maintains equivalent behavior while simplifying the code. Exception handling is preserved in the formatter itself, and this change aligns perfectly with the PR objective to remove deprecated AccessController references.async-query/src/main/java/org/opensearch/sql/spark/config/SparkExecutionEngineConfigClusterSettingLoader.java (1)
25-26: Verify that removingAccessController.doPrivileged()doesn't affect functionality.The direct call to
toSparkExecutionEngineConfig()previously ran underAccessController.doPrivileged(). Ensure this method and any operations it performs don't require elevated privileges to function correctly. Check the implementation for security-sensitive operations like reflection, classloading, or file I/O that may depend on privilege escalation.opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/core/ExpressionScript.java (1)
49-53: LGTM: AccessController removal in constructor.The direct calls to
extractFieldsandbuildValueFactoryare correct. These internal initialization operations don't require privileged execution or additional error handling.async-query-core/src/main/java/org/opensearch/sql/spark/client/EmrServerlessClientImpl.java (3)
62-73: LGTM! Privileged action correctly replaced with direct execution.The refactoring correctly removes the
AccessController.doPrivilegedwrapper while preserving all error handling semantics:
- Exception logging maintained
- Metrics increment preserved
ValidationExceptiontoIllegalArgumentExceptiontranslation intact- Generic error handling for other exceptions unchanged
82-89: LGTM! Clean refactoring with preserved error handling.The privileged action removal is correct, maintaining the original behavior:
- Direct EMR API call without wrapper
- Error logging and metrics increment preserved
- Generic exception handling unchanged
99-114: LGTM! Exception propagation logic correctly preserved.The refactoring properly maintains the conditional exception handling:
- The
allowExceptionPropagationflag behavior is preserved (early rethrow on line 104)- Error logging with jobId context maintained
- Metrics increment preserved
ValidationExceptiontoIllegalArgumentExceptiontranslation intactasync-query-core/src/main/java/org/opensearch/sql/spark/client/EMRServerlessClientFactoryImpl.java (1)
57-62: Client creation correctly preserves logic after privileged action removal.The code snippet shows proper implementation: the
DefaultAWSCredentialsProviderChainis initialized at the correct scope, and the AWS client is properly wrapped inEmrServerlessClientImpl. However, the broader impact of removing the privileged action wrapper cannot be fully verified without access to the complete codebase to check for:
- Remaining
AccessController.doPrivileged()usages in related credential initialization code- Integration test results confirming credential provider chain behavior in the target deployment environment
- Security policy configurations that may affect system property and environment variable access
The code change itself is structurally sound, but security implications require verification in the deployment context.
legacy/src/main/java/org/opensearch/sql/legacy/cursor/DefaultCursor.java (1)
201-206: Deserialization error handling looks correct and consistent.Catching
JsonProcessingExceptionaroundobjectMapper.readValue(json.getString(SORT_FIELDS), Object[].class)and wrapping it in aRuntimeExceptionmatches the existing error-handling style in this class and clearly reports JSON parsing issues forsortFields.Please just confirm that this behavior (failing fast with
RuntimeExceptionon malformed cursor JSON) is acceptable for all callers ofDefaultCursor.from, since it will turn any corruptedSORT_FIELDSpayload into an unrecoverable error rather than a more graceful fallback.direct-query/src/main/java/org/opensearch/sql/directquery/transport/model/ExecuteDirectQueryActionResponse.java (1)
69-73: Unable to verify error handling inconsistency due to inaccessible repository.The review comment raises concerns about error handling inconsistency in lines 69-73, specifically that catching
IOExceptionand wrapping it inRuntimeExceptionviolates the declaredthrows IOExceptioncontract. However, I cannot access the repository to verify:
- The actual method signature and declared exceptions
- The implementation details at the specified line numbers
- The error handling approach in the
parseResultmethod for comparison- Conventions used in similar response classes in the codebase
Manual verification is needed to confirm whether:
- The method actually declares
throws IOExceptionand if wrapping IOException in RuntimeException is truly a contract violation- The inconsistency with
parseResulterror handling is as described- This pattern is intentional per OpenSearch conventions or a genuine issue
core/src/main/java/org/opensearch/sql/executor/QueryService.java (1)
101-108: Calcite execution path after removing privileged wrapper looks correctCreating
CalcitePlanContextlocally and runninganalyze → mergeAdjacentFilters → optimize → convertToCalcitePlan → executionEngine.executedirectly inside the existingCalcitePlanContext.runkeeps the control flow and fallback handling unchanged while removing the privileged block. No issues spotted here.direct-query-core/src/main/java/org/opensearch/sql/prometheus/utils/PrometheusClientUtils.java (1)
53-82: Direct OkHttpClient construction without privileged block looks goodClient configuration (timeouts, redirect handling, URI deny-list interceptor, and auth interceptors for BASIC/AWSSIGV4) is preserved while removing the privileged wrapper. As long as
settingsis non-null at call sites, this is behaviorally equivalent.prometheus/src/main/java/org/opensearch/sql/prometheus/storage/PrometheusStorageFactory.java (1)
54-62: Storage engine creation flow remains sound after removing privileged blockValidating properties first and then constructing
PrometheusClientImplwithPrometheusClientUtils.getHttpClient(requiredConfig, settings)andnew URI(requiredConfig.get(URI))inside atrythat catchesURISyntaxException | UnknownHostExceptionpreserves the prior behavior (wrapping URI issues inIllegalArgumentException) while simplifying control flow. No functional concerns here.prometheus/src/test/java/org/opensearch/sql/prometheus/storage/PrometheusStorageFactoryTest.java (1)
133-133: Updated test URIs tohttps://opensearch.orgare fineSwitching the Prometheus URI in these tests to
https://opensearch.orgkeeps the assertions intact and avoids depending on a placeholder domain. No behavioral change to the tests beyond the literal value.Also applies to: 153-154
protocol/src/main/java/org/opensearch/sql/protocol/response/format/ErrorFormatter.java (1)
18-19: Direct Gson usage without privileged wrappers appears behaviorally equivalentInitializing
PRETTY_PRINT_GSONandGSONviaSerializeUtils.getGsonBuilder()(with pretty-printing + disabled HTML escaping vs. compact + disabled HTML escaping) and usingtoJsondirectly incompactJsonify/prettyJsonifykeeps the JSON output semantics the same while removing the security-manager-related indirection. No issues spotted with the change itself; just ensureSerializeUtils.getGsonBuilder()remains configured as expected for error payloads.Also applies to: 34-39
opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java (1)
160-212: Verify connection lifecycle in OpenSearchRelRunners.run() and async error handling in execute(RelNode, ...)The review raises two concerns that require verification against the actual implementation:
Potential PreparedStatement/Connection lifecycle issue: If
OpenSearchRelRunners.run()closes theConnectionvia try-with-resources before returning thePreparedStatement, thenstatement.executeQuery()at line 207 would operate on a closed connection. Confirm whether:
- The connection is managed within
run()and closed before return, or- Connection lifecycle is managed at the call site, or
- The query is fully executed within
run()before closing.Error handling inconsistency: The
execute(RelNode, ...)method catchesSQLExceptionand rethrowsRuntimeExceptionwithout notifying the listener, unlike theexecute(PhysicalPlan, ...)overload which callslistener.onFailure(e). Verify whetherclient.schedule()automatically translates thrown exceptions into listener callbacks, or if missinglistener.onFailure()causes silent failures.
| try { | ||
| serializedResult = OBJECT_MAPPER.writeValueAsString(result); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException("Failed to serialize result", e); | ||
| } |
There was a problem hiding this comment.
Same error handling issue: RuntimeException bypasses IOException contract.
This has the same problem as the constructor (lines 69-73): catching IOException and wrapping it in RuntimeException violates the throws IOException declaration at line 87. Since writeTo overrides ActionResponse.writeTo(StreamOutput), maintaining the IOException contract is especially important for polymorphic behavior.
Apply the same fix as suggested for the constructor - either let IOException propagate directly or document why RuntimeException is necessary.
- // Serialize the data source result to JSON
- final String serializedResult;
- try {
- serializedResult = OBJECT_MAPPER.writeValueAsString(result);
- } catch (IOException e) {
- throw new RuntimeException("Failed to serialize result", e);
- }
+ // Serialize the data source result to JSON
+ String serializedResult = OBJECT_MAPPER.writeValueAsString(result);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| serializedResult = OBJECT_MAPPER.writeValueAsString(result); | |
| } catch (IOException e) { | |
| throw new RuntimeException("Failed to serialize result", e); | |
| } | |
| // Serialize the data source result to JSON | |
| String serializedResult = OBJECT_MAPPER.writeValueAsString(result); |
🤖 Prompt for AI Agents
In
direct-query/src/main/java/org/opensearch/sql/directquery/transport/model/ExecuteDirectQueryActionResponse.java
around lines 108-112 (same issue as constructor at lines 69-73): the code
catches IOException from OBJECT_MAPPER.writeValueAsString and wraps it in a
RuntimeException which violates the declared throws IOException for writeTo;
remove the try/catch and let the IOException propagate (or rethrow it as an
IOException) so the method adheres to the IOException contract expected by
ActionResponse.writeTo.
There was a problem hiding this comment.
Will skip this, it's pre-existing behavior and I'm not sure what the impact would be to change it
Long-term: use a report wrapper #4919
legacy/src/main/java/org/opensearch/sql/legacy/cursor/DefaultCursor.java
Show resolved
Hide resolved
opensearch/src/main/java/org/opensearch/sql/opensearch/security/SecurityAccess.java
Outdated
Show resolved
Hide resolved
…ursor.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Simeon Widdis <[email protected]> Signed-off-by: Simeon Widdis <[email protected]>
Swiddis
force-pushed
the
maintenance/remove-access-controller
branch
from
2dc01fe to
574f33b
Compare
Signed-off-by: Simeon Widdis <[email protected]>
3 participants
Description
Removes all uses of AccessController, which is deprecated since version 3.0. Finishing what #4900 started.
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.