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

Assert on unsupported route method #4494

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kanongil
Copy link
Contributor

Use the node.js built-in parseable http method list to reject routes that can never be reached.

Given that this will make hapi fail to start on existing code that inadvertently uses an unsupported method (maybe a typo), this might be more suited for including in a breaking release.

@kanongil kanongil added the feature New functionality or improvement label Mar 21, 2024
@Marsup
Copy link
Contributor

Marsup commented Apr 3, 2024

So this would only fail for people using verbs that are outside of any http spec? That doesn't look like a breaking change in my book. Could that be a problem for people using unix sockets maybe?

@kanongil
Copy link
Contributor Author

kanongil commented Apr 4, 2024

So this would only fail for people using verbs that are outside of any http spec? That doesn't look like a breaking change in my book. Could that be a problem for people using unix sockets maybe?

Not exactly. The HTTP spec allows any combination of letters with case sensitivity. However, the node http parser only supports a limited subset (as listed in Http.METHODS), and any outside this will error. As such, any routes with these methods will be unreachable.

@kanongil
Copy link
Contributor Author

kanongil commented Apr 4, 2024

Whether unix sockets are used is irrelevant, as this relates to the content of the stream, not how it is made.

Copy link
Member

@Nargonath Nargonath left a comment

Choose a reason for hiding this comment

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

I understand that it could prevent a server from starting with this new release hence the breaking change. It targets code that would never be reachable anyway so I'd be tempted to stamp it as bugfix. However if we take the strict definition of breaking change which is, IMO, a release that breaks users flow without any change on their side it should be flagged as BC. It's tricky though since most people actually have proper HTTP verbs if they want their code to work so I'd expect the impact to be low.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants