MLE-26427 Fixing Polaris issues #1881
Conversation
|
Copyright Validation Results ✅ Valid Files
✅ All files have valid copyright headers! |
There was a problem hiding this comment.
Pull request overview
This PR addresses Polaris static analysis issues by adding null checks throughout the codebase. The changes focus on defensive programming to prevent potential null pointer exceptions, particularly when handling HTTP responses and JSON operations.
Changes:
- Added null checks for HTTP response objects before accessing their methods
- Added null checks for JSON pointer operations and serialized content
- Added null check for consumer callback before invocation
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| OkHttpServices.java | Added defensive null checks for HTTP response objects and response body before accessing their properties and methods |
| IncrementalWriteFilter.java | Added null checks for serialized content and consumer callback to prevent null pointer exceptions |
| ContentExclusionUtil.java | Added null check for JSON pointer last segment before accessing its properties |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| int status = response != null ? response.code() : 0; | ||
| if (status == STATUS_NOT_FOUND) { |
There was a problem hiding this comment.
When response is null, status is set to 0, but the subsequent logic continues to check specific status codes and may attempt to extract error fields from the null response. Consider adding an early return or throwing an exception when response is null, or ensure that all downstream methods like extractErrorFields() handle null responses gracefully.
| ResponseBody body = response != null ? response.body() : null; | ||
| Object entity = (body != null && body.contentLength() != 0) ? getEntity(body, as) : null; |
There was a problem hiding this comment.
The null check for response is repeated multiple times in this method. Since the response is checked at line 622 and subsequent logic depends on it being non-null (line 624 throws an exception for STATUS_NOT_FOUND), the additional null checks at lines 651, 663 become redundant. Consider adding a single null check early in the method or documenting why response could legitimately be null at these points despite earlier checks.
| final String location = response != null ? response.headers().get("Location") : null; | ||
|
|
||
| final List<ClientCookie> cookies = new ArrayList<>(); |
There was a problem hiding this comment.
The location and cookies variables are marked as final but their values are only used after the response null check. Since they're declared final, consider moving the declarations inside the null check block or removing the final modifier if they need to be conditionally initialized.
| final String location = response != null ? response.headers().get("Location") : null; | |
| final List<ClientCookie> cookies = new ArrayList<>(); | |
| String location = response != null ? response.headers().get("Location") : null; | |
| List<ClientCookie> cookies = new ArrayList<>(); |
| } | ||
|
|
||
| closeResponse(response); | ||
| if (location == null) throw new MarkLogicInternalException("transaction open failed to provide location"); |
There was a problem hiding this comment.
When response is null, location will be null (line 1517), leading to this exception being thrown. However, the error message 'transaction open failed to provide location' is misleading when the actual issue is a null response. Consider checking for null response explicitly before this line and providing a more accurate error message.
| ResponseBody body = response.body(); | ||
| T entity = body.contentLength() != 0 ? getEntity(body, as) : null; | ||
| ResponseBody body = response != null ? response.body() : null; | ||
| T entity = body != null && body.contentLength() != 0 ? getEntity(body, as) : null; |
There was a problem hiding this comment.
The condition 'body != null && body.contentLength() != 0' is repeated in multiple locations (lines 664, 927, 2586, 4846, 5065). Consider extracting this logic into a helper method like 'hasContent(ResponseBody body)' to improve maintainability and ensure consistency.
| // Not sure if the doc can have null content - possibly for a naked properties document? - but if it | ||
| // does, just include it in the write set. |
There was a problem hiding this comment.
The comment expresses uncertainty about when serializedContent can be null ('Not sure if the doc can have null content'). This uncertainty should be resolved through code analysis or documentation review. Update the comment to either explain the specific scenarios where null content occurs or remove the speculative language if the behavior is now understood.
| // Not sure if the doc can have null content - possibly for a naked properties document? - but if it | |
| // does, just include it in the write set. | |
| // If the content cannot be serialized (serializeContent returned null), include the document in the write set unchanged. |
rjrudin
force-pushed
the
feature/polaris
branch
2 times, most recently
from
09a5b1e to
1d66359
Compare
Most of these already existed in OkHttpServices, but Polaris decided to report them recently
rjrudin
force-pushed
the
feature/polaris
branch
from
1d66359 to
ff07804
Compare
Most of these already existed in OkHttpServices, but Polaris decided to report them recently