allow multiple versioning strategies simultaneously #26

Closed
wants to merge 4 commits into from

3 participants

@twinge

I'm working on an application where one of the API consumers uses JSONP, which isn't able to pass additional headers.

The "preferred" way of accessing the API is to pass the version number as a header, but I also need to support either path or parameter methods for the JSONP to work. The original code made the various methods mutually exclusive. I made some very minor changes to allow multiple strategies.

All the specs still pass, as well as my own production specs. Do you feel strongly about not allowing multiple options, or is this a change you might consider?

Thanks!

@bploetz
Owner

Kick ass! I'd love to see this feature.

We need explicit tests though to verify that an API version configured with N different strategies actually works correctly for all of the configured strategies simultaneously. Can you add a test for that? Should be as simple as adding a new multi strategy context here:

https://github.com/bploetz/versionist/blob/master/spec/api_routing_spec.rb#L72

Thanks.

@bploetz
Owner

Oh, and an example of this feature would be helpful in the README too.

@twinge
@bploetz
Owner

Actually, this is going to be problematic if you only support a single :value key in the config hash, and you want your Header value to be different than your Request Parameter value. For example, you want to support both of these at the same time:

Accept: application/vnd.mycompany.com; version=1
GET /foos.json
GET /foos.json?version=1

Since the Header and Request Parameter strategies both take a :value config key, you won't be able to specify both at the same time.

  # header on it's own
  api_version(:module => "V1", :header => "Accept", :value => "application/vnd.mycompany.com; version=1") do
    match '/foos.(:format)' => 'foos#index', :via => :get
  end

  # request parameter on it's own
  api_version(:module => "V1", :parameter => "version", :value => "1") do
    match '/foos.(:format)' => 'foos#index', :via => :get
  end

  # both header and request parameter
  api_version(:module => "V1", :parameter => "version", :header => "Accept", :value => "?????") do
    match '/foos.(:format)' => 'foos#index', :via => :get
  end
@twinge

I thought of this constraint, and for now I see it as a trade-off that a developer can chose. The Hypermedia purists probably won't chose it, but you can say that if you want to have multiple simultaneous strategies, then you'll probably want to use a custom header like your 'API-VERSION' suggestion that has a value of 'v1'.

At a minimum it's still fully backwards compatible, and gives people more choice, not less. Something more flexible (and possibly less backwards compatible) could be worked out when you're ready to do a major version bump?

  # both header and request parameter
  api_version(:module => "V1", :parameter => {:name => "version", :value => 'v1':}, :header => {:name => "Accept", :value => "application/vnd.mycompany.com; version=1"}) do
    match '/foos.(:format)' => 'foos#index', :via => :get
  end
@bploetz
Owner

Yeah, we should definitely fix the config ambiguity in a major version release, and I like the :strategy => {} approach to the config. Will get that on the road map.....

@twinge

I added the specs and example. For the specs, i just put one positive test of each strategy. Putting more than that seemed to me like it would just be duplication of existing coverage, meaning slower specs without actually improving coverage.

@bploetz
Owner

This fails when you add :default => true to the config hash passed to api_version(), as it tries to set each strategy object created as the default, and you end up getting:

[VERSIONIST] attempt to set more than one default api version

Rather than shoe-horning this into the existing single config hash version of api_verison(), which clearly isn't set up to support this feature, I think I'd like to do this the right way by introducing the config hash per versioning strategy that you describe above, and doing this in a major version bump.

If you want to take a crack at that let me know, otherwise, I'll try to get going on that as soon as I can.

@twinge

I completely understand your hesitation to "hacking in" something that was never intended to work instead of refactoring properly.

That said, I went ahead and added that case to the spec and made the one line change necessary for it to work. The shoe-horned method isn't really creating any code mess. I've only changed 5 lines total, and the changes were extremely minor.

I guess i'll just continue using my branch until one of us finds time for the refactoring :)

@twinge twinge Merge remote branch 'bploetz/master' into multiple_versioning_strategies
Conflicts:
	lib/versionist/versioning_strategy/base.rb
1c546e7
@adamburmister

Has this made any advances? I'd love to see this functionality in the master

@bploetz
Owner

I have not forgotten about this. My wife and I welcomed a little girl into the world recently, so free time (and sleep) has been at a premium. I actually have the core refactoring done, I just need to finish updating the generators and it will be complete. I hope to push this up on a branch for you guys to play within a week or so.

Stay tuned....

@adamburmister

I can't even begin to imagine, Brian! Congrats and good luck :)

@bploetz bploetz pushed a commit that referenced this pull request Jan 7, 2013
Brian Ploetz Issue #26: allow multiple versioning strategies simultaneously 7d20b33
@bploetz
Owner

This feature has been added in the release-1.0 branch. Before merging this to master and releasing version 1.0 with this change, I'd appreciate it if the folks interested in this feature could test this out with your API and let me know if you run into any issues. Change the dependency on versionist in your Gemfile to the following....

gem 'versionist', :git => 'git://github.com/bploetz/versionist.git', :branch => 'release-1.0'

...and run bundle update versionist.

Please read the "Uprgrading From Versionist 0.x to 1.x" section in the README for details:

https://github.com/bploetz/versionist/tree/release-1.0#upgrading-from-versionist-0x-to-1x

@twinge

I tested with this:

api_version(module: 'V1', header: {name: 'API-VERSION', value: 'v1'}, parameter: {name: "version", value: 'v1'}, path: {value: 'v1'})

and it worked fine for me for all 3 methods.

Thanks, and congrats on the new child :)

@twinge twinge closed this Jan 7, 2013
@bploetz bploetz pushed a commit that referenced this pull request Jan 21, 2013
Brian Ploetz Issue #26: allow multiple versioning strategies simultaneously dd232ab
@bploetz
Owner

Versionist 1.0.0 has been released with this change. Let me know if you run into any issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment