Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Supporting multiple API versions #2353

Open
wants to merge 11 commits into
base: master
from

Conversation

@gravitystorm
Copy link
Collaborator

commented Aug 21, 2019

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:

- 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.

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!

@mmd-osm

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

I haven't looked through all the details yet, so please bear with me. I was wondering a bit how you would handle evolutionary or even revolutionary changes to the data model. Typically this is one of the most troubling and complicated aspects of new API versions, and I think it would be good to have some basic concept in place as well here.

@gravitystorm

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 22, 2019

I was wondering a bit how you would handle evolutionary or even revolutionary changes to the data model.

I can sum this up with the phrase "it depends"!

  • Different versions of the API can use different views, so if we added something to a future api (for example "node coordinate precision" or somesuch) it can be shown in the latest version and ignored in previous versions, or shown in a different way (e.g. as tags).
  • Different api versions can use different controllers too, as already demoed in this PR for the capabilities controller. This allows more subtantial changes in behaviour between api versions, and changes like fixing response codes.
  • We could use different models too. I'm not sure why we'd want to, but e.g. api/0.7/node/1 could call a node_controller that fetches data from the ShinyNewNode model.

But it all really depends. I think the underlying question might be "how do we handle drastically non-backwards-compatible changes, like converting all closed ways into an area type (or if we introduce an area type into API N+1, how does that work with API N clients); or how would a change like removing segments work with two api versions running in parallel. I don't have an answer for these major structural changes, except to say that if it's logically possible at all to access the data between versions, the code approach in this PR will be able to handle it. And perhaps there'll be some change to the datastructure that prevents parallel API versions and it'll trigger a hard cutover between versions.

But lets not get stuck on the biggest problem, and one that's not yet in hand. In the meantime, there's a ton of backwards-compatible but needs-API-bump changes that have been stuck for years, so we can at least sort out those ones 😄

@gravitystorm

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 22, 2019

I should have said, if there's specific API changes that you're thinking about, even if they are just to illustrate a point, let me know and I'll see how they fit.

@simonpoole

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

But lets not get stuck on the biggest problem, and one that's not yet in hand. In the meantime, there's a ton of backwards-compatible but needs-API-bump changes that have been stuck for years, so we can at least sort out those ones 😄

Likely making myself very unpopular:

  • where are all these "backward-compatible but needs-API-bump changes that have been stuck for years" changes documented?
  • in general I oppose all of this on the grounds that the GDPR related changes need to be implemented first and I'm actually surprised that "nice to have" API changes are even being given a seconds thought in the current situation.
@gravitystorm

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 22, 2019

where are all these "backward-compatible but needs-API-bump changes that have been stuck for years changes" documented?

Find your favourite API-0.7 wishlist, ignore the stuff about areas, and there's your list. 😄

Different people have created different lists over the years. I'm not intending to implement many changes, just the ones that I've been personally complaining about since API 0.6 was released over a decade ago (like incorrect http status codes, and plain-text responses, and stuff like that).

in general I oppose all of this on the grounds that the GDPR related changes need to be implemented first

I know that you want them first, but that doesn't mean that they need to be done first.

In particular, if the GDPR-related changes can be made without changing the API version then they can be implemented in parallel to this work, and so they are not interdependent. If they need to break API compatibility, then this work will need to come first anyway. So either way, they don't need to come first.

@bhousel

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

I should have said, if there's specific API changes that you're thinking about, even if they are just to illustrate a point, let me know and I'll see how they fit.

Just off the top of my head, we'd see some immediate performance improvement on the iD side from:

  • JSON API (#2221)
  • Multi-fetch GET with /full (#2348)

(these require some coordination from the CGImap side too, but I don't think that's a blocker)

@simonpoole

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

Except, naturally, that neither of #2221 and #2348 depend on this PR.

Without it (this PR) we would have simply added them as a 0.6 feature, as we have done so many times before and not doing that raises the whole question of versioning of the OSM API in general which is a rabbit hole of its own.

Add a latest_api_version and use it on the browse pages
This ensures that raw XML links point to the latest available version
Hardcode some links to the 0.6 api, for now
This could be reworked with some meta programming to get the latest API version in future.
Rework some relations tests that rely on creating changesets
This is because the changesets api is now multi-version

@gravitystorm gravitystorm changed the title [WIP] Supporting multiple API versions Supporting multiple API versions Aug 28, 2019

@gravitystorm

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 28, 2019

I've updated this PR so that the test suite passes, and it's now ready for review.

Currently only a few routes have been adapted for multiple API version support, namely:

  • capabilities
  • permissions
  • changesets

I intend to work on the rest of the routes in subsequent PRs. The settings in this PR ensure that only 0.6 is deployed by default, so it can be merged without any side effects.

@simonpoole

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

Could we get it on the sandbox first?

@gravitystorm

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 28, 2019

Could we get it on the sandbox first?

First? Do you mean before code review?

Or if you want it before merging, to what end? So far this PR is just internal code refactoring, there's no changes to the API (other than dropping one line from the api/0.7/capabilities response) and even if this is merged there will still be no changes since 0.7 is disabled. So we can ask Tom to set up a sandbox "first", but I'm not sure what you would want to do with it?

Of course, it'll be worth having a sandbox available later on, but I don't think it's worthwhile effort at this stage.

If you still have concerns, let me know what I can do to help.

@simonpoole

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

Could we get it on the sandbox first?

First? Do you mean before code review?

Before deployment, which in our case implies before merger.

Or if you want it before merging, to what end? So far this PR is just internal code refactoring, there's no changes to the API (other than dropping one line from the api/0.7/capabilities response) and even if this is merged there will still be no changes since 0.7 is disabled.

Famous last words. In reality there are always things that might break, for example as when the authorisation refactoring was deployed.

Being able to test against a deployment, while not a panacea, at least gives us a fighting chance to ferret out any assumptions that no longer hold true and so on.

@mmd-osm

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

I'm not intending to implement many changes, just the ones that I've been personally complaining about since API 0.6 was released over a decade ago (like incorrect http status codes, and plain-text responses, and stuff like that).

That's the bit I least like about the current multiple API version idea: it seems to focus on something I would describe as cosmetic changes only. That's ok, except for it only adds work to consumers of the API while offering no real value to them at all. Since they already have a working API integration, they would need to spend extra development time and effort to get back to a status quo.

Isn't there anything more compelling to do that would give API consumers more of an incentive to move to the latest and greatest version?


Unrelated question: do we want to support API clients using both 0.6 and 0.7 at the same time to support a gradual transition phase?

@gravitystorm

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 30, 2019

adds work to consumers of the API

existing consumers of the API. Some of these headaches and quirks need to be solved by every API consumer, and the total number of future API consumers yet to be written vastly exceeds the ones written so far. So the sooner we fix them, the better, and it's a shame they've been known about and unfixed for so long already.

Isn't there anything more compelling to do that would give API consumers more of an incentive to move to the latest and greatest version?

Maybe we will want to put some of the more compelling things in v0.8, or v0.9, or something. But I'm determined to avoid getting into the same situation as has happened over, and over, and over again, where the scope of v0.7 expands inexorably until it collapses under its own weight! I'd rather break up the logjam and work on smaller, more frequent improvements (e.g. every 18-36 months, rather than 10+ years and counting) to the API. And I'd rather keep the API changes small so that a developer can upgrade their app with a small amount of code changes that's feasible in a weekend of hacking, rather than some significant API upgrade that puts them off doing any of it and leaves them stuck on v0.6. And making life easier for the developers is the whole point of adding multiple version support, so that they can upgrade when it suits them best.

Anyway, let's talk about this further in a future issue, since we're burying the point of this PR ("is this the best code approach to multiple version support? Can you see a better way of coding the tests?") in wider discussions.

@simonpoole

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

@gravitystorm you might want to consider making a statement as to versioning of the API (for example if it will follow semver semantics going forward), or will we have to continue to assume that every version change is breaking as it is now (which is the real reason why the version is stuck at 0.6).

PS: you probably have to consider decoupling data model version from the API version too, as changing the versions used for the former will likely break every tool out there.

@@ -7,27 +7,54 @@ class CapabilitiesControllerTest < ActionController::TestCase
def test_routes
assert_routing(
{ :path => "/api/capabilities", :method => :get },
{ :controller => "api/capabilities", :action => "show" }
{ :controller => "api/v06/capabilities", :action => "show", :api_version => "0.6" }

This comment has been minimized.

Copy link
@mmd-osm

mmd-osm Aug 30, 2019

Contributor

Say we want to use semantic versioning like 1.2.0, how would this be reflected specifically in directory names like v06?

This comment has been minimized.

Copy link
@simonpoole

simonpoole Aug 30, 2019

Contributor

@mmd-osm a reasonable assumption IMHO would be to only actually have different "directory" names for major versions (aka breaking changes), the client can then determine from the capabilities which minor version is actually supported and from that determine which backwards compatible features are present.

@@ -10,7 +10,7 @@ OSM = {
MAX_REQUEST_AREA: <%= Settings.max_request_area.to_json %>,
SERVER_PROTOCOL: <%= Settings.server_protocol.to_json %>,
SERVER_URL: <%= Settings.server_url.to_json %>,
API_VERSION: <%= Settings.api_version.to_json %>,
API_VERSION: <%= Settings.api_versions.min_by(&:to_f).to_json %>,

This comment has been minimized.

Copy link
@mmd-osm

mmd-osm Aug 30, 2019

Contributor

Why is this value set to "min_by", and what are the implications of it? Does &:to_f play nice with semver (e.g. 1.2.0)?

This comment has been minimized.

Copy link
@gravitystorm

gravitystorm Aug 30, 2019

Author Collaborator

It's mainly just min_by to keep everything working for now, since it will pick 0.6 unless the site operator chooses to only deploy 0.7.

It's used for the bits of the website that talk to the API, like notes and changeset comments. Since there's no changes yet, it's more of a "pick either" situation. .max_by would work fine too.

On a wider point, I'd rather work on refactoring those bits of the site to just be regular webpages like diary entry comments, but that's a different project!

# Our api versions are decimals, but controllers cannot start with a number
# or contain punctuation
def v_string(version)
"v#{version.delete('.')}"

This comment has been minimized.

Copy link
@mmd-osm

mmd-osm Aug 30, 2019

Contributor

Thinking of semver again, would it be better to replace "." by "_" maybe, so it's v0_7 and v1_2 ? How about patch version number? Ignore or include?

This comment has been minimized.

Copy link
@gravitystorm

gravitystorm Aug 30, 2019

Author Collaborator

As discussed in the comments, semver would only use major version numbers here. So e.g. v7 or v124. But it's a great point to raise, thank you.


# simple diff to create a node way and relation using placeholders
diff = <<CHANGESET.strip_heredoc
<osmChange>

This comment has been minimized.

Copy link
@mmd-osm

mmd-osm Aug 30, 2019

Contributor

At one point we'd also need to include a version number in <osmChange> (maybe not now)

This comment has been minimized.

Copy link
@gravitystorm

gravitystorm Aug 30, 2019

Author Collaborator

Yeah, I noticed that during the refactoring, but lets leave that for now since we're not planning any changes to the osmchange format (afaik).

This comment has been minimized.

Copy link
@simonpoole

simonpoole Aug 30, 2019

Contributor

As I pointed out above, we return the data model version in all kinds of places, not just in OSC format. That needs to be decoupled from the API version.

@mmd-osm

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

I'd rather break up the logjam and work on smaller, more frequent improvements (e.g. every 18-36 months, rather than 10+ years and counting) to the API.

This brings up the topic how long you want to support previous API versions. Effectively, we need a Versioning Policy and a clear messaging to API consumers about EOL dates for each version well ahead of time. Otherwise you'll end up maintaining a zoo of different versions you'll never get rid of.

@gravitystorm

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 30, 2019

every version change is breaking as it is now

That will always be the case. If it's not breaking, then there's no need to change the API version in the first place.

I'm sure you're aware that the current abilities of API 0.6 are quite expanded from the initial version back in 2009, but so far it's had to be backwards compatible to avoid breaking clients (with some minor exceptions, like the swf endpoint). Because we've only (been able to|chosen to) deploy backwards-compatible changes, we haven't incremented the version number.

https://stackoverflow.com/questions/27901549/semantic-versioning-of-rest-apis explains it clearly - if we moved to semantic versioning, then only the major number is of interest to API consumers. So that's what we'd use in the URLs. If we wanted to move to semantic versioning for the next version of the API, then I would suggest calling it "API v7".

We'd never need to have an endpoint like /api/v7.1.0/node/1 since, by the definition of semantic versioning, any call to that would be backwards-compatible with any other API 7.x endpoint. If it wasn't compatible, we'd be on API v8, not v7.1.

I'd be happy to move to using integers instead of what we have now, but I suggest we park this for the release-after-next.

@gravitystorm

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 30, 2019

@mmd-osm Thanks for looking at the code! That's really helpful and I appreciate it. If you have any suggestions or more feedback I'd love to hear it.

@simonpoole

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

every version change is breaking as it is now

That will always be the case. If it's not breaking, then there's no need to change the API version in the first place.

I'm sure you're aware that the current abilities of API 0.6 are quite expanded from the initial version back in 2009, but so far it's had to be backwards compatible to avoid breaking clients (with some minor exceptions, like the swf endpoint). Because we've only (been able to|chosen to) deploy backwards-compatible changes, we haven't incremented the version number.

https://stackoverflow.com/questions/27901549/semantic-versioning-of-rest-apis explains it clearly - if we moved to semantic versioning, then only the major number is of interest to API consumers. So that's what we'd use in the URLs. If we wanted to move to semantic versioning for the next version of the API, then I would suggest calling it "API v7".

Neither the original semver definition nor the SO article say that.

We agree that in the calls, only the major version needs to be used, but, because semver allows backwards compatible additions you still need some way of determining what version you -exactly- have in front of you to know if a specific minor version addition is present or not.

In the case of the OSM API that is easily achieved by returning the actual semver (so if I want to use feature X any minor version higher than the one it was introduced in are OK). in the capabilities call That we can't do that currently with 0.6 is the major issue with all the additions that have happened over the years.

@simonpoole
Copy link
Contributor

left a comment

This will change the data model version in sync with the API version. Which a) normally wouldn't be the case, b) will lead to all kind of breakage in the XML parsers.

@gravitystorm

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 30, 2019

you still need some way of determining what version you -exactly- have in front of you

So let's take a feature like "notes" which was added in during the 0.6 era. I think your use-case is along the lines of "I'm an editor developer, and I want to know if notes are available on this endpoint, which might be osm.org or might be some other deployment that is running an older codebase that doesn't have notes yet, so that I can show/hide the relevant UI". If I've misunderstood let me know.

You're suggesting that we should therefore introduce the minor API version in responses. I suspect you don't need to know the minor version in every response (e.g. /api/X/ways/1 -> <osm version="7.9" ...), but you need a response from the server to let you know what the server is capable of doing. So maybe just in the /api/X/capabilities call? And then you can show/hide notes functionality depending on whether X.Y is greater than the version number where notes were introduced.

But then, why bother with lists of features and their corresponding minor version numbers? That's a bit indirect, since we could just put the additional features into the capabilities response e.g. <feature name="notes" /> to indicate that the endpoint now supports notes. Would that work?

@gravitystorm

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 30, 2019

This will change the data model version in sync with the API version. Which a) normally wouldn't be the case, b) will lead to all kind of breakage in the XML parsers.

I've read this a few times, but I don't understand what you mean. Could you give an example?

@simonpoole

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

Consider for example the output of https://api.openstreetmap.org/api/0.6/map?bbox=-122.083819,37.4217991,-122.0834335,37.4222209

Which starts with

<osm version="0.6" generator="CGImap 0.7.5 (14135 thorn-03.openstreetmap.org)" copyright="OpenStreetMap and contributors" attribution="http://www.openstreetmap.org/copyright" license="http://opendatacommons.org/licenses/odbl/1-0/"> <bounds minlat="37.4217991" minlon="-122.0838190" maxlat="37.4222209" maxlon="-122.0834335"/>

Giving the version of the OSM XML document as 0.6, as this is the only indication of the format in use and OSM does not refer to any external schema docs, you will find that a lot of stuff will bork if you change that 0.6 to 0.7 or whatever.

@mmd-osm

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

I tend to agree that we have multiple lifecycles/versioning depending on what you look at. One the one hand, we add features over time to the API in an incremental/non breaking way by either extending existing endpoints or adding new ones. The downside of the current approach is that we have no mechanism to communicate those changes to the outside world. I think this would be a good ft for some semantic versioning, and now would be a good time to seriously think about it, rather than introducing some 0.7 version with some fuzzy requirements.

Now, regarding the XML documents: everything that has been done in the recent years mostly left those unchanged. Some minor tweaks like adding an if-unused attribute in the OsmChange message happened on a message that isn't versioned anyway. For the OSM XML format, I think we never did any changes (?!), and any proposed changes (apart from introducing an area model) don't seem to force us to create a new version of the OSM XML format.

Re. the future OSM JSON format (which will be using the Overpass API JSON format), I'm not sure what the best approach would be. Technically, it's still using the same OSM API 0.6 data model underneath, so we might just call that 0.6 as well?

I think it would be good to establish an independent versioning for the different parts.

@simonpoole

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

@mmd-osm well the only data model change that is in a stage of serious consideration (changing way geometry representation), would require a complete switch over (as a backwards compatibility would be very very difficult to provide), so IMHO not worth considering in the context of this PR.

With other words the version in the documents could just as well be hardwired to 0.6 for now.

@bhousel

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

Probably worth mentioning that semantic versioning guidelines explicitly allow a 0.x major version as a signal that anything can change at any time.

Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

So we are, in a sense, already doing semver the way it's intended. If the OSM API ever gets to v1 then we need to start thinking more about backward compatibility and breakage.

@simonpoole

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

At the time the 0.6 api was released semver was still two years away. Trying to retroactively apply senver semantics doesn't really work.

@bhousel

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

trying to retroactively apply senver semantics doesn't really work.

It's a number. it either works or it doesn't. (in this case, it does)

@simonpoole

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

trying to retroactively apply senver semantics doesn't really work.

It's a number. it either works or it doesn't. (in this case, it does)

You were implying to that the *number" gives us licence to break everything at will, not only is that not supported by the facts (as said the "number" predates the concept of semver), it relveals a very hostile attitude towards other developers,

@mmd-osm

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

You were implying to that the *number" gives us licence to break everything at will,

It may be a red flag to other developers that things may change, and we could break things at our own discretion. In fact our Terms of Usage even support that approach. IX Disclaimer says:

We may add, change, replace, remove, or discontinue the features and functions of the Services, including APIs. It is your responsibility to ensure that calls or requests you make to the Services are compatible with then-current APIs and authentication methods, and that you handle responses properly.

Of course, we're trying to be nice, but we're in no way obliged to do so.

@simonpoole

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

@mmd-osm given that I'm one of the authors of that text, I can assure you that it simply is designed to limit the liability of the OSMF if we or somebody else screws up. It is completely orthogonal to any ethical, moral and professional obligation to behave reasonably.

@gravitystorm

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 4, 2019

Consider for example the output of https://api.openstreetmap.org/api/0.6/map?bbox=-122.083819,37.4217991,-122.0834335,37.4222209
Which starts with
<osm version="0.6"

Right, and I would expect the output for any requests made to api/0.7 will start <osm version="0.7">. If a client is making requests to the api, then it would expect the response version to match the request version. If it wants a 0.6 response, it should make a request to the 0.6 endpoint.

Now if that response is saved somewhere, then it's useful to know which version of the API originally created that document, so that you know how to parse it. The version in the document is the version of the response. It's not related to some underlying abstract data model, if there is such a thing.

For example, imagine if we keep this underlying abstract data model exactly the same, but change the response format. This could be* changing tags to have "key" and "value" attributes instead of "k" and "v". Then it seems obvious that the response should be <osm version="0.7" ... <tag key="..." value="...">, since software will need to know the version of the response to know how to parse it. But this illustrates my point - the version in the document is tied to the representation of the data, not the underlying abstract data model, which hasn't changed.

So this PR follows what I expect most other people expect - that the version in the returned document matches the version of the API that requested the document.

(* purely for example; I'm not proposing this)

@gravitystorm

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 4, 2019

So, trying (desperately) to drag the conversation back to the contents of this PR - does anyone have any comments on handling the indentation of the tests? I really dislike PRs that combine widespread indentation with additional changes to the code, since I find it very hard to spot where the real changes are and which lines are just indented with no other change.

I've been considering whether I should split out the indentation into a separate PR, with a dummy indent method like

def indent
  yield
end

and then make a PR with the methods indented but with no other changes

  def test_something
-   code
-   code
-   post :foo
-   code
+   indent
+    code
+    code
+    post :foo
+    code
+   end
  end

and then in the second PR it will be more obvious which of the lines have real changes.

  def test_other_thing
-   indent
+   all_api_versions.each do |version|
      code
      code
-     post :foo
+     post :foo, params => { etc }
      code
    end
end

Or am I worrying about this too much, and the PR is fine the way it is now?

@iandees

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

@gravitystorm If it helps, one of the options in the GitHub pull request view is to ignore whitespace changes. It's in one of the hamburger menus when viewing the diffs.

@simonpoole

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

@gravitystorm you are essentially making an argument for versioning the representation too. Up to now the abstract underlying data representation has essentially been documented (if you so want) by the XML representation and has been in lock step with that.

Now clearly we could change the representation in isolation, as your example suggests, without changing the API nor the underlying abstract data model and this would clearly require changing the API version too, because without that you would not know that you are getting a non-backwards compatible XML doc. But the ITU lies in the direction of doing and supporting that kind of change, so I hope we are not venturing there.

But the other way around does not imply the same thing, for example all the changes that have been made to the API to date work just fine with 0.6 XML documents. And for example it would be -very- surprising if implementing #2348 and jacking up the API version would suddenly result in output that is not parseable by tools (at least those that do proper validation of their input) that would in principle work just fine if the gratious change to the "representation version" hadn't been made.

@gravitystorm

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 4, 2019

you are essentially making an argument for versioning the representation too.

No, I'm explaining that the version is the API response version, not a version of the data model. For example, nothing changed in the trace responses when we last changed the API version, but the responses to api/0.6/gpx/nnn are all <osm version="0.6"> and not 0.4 or 0.3 or whenever that part of the underlying data model changed. So in API 0.7, if they have the same underlying data model, or even the same representation, they will definitely have 0.7 in their responses.

So I'll say it again, the version in the documents indicates the version of the API used, not the version of any underlying data model.

Up to now the abstract underlying data representation has essentially been documented (if you so want) by the XML representation and has been in lock step with that.

No, it hasn't. There's been lots of changes to the underlying data model in the last 10 years, none of which have had a change in API version number - because even though the data model has changed, the API request/responses have only changed in backwards compatible ways. For example, we added notes. That was a backwards-incompatible change to the data model (you can't fit notes into a database that doesn't have a table for them, for example) but a backwards-compatible change to the API, hence no version number change.

And for example it would be -very- surprising if implementing #2348 and jacking up the API version

Nobody has suggested that adding a new backwards-compatible API call needs a new API version - except for you, when you wrote "Without it (this PR) we would have simply added them as a 0.6 feature". So I'm not going to rebut something of your own creation that you are now arguing against.

If you have another example, that would be helpful.

if the gratious change to the "representation version" hadn't been made.

I'm not proposing any "gratuitous" changes to the API version. The only reason I'm proposing to change the API version is to allow us to make backwards-incompatible changes that can't otherwise be made. The whole point of this PR is to allow us to run multiple API versions in parallel - it's even in the title! So any tools that can only parse one version can keep using that version. Gratuitous change would be to yank the old version when the new one is deployed, or to bump version numbers for backwards-compatible changes, and I'm proposing neither of those.

@gravitystorm

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 4, 2019

@gravitystorm If it helps, one of the options in the GitHub pull request view is to ignore whitespace changes. It's in one of the hamburger menus when viewing the diffs.

Thanks @iandees, that's really helpful. Took me a while to find it, even after you'd told me it was there somewhere!

@simonpoole

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

If you have another example, that would be helpful.

OK, you change the error responses to be returned in a structured format and not the current plain text.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.