From 49fac49f9d6a3d49c2cd56b605ee1ba9323dffb6 Mon Sep 17 00:00:00 2001 From: Anton Khorev Date: Thu, 20 Feb 2025 03:41:04 +0300 Subject: [PATCH] Create api changeset download resource --- app/abilities/api_ability.rb | 2 +- .../api/changesets/downloads_controller.rb | 61 ++++++++++++ app/controllers/api/changesets_controller.rb | 52 ----------- .../show.xml.builder} | 6 +- app/views/changesets/index.atom.builder | 2 +- app/views/changesets/show.html.erb | 2 +- config/routes.rb | 5 +- .../changesets/downloads_controller_test.rb | 93 +++++++++++++++++++ .../api/changesets_controller_test.rb | 86 +---------------- 9 files changed, 166 insertions(+), 143 deletions(-) create mode 100644 app/controllers/api/changesets/downloads_controller.rb rename app/views/api/changesets/{download.xml.builder => downloads/show.xml.builder} (55%) create mode 100644 test/controllers/api/changesets/downloads_controller_test.rb diff --git a/app/abilities/api_ability.rb b/app/abilities/api_ability.rb index edf051fae..9a7bf254a 100644 --- a/app/abilities/api_ability.rb +++ b/app/abilities/api_ability.rb @@ -10,7 +10,7 @@ class ApiAbility can [:read, :feed, :search], Note can :create, Note unless user - can [:read, :download], Changeset + can :read, Changeset can :read, ChangesetComment can :read, Tracepoint can :read, User diff --git a/app/controllers/api/changesets/downloads_controller.rb b/app/controllers/api/changesets/downloads_controller.rb new file mode 100644 index 000000000..99f0182b5 --- /dev/null +++ b/app/controllers/api/changesets/downloads_controller.rb @@ -0,0 +1,61 @@ +module Api + module Changesets + class DownloadsController < ApiController + authorize_resource :changeset + + before_action :set_request_formats + + ## + # download the changeset as an osmChange document. + # + # to make it easier to revert diffs it would be better if the osmChange + # format were reversible, i.e: contained both old and new versions of + # modified elements. but it doesn't at the moment... + # + # this method cannot order the database changes fully (i.e: timestamp and + # version number may be too coarse) so the resulting diff may not apply + # to a different database. however since changesets are not atomic this + # behaviour cannot be guaranteed anyway and is the result of a design + # choice. + def show + changeset = Changeset.find(params[:changeset_id]) + + # get all the elements in the changeset which haven't been redacted + # and stick them in a big array. + elements = [changeset.old_nodes.unredacted, + changeset.old_ways.unredacted, + changeset.old_relations.unredacted].flatten + + # sort the elements by timestamp and version number, as this is the + # almost sensible ordering available. this would be much nicer if + # global (SVN-style) versioning were used - then that would be + # unambiguous. + elements.sort_by! { |e| [e.timestamp, e.version] } + + # generate an output element for each operation. note: we avoid looking + # at the history because it is simpler - but it would be more correct to + # check these assertions. + @created = [] + @modified = [] + @deleted = [] + + elements.each do |elt| + if elt.version == 1 + # first version, so it must be newly-created. + @created << elt + elsif elt.visible + # must be a modify + @modified << elt + else + # if the element isn't visible then it must have been deleted + @deleted << elt + end + end + + respond_to do |format| + format.xml + end + end + end + end +end diff --git a/app/controllers/api/changesets_controller.rb b/app/controllers/api/changesets_controller.rb index 3df7b75ce..517cff473 100644 --- a/app/controllers/api/changesets_controller.rb +++ b/app/controllers/api/changesets_controller.rb @@ -130,58 +130,6 @@ module Api end end - ## - # download the changeset as an osmChange document. - # - # to make it easier to revert diffs it would be better if the osmChange - # format were reversible, i.e: contained both old and new versions of - # modified elements. but it doesn't at the moment... - # - # this method cannot order the database changes fully (i.e: timestamp and - # version number may be too coarse) so the resulting diff may not apply - # to a different database. however since changesets are not atomic this - # behaviour cannot be guaranteed anyway and is the result of a design - # choice. - def download - changeset = Changeset.find(params[:id]) - - # get all the elements in the changeset which haven't been redacted - # and stick them in a big array. - elements = [changeset.old_nodes.unredacted, - changeset.old_ways.unredacted, - changeset.old_relations.unredacted].flatten - - # sort the elements by timestamp and version number, as this is the - # almost sensible ordering available. this would be much nicer if - # global (SVN-style) versioning were used - then that would be - # unambiguous. - elements.sort_by! { |e| [e.timestamp, e.version] } - - # generate an output element for each operation. note: we avoid looking - # at the history because it is simpler - but it would be more correct to - # check these assertions. - @created = [] - @modified = [] - @deleted = [] - - elements.each do |elt| - if elt.version == 1 - # first version, so it must be newly-created. - @created << elt - elsif elt.visible - # must be a modify - @modified << elt - else - # if the element isn't visible then it must have been deleted - @deleted << elt - end - end - - respond_to do |format| - format.xml - end - end - ## # updates a changeset's tags. none of the changeset's attributes are # user-modifiable, so they will be ignored. diff --git a/app/views/api/changesets/download.xml.builder b/app/views/api/changesets/downloads/show.xml.builder similarity index 55% rename from app/views/api/changesets/download.xml.builder rename to app/views/api/changesets/downloads/show.xml.builder index 1e400cd9f..88e05b587 100644 --- a/app/views/api/changesets/download.xml.builder +++ b/app/views/api/changesets/downloads/show.xml.builder @@ -3,17 +3,17 @@ xml.instruct! :xml, :version => "1.0" xml.osmChange(OSM::API.new.xml_root_attributes) do |osm| @created.each do |elt| osm.create do |create| - create << render(elt) + create << render(:partial => "api/#{elt.to_partial_path}", :object => elt) end end @modified.each do |elt| osm.modify do |modify| - modify << render(elt) + modify << render(:partial => "api/#{elt.to_partial_path}", :object => elt) end end @deleted.each do |elt| osm.delete do |delete| - delete << render(elt) + delete << render(:partial => "api/#{elt.to_partial_path}", :object => elt) end end end diff --git a/app/views/changesets/index.atom.builder b/app/views/changesets/index.atom.builder index 4ba79f797..6556f12bf 100644 --- a/app/views/changesets/index.atom.builder +++ b/app/views/changesets/index.atom.builder @@ -21,7 +21,7 @@ atom_feed(:language => I18n.locale, :schema_date => 2009, :href => api_changeset_url(changeset, :only_path => false), :type => "application/osm+xml" entry.link :rel => "alternate", - :href => changeset_download_url(changeset, :only_path => false), + :href => api_changeset_download_url(changeset, :only_path => false), :type => "application/osmChange+xml" if !changeset.tags.empty? && changeset.tags.key?("comment") diff --git a/app/views/changesets/show.html.erb b/app/views/changesets/show.html.erb index 702e61f92..2de744a93 100644 --- a/app/views/changesets/show.html.erb +++ b/app/views/changesets/show.html.erb @@ -107,7 +107,7 @@
<%= link_to t(".changesetxml"), api_changeset_path(@changeset) %> · - <%= link_to(t(".osmchangexml"), :controller => "api/changesets", :action => "download") %> + <%= link_to t(".osmchangexml"), api_changeset_download_path(@changeset) %>
<% if @next_by_user || @prev_by_user %> diff --git a/config/routes.rb b/config/routes.rb index 602d35d7a..bdda8edea 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -18,7 +18,6 @@ OpenStreetMap::Application.routes.draw do get "permissions" => "permissions#show" post "changeset/:id/upload" => "changesets#upload", :as => :changeset_upload, :id => /\d+/ - get "changeset/:id/download" => "changesets#download", :as => :changeset_download, :id => /\d+/ post "changeset/:id/subscribe" => "changesets#subscribe", :as => :api_changeset_subscribe, :id => /\d+/ post "changeset/:id/unsubscribe" => "changesets#unsubscribe", :as => :api_changeset_unsubscribe, :id => /\d+/ put "changeset/:id/close" => "changesets#close", :as => :changeset_close, :id => /\d+/ @@ -29,7 +28,9 @@ OpenStreetMap::Application.routes.draw do namespace :api, :path => "api/0.6" do resources :changesets, :only => [:index, :create] - resources :changesets, :path => "changeset", :id => /\d+/, :only => [:show, :update] + resources :changesets, :path => "changeset", :id => /\d+/, :only => [:show, :update] do + resource :download, :module => :changesets, :only => :show + end put "changeset/create" => "changesets#create", :as => nil resources :changeset_comments, :only => :index diff --git a/test/controllers/api/changesets/downloads_controller_test.rb b/test/controllers/api/changesets/downloads_controller_test.rb new file mode 100644 index 000000000..2a38951eb --- /dev/null +++ b/test/controllers/api/changesets/downloads_controller_test.rb @@ -0,0 +1,93 @@ +require "test_helper" + +module Api + module Changesets + class DownloadsControllerTest < ActionDispatch::IntegrationTest + ## + # test all routes which lead to this controller + def test_routes + assert_routing( + { :path => "/api/0.6/changeset/1/download", :method => :get }, + { :controller => "api/changesets/downloads", :action => "show", :changeset_id => "1" } + ) + end + + def test_show + changeset = create(:changeset) + node = create(:node, :with_history, :version => 1, :changeset => changeset) + tag = create(:old_node_tag, :old_node => node.old_nodes.find_by(:version => 1)) + node2 = create(:node, :with_history, :version => 1, :changeset => changeset) + _node3 = create(:node, :with_history, :deleted, :version => 1, :changeset => changeset) + _relation = create(:relation, :with_history, :version => 1, :changeset => changeset) + _relation2 = create(:relation, :with_history, :deleted, :version => 1, :changeset => changeset) + + get api_changeset_download_path(changeset) + + assert_response :success + # FIXME: needs more assert_select tests + assert_select "osmChange[version='#{Settings.api_version}'][generator='#{Settings.generator}']" do + assert_select "create", :count => 5 + assert_select "create>node[id='#{node.id}'][visible='#{node.visible?}'][version='#{node.version}']" do + assert_select "tag[k='#{tag.k}'][v='#{tag.v}']" + end + assert_select "create>node[id='#{node2.id}']" + end + end + + def test_show_should_sort_by_timestamp + changeset = create(:changeset) + node1 = create(:old_node, :version => 2, :timestamp => "2020-02-01", :changeset => changeset) + node0 = create(:old_node, :version => 2, :timestamp => "2020-01-01", :changeset => changeset) + + get api_changeset_download_path(changeset) + + assert_response :success + assert_dom "modify", :count => 2 do |modify| + assert_dom modify[0], ">node", :count => 1 do |node| + assert_dom node, ">@id", node0.node_id.to_s + end + assert_dom modify[1], ">node", :count => 1 do |node| + assert_dom node, ">@id", node1.node_id.to_s + end + end + end + + def test_show_should_sort_by_version + changeset = create(:changeset) + node1 = create(:old_node, :version => 3, :timestamp => "2020-01-01", :changeset => changeset) + node0 = create(:old_node, :version => 2, :timestamp => "2020-01-01", :changeset => changeset) + + get api_changeset_download_path(changeset) + + assert_response :success + assert_dom "modify", :count => 2 do |modify| + assert_dom modify[0], ">node", :count => 1 do |node| + assert_dom node, ">@id", node0.node_id.to_s + end + assert_dom modify[1], ">node", :count => 1 do |node| + assert_dom node, ">@id", node1.node_id.to_s + end + end + end + + ## + # check that the changeset download for a changeset with a redacted + # element in it doesn't contain that element. + def test_show_redacted + changeset = create(:changeset) + node = create(:node, :with_history, :version => 2, :changeset => changeset) + node_v1 = node.old_nodes.find_by(:version => 1) + node_v1.redact!(create(:redaction)) + + get api_changeset_download_path(changeset) + assert_response :success + + assert_select "osmChange", 1 + # this changeset contains the node in versions 1 & 2, but 1 should + # be hidden. + assert_select "osmChange node[id='#{node.id}']", 1 + assert_select "osmChange node[id='#{node.id}'][version='1']", 0 + end + end + end +end diff --git a/test/controllers/api/changesets_controller_test.rb b/test/controllers/api/changesets_controller_test.rb index 1da53a704..87deb3bdf 100644 --- a/test/controllers/api/changesets_controller_test.rb +++ b/test/controllers/api/changesets_controller_test.rb @@ -33,10 +33,6 @@ module Api { :path => "/api/0.6/changeset/1/upload", :method => :post }, { :controller => "api/changesets", :action => "upload", :id => "1" } ) - assert_routing( - { :path => "/api/0.6/changeset/1/download", :method => :get }, - { :controller => "api/changesets", :action => "download", :id => "1" } - ) assert_routing( { :path => "/api/0.6/changeset/1/subscribe", :method => :post }, { :controller => "api/changesets", :action => "subscribe", :id => "1" } @@ -2184,7 +2180,7 @@ module Api assert_response :success, "can't upload multiple versions of an element in a diff: #{@response.body}" - get changeset_download_path(changeset_id) + get api_changeset_download_path(changeset_id) assert_response :success assert_select "osmChange", 1 @@ -2242,7 +2238,7 @@ module Api assert_response :success, "can't upload a diff from JOSM: #{@response.body}" - get changeset_download_path(changeset_id) + get api_changeset_download_path(changeset_id) assert_response :success assert_select "osmChange", 1 @@ -2297,7 +2293,7 @@ module Api assert_response :success, "can't upload multiple versions of an element in a diff: #{@response.body}" - get changeset_download_path(changeset_id) + get api_changeset_download_path(changeset_id) assert_response :success assert_select "osmChange", 1 @@ -2310,63 +2306,6 @@ module Api assert_select "osmChange>modify>way", 1 end - def test_changeset_download - changeset = create(:changeset) - node = create(:node, :with_history, :version => 1, :changeset => changeset) - tag = create(:old_node_tag, :old_node => node.old_nodes.find_by(:version => 1)) - node2 = create(:node, :with_history, :version => 1, :changeset => changeset) - _node3 = create(:node, :with_history, :deleted, :version => 1, :changeset => changeset) - _relation = create(:relation, :with_history, :version => 1, :changeset => changeset) - _relation2 = create(:relation, :with_history, :deleted, :version => 1, :changeset => changeset) - - get changeset_download_path(changeset) - - assert_response :success - - # FIXME: needs more assert_select tests - assert_select "osmChange[version='#{Settings.api_version}'][generator='#{Settings.generator}']" do - assert_select "create", :count => 5 - assert_select "create>node[id='#{node.id}'][visible='#{node.visible?}'][version='#{node.version}']" do - assert_select "tag[k='#{tag.k}'][v='#{tag.v}']" - end - assert_select "create>node[id='#{node2.id}']" - end - end - - test "sorts downloaded elements by timestamp" do - changeset = create(:changeset) - node1 = create(:old_node, :version => 2, :timestamp => "2020-02-01", :changeset => changeset) - node0 = create(:old_node, :version => 2, :timestamp => "2020-01-01", :changeset => changeset) - - get changeset_download_path(changeset) - assert_response :success - assert_dom "modify", :count => 2 do |modify| - assert_dom modify[0], ">node", :count => 1 do |node| - assert_dom node, ">@id", node0.node_id.to_s - end - assert_dom modify[1], ">node", :count => 1 do |node| - assert_dom node, ">@id", node1.node_id.to_s - end - end - end - - test "sorts downloaded elements by version" do - changeset = create(:changeset) - node1 = create(:old_node, :version => 3, :timestamp => "2020-01-01", :changeset => changeset) - node0 = create(:old_node, :version => 2, :timestamp => "2020-01-01", :changeset => changeset) - - get changeset_download_path(changeset) - assert_response :success - assert_dom "modify", :count => 2 do |modify| - assert_dom modify[0], ">node", :count => 1 do |node| - assert_dom node, ">@id", node0.node_id.to_s - end - assert_dom modify[1], ">node", :count => 1 do |node| - assert_dom node, ">@id", node1.node_id.to_s - end - end - end - ## # check that the bounding box of a changeset gets updated correctly # FIXME: This should really be moded to a integration test due to the with_controller @@ -2564,25 +2503,6 @@ module Api "element limit.") end - ## - # check that the changeset download for a changeset with a redacted - # element in it doesn't contain that element. - def test_diff_download_redacted - changeset = create(:changeset) - node = create(:node, :with_history, :version => 2, :changeset => changeset) - node_v1 = node.old_nodes.find_by(:version => 1) - node_v1.redact!(create(:redaction)) - - get changeset_download_path(changeset) - assert_response :success - - assert_select "osmChange", 1 - # this changeset contains the node in versions 1 & 2, but 1 should - # be hidden. - assert_select "osmChange node[id='#{node.id}']", 1 - assert_select "osmChange node[id='#{node.id}'][version='1']", 0 - end - ## # test subscribe success def test_subscribe_success -- 2.39.5