Skip to content

fix: partial write error handling - #318

Merged
NguyenHoangSon96 merged 1 commit into
mainfrom
fix/partial-write-error-handling
Oct 15, 2026
Merged

NguyenHoangSon96 merged 1 commit into
mainfrom
fix/partial-write-error-handling

Conversation

@NguyenHoangSon96

@NguyenHoangSon96 NguyenHoangSon96 commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

Closes #

Proposed Changes

  • How the errors are handled and test cases adjustment in this PR are heavily influenced by this PR.

Changes

  • Tests were modified to accommodate with new definitions of partial write errors, which are:
    - Error response status code is 400.
    - Error response format {"error":"...","data":[{"error_message":"...","line_number":2,"original_line": "..."}]} is returned with data must be an array.
    - accept_partial is set to true.
    - Write endpoint must be api/v3/write_lp.

Checklist

  • CHANGELOG.md updated
  • Rebased/mergeable
  • A test has been added if appropriate
  • Tests pass
  • Commit messages are conventional
  • Sign CLA (if not already signed)

@NguyenHoangSon96 NguyenHoangSon96 self-assigned this Sep 8, 2026
Copilot AI lite review requested due to automatic review settings September 8, 2026 08:23
@NguyenHoangSon96
NguyenHoangSon96 marked this pull request as draft September 8, 2026 08:23

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

There are confirmed correctness/quality issues in the updated error-message formatting logic and test setup (notably null content-type handling and a misleading integration test name) that should be fixed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR tightens how v3 write (/api/v3/write_lp) “partial write” responses are detected and surfaced, aiming to throw InfluxDBPartialWriteException only when the response matches the expected partial-write shape and the client accepts partial writes, while improving error-message formatting and expanding unit/integration coverage.

Changes:

  • Classify partial-write errors based on HTTP status, endpoint path, accept_partial, and JSON shape (data array), rather than string-matching error text.
  • Improve formatting for v3 error responses that include data.error_message / line_number / original_line.
  • Update and expand tests to cover structured vs unstructured partial write behavior and message formatting.
File summaries
File Description
Client/Internal/RestClient.cs Refines partial-write detection and error parsing/formatting for v3 write errors.
Client.Test/Internal/RestClientTest.cs Updates existing assertions and adds parameterized coverage for partial-write classification and formatting.
Client.Test.Integration/WriteTest.cs Adjusts integration expectations around AcceptPartial=false behavior.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 4
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread Client.Test/Internal/RestClientTest.cs Outdated
Comment thread Client/Internal/RestClient.cs Outdated
Comment thread Client.Test.Integration/WriteTest.cs Outdated
Comment thread Client/Internal/RestClient.cs Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The behavioral change is well-covered by expanded tests, and the remaining review notes are minor robustness/cleanliness improvements.

Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 3
  • Review effort level: Lite

Comment thread Client/Internal/RestClient.cs Outdated
Comment thread Client/Internal/RestClient.cs Outdated
Comment thread Client/Internal/RestClient.cs Outdated
@NguyenHoangSon96
NguyenHoangSon96 force-pushed the fix/partial-write-error-handling branch 2 times, most recently from bdc986f to a204845 Compare September 8, 2026 09:46
@NguyenHoangSon96
NguyenHoangSon96 marked this pull request as ready for review September 9, 2026 07:18
@bednar
bednar removed request for alespour and bednar September 9, 2026 09:13

@karel-rehor karel-rehor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comparison with the similar changes in GO is OK. Where in GO I see <Tokens>Error here we have <Tokens>Exception where the preceding tokens reflect the same naming.

I see that Go wraps the logic starting on line 84 of RestClient.cs into a method classifyWriteError(err,options). I might suggest a similar method here

internal Exception ClassifyException(result, queryParams) {
   // logic from current block
}
...
   if(!result.IsSuccessStatusCode)
   {
      throw ClassifyException(result, queryParams)
   }

However since this code is all internal and not public facing, this is just a suggestion.

Still we should probably clean up warnings about possible derefrencing of possible null arguments.

Further review TBC.

Comment thread Client/Internal/RestClient.cs

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread Client.Test/Internal/RestClientTest.cs Outdated
@NguyenHoangSon96

Copy link
Copy Markdown
Contributor Author

Hi @karel-rehor
Changes pushed, please check again.

@karel-rehor karel-rehor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one small quibble about name of a self-descriptive local variable, which required some extra time for understanding the block where it is declared.

Comment thread Client/Internal/RestClient.cs Outdated

@karel-rehor karel-rehor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests pass locally.

Looks good to me. 🚴 🏁

@NguyenHoangSon96
NguyenHoangSon96 force-pushed the fix/partial-write-error-handling branch from ba6d1f9 to 255a338 Compare October 15, 2026 11:31
@NguyenHoangSon96
NguyenHoangSon96 force-pushed the fix/partial-write-error-handling branch from 255a338 to c8cb4e5 Compare October 15, 2026 11:32
@NguyenHoangSon96
NguyenHoangSon96 merged commit d5a53c2 into main Oct 15, 2026
15 checks passed
@NguyenHoangSon96
NguyenHoangSon96 deleted the fix/partial-write-error-handling branch October 15, 2026 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants