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

{path*} segments disappear when empty #24

Closed
kanongil opened this issue Oct 9, 2016 · 11 comments
Assignees
Milestone

Comments

@kanongil
Copy link
Member

@kanongil kanongil commented Oct 9, 2016

The Inert directory handler behaves incorrectly when a route contains dynamic segments and the incoming {path*} part is empty.

Eg. the route /{domain}/{path*} matches /main/, but only includes the domain part in the output with params: { domain: 'main' }, paramsArray: [ 'main' ].

I would expect params: { domain: 'main', path: '' }, paramsArray: [ 'main', '' ].

Note that Inert depends on the length of the paramsArray to determine the last path segment for the directory handler. See hapijs/inert#74 for some background.

@kanongil

This comment has been minimized.

Copy link
Member Author

@kanongil kanongil commented Oct 9, 2016

Note that this issue also effects optional params, which are omitted from the output when empty, contrary to what the docs say.

@kanongil

This comment has been minimized.

Copy link
Member Author

@kanongil kanongil commented Oct 9, 2016

It seems this used to work but got broken with fc7978b.

The internal lookup still returns the correct values but unfortunately it no longer includes empty strings in the final output, due to the check here:

if (value) {

@kanongil

This comment has been minimized.

Copy link
Member Author

@kanongil kanongil commented Oct 19, 2016

So this issue has been there since hapijs/hapi#1644, which initially broke the directory handler for paths containing dynamic segments.

At the time you seem to think that you fixed a bug, though it is not apparent which one. What is apparent is that the documentation still reflects the old behaviour, and that it broke some uses of the directory handler, where the implementation expects the length of the paramsArray to equal the number of segments in the route in order to extract the value in the final segment.

For reference, this is the line in the docs that specifies the old behaviour:

For example, the route '/book/{id?}' matches '/book/' with the value of request.params.id set to an empty string ''.

While I wouldn't mind fixing this in inert, it is not possible to do this without changing the api, and no longer use paramsArray. As such, I would prefer if this is fixed in hapi/call, by always including every segment in the matched params. I have made an attempt at this here https://github.com/kanongil/call/tree/empty-segment-fix, which can be installed into hapi, passing all existing tests.

@kanongil

This comment has been minimized.

Copy link
Member Author

@kanongil kanongil commented Nov 14, 2016

@hueniverse ping?

@hueniverse hueniverse added the bug label Nov 29, 2016
@hueniverse hueniverse self-assigned this Nov 29, 2016
@hueniverse hueniverse added this to the 3.0.5 milestone Nov 29, 2016
@kanongil

This comment has been minimized.

Copy link
Member Author

@kanongil kanongil commented Nov 30, 2016

While your patch helps, I suspect your fix will still cause issues with Inert, without the undefined entries from my fix. Let me get back to you on this.

@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented Nov 30, 2016

Returning an object with keys explicitly set to undefined is a bad API. You can get the number of total params from the route info.

@kanongil

This comment has been minimized.

Copy link
Member Author

@kanongil kanongil commented Dec 14, 2016

@hueniverse So this is definitely still an issue for inert when the slash is missing.

I wouldn't mind retrieving the total count from the route, however this is not exposed in the request.route object. I can only retrieve the configured path, or the fingerprint. While I could parse this, it seems prone to errors and wasteful to do on each request.

Btw. I don't care about undefined in the params object. Only in the paramsArray.

@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented Dec 17, 2016

So either you get the number of path parameters in the config or null in the array?

@kanongil

This comment has been minimized.

Copy link
Member Author

@kanongil kanongil commented Dec 17, 2016

Yes, I will need one of those. If it's null / undefined, the old expected behavior is restored. That solution has the bonus ability to also signal missing elements elsewhere in the path, though I won't need that (is it even possible?).

@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented Dec 17, 2016

How do you handle a path like /{p1}/a{p2?}b/{p3?}? Even when p2 is null, you still have 3 segments.

@kanongil

This comment has been minimized.

Copy link
Member Author

@kanongil kanongil commented Dec 18, 2016

Hmm, I think relying on paramsArray is not a good idea. I found another way to do this in hapijs/inert#79, which fixes my issue for any version of hapi.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.