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

Browse deleted objects #1448

Closed

Conversation

@woodpeck
Copy link
Contributor

commented Feb 19, 2017

This changes the "browse" view of nodes, ways, and relations to use a newly introduced API call named "fordisplay". The "fordisplay" call behaves identical to "full" for visible objects, but for invisible objects will return a previous, visible version of the object, together with either current or previous versions of dependencies. When "fordisplay" returns an object that is not currently visible, it will add a "deleted=true" attribute to the root element of the XML document, enabling the Javascript frontend code responsible for displaying the object on Leaflet to choose another style.

This is how a deleted object is represented on the browse page:

deleted

This pull request fixes the nuisance that if you examine a changeset that deletes an object, and you click on the struck-through object ID, you're stranded with a browse page that doesn't even tell you where the object used to be.

This pull request does not contain implementation of the "fordisplay" API calls in cgimap.

Also, this pull request adds a few fixtures for tests and this introduces one test failure in AmfController that I was unable to understand or fix.

@tomhughes

This comment has been minimized.

Copy link
Member

commented Feb 19, 2017

While I have no problem with the principle of display the last geometry of the way in the browse view like this it needs to be done properly, which basically means solving the "history api" problem properly.

No way are we going to be adding more back door history hacks like this - we already did that once with amf_controller and Potlatch 1 and we're not going to repeat it.

In addition we shouldn't be adding new API calls to the ruby code anyway - the goal is to move the API to cgimap so implementing this as ruby only is not going to work.

@tomhughes tomhughes closed this Feb 19, 2017
@simonpoole

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2017

@zerebubuth @gravitystorm @tomhughes wrt the last point Tom raises: we were discussing yesterday in Karlsruhe that we are missing some guidance on how to proceed with API changes/additions: implement them both in rails and cgimap or just in one, and if just in one what would be preferred?

Naturally just adding something to cgimap is rather problematic at this point in time.

@pnorman

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2017

@zerebubuth @gravitystorm @tomhughes wrt the last point Tom raises: we were discussing yesterday in Karlsruhe that we are missing some guidance on how to proceed with API changes/additions: implement them both in rails and cgimap or just in one, and if just in one what would be preferred?

I take the view that adding a new call to just rails is okay, although not ideal for many calls.

@gravitystorm

This comment has been minimized.

Copy link
Collaborator

commented Feb 20, 2017

I'm fine with adding new API calls to just the rails code.

But this is because while @zerebubuth and I agree on 95% of cgimap/rails discussions, the one thing we slightly disagree on is the final destination.

My goal is to have the API powered by rails, and use cgimap-ruby to do the heavy lifting for some/many of the API calls. This will remove the duplication (of having both a rails version of e.g. /map and a cgimap version) yet leaves the whole thing integrated. This saves having to write a C++ version of every single API call, and while most of our API calls are currently geodata related, I see more and more being things like friends and home locations and groups and stuff that has little advantage in being (re-)written in C++.

@zerebubuth's goal with cgimap is to run all of the API calls with no ruby/rails involved whatsoever, and to have cgimap-ruby as a shim so that it's easy for developers to have a working API when doing local development or small deployments (without having to set up both the website and also cgimap processes to get thigns working). For example, this is why cgimap now can handle oauth signed requests directly, whereas I would let rails continue to do the oauth and then hand over to cgimap.

So both of us want cgimap, and both of us want cgimap-ruby to be fully integrated into the rails codebase, but for different reasons :-)

As I said at the top, I think it's fine to have ruby-only API calls, since that's part of my long-term planning for openstreetmap-website. But other developers have slightly different long-term plans.

@tomhughes

This comment has been minimized.

Copy link
Member

commented Feb 20, 2017

Regardless of the general position regarding API calls the very name fordisplay just stinks of a gross hack that we shouldn't be letting anywhere near the API to my mind.

@Zverik

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2017

  1. You have deleted db/structure.sql in this PR. Not sure if it was needed, since every installation overwrites it one way or another.
  2. I'd rename the enpoint to /last_visible.
@zerebubuth

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2017

But this is because while @zerebubuth and I agree on 95% of cgimap/rails discussions, the one thing we slightly disagree on is the final destination.

@gravitystorm is correct: We disagree! 😉

In this situation, my preferred solution would not involve the editing API at all, but instead be a completely separate service which could be run and scaled entirely independently. I think that the more we add to the editing API, the more difficult it becomes to run, maintain and modify in the future.

Having said that, I also don't think that my hand-waving opinions should stand in the way of adding useful functionality. I think that adding something like this would be a good idea, and would be useful.

The "fordisplay" call behaves identical to "full" for visible objects, but for invisible objects will return a previous, visible version of the object, together with either current or previous versions of dependencies.

Is there any restriction on which previous, visible version of the object will be returned? It would seem sensible that it would return the most recent version that was visible. Are there difficulties guaranteeing that? What would happen if the current version is deleted and the only previous version is redacted?

If it can return either the current or the previous versions of dependencies, is it possible that it returns deleted versions of nodes used in a way? Could the API call be defined to return the versions of dependencies which were visible at the time that the element was deleted?

I'm slightly concerned about putting deleted="true" at the root of the XML document as well, as it would be effectively extending the XML content with style information. That seems like something that could be handled in the client javascript by falling back to /fordisplay (I prefer @Zverik's suggestion of /last_visible) if the current /full returns a visible="false" or 404 entry. Or perhaps the /fordisplay call could return the current version always, plus the last visible version if the current version is not visible?

... implement them both in rails and cgimap or just in one, and if just in one what would be preferred?

Naturally just adding something to cgimap is rather problematic at this point in time.

Yes, an implementation in both would be very welcome, or just Rails if only one is being done. Unfortunately, we're still stuck halfway without a full replacement for the Rails API in cgimap(-ruby). If anyone would like to help with that, then please take a look at the Cgimap issues, which I've tried to mark up with difficulty levels. Please get in touch with me if there's something I can do to help you get started with Cgimap.

@mmd-osm

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2018

Please pardon my ignorance, but what was the main motivation for using cgimap-ruby in the first place, rather than say having Rails port and cgimap completely separate as it is today?

Not using cgimap-ruby would result in some double maintenance. OTOH, Rails port is fairly stable today, and an ideal self contained platform, which is a nice property for prototyping.

Adding calls to cgimap via cgimap-ruby would require changes to the Rails port itself and have a working cgimap-ruby, which in turn introduces lots of tricky dependencies. In addition, future changes in cgimap might be more difficult due to the reverse dependency on the rails port.

I find the tight coupling on Ruby<>C++ level introduced by cgimap-ruby a bit troubling and would be less concerned to have an HTTP based integration, if at all.

My recommendation would be:

  • focus all efforts on improving cgimap
  • keep Rails port and cgimap in sync from a functional point of view
  • don't try to link them two in any way, except for the database table layout (owned by Rails port)
  • deprecate parts of the Rails port over time if the need arises

btw: I tried to get cgimap-ruby running on Ubuntu 16.04 and unfortunately failed to even compile the sources :(

@zerebubuth's goal with cgimap is to run all of the API calls with no ruby/rails involved whatsoever

Agree with this one as well. Having all API calls reimplemented in cgimap is probably not reasonable, though.

@gravitystorm

This comment has been minimized.

Copy link
Collaborator

commented Mar 13, 2018

Please pardon my ignorance, but what was the main motivation for using cgimap-ruby in the first place, rather than say having Rails port and cgimap completely separate as it is today

One of the motivations is because if you start using the openstreetmap-website code today on a new project (or even just for testing locally) you're using a lot of code that isn't actually used in production. Vast amounts of ruby code that builds API responses in a slow, memory hungry fashion that was the reason for creating cgimap in the first place. So anyone who wants to genuinely use this codebase for a project then needs to set up cgimap, and deal with cgi processes and marshalling calls between the two codebases based on incoming url paths and yada yada. Instead, we could have a cgimap-ruby gem, so that the out-of-the-box experience for openstreetmap-website is much more performant, much closer to what OSM uses in production, and contains a lot less gnarly old unloved ruby code.

Not using cgimap-ruby would result in some double maintenance. OTOH, Rails port is fairly stable today, and an ideal self contained platform, which is a nice property for prototyping.

It's easy to argue that having to duplicate any new API calls (or make changes to existing ones) both in here and in cgimap is a way of slowing down progress - what you call stable others might call ossified. I'd rather we were in a place where new ideas need only one implementation.

would require changes to the Rails port itself and have a working cgimap-ruby

The latter of which makes most of this discussion academic for now! Afaik nobody has been working on cgimap-ruby or attempting the integration into openstreetmap-website recently, but I'd be interested to know if anyone is looking at it.

btw: I tried to get cgimap-ruby running on Ubuntu 16.04 and unfortunately failed to even compile the sources :(

Ah, that sucks. I'll try to look into that for you (on the cgimap-ruby tracker) - it's been a while since I last compiled it myself.

@mmd-osm

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2018

Thanks a lot for your detailed feedback!

So anyone who wants to genuinely use this codebase for a project then needs to set up cgimap, and deal with cgi processes and marshalling calls between the two codebases based on incoming url paths and yada yada.

I don't think this is too bad. The logic to route requests to either Rails port or cgimap is a just a few lines in the Apache configuration and can be easily adjusted as needed. Running cgimap itself seems to be manageable quite well via a systemd script. Overall this approach seems very reasonable to me, even for third parties.

out-of-the-box experience for openstreetmap-website is much more performant, much closer to what OSM uses in production, and contains a lot less gnarly old unloved ruby code

This makes sense. Yet I wonder a bit if this OSM website/API project is really meant to be a general purpose editing API for third parties to build their own site upon.

I'd rather we were in a place where new ideas need only one implementation.

Fully agree that double effort might be slowing things down and a single implementation would be ideal.

I see this a bit from a different angle. Today, you can make some pretty good progress if you're familiar with either Ruby or C++. I'm worried that once cgimap-ruby comes into the picture, it requires (development, bug chasing, performance, ,... ) knowledge in both Ruby and C++, a skillset that is very rare out there.

but I'd be interested to know if anyone is looking at it.

AFAIK, Paul suggested it as GSoC 2018 project, or it has been moved from the GSoC 2017 page to the current one. Maybe a student is brave enough to look into it.

I'll try to look into that for you (on the cgimap-ruby tracker) - it's been a while since I last compiled it myself.

Great, thanks a lot for looking into this. 👍

@gravitystorm

This comment has been minimized.

Copy link
Collaborator

commented Mar 14, 2018

The logic to route requests to either Rails port or cgimap is a just a few lines in the Apache configuration and can be easily adjusted as needed. Running cgimap itself seems to be manageable quite well via a systemd script. Overall this approach seems very reasonable to me, even for third parties.

That pre-supposes that you're using Apache, or are running on your own machine, etc. If you want to spin up an openstreetmap-website instance on e.g. Heroku, OpenShift or some other Rails hosting, then having everything as gem dependencies would be better. We know that deployments have to use "apt get" and "bundle install" so it's worth trying to remove additional complexity.

knowledge in both Ruby and C++, a skillset that is very rare out there.

That's definitely a risk. I'm not yet sure how big the cgimap-ruby surface area would be, or whether it's a thin layer between cgimap and ruby that won't need much maintaining when it's working.

AFAIK, Paul suggested it as GSoC 2018 project, or it has been moved from the GSoC 2017 page to the current one. Maybe a student is brave enough to look into it.

That would be great if it happened!

@mmd-osm

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2018

In this situation, my preferred solution would not involve the editing API at all, but instead be a completely separate service which could be run and scaled entirely independently

If anyone feels like this is something for Overpass API, here's a query proposal to fetch the object at a given point in time. Ideally, this should be one second before the object was deleted:

[timeout:5]
[date:"2018-03-12T22:47:00Z"];
way(id:539940355);
(._;>;);
out;

http://overpass-turbo.eu/s/x4C

Original object: https://www.openstreetmap.org/way/539940355

I don't have an idea how often deleted features are shown as single object today, so it's very difficult from the outside to assess the performance impact of such a query.

@mmd-osm mmd-osm referenced this pull request Jun 6, 2018
@woodpeck woodpeck deleted the woodpeck:browse-deleted-objects branch Mar 27, 2019
@mmd-osm mmd-osm referenced this pull request Sep 29, 2019
@mmd-osm

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2019

In #130 (comment) a new prototype to retrieve a way's history including all of its node versions is presented.

This might be useful to render a previous version of an object. More details are available in #130.

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