p-meier / hapi-api-version

An API versioning plugin for hapi.
Apache License 2.0
74 stars 26 forks source link

Support path parameters #3

Closed alexZielonko closed 8 years ago

alexZielonko commented 8 years ago

5fa65c3 fails to account for path parameters and thus introduces a breaking change:

ex) route.path: /v2/user/{id} versionedPath: /v2/user/123

if (route && route.path === versionedPath) {
    request.setUrl('/v' + requestedVersion + request.url.path); //required to preserve query parameters
}

As '/v2/user/{id}' is obviously not equal to '/v2/user/123', the expected versioned url will never be set. This is unfortunate, path parameters are great! This fix checks the route.fingerprint against the versionedPath to add support for path parameters while maintaining support for unversioned, versioned, and wildcard paths.

p-meier commented 8 years ago

Thanks a lot for your PR. I thought a little bit about the problem and your solution. There are some cases left that are not covered - but this has nothing todo with your specific solution. They did not work before.


First thing is that we need to use route.fingerprint instead of route.path to support routes like this (even if this means the plugin is not compatible with hapi v10 anymore):

    server.route({
        method: 'GET',
        path: '/v2/test/{hello*2}/withPathParams',
        handler: function (request, reply) {

            // Return the api-version which was requested
            return reply(request.params.hello);
        }
    });

Besides where did you find out about the fingerprint property of route? Could not find anything in the docs - only in the hapi source code. Was this introduced in hapi v11?


Second thing is a route like this:

  server.route({
        method: 'GET',
        path: '/v2/test2/{hello*}',
        handler: function (request, reply) {

            // Return the api-version which was requested
            return reply(request.params.hello);
        }
    });

Here the fingerprint of the route for a request to /v2/test2/1/2/3 is /v2/test2/#. So only comparing the length of the parts is not enough to cover this case. One point to start with is that there can only be one such open ended multi-segment parameter per path and it has to be the last one.


Third thing is a route with optional params:

  server.route({
        method: 'GET',
        path: '/v2/test3/{hello?}',
        handler: function (request, reply) {

            // Return the api-version which was requested
            return reply(request.params.hello ? request.params.hello : 'not provided param');
        }
    });

Here the fingerprint is always /v2/test3/?. No matter if the request is /v2/test3 or /v2/test3/1. Again as far as I can tell there can only be one optional parameter per path and again it has to be the last one.

alexZielonko commented 8 years ago

I failed to consider other path parameter options as they're unused in my current project. This seems like a good time to provide support for those path parameter options. So, thank you for digging deeper into my PR.

I did not see any documentation for the fingerprint property either. I discovered it when reviewing the Hapi source and thought it seemed fitting for this use case.


While looking into providing complete path parameter support with the plugin, I believe I found a simpler solution.

const route = server.match(request.method, versionedPath);

let versionedRoute = false;

//Check if returned route's path contains version
if (!!route && route.hasOwnProperty('path')) {
    versionedRoute = route.path.indexOf('/v' + requestedVersion + '/') === 0;
}

if (versionedRoute) {
    request.setUrl('/v' + requestedVersion + request.url.path); //required to preserve query parameters
}

It seems as if we can safely assume server.match will resolve the best potential route public interface. If this is a safe assumption, then the plugin doesn't need to compare the versionedPath value against the fingerprint. It only needs to determine if the route's path contains a version, such as '/v2/. If the returned route's path contains a version, request.setUrl should be invoked.

I added tests for versioned and unversioned 'catch all', optional, and multi segment path parameters. This approach appears to handle those cases.

p-meier commented 8 years ago

This seems to be the solution. Thanks for your help.

I may have to refactor the tests a little bit as they have become hard to read (not your fault) - maybe I need to give them more structure.