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

Listener does not correctly handle optional & wildcard routes. #336

Open
SippieCup opened this issue Nov 2, 2024 · 2 comments
Open

Listener does not correctly handle optional & wildcard routes. #336

SippieCup opened this issue Nov 2, 2024 · 2 comments
Labels
bug Bug or defect

Comments

@SippieCup
Copy link

SippieCup commented Nov 2, 2024

Runtime

node.js

Runtime version

20.14.0

Module version

14.0.0

Last module version without issue

No response

Used with

Hapi

Any other relevant information

when adding subscriptions to the listener, wildcard paths are not handled correctly in internals.Subscribers.prototype.add . This leads to internals.Listener.prototype._publish incorrectly determining which subscribers to send the message to, due to only checking for length, and not number of parameters as wildcard parameters appear in the paramsArray as a single string and must be manually split.

This means that you can't have a greedy listener, or optional params as they won't match when going backwards, since they originally have 0 parameters provided or has multiple parameters merged within the last value of match.paramArray

potential solution:

On seeing a wildcard route, the last value of the paramsArray should be split on / and flattened into the paramsArray.

What are you trying to achieve or the steps to reproduce?

I am trying to use a wildcard parameter to publish changes in real time from a broad view to then be filtered to more specific ones. similar to the REST spec.

An example:

model of products. there are two parameters:

    actions?: 'insert' | 'delete' | 'update'
    id?: number
}

the route would be /products/changes/{params*2} or just /products/changes/{params*}

this route should result in this table:

users absolute path desired result
1 /products/changes/ should get every message send back to the route
2 /product/changes/insert should get every insert message
3 /products/changes/update should get every update message
4 products/changes/update/5 should only get update messages for product id 5

if you attempt to use a wildcard for multiple parameters, or an optional parameter, on a route, it still needs to be published as the full path to match the more specific filters.

What was the result you got?

Only the fully defined path /products/changes/updates/5 gets the message, rather than every listener leading up to that point.

What result did you expect?

the lookup going backwards fails because it must be published to /products/changes/update/5 to support the other conditions, and thus, only user 4 gets the message. instead of user 1, 3 and 4 even though they all matched the route.

@SippieCup SippieCup added the bug Bug or defect label Nov 2, 2024
@SippieCup
Copy link
Author

SippieCup commented Nov 12, 2024

When Itemizing, there is an absolute path check.

item.paths.indexOf(path) !== -1)) {

If the absolute path does not match, which is not garanteed on wildcard or optional params, it never calls the each function, which has the inclusion of checking for if the params match & looks to be where it is supposed to filter it out using the route information not earlier in the flow when itemizing for forEachSubscriber

return route.subscribers._forEachSubscriber(match.paramsArray.length ? path : null, options, each);

I'm not sure on how to move forward in fixing this without breaking other workflows. Please advise.

@damusix
Copy link
Contributor

damusix commented Nov 18, 2024

@SippieCup Any chance you can get us an example of the problem to replicate? It'll help us debug a bit easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug or defect
Projects
None yet
Development

No branches or pull requests

2 participants