Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add headers to http error responses #1019

Merged
merged 6 commits into from
Jul 25, 2024

Conversation

twosdai
Copy link
Contributor

@twosdai twosdai commented Jul 24, 2024

Fixes #1018

Proposed Changes

Adds headers to the HTTP error response.

Checklist

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

feat: add headers to http error responses
@twosdai
Copy link
Contributor Author

twosdai commented Jul 24, 2024

I am not super familiar with this repo, and this is my first contribution here. Please take a bit more time and review the code. Happy to make any changes you callout.

The basic idea of this change is to include a headers object in the HttpError thrown by influx. There are multiple different "types" of header formats, I chose to boil them down to the internal format which is basically just a simple Record<string, string> This means that for the browser client we need to go over each header value and map it into a object and then pass it in. Since the default Headers from the response object will cause typescript to error out at compile time.

@codecov-commenter
Copy link

codecov-commenter commented Jul 24, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (e65b8fb) to head (70a8805).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #1019   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           35        35           
  Lines         1448      1449    +1     
  Branches       344       344           
=========================================
+ Hits          1448      1449    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

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

Thank you for your PR! 👍 Before we can proceed with accepting it, there are a few requirements that need to be met:

twosdai added 2 commits July 25, 2024 09:32
… and changed typed to remove undefined check
feat: add headers to http error responses

feat: updates per code review to use internal headers getter function and changed typed to remove undefined check
@twosdai
Copy link
Contributor Author

twosdai commented Jul 25, 2024

@bednar Thanks for the comments, I've implemented the change here: c3fb1fa

Let me know if you need / want anything else.

Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

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

@twosdai, thank you again for your fantastic PR! 👍

Could you please update the CHANGELOG.md with the following entry:

### Features

1. [1019](https://github.com/influxdata/influxdb-client-js/pull/1019/): Propagates headers from HTTP response when an error is returned from the server.

Once that's done, we'll be ready to merge this PR.

@twosdai
Copy link
Contributor Author

twosdai commented Jul 25, 2024

@bednar See the commit here for the changelog update: 643e6f4

Let me know if you need anything else! Thanks for the quick review :)

Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@bednar bednar merged commit 8cfa06c into influxdata:master Jul 25, 2024
5 checks passed
@bednar bednar added this to the 1.34.0 milestone Jul 25, 2024
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.

HTTP headers should be included when an HttpError is thrown.
3 participants