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] added ability to opt-out from mime formatted header names #145

Merged
merged 12 commits into from
May 6, 2021

Conversation

aricart
Copy link
Member

@aricart aricart commented Apr 15, 2021

added an optional argument that allows an application's use of headers to opt-out of mime header formatting. To retrieve values, they must also opt-out

@aricart aricart requested a review from wallyqs April 15, 2021 21:06
…serialized in a canonical form.

[change] `Status` and `Description` header entries are removed. These values are accessible from `status`, `code`, and `description`
[change] accessing values, will return a list of all the values stored under all headers that have the same case-insensitive name.
[change] to access specific keys in a case-sensitive way, iterate over the header values.
@aricart
Copy link
Member Author

aricart commented Apr 16, 2021

Implemented as specified here: #147

@aricart
Copy link
Member Author

aricart commented Apr 16, 2021

nats.deno is going to implement it as described in #147.

  • set() will write header as per the user
  • get() will return the first value in the first key that matches in a case insensitive way
  • values() return the union of all keys that match case-insensitively.

No key normalization, headers are insensitive, if the user wants to store a canonical header, they can use the provided function to transform the key.

@aricart aricart requested a review from wallyqs April 16, 2021 22:32
Copy link
Member

@wallyqs wallyqs left a comment

Choose a reason for hiding this comment

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

LGTM

@aricart aricart requested a review from wallyqs April 20, 2021 21:58
…`delete`. If true, case sensitive operations are performed.
@aricart
Copy link
Member Author

aricart commented May 3, 2021

@wallyqs @ColinSullivan1

All options with the exception of append, have an exact option which will constrain any operation to be case-sensitive
only. This optional option (false by default) will:

  • set(), only replaces an exact match header
  • get(), only returns an exact match header
  • values() only returns an exact match header

append doesn't provide this setting, as any append is case-preserving, so only adds to an exact match.

This should honor what the user wants while providing a mechanism to further clamp for more constrained requirements.

nats-base-client/headers.ts Outdated Show resolved Hide resolved
nats-base-client/headers.ts Outdated Show resolved Hide resolved
nats-base-client/headers.ts Show resolved Hide resolved
@aricart aricart requested a review from kozlovic May 5, 2021 15:29
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

LGTM, minor comment about performing canonicalMIMEHeaderKey() twice on a set?

nats-base-client/headers.ts Outdated Show resolved Hide resolved
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

LGTM

@aricart
Copy link
Member Author

aricart commented May 5, 2021

@kozlovic raised the point that append what preserving the input's case, but options provided could be in disagreement.

Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

LGTM

@aricart aricart merged commit e0804b8 into main May 6, 2021
@aricart aricart deleted the user-headers branch November 2, 2021 20:32
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.

3 participants