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

Add the option to add multiple headers with the same name #48

Merged
merged 1 commit into from
Feb 2, 2025

Conversation

me-no-dev
Copy link
Member

@me-no-dev me-no-dev commented Feb 2, 2025

There are some exceptions that are always allowed only once.

Fixes: #47

Copy link
Member

@mathieucarbou mathieucarbou left a comment

Choose a reason for hiding this comment

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

comments above ;-)

@KGrzeg
Copy link

KGrzeg commented Feb 2, 2025

Hello! With adding support for multiple headers with the same name, it might be handy to have a method that removes a single instance of the header type. Right now, the removeHeader deletes all the headers with matching name and return true if at least one was deleted. It might return a number of deleted items instead of true/false. Would be nice to have 2nd method for deleting a single occurence of the header, or you can add an optional index parameter to this method.
I am not sure how to do this the clean way. Sorry if that's not a right place to dicuss about extending the API, if you just need to fix the bug.

@mathieucarbou
Copy link
Member

mathieucarbou commented Feb 2, 2025

Hello! With adding support for multiple headers with the same name, it might be handy to have a method that removes a single instance of the header type. Right now, the removeHeader deletes all the headers with matching name and return true if at least one was deleted. It might return a number of deleted items instead of true/false. Would be nice to have 2nd method for deleting a single occurence of the header, or you can add an optional index parameter to this method. I am not sure how to do this the clean way. Sorry if that's not a right place to dicuss about extending the API, if you just need to fix the bug.

This would break the API if we modify, so would need to be added as a second method.

I see a valid point in have something like: remove(name, value), in order to remove a specific entry we know about. indeed. This is particularly useful if we allow multiple headers with same name.

Do you know some use cases for that ?

For now, this could be implemented by getting the headers(), iterating and keeping the ones we want, then call remove to remove all, then call add again to re-add only the ones we want to add. This is inefficient, but can be done.

@me-no-dev : maybe we add remove(name, value) in this PR also ? To have a way to remove a specific header ?

@KGrzeg
Copy link

KGrzeg commented Feb 2, 2025

I am not sure if I can find any real life scenario, when that feature is irreplaceable. In most cases it would be enough to just not add the particular header, if you have full control over your code.

The feature of deleting single header occurence might be useful when integrating some 3rd party library into our codebase, which set headers. You might want to cancel some of them later in the code. Or on the other side, if you're the author of a 3rd party library using ESPAsyncWebServer, you might want to cancel user defined headers, like cookies or specific caching settings.

Copy link
Member

@mathieucarbou mathieucarbou left a comment

Choose a reason for hiding this comment

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

added a comment to not forget about adding a Header.ino example

Copy link
Member

@mathieucarbou mathieucarbou left a comment

Choose a reason for hiding this comment

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

Juste a little comment line to change or remove.

I am approving so that we can merge after :-)

There are some exceptions that are always allowed only once
@me-no-dev me-no-dev force-pushed the feature/multiple_headers branch from d03ef70 to e171e8f Compare February 2, 2025 15:18
@mathieucarbou mathieucarbou merged commit 3b3064c into main Feb 2, 2025
22 checks passed
@mathieucarbou mathieucarbou deleted the feature/multiple_headers branch February 2, 2025 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow duplicate headers in response according to RFC
3 participants