p-meier / hapi-api-version

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

Adding ability to parse Accept headers of different formats #5

Closed kdstew closed 8 years ago

kdstew commented 8 years ago

Adds ability to override the accept header subtype validity check by passing a regex in registration options. Adds ability to override the functionality for version extraction from the header.

Motivation for the addition of these overrides:

We have multiple apps running on different frameworks and we are wanting to get the version string in the Accept header to be consistent across the apps.

For instance we have a hapi app that looks for an Accept header like the following: application/vnd.myapi.v2+json

And we have a rails app that is looking for an Accept header like the following: application/vnd.myapi+json; version=2

Example of options added that would allow an Accept head like the following

application/vnd.myapi+json; version=2

{
  acceptHeader: {
    subtypeTest: /^vnd.[a-zA-Z0-9]+$/,
    extractVersion: function (media) {
      return media.parameters.version;
    }
  }
}
p-meier commented 8 years ago

Thanks for your PR. I will look into it in more detail on this weekend...

p-meier commented 8 years ago

I reverted the merge of your PR because I recognized it does not take the vendorName option into account. It actually becomes obsolete by your PR and the plugin would not be backwards compatible anymore.

kdstew commented 8 years ago

I'm not following how the plugin is not backwards compatible after the changes. Could you elaborate?

p-meier commented 8 years ago

Your changes ignore the vendorName option. Also this option is a required one and has to be specified. If someone relies on this option it is just not taken into account - so not backwards compatible.

kdstew commented 8 years ago

The vendorName is still being checked as it is in the current version https://github.com/p-meier/hapi-api-version/pull/5/files#diff-168726dbe96b3ce427e7fedce31bb0bcR49. Are you saying that the vendorName option would logically need to be grouped with subtypeTest and extractVersion under acceptHeader in the options and that would be a breaking change?

Do you have suggestions on how this functionality could be added in a backwards compatible way?

p-meier commented 8 years ago

Sorry. You are right - I just looked at the changes not thorough enough. So this is not a breaking change.

But it still does not feel right. First I think as you already suggested that vendorName should be under acceptHeader. Second I don't like to specify both (vendorName and subtypeTest) when overriding the defaults - specifying subtypeTest should be enough in this case. Here an example of what I have in mind...

Options - using defaults

Specifying the vendorName should be enough.

{  
   acceptHeader: {  
      vendorName: 'mysuperapi'
   }
}

Options - override

The vendorName is not needed as it should be part of the subtypeTest (something like /^vnd\.mysuperapi\.v[0-9]+$/).

{  
   acceptHeader:{  
      subtypeTest: /^vnd.[a-zA-Z0-9]+$/,
      extractVersion:function (media)      {  
         return media.parameters.version;
      }
   }
}

What do you think?

Also the documentation, example and tests need to be updated.