fix: write error handling - #425
Conversation
ca5ff0d to
78953cb
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors how RestClient formats and classifies write-related HTTP error responses (notably InfluxDB 3 write/partial-write formats), and updates unit/integration tests to reflect the new behavior.
Changes:
- Added an overloaded
RestClient.request(...)that propagatesacceptPartial/useV2Apiflags so partial-write handling can be gated by client options. - Reworked write-error parsing to build
InfluxDBPartialWriteExceptiondetails from JSON responses (and adjusted expected messages in tests). - Introduced a small
Utils.isNumerichelper and expanded unit test coverage with a table-driven set of partial-write cases.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/com/influxdb/v3/client/internal/RestClient.java | Refactors error parsing/exception selection for write endpoints; adds partial-write gating logic. |
| src/main/java/com/influxdb/v3/client/internal/InfluxDBClientImpl.java | Passes acceptPartial / useV2Api flags into RestClient.request(...) for write calls. |
| src/main/java/com/influxdb/v3/client/internal/Utils.java | Adds numeric-check helper used by error parsing. |
| src/main/java/com/influxdb/v3/client/InfluxDBPartialWriteException.java | Adjusts LineError nullability annotations. |
| src/test/java/com/influxdb/v3/client/internal/RestClientTest.java | Updates and expands tests for partial-write parsing/message formatting. |
| src/test/java/com/influxdb/v3/client/integration/E2ETest.java | Updates integration expectation for non-accept-partial write errors. |
Suppressed comments (2)
src/main/java/com/influxdb/v3/client/InfluxDBPartialWriteException.java:83
- Changing
LineError.errorMessageto@Nullableis a public API contract change and it’s inconsistent with current callers (tests andRestClient.createErrorMsgDetails) that treat it as always present. If the intent is still that a line error always has an error message, keep it@Nonnulland avoid propagating nulls to API consumers.
This issue also appears on line 95 of the same file.
public LineError(@Nullable final Integer lineNumber,
@Nullable final String errorMessage,
@Nullable final String originalLine) {
this.lineNumber = lineNumber;
this.errorMessage = errorMessage;
this.originalLine = originalLine;
src/main/java/com/influxdb/v3/client/InfluxDBPartialWriteException.java:100
- If
errorMessage()is intended to be always present for aLineError, its accessor should stay@Nonnullto match the API contract and existing usage.
* @return line-level error message
*/
@Nullable
public String errorMessage() {
return errorMessage;
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
d4a06d2 to
9e7e4cd
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Suppressed comments (6)
Previously missed (3) — in code that hasn't changed since the last review.
src/main/java/com/influxdb/v3/client/InfluxDBPartialWriteException.java:82
InfluxDBPartialWriteException.LineError#errorMessagewas previously non-null in the public API contract. Making it@Nullableis a breaking/loosening change for callers and also increases the risk of NPEs in formatting code that assumes a message exists. Unless null is a valid state that callers must handle, keep this@Nonnull.
This issue also appears on line 97 of the same file.
public LineError(@Nullable final Integer lineNumber,
@Nullable final String errorMessage,
@Nullable final String originalLine) {
this.lineNumber = lineNumber;
this.errorMessage = errorMessage;
src/main/java/com/influxdb/v3/client/internal/RestClient.java:263
if (root == null || root.toString().isEmpty())is effectively dead code (Jackson returns a non-null node, andtoString()won’t be empty). As a result, header-based fallbacks won’t execute anymore for empty bodies. Switching this to abodyemptiness check restores the intended fallback behavior.
if (root == null || root.toString().isEmpty()) {
reason = Stream.of("X-Platform-Error-Code", "X-Influx-Error", "X-InfluxDb-Error")
.map(name -> response.headers().firstValue(name).orElse(null))
.filter(message -> message != null && !message.isEmpty()).findFirst()
.orElse("");
src/main/java/com/influxdb/v3/client/internal/RestClient.java:243
- The response
Content-Typecan include parameters (e.g.text/plain; charset=utf-8). Using strict equality here can cause non-JSON text responses to be routed into JSON parsing unnecessarily. Consider a case-insensitive prefix match instead.
if (contentType != null && contentType.equals("text/plain")) {
var message = String.format("HTTP status code: %d; Message: %s", statusCode, body);
throw new InfluxDBApiHttpException(message, response.headers(), response.statusCode());
src/main/java/com/influxdb/v3/client/internal/RestClient.java:336
- This method return type uses
@NonNull(jspecify), which will fail to compile if jspecify isn’t on the classpath and is also inconsistent with the rest of this class’javax.annotation.Nonnullusage. Use@Nonnull(or omit the redundant annotation) instead.
@Nonnull
private @NonNull List<String> createErrorMsgDetails(
@Nullable final ParseLineErrorResult result,
@Nullable final JsonNode root
) {
src/test/java/com/influxdb/v3/client/internal/RestClientTest.java:1185
@NonNullontoString()requires the jspecify dependency (and isn’t providing much value here). Removing the annotation keeps the test independent of that external annotation library.
@Override
public @NonNull String toString() {
src/main/java/com/influxdb/v3/client/InfluxDBPartialWriteException.java:99
- For consistency with the constructor contract and to avoid forcing callers to handle nulls that should never occur,
errorMessage()should remain@Nonnull.
@Nullable
public String errorMessage() {
return errorMessage;
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #425 +/- ##
==========================================
- Coverage 88.73% 88.41% -0.33%
==========================================
Files 21 22 +1
Lines 1553 1536 -17
Branches 281 271 -10
==========================================
- Hits 1378 1358 -20
- Misses 77 78 +1
- Partials 98 100 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
4428d3b to
5a70263
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Suppressed comments (1)
src/test/java/com/influxdb/v3/client/internal/UtilsTest.java:77
- This test duplicates coverage already provided by the
@NullAndEmptySourceinisIntegerInvalid, increasing test noise without adding assertions. Consider removing it and relying on the parameterized test.
@Test
void testEmpty() {
Assertions.assertThat(Utils.isInteger("")).isFalse();
}
65f0552 to
afa8797
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
src/main/java/com/influxdb/v3/client/InfluxDBPartialWriteException.java:107
LineError.originalLine()is a record component accessor; it does not override any supertype method, so@Overridewill not compile here. Remove the@Overrideannotation.
@Override
@Nullable
public String originalLine() {
return originalLine;
}
src/main/java/com/influxdb/v3/client/InfluxDBPartialWriteException.java:98
LineError.errorMessage()is the record component accessor; the@Overrideannotation here will not compile, and the accessor should remain@Nonnullto match the constructor contract (line-level error message is required).
@Override
@Nullable
public String errorMessage() {
return errorMessage;
}
c82babc to
1e8aafc
Compare
3f93a73 to
fa7f0c6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
src/main/java/com/influxdb/v3/client/InfluxDBPartialWriteException.java:78
- The LineError record allows null
errorMessagedespite being annotated @nonnull, and the constructor doesn't enforce non-null. This can lead to nulls flowing into error formatting (e.g., message details rendering as "null"). Enforce non-null at construction time.
public LineError(@Nullable final Integer lineNumber,
@Nonnull final String errorMessage,
@Nullable final String originalLine) {
this.lineNumber = lineNumber;
this.errorMessage = errorMessage;
src/main/java/com/influxdb/v3/client/InfluxDBPartialWriteException.java:96
errorMessage()is documented as a required field (and the constructor is annotated @nonnull), but the accessor is annotated @nullable. This is an API contract mismatch for callers; it should be@Nonnullif null is not allowed.
* @return line-level error message
*/
@Nullable
public String errorMessage() {
return errorMessage;
}
src/main/java/com/influxdb/v3/client/internal/RestClient.java:348
- The Javadoc for
createErrorMsgDetailssaysresult/rootmay be null and that the method returns an empty list if they are null, but the parameters are annotated@Nonnulland the implementation doesn't handle nulls. Please align the Javadoc with the actual contract.
* @param result the parsing result containing details about failed lines; may be null
* @param root the JSON node representing the response data; may be null
* @return a list of generated error message details; an empty list if the input parameters are null
*/
src/main/java/com/influxdb/v3/client/internal/RestClient.java:440
errIsJsonLikeContentTypeis now unused after the refactor (the request path always attempts JSON parsing unless Content-Type is text/plain). Keeping an unused private helper adds noise and may fail builds that treat warnings as errors.
private boolean errIsJsonLikeContentType(@Nullable final String contentType) {
return contentType == null
|| contentType.isEmpty()
|| contentType.regionMatches(true, 0, "application/json", 0, "application/json".length());
}
3cda9e1 to
03fd081
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
src/main/java/com/influxdb/v3/client/InfluxDBPartialWriteException.java:96
LineError#errorMessage()is documented/constructed as non-null (@Nonnullctor param) but the overridden accessor is annotated@Nullable, which weakens the public API contract and may confuse nullness tooling. It should remain@Nonnull(or drop the override entirely and annotate the record component instead).
/**
* @return line-level error message
*/
@Nullable
public String errorMessage() {
return errorMessage;
}
8ea6513 to
11b2036
Compare
08b9acd to
fc1eba2
Compare
Closes #
Proposed Changes
Changes
objectMapper.readTree(body)function to only once.- Error response status code is
400.- Error response format
{"error":"...","data":[{"error_message":"...","line_number":2,"original_line": "..."}]}is returned withdatamust be an array.-
accept_partialis set totrue.- Write endpoint must be
api/v3/write_lp.Checklist