Create api changeset download resource

diff --git a/app/abilities/api_ability.rb b/app/abilities/api_ability.rb
index edf051faeb8f324b1a1767bbafeff07149dd222b..9a7bf254a78fa038c5e0fc2c09e6d10b40b7d867 100644 (file)
--- 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, :feed, :search], Note
       can :create, Note unless user
 

-      can [:read, :download], Changeset
+      can :read, Changeset

       can :read, ChangesetComment
       can :read, Tracepoint
       can :read, User
       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 (file)
index 0000000..99f0182
--- /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 3df7b75cea752aeb11a0681b728377d6f6a0d0e0..517cff47326d28853cacd96416c718c8b5d81b69 100644 (file)
--- a/app/controllers/api/changesets_controller.rb
+++ b/app/controllers/api/changesets_controller.rb
@@ -130,58 +130,6 @@ module Api
       end
     end
 
       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.
     ##
     # 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 1e400cd9feceef34b4d857e31fa0a6d6497418c9..88e05b587c4c08600cabef92efdf8770169c1453 100644 (file)
--- 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|
 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|
     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|
     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
     end
   end
 end

diff --git a/app/views/changesets/index.atom.builder b/app/views/changesets/index.atom.builder
index 4ba79f797996bb075eaa7c8f6a5386fb6f574a46..6556f12bfe1641f54d79522abfb71510a2d3fb9b 100644 (file)
--- 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 => 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")
                  :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 702e61f9287bbe8e9a8e3311857e308b3251ed53..2de744a9383d4c5bc0f63a1b45b25c6baba65721 100644 (file)
--- a/app/views/changesets/show.html.erb
+++ b/app/views/changesets/show.html.erb
@@ -107,7 +107,7 @@
 <div class='secondary-actions'>
   <%= link_to t(".changesetxml"), api_changeset_path(@changeset) %>
   &middot;
 <div class='secondary-actions'>
   <%= link_to t(".changesetxml"), api_changeset_path(@changeset) %>
   &middot;

-  <%= link_to(t(".osmchangexml"), :controller => "api/changesets", :action => "download") %>
+  <%= link_to t(".osmchangexml"), api_changeset_download_path(@changeset) %>

 </div>
 
 <% if @next_by_user || @prev_by_user %>
 </div>
 
 <% if @next_by_user || @prev_by_user %>

diff --git a/config/routes.rb b/config/routes.rb
index 602d35d7a023279efee6fbc450652305b4d8e42b..bdda8edea55787ceb5a1bb1c591111c824298a95 100644 (file)
--- 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 "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+/
     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]
 
   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
     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 (file)
index 0000000..2a38951
--- /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 1da53a704030b4b1bf0b63c6f48f9e6bf53cb4e7..87deb3bdfcb2c1b064eb8b7be3ed8c4fd5b9836b 100644 (file)
--- 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" }
       )
         { :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" }
       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}"
 
       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
       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}"
 
       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
       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}"
 
       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
       assert_response :success
 
       assert_select "osmChange", 1


@@ -2310,63 +2306,6 @@ module Api
       assert_select "osmChange>modify>way", 1
     end
 
       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
     ##
     # 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
 
                  "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
     ##
     # test subscribe success
     def test_subscribe_success