From a6e614e44e0735160c8d9ca9e3b43fd295e7e514 Mon Sep 17 00:00:00 2001 From: Anton Khorev Date: Sun, 2 Feb 2025 13:38:46 +0300 Subject: [PATCH] Declare api node relations as nested resources --- app/abilities/api_ability.rb | 2 +- .../api/nodes/relations_controller.rb | 25 +++++++ app/controllers/api/relations_controller.rb | 4 - .../api/nodes/relations/index.json.jbuilder | 5 ++ .../api/nodes/relations/index.xml.builder | 5 ++ .../relations_for_node.json.jbuilder | 5 -- .../relations/relations_for_node.xml.builder | 5 -- config/routes.rb | 2 +- .../api/nodes/relations_controller_test.rb | 75 +++++++++++++++++++ .../api/relations_controller_test.rb | 34 --------- 10 files changed, 112 insertions(+), 50 deletions(-) create mode 100644 app/controllers/api/nodes/relations_controller.rb create mode 100644 app/views/api/nodes/relations/index.json.jbuilder create mode 100644 app/views/api/nodes/relations/index.xml.builder delete mode 100644 app/views/api/relations/relations_for_node.json.jbuilder delete mode 100644 app/views/api/relations/relations_for_node.xml.builder create mode 100644 test/controllers/api/nodes/relations_controller_test.rb diff --git a/app/abilities/api_ability.rb b/app/abilities/api_ability.rb index 2b1c664d6..cc1964ffb 100644 --- a/app/abilities/api_ability.rb +++ b/app/abilities/api_ability.rb @@ -16,7 +16,7 @@ class ApiAbility can :read, Tracepoint can :read, User can :read, [Node, Way] - can [:read, :relations_for_node, :relations_for_way, :relations_for_relation], Relation + can [:read, :relations_for_way, :relations_for_relation], Relation can [:history, :read], [OldNode, OldWay, OldRelation] can :read, UserBlock diff --git a/app/controllers/api/nodes/relations_controller.rb b/app/controllers/api/nodes/relations_controller.rb new file mode 100644 index 000000000..0f0409e19 --- /dev/null +++ b/app/controllers/api/nodes/relations_controller.rb @@ -0,0 +1,25 @@ +module Api + module Nodes + class RelationsController < ApiController + authorize_resource + + before_action :set_request_formats + + def index + relation_ids = RelationMember.where(:member_type => "Node", :member_id => params[:node_id]).collect(&:relation_id).uniq + + @relations = [] + + Relation.find(relation_ids).each do |relation| + @relations << relation if relation.visible + end + + # Render the result + respond_to do |format| + format.xml + format.json + end + end + end + end +end diff --git a/app/controllers/api/relations_controller.rb b/app/controllers/api/relations_controller.rb index 006b3e8a6..cc6f7c210 100644 --- a/app/controllers/api/relations_controller.rb +++ b/app/controllers/api/relations_controller.rb @@ -127,10 +127,6 @@ module Api relations_for_object("Way") end - def relations_for_node - relations_for_object("Node") - end - def relations_for_relation relations_for_object("Relation") end diff --git a/app/views/api/nodes/relations/index.json.jbuilder b/app/views/api/nodes/relations/index.json.jbuilder new file mode 100644 index 000000000..1d8b9f2cd --- /dev/null +++ b/app/views/api/nodes/relations/index.json.jbuilder @@ -0,0 +1,5 @@ +json.partial! "api/root_attributes" + +json.elements do + json.array! @relations, :partial => "api/relations/relation", :as => :relation +end diff --git a/app/views/api/nodes/relations/index.xml.builder b/app/views/api/nodes/relations/index.xml.builder new file mode 100644 index 000000000..efe64f159 --- /dev/null +++ b/app/views/api/nodes/relations/index.xml.builder @@ -0,0 +1,5 @@ +xml.instruct! + +xml.osm(OSM::API.new.xml_root_attributes) do |osm| + osm << (render(:partial => "api/relations/relation", :collection => @relations) || "") +end diff --git a/app/views/api/relations/relations_for_node.json.jbuilder b/app/views/api/relations/relations_for_node.json.jbuilder deleted file mode 100644 index 9b0f65bf1..000000000 --- a/app/views/api/relations/relations_for_node.json.jbuilder +++ /dev/null @@ -1,5 +0,0 @@ -json.partial! "api/root_attributes" - -json.elements do - json.array! @relations, :partial => "relation", :as => :relation -end diff --git a/app/views/api/relations/relations_for_node.xml.builder b/app/views/api/relations/relations_for_node.xml.builder deleted file mode 100644 index f39a20bb9..000000000 --- a/app/views/api/relations/relations_for_node.xml.builder +++ /dev/null @@ -1,5 +0,0 @@ -xml.instruct! - -xml.osm(OSM::API.new.xml_root_attributes) do |osm| - osm << (render(@relations) || "") -end diff --git a/config/routes.rb b/config/routes.rb index de5fd5826..95b134f95 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -30,7 +30,6 @@ OpenStreetMap::Application.routes.draw do post "changeset/comment/:id/hide" => "changeset_comments#destroy", :as => :changeset_comment_hide, :id => /\d+/ post "changeset/comment/:id/unhide" => "changeset_comments#restore", :as => :changeset_comment_unhide, :id => /\d+/ - get "node/:id/relations" => "relations#relations_for_node", :as => :node_relations, :id => /\d+/ get "node/:id/history" => "old_nodes#history", :as => :api_node_history, :id => /\d+/ post "node/:id/:version/redact" => "old_nodes#redact", :as => :node_version_redact, :version => /\d+/, :id => /\d+/ get "node/:id/:version" => "old_nodes#show", :as => :api_old_node, :id => /\d+/, :version => /\d+/ @@ -51,6 +50,7 @@ OpenStreetMap::Application.routes.draw do resources :nodes, :path => "node", :id => /\d+/, :only => [:show, :update, :destroy] do scope :module => :nodes do resources :ways, :only => :index + resources :relations, :only => :index end end put "node/create" => "nodes#create", :as => nil diff --git a/test/controllers/api/nodes/relations_controller_test.rb b/test/controllers/api/nodes/relations_controller_test.rb new file mode 100644 index 000000000..168fd7153 --- /dev/null +++ b/test/controllers/api/nodes/relations_controller_test.rb @@ -0,0 +1,75 @@ +require "test_helper" + +module Api + module Nodes + class RelationsControllerTest < ActionDispatch::IntegrationTest + ## + # test all routes which lead to this controller + def test_routes + assert_routing( + { :path => "/api/0.6/node/1/relations", :method => :get }, + { :controller => "api/nodes/relations", :action => "index", :node_id => "1" } + ) + assert_routing( + { :path => "/api/0.6/node/1/relations.json", :method => :get }, + { :controller => "api/nodes/relations", :action => "index", :node_id => "1", :format => "json" } + ) + end + + ## + # check that all relations containing a particular node, and no extra + # relations, are returned. + def test_index + node = create(:node) + # should include relations with that node as a member + relation_with_node = create(:relation_member, :member => node).relation + # should ignore relations without that node as a member + _relation_without_node = create(:relation_member).relation + # should ignore relations with the node involved indirectly, via a way + way = create(:way_node, :node => node).way + _relation_with_way = create(:relation_member, :member => way).relation + # should ignore relations with the node involved indirectly, via a relation + second_relation = create(:relation_member, :member => node).relation + _super_relation = create(:relation_member, :member => second_relation).relation + # should combine multiple relation_member references into just one relation entry + create(:relation_member, :member => node, :relation => relation_with_node) + # should not include deleted relations + deleted_relation = create(:relation, :deleted) + create(:relation_member, :member => node, :relation => deleted_relation) + + get api_node_relations_path(node) + + assert_response :success + + # count one osm element + assert_select "osm[version='#{Settings.api_version}'][generator='#{Settings.generator}']", 1 + + # we should have only the expected number of relations + expected_relations = [relation_with_node, second_relation] + assert_select "osm>relation", expected_relations.size + + # and each of them should contain the element we originally searched for + expected_relations.each do |containing_relation| + # The relation should appear once, but the element could appear multiple times + assert_select "osm>relation[id='#{containing_relation.id}']", 1 + assert_select "osm>relation[id='#{containing_relation.id}']>member[type='node'][ref='#{node.id}']" + end + end + + def test_index_json + node = create(:node) + containing_relation = create(:relation_member, :member => node).relation + + get api_node_relations_path(node, :format => "json") + + assert_response :success + js = ActiveSupport::JSON.decode(@response.body) + assert_not_nil js + assert_equal 1, js["elements"].count + js_relations = js["elements"].filter { |e| e["type"] == "relation" } + assert_equal 1, js_relations.count + assert_equal containing_relation.id, js_relations[0]["id"] + end + end + end +end diff --git a/test/controllers/api/relations_controller_test.rb b/test/controllers/api/relations_controller_test.rb index 803a57acf..4dd38fc3f 100644 --- a/test/controllers/api/relations_controller_test.rb +++ b/test/controllers/api/relations_controller_test.rb @@ -42,10 +42,6 @@ module Api { :controller => "api/relations", :action => "destroy", :id => "1" } ) - assert_routing( - { :path => "/api/0.6/node/1/relations", :method => :get }, - { :controller => "api/relations", :action => "relations_for_node", :id => "1" } - ) assert_routing( { :path => "/api/0.6/way/1/relations", :method => :get }, { :controller => "api/relations", :action => "relations_for_way", :id => "1" } @@ -54,10 +50,6 @@ module Api { :path => "/api/0.6/relation/1/relations", :method => :get }, { :controller => "api/relations", :action => "relations_for_relation", :id => "1" } ) - assert_routing( - { :path => "/api/0.6/node/1/relations.json", :method => :get }, - { :controller => "api/relations", :action => "relations_for_node", :id => "1", :format => "json" } - ) assert_routing( { :path => "/api/0.6/way/1/relations.json", :method => :get }, { :controller => "api/relations", :action => "relations_for_way", :id => "1", :format => "json" } @@ -228,32 +220,6 @@ module Api assert_equal node.id, js_nodes[0]["id"] end - ## - # check that all relations containing a particular node, and no extra - # relations, are returned from the relations_for_node call. - def test_relations_for_node - node = create(:node) - # should include relations with that node as a member - relation_with_node = create(:relation_member, :member => node).relation - # should ignore relations without that node as a member - _relation_without_node = create(:relation_member).relation - # should ignore relations with the node involved indirectly, via a way - way = create(:way_node, :node => node).way - _relation_with_way = create(:relation_member, :member => way).relation - # should ignore relations with the node involved indirectly, via a relation - second_relation = create(:relation_member, :member => node).relation - _super_relation = create(:relation_member, :member => second_relation).relation - # should combine multiple relation_member references into just one relation entry - create(:relation_member, :member => node, :relation => relation_with_node) - # should not include deleted relations - deleted_relation = create(:relation, :deleted) - create(:relation_member, :member => node, :relation => deleted_relation) - - check_relations_for_element(node_relations_path(node), "node", - node.id, - [relation_with_node, second_relation]) - end - def test_relations_for_way way = create(:way) # should include relations with that way as a member -- 2.39.5