[openstreetmap/openstreetmap-website] [WIP] Supporting multiple API versions (#2353)

[Andy Allan]

I'd like to support multiple API versions, so that we can deploy API 0.7 in parallel to API 0.6. It's not something we've ever done before, but I think it's the only reasonable way to handle API version changes these days!

I'm opening this PR just to receive any initial feedback on this proposed approach to the code changes. So far this approach demonstrates the basic concepts, including:

* Supporting different scenarios, e.g. dropping old api calls, adding new api calls, api calls that are the same in each version, api calls that are similar but return different results (e.g. different response codes)
* Optimised for the assumption that almost all api calls are the same between different versions.
* Being able to choose which versions are deployed. This allows the codebase to support multiple API versions during development and when running the tests, and so it avoids long-running branches.
* Only code that is different between api versions is in any way duplicated. Otherwise the same code is used for each api version.
* Establishes a naming convention for controllers that behave differently in different API versions. The general idea is that old code lives in specific controllers, e.g. `API::V06::CapabilitiesController`, and new code lives in the normal place e.g. `API::CapabilitiesController`. This makes future upgrades easier, since the assumption is that new code will be used in version N+1 too.
* It's all designed to allow multiple, non-contiguous versions. For example, if we yank a future version 0.8, it'll cope fine with e.g. deployed_versions = ["0.6", "0.7", "0.9"]

If you are interested in seeing how it works, then the changes to "config/routes.rb" along with the output from "bundle exec rake routes" are the best way to see the overall idea.

The biggest drawbacks so far are around the tests. 

First, I think it makes sense that every API version that the codebase knows about is fully tested, i.e. even if the result is expected to be the same, a given api test should run once for each api version. This leads to the indentation of all the tests changing, since we need to add a "all_api_versions" loop around almost every test. So the diffs and git blame are horrible.

Secondly, the controller methods involve lots of changes like this:

```diff
- get :show
+ get :show, :params => { :api_version => version }
```
It leads to a lot of extra `params => ...` to skim read, which isn't ideal. I've tried working around this but [the workarounds have their own drawbacks](https://github.com/gravitystorm/openstreetmap-website/issues/28#issuecomment-523427402).

The tests don't all pass yet, so this is not yet in shape to be committed, but I hope to get it there soon. In the meantime, and before I make any further changes that you might not be happy with, any feedback is very welcome!
You can view, comment on, or merge this pull request online at:

  https://github.com/openstreetmap/openstreetmap-website/pull/2353

-- Commit Summary --

  * Remove use of Settings.api_version in tests
  * Allow callers to set the api_version for xml document creation
  * Initial sketch for multiple API version support
  * Different capabilities responses for 0.6 and 0.7
  * WIP changesets for all api versions and refactor tests
  * Allow diffReader callers to set the api version of the returned XML

-- File Changes --

    M app/assets/javascripts/osm.js.erb (2)
    M app/controllers/api/capabilities_controller.rb (5)
    M app/controllers/api/changesets_controller.rb (4)
    A app/controllers/api/v06/capabilities_controller.rb (13)
    M app/controllers/api/versions_controller.rb (4)
    M app/controllers/api_controller.rb (5)
    M app/views/api/capabilities/show.builder (3)
    M app/views/api/changesets/changeset.builder (2)
    M app/views/api/changesets/changesets.builder (2)
    M app/views/api/permissions/show.builder (2)
    A app/views/api/v06/capabilities/show.builder (22)
    A config/initializers/api_versions.rb (9)
    M config/initializers/config.rb (2)
    M config/routes.rb (69)
    M config/settings.yml (4)
    M config/settings/test.yml (2)
    M lib/diff_reader.rb (6)
    M lib/osm.rb (7)
    M test/controllers/api/capabilities_controller_test.rb (55)
    M test/controllers/api/changes_controller_test.rb (6)
    M test/controllers/api/changesets_controller_test.rb (3157)
    M test/controllers/api/map_controller_test.rb (8)
    M test/controllers/api/permissions_controller_test.rb (64)
    M test/controllers/api/relations_controller_test.rb (2)
    M test/controllers/api/versions_controller_test.rb (2)
    M test/integration/user_blocks_test.rb (10)
    M test/integration/user_terms_seen_test.rb (4)
    M test/test_helper.rb (8)

-- Patch Links --

https://github.com/openstreetmap/openstreetmap-website/pull/2353.patch
https://github.com/openstreetmap/openstreetmap-website/pull/2353.diff

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/2353
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20190821/44608c39/attachment.html>