Skip to content

Deprecate/Remove /api/0.6/changes endpoint#2486

Closed
mmd-osm wants to merge 1 commit intoopenstreetmap:masterfrom
mmd-osm:patch/remove_changes_endpoint
Closed

Deprecate/Remove /api/0.6/changes endpoint#2486
mmd-osm wants to merge 1 commit intoopenstreetmap:masterfrom
mmd-osm:patch/remove_changes_endpoint

Conversation

@mmd-osm
Copy link
Copy Markdown
Contributor

@mmd-osm mmd-osm commented Dec 28, 2019

In #2383, @zerebubuth proposed to deprecate and remove the /api/0.6/changes endpoint.

From his reasoning: This API call is little-used and IMHO should be deprecated and removed.

This pull request removes controller, api_ability, route and test case for this endpoint.

/api/0.6/changes is undocumented at this time and a rather obscure and specialized feature.

One (former?) user I could identify: @iandees has an 8 years old script out there (https://gist.github.com/iandees/1271803) to remove outdated tiles in some TileStache disk cache. Not sure, if this is still in use today?

TODO:

  • Check production log to identify any remaining users of this endpoint

@iandees
Copy link
Copy Markdown
Contributor

iandees commented Dec 28, 2019

My code is not in use anywhere that I know of.

@simonpoole
Copy link
Copy Markdown
Contributor

While I suspect this is nitpicking, could we agree that we only remove support without proper deprecation notice and bumping the version for stuff that is a) not actually in use. not "little-used" (which I suspect is the the case) and b) not documented?

Doing anything else would just seem to be a massive best-practices violation (and yes I know we already did that with the expand_bbox call).

@mmd-osm
Copy link
Copy Markdown
Contributor Author

mmd-osm commented Dec 28, 2019

Exactly, that’s why I wrote „any remaining users“ in the todo list. Removing little used endpoints is obviously not going to work without deprecation, etc.

@mmd-osm
Copy link
Copy Markdown
Contributor Author

mmd-osm commented Dec 29, 2019

I found one or two other posts from 10 years ago mentioning this endpoint in the context of Tiles@Home, a project which is essentially dead/deprecated/discontinued since 2012.

https://talk.openstreetmap.narkive.com/MY1AhwJX/osm-talk-list-of-changes-from-api-only-gives-target-tile-of-moved-node

https://svn.openstreetmap.org/!svn/bc/20000/sites/other/tilesAtHome_tahngo/requests/views.py

@zerebubuth
Copy link
Copy Markdown
Contributor

TODO: Check production log to identify any remaining users of this endpoint

I looked back over the past 19 days of logs, and saw what looks like one actual user, plus a bunch of other random requests.

Random requests

  • 7 web spiders (following a link from the wiki - "List of slippy map tiles which have changed recently..."),
  • 2 browser visits following the same link,
  • 3 visits with a browser-like UA but no referer,

Only one of the above requests was made with any query parameters, suggesting to me that they're one-off experiments or links followed without knowing that they lead to an API rather than a web page.

Single user

There was an hourly request from a single IP for the changes over the past hour. Sadly, I could not find any useful information about that IP; it did not reverse into anything useful and the whois result just returned a popular VPS company.

The requests are made with no User-Agent or Referer headers (so if this was a tile request then the policy would prohibit this use). The requests are made over HTTP/1.0, suggesting that this is perhaps an old script, one using an old HTTP library, or one that makes HTTP requests manually.

What to do?

With the information available, it seems it would be difficult to figure out how to contact this user. A couple of options that occur to me:

  1. Break or temporarily disable the API call, and hope the user notices and gets in touch with us, so that we can provide alternative methods of getting the same information.
  2. Provide a legacy API capable of handling only the time-windowed kind of requests that are being made (i.e: providing start and end query parameters, but leaving everything else defaulted) based off the minute diffs and run that separately from the main website, allowing us to remove this API from the Rails code (and the function from the database).

As Simon points out, the former option is generally considered bad practice. However, it's the smallest amount of work 😉

On the other hand, writing a small, self-contained api/0.6/changes server might be a fun exercise - is anyone interested?

@mmd-osm
Copy link
Copy Markdown
Contributor Author

mmd-osm commented Dec 30, 2019

Thanks! Do we know if the VPS company has an abuse contact address where we could report those „unsolicited“ requests and try to get in touch with whoever is running the script? I think that’s not totally inappropriate given that the client fails to provide a valid user agent as per https://operations.osmfoundation.org/policies/api/

@zerebubuth
Copy link
Copy Markdown
Contributor

Yes, they have an abuse contact address. Although contacting them seems to me like it should be a last resort, given that the requests are not otherwise causing any kind of operational problem. (The issue we have is a development one, in that we'd like to simplify the code by removing this API call.)

@mmd-osm
Copy link
Copy Markdown
Contributor Author

mmd-osm commented Jan 1, 2020

Two more options come to mind:

  • any web server running on that IP (http://... or https://...)?
  • check if the IP uses any of the other APIs/tiles/minutely diff/etc. The idea is that the changes endpoint has rather limited value on its own.

@matkoniecz
Copy link
Copy Markdown
Contributor

Maybe given

The requests are made with no User-Agent or Referer headers

it would be fine to ignore this single user?

@mmd-osm
Copy link
Copy Markdown
Contributor Author

mmd-osm commented Jul 4, 2020

This topic was already discussed back in 2013: https://wiki.osmfoundation.org/wiki/Working_Group_Minutes/EWG_2013-06-17

18:06:24 once an hour a machine at OVH France checks it
18:06:57 possibly the french proxy thing?
[...]
18:11:20 so they can definitely go. the only question is how do we announce it, and can we get in touch with this single user?
[...]
18:14:41 sure, i was hoping we could just contact that single user. perhaps it's an old thing running, and we can just switch off changes tomorrow.
18:15:19 but i can't see any identifying information, the UA is "-" and DNS reverse lookup isn't helpful.
18:15:38 well we could block them for violating the user-agent policy and then we'd have no users :)

@mmd-osm mmd-osm closed this Jul 17, 2020
@mmd-osm mmd-osm reopened this Dec 29, 2020
@mmd-osm
Copy link
Copy Markdown
Contributor Author

mmd-osm commented Dec 29, 2020

More items to be removed here:

  • functions/maptile.c - definition of maptile_for_point, which is only being used for the changes endpoint
  • functions/functions.sql - maptile_for_point as pure sql version

This obsolete endpoint adds complexity to our Docker based setup for no good reason, and it would be best to get rid of it altogether.

Also removes sql functions which are only used by this endpoint
@mmd-osm
Copy link
Copy Markdown
Contributor Author

mmd-osm commented Dec 30, 2020

All tests are passing again. There's only some issue with sending test results to coveralls, which I cannot fix.

Finished in 180.878558s, 6.9273 runs/s, 2953.9322 assertions/s.
1253 runs, 534303 assertions, 0 failures, 0 errors, 0 skips

Do we need some db migration to get rid of the old sql function as well?

@gravitystorm
Copy link
Copy Markdown
Collaborator

I'm happy to see this merged. I mean, I would prefer for it to be properly announced as deprecated, not included in the next API version, retired when 0.7 is deactivated, all that kind of thing - but lets face it, that approach is not going to happen any time soon.

So I think we should just remove it and move on. If nothing has changed on this topic since 2013, then I don't think removing it now is rushing anything.

@tomhughes
Copy link
Copy Markdown
Member

As far as I can see nobody has called it in the last few weeks, other than me just now...

@gravitystorm
Copy link
Copy Markdown
Collaborator

I rebased this as 4e6d729 in order to investigate the test failures. Since all the tests passed, I've merged this as 5e307c1 .

Thanks @mmd-osm for working on this, and updating the PR with further simplifications!

@mmd-osm mmd-osm deleted the patch/remove_changes_endpoint branch February 13, 2021 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants