search

p-meier / hapi-api-version

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

Added support for a dynamic, parameterized basePath #11

Closed wk-plumbline closed 7 years ago

wk-plumbline commented 8 years ago

Hi. I have created a pull request for a feature that I need. You are free to review the code, make changes or suggestions, or to drop the feature completely if you think it's a contribution that people will not generally find useful. I thought I'd just give you the option, in case you wanted to include it. I am happy to keep my changes in a separate fork, but obviously it would be first prize to have the feature supported officially.

The context for the feature is that I want to build a tenant aware API, and so have configured my Hapi URL's to look like /{tenant}/v1/some_api_call, which can be called, from a client, by using /tenant_abc/some_api_call or /tenant_xyz/some_api_call. This is obviously slightly different from having a fixed basePath, since the tenant ID I get from the call will vary from tenant to tenant.

When reviewing the code, bear in mind that I am new to both Javascript and Hapi JS. Coming from a Java background, this is my first real project in JS, so the code might not be optimal, and slightly more verbose than necessary. I tried to add the feature in such a way that the existing code path has the most minimal amount of change, and the added code will only be invoked if a dynamic basePath is detected.

Either way, thanks. Appreciate your work.

p-meier commented 8 years ago

Hi. Thanks for your PR. I just took a short look on it because I'm a little busy right now. Maybe I have some more time in a few days but I can't promise.

Am I understanding it right, that you would like to know which tenant made the request to your API and therefore you add it to the path?

I just want to clearly understand the purpose before deciding if it should go into the official plugin version.

wk-plumbline commented 8 years ago

Hi, no problem taking your time, my stuff currently works, so no pressure from this side. Yes, you understand the problem right. In Hapi I have configured a route, for example, like such as /{tenant}/users/{user}, so that when they make a call to the API, the URL would look like /tenant_abc/users/user_xyz

The values for tenant (tenant_abc) and user (user_xyz) is what I retrieve from the request.params. If I want to introduce versioning, I can either take the approach /v1/{tenant}/users and no changes would be required. Of course, I'd prefer to rather go with /{tenant}/v1/users, which means I need to treat the {tenant} as a basePath in hapi-api-version, except that I can receive any value on that path, so need to put that value back in the final URL.

Hope that makes sense.

I understand that it's a pretty unique feature, and once it is in the code it has to be supported, so really, only include it if you feel it is of benefit to the project. The check that I use to determine whether it is a dynamic basePath value is a very rudimentary one at this point, and if the check is false, then everything else is pretty much unchanged.

p-meier commented 8 years ago

Sry for my late answer. I think this is a pretty special feature and if you are happy working on your own fork this would be the best solution.

But one thing that comes to my mind when reading your requirement is why does the tenant have to be a path param? If you use some form of authentication you should know who the client/tenant is.

But thanks a lot for your time and effort in contributing. I really appreciate it.

wk-plumbline commented 8 years ago

Hey, no problem. To answer the question, the tenant as a path parameter is an optional requirement for the initial authentication, since we don't know at that point which tenant is making the request. The other option they have, and the preferred choice, is to pass the tenant id via a header. Either way, once the tenant has been authenticated, no further passing of tenant info is required.