Impossible to define route.name along with more than one version

[DiegoZoracKy]

In the documentation, we have this snippet, about versioning:

var PATH = '/hello/:name';
server.get({path: PATH, version: '1.1.3'}, sendV1);
server.get({path: PATH, version: '2.0.0'}, sendV2);

Which works fine. But if we define the name property, like:

var PATH = '/hello/:name';
var NAME = 'routeName';
server.get({path: PATH, name: NAME, version: '1.1.3'}, sendV1);
server.get({path: PATH, name: NAME, version: '2.0.0'}, sendV2);

It won't work as expected. Only the first defined route will be registered. In this case we would get a response of InvalidVersion for Accept-Version:2.0.0.

However, the array style will work:

var PATH = '/hello/:name';
var NAME = 'routeName';
server.get({path: PATH, name: NAME, version: ['1.1.3', '2.0.0']}, sendV1);

I've saw that at /lib/router.js, within the function mount, which adds a route, you have this:

    exists = routes.some(function (r) {
        return (r.name === name);
    });

    if (exists) {
        return (false);
    }

To prevent the creation of a route already registered. But it only checks for the name, which breaks the case in subject. So i believe that it should be checking for the name && version, like:

    exists = routes.some(function (r) {
        return (r.name === name && versions.some(version => r.versions.indexOf(version) >= 0));
    });

I've made this changing on the source code, and it fixed the problem of no registering all the routes with different versions for a same name. I thought about send a pull request, but i don't know yet if this change will affect other aspects of the system.

[micahr]

I think the break down here, is that naming routes means the names have to be globally unique, not just by version. If you appended the version to the name, you would be okay.

[DiegoZoracKy]

It would work with the version (or anything else that would make it a non unique name) appended, but IMHO it wouldn't be semantically correct. I believe that we can have multiple versions for one same scope, behaviour of a API.

I will give you an example about why i think about it in that way:

Let's suppose that you have some middlewares, and events handlers to work on some basic stuff of a specific api route. login for example. On each one of these blocks you are looking for req.route.name == {routeName}. This handles well the case where you decided to change your route path, for example, you wouldn't need to change everywhere as if you would by using req.route.path instead of req.route.name. I believe that this same case applies to versioning. If your project gets a new version for login route, but it doesn't implies on any changes on any of the already created handlers (errors, middlewares, etc.) for the behaviour of login. It would be correct to have the need of updating the route reference (req.route.name) everywhere, and always when newer versions come?

[DiegoZoracKy]

So, is it a Bug or an intended behaviour?

[vellotis]

In my mind it is a intended behavior which should be changed. I have ran into this behavior many times while trying to have versioned route.

[vellotis]

https://github.com/restify/node-restify/blob/4.x/lib/server.js#L481-L495
https://github.com/restify/node-restify/blob/5.x/lib/server.js#L495-L507
Hasn't changed much from version 4.

[DiegoZoracKy]

@yunong, @micahr won't you think this would be a good to have this behaviour on v5? Are you open to get a PR, to handle this case?

[yunong]

@DiegoZoracKy Yes, if you're willing to send a PR, that would be appreciated.

[DiegoZoracKy]

@yunong, i'll try to find some time to tackle this.

[yunong]

Hooray!

…

On 12 January 2017 at 15:44, Diego ZoracKy ***@***.***> wrote: @yunong <https://github.com/yunong>, i'll try to find some time to tackle this. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1134 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABK2EO23f-NrCJrvEHFggdccaivwsSKbks5rRrrSgaJpZM4IyY3S> .

[DiegoZoracKy]

@yunong it seems that this issue needs to go a bit further. The "name only" is used as a key in some places like this.mounts[route.name] and by some methods like Router.unmount and Router.render. Also, server.routes is an Object that uses route.name (when defined) as a key.

To move forward on this path to have more than one handler, one for each version for a route with a same name, i can think about two options:

1 - Everywhere which is an Object of routes today would be turned into an Array and instead of lookup as this.mounts[route.name], Array methods would then be used like: this.mounts.find, this.mounts.filter.

2 - Keep the current structure as an Object but for the key, always concatenate name and version, with some kind of special string between them (e.g name-----version). So the current existent methods that expected just the routeName as a parameter, would stay with the same signature, an internally the lookup would discard the special string. Although it is an option, i don't like much this sort of hack way to handle it.

Note: I haven't looked much of the restify engine yet to estimate the impact of these ideas. I just read lib/router.js and lib/server.js.

[yunong]

Why don't we have an object for a route name with keys as the versions.

e.g. If I had a GET route foo with versions 1, 2, 3
My routes object would look like would look like

"foo": {
    "1": ...,
    "2": ...,
    "3": ...
}

If I need to look up a route by version, I simply do foo[version] to find the version.

[DiegoZoracKy]

And then for the case where we haven't defined any version for the route we could have:

"foo": {
    "default": fn
}

or

"foo":  fn

[yunong]

@DiegoZoracKy I like the first option. We can then do

if (!route[version] || !route[debug]) {
// return 404
}