From 75a49786f8bcb76715cf4009ba807a844baae282 Mon Sep 17 00:00:00 2001 From: mmd-osm Date: Sat, 28 Dec 2019 09:54:37 +0100 Subject: [PATCH 01/10] API JSON output --- .rubocop_todo.yml | 3 ++ app/controllers/api/map_controller.rb | 12 ++++- app/controllers/api/nodes_controller.rb | 17 +++++- app/controllers/api/old_controller.rb | 17 +++++- app/controllers/api/relations_controller.rb | 27 ++++++++-- app/controllers/api/ways_controller.rb | 27 ++++++++-- app/views/api/map/_bounds.json.jbuilder | 6 +++ .../api/map/_root_attributes.json.jbuilder | 5 ++ app/views/api/map/index.json.jbuilder | 9 ++++ app/views/api/nodes/_node.json.jbuilder | 15 ++++++ app/views/api/nodes/index.json.jbuilder | 5 ++ app/views/api/nodes/show.json.jbuilder | 5 ++ .../api/old_nodes/_old_node.json.jbuilder | 15 ++++++ app/views/api/old_nodes/history.json.jbuilder | 5 ++ app/views/api/old_nodes/version.json.jbuilder | 5 ++ .../old_relations/_old_relation.json.jbuilder | 19 +++++++ .../api/old_relations/history.json.jbuilder | 5 ++ .../api/old_relations/version.json.jbuilder | 5 ++ app/views/api/old_ways/_old_way.json.jbuilder | 13 +++++ app/views/api/old_ways/history.json.jbuilder | 5 ++ app/views/api/old_ways/version.json.jbuilder | 5 ++ .../api/relations/_relation.json.jbuilder | 19 +++++++ app/views/api/relations/full.json.jbuilder | 7 +++ app/views/api/relations/index.json.jbuilder | 5 ++ .../relations_for_node.json.jbuilder | 5 ++ .../relations_for_relation.json.jbuilder | 5 ++ .../relations/relations_for_way.json.jbuilder | 5 ++ app/views/api/relations/show.json.jbuilder | 5 ++ app/views/api/ways/_way.json.jbuilder | 13 +++++ app/views/api/ways/full.json.jbuilder | 7 +++ app/views/api/ways/index.json.jbuilder | 5 ++ app/views/api/ways/show.json.jbuilder | 5 ++ .../api/ways/ways_for_node.json.jbuilder | 5 ++ test/controllers/api/map_controller_test.rb | 52 +++++++++++++++++++ 34 files changed, 350 insertions(+), 13 deletions(-) create mode 100644 app/views/api/map/_bounds.json.jbuilder create mode 100644 app/views/api/map/_root_attributes.json.jbuilder create mode 100644 app/views/api/map/index.json.jbuilder create mode 100644 app/views/api/nodes/_node.json.jbuilder create mode 100644 app/views/api/nodes/index.json.jbuilder create mode 100644 app/views/api/nodes/show.json.jbuilder create mode 100644 app/views/api/old_nodes/_old_node.json.jbuilder create mode 100644 app/views/api/old_nodes/history.json.jbuilder create mode 100644 app/views/api/old_nodes/version.json.jbuilder create mode 100644 app/views/api/old_relations/_old_relation.json.jbuilder create mode 100644 app/views/api/old_relations/history.json.jbuilder create mode 100644 app/views/api/old_relations/version.json.jbuilder create mode 100644 app/views/api/old_ways/_old_way.json.jbuilder create mode 100644 app/views/api/old_ways/history.json.jbuilder create mode 100644 app/views/api/old_ways/version.json.jbuilder create mode 100644 app/views/api/relations/_relation.json.jbuilder create mode 100644 app/views/api/relations/full.json.jbuilder create mode 100644 app/views/api/relations/index.json.jbuilder create mode 100644 app/views/api/relations/relations_for_node.json.jbuilder create mode 100644 app/views/api/relations/relations_for_relation.json.jbuilder create mode 100644 app/views/api/relations/relations_for_way.json.jbuilder create mode 100644 app/views/api/relations/show.json.jbuilder create mode 100644 app/views/api/ways/_way.json.jbuilder create mode 100644 app/views/api/ways/full.json.jbuilder create mode 100644 app/views/api/ways/index.json.jbuilder create mode 100644 app/views/api/ways/show.json.jbuilder create mode 100644 app/views/api/ways/ways_for_node.json.jbuilder diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 06a9c76f0..080b7b3a5 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -199,6 +199,9 @@ Style/FormatStringToken: - 'test/controllers/api/relations_controller_test.rb' - 'test/controllers/api/ways_controller_test.rb' - 'test/lib/bounding_box_test.rb' + - 'app/views/api/map/_bounds.json.jbuilder' + - 'app/views/api/nodes/_node.json.jbuilder' + - 'app/views/api/old_nodes/_old_node.json.jbuilder' # Offense count: 539 # Cop supports --auto-correct. diff --git a/app/controllers/api/map_controller.rb b/app/controllers/api/map_controller.rb index b0998f7eb..34257fc99 100644 --- a/app/controllers/api/map_controller.rb +++ b/app/controllers/api/map_controller.rb @@ -5,6 +5,13 @@ module Api before_action :check_api_readable around_action :api_call_handle_error, :api_call_timeout + before_action :default_format_xml + + # Set format to xml unless client requires a specific format + def default_format_xml + request.format = "xml" unless params[:format] + end + # This is probably the most common call of all. It is used for getting the # OSM data for a specified bounding box, usually for editing. First the # bounding box (bbox) is checked to make sure that it is sane. All nodes @@ -90,7 +97,10 @@ module Api response.headers["Content-Disposition"] = "attachment; filename=\"map.osm\"" # Render the result - render :formats => [:xml] + respond_to do |format| + format.xml + format.json + end end end end diff --git a/app/controllers/api/nodes_controller.rb b/app/controllers/api/nodes_controller.rb index 2962ce070..1827603a7 100644 --- a/app/controllers/api/nodes_controller.rb +++ b/app/controllers/api/nodes_controller.rb @@ -13,6 +13,13 @@ module Api before_action :check_api_readable, :except => [:create, :update, :delete] around_action :api_call_handle_error, :api_call_timeout + before_action :default_format_xml + + # Set format to xml unless client requires a specific format + def default_format_xml + request.format = "xml" unless params[:format] + end + # Create a node from XML. def create assert_method :put @@ -32,7 +39,10 @@ module Api if @node.visible # Render the result - render :formats => [:xml] + respond_to do |format| + format.xml + format.json + end else head :gone end @@ -73,7 +83,10 @@ module Api @nodes = Node.find(ids) # Render the result - render :formats => [:xml] + respond_to do |format| + format.xml + format.json + end end end end diff --git a/app/controllers/api/old_controller.rb b/app/controllers/api/old_controller.rb index 862d14716..92c6d9295 100644 --- a/app/controllers/api/old_controller.rb +++ b/app/controllers/api/old_controller.rb @@ -16,6 +16,13 @@ module Api before_action :lookup_old_element, :except => [:history] before_action :lookup_old_element_versions, :only => [:history] + before_action :default_format_xml + + # Set format to xml unless client requires a specific format + def default_format_xml + request.format = "xml" unless params[:format] + end + def history # the .where() method used in the lookup_old_element_versions # call won't throw an error if no records are found, so we have @@ -30,7 +37,10 @@ module Api end # Render the result - render :formats => [:xml] + respond_to do |format| + format.xml + format.json + end end def version @@ -41,7 +51,10 @@ module Api response.last_modified = @old_element.timestamp # Render the result - render :formats => [:xml] + respond_to do |format| + format.xml + format.json + end end end diff --git a/app/controllers/api/relations_controller.rb b/app/controllers/api/relations_controller.rb index ba0dd0c6b..c33840978 100644 --- a/app/controllers/api/relations_controller.rb +++ b/app/controllers/api/relations_controller.rb @@ -11,6 +11,13 @@ module Api before_action :check_api_readable, :except => [:create, :update, :delete] around_action :api_call_handle_error, :api_call_timeout + before_action :default_format_xml + + # Set format to xml unless client requires a specific format + def default_format_xml + request.format = "xml" unless params[:format] + end + def create assert_method :put @@ -26,7 +33,10 @@ module Api response.last_modified = @relation.timestamp if @relation.visible # Render the result - render :formats => [:xml] + respond_to do |format| + format.xml + format.json + end else head :gone end @@ -117,7 +127,10 @@ module Api @relations << relation # Render the result - render :formats => [:xml] + respond_to do |format| + format.xml + format.json + end else head :gone end @@ -133,7 +146,10 @@ module Api @relations = Relation.find(ids) # Render the result - render :formats => [:xml] + respond_to do |format| + format.xml + format.json + end end def relations_for_way @@ -160,7 +176,10 @@ module Api end # Render the result - render :formats => [:xml] + respond_to do |format| + format.xml + format.json + end end end end diff --git a/app/controllers/api/ways_controller.rb b/app/controllers/api/ways_controller.rb index 9af087d83..a721f82f7 100644 --- a/app/controllers/api/ways_controller.rb +++ b/app/controllers/api/ways_controller.rb @@ -11,6 +11,13 @@ module Api before_action :check_api_readable, :except => [:create, :update, :delete] around_action :api_call_handle_error, :api_call_timeout + before_action :default_format_xml + + # Set format to xml unless client requires a specific format + def default_format_xml + request.format = "xml" unless params[:format] + end + def create assert_method :put @@ -28,7 +35,10 @@ module Api if @way.visible # Render the result - render :formats => [:xml] + respond_to do |format| + format.xml + format.json + end else head :gone end @@ -75,7 +85,10 @@ module Api end # Render the result - render :formats => [:xml] + respond_to do |format| + format.xml + format.json + end else head :gone end @@ -93,7 +106,10 @@ module Api @ways = Way.find(ids) # Render the result - render :formats => [:xml] + respond_to do |format| + format.xml + format.json + end end ## @@ -106,7 +122,10 @@ module Api @ways = Way.where(:id => wayids, :visible => true) # Render the result - render :formats => [:xml] + respond_to do |format| + format.xml + format.json + end end end end diff --git a/app/views/api/map/_bounds.json.jbuilder b/app/views/api/map/_bounds.json.jbuilder new file mode 100644 index 000000000..98755900e --- /dev/null +++ b/app/views/api/map/_bounds.json.jbuilder @@ -0,0 +1,6 @@ +json.bounds do + json.minlat format("%.7f", @bounds.min_lat) + json.minlon format("%.7f", @bounds.min_lon) + json.maxlat format("%.7f", @bounds.max_lat) + json.maxlon format("%.7f", @bounds.max_lon) +end diff --git a/app/views/api/map/_root_attributes.json.jbuilder b/app/views/api/map/_root_attributes.json.jbuilder new file mode 100644 index 000000000..d8fbef9fd --- /dev/null +++ b/app/views/api/map/_root_attributes.json.jbuilder @@ -0,0 +1,5 @@ +json.version Settings.api_version +json.generator Settings.generator +json.copyright Settings.copyright_owner +json.attribution Settings.attribution_url +json.license Settings.license_url diff --git a/app/views/api/map/index.json.jbuilder b/app/views/api/map/index.json.jbuilder new file mode 100644 index 000000000..7cc983aef --- /dev/null +++ b/app/views/api/map/index.json.jbuilder @@ -0,0 +1,9 @@ +json.partial! "root_attributes" + +json.partial! "bounds" + +all = @nodes + @ways + @relations + +json.elements(all) do |obj| + json.partial! obj +end diff --git a/app/views/api/nodes/_node.json.jbuilder b/app/views/api/nodes/_node.json.jbuilder new file mode 100644 index 000000000..12cf2fb95 --- /dev/null +++ b/app/views/api/nodes/_node.json.jbuilder @@ -0,0 +1,15 @@ +json.type "node" +json.id node.id +if node.visible + json.lat format("%.7f", node.lat.to_f) + json.lon format("%.7f", node.lon.to_f) +end +json.timestamp node.timestamp.xmlschema +json.version node.version +json.changeset node.changeset_id +json.user node.changeset.user.display_name +json.uid node.changeset.user_id + +json.visible node.visible unless node.visible + +json.tags node.tags unless node.tags.empty? diff --git a/app/views/api/nodes/index.json.jbuilder b/app/views/api/nodes/index.json.jbuilder new file mode 100644 index 000000000..f42f7563b --- /dev/null +++ b/app/views/api/nodes/index.json.jbuilder @@ -0,0 +1,5 @@ +json.partial! "api/map/root_attributes" + +json.elements(@nodes) do |node| + json.partial! node +end diff --git a/app/views/api/nodes/show.json.jbuilder b/app/views/api/nodes/show.json.jbuilder new file mode 100644 index 000000000..c87777a87 --- /dev/null +++ b/app/views/api/nodes/show.json.jbuilder @@ -0,0 +1,5 @@ +json.partial! "api/map/root_attributes" + +json.elements([@node]) do |node| + json.partial! node +end diff --git a/app/views/api/old_nodes/_old_node.json.jbuilder b/app/views/api/old_nodes/_old_node.json.jbuilder new file mode 100644 index 000000000..5042c84a7 --- /dev/null +++ b/app/views/api/old_nodes/_old_node.json.jbuilder @@ -0,0 +1,15 @@ +json.type "node" +json.id old_node.node_id +if old_node.visible + json.lat format("%.7f", old_node.lat.to_f) + json.lon format("%.7f", old_node.lon.to_f) +end +json.timestamp old_node.timestamp.xmlschema +json.version old_node.version +json.changeset old_node.changeset_id +json.user old_node.changeset.user.display_name +json.uid old_node.changeset.user_id + +json.visible old_node.visible unless old_node.visible + +json.tags old_node.tags unless old_node.tags.empty? diff --git a/app/views/api/old_nodes/history.json.jbuilder b/app/views/api/old_nodes/history.json.jbuilder new file mode 100644 index 000000000..b0e97e832 --- /dev/null +++ b/app/views/api/old_nodes/history.json.jbuilder @@ -0,0 +1,5 @@ +json.partial! "api/map/root_attributes" + +json.elements(@elems) do |old_node| + json.partial! old_node +end diff --git a/app/views/api/old_nodes/version.json.jbuilder b/app/views/api/old_nodes/version.json.jbuilder new file mode 100644 index 000000000..992230bc0 --- /dev/null +++ b/app/views/api/old_nodes/version.json.jbuilder @@ -0,0 +1,5 @@ +json.partial! "api/map/root_attributes" + +json.elements([@old_element]) do |old_node| + json.partial! old_node +end diff --git a/app/views/api/old_relations/_old_relation.json.jbuilder b/app/views/api/old_relations/_old_relation.json.jbuilder new file mode 100644 index 000000000..c52ca20de --- /dev/null +++ b/app/views/api/old_relations/_old_relation.json.jbuilder @@ -0,0 +1,19 @@ +json.type "relation" +json.id old_relation.relation_id +json.timestamp old_relation.timestamp.xmlschema +json.version old_relation.version +json.changeset old_relation.changeset_id +json.user old_relation.changeset.user.display_name +json.uid old_relation.changeset.user_id + +json.visible old_relation.visible unless old_relation.visible + +unless old_relation.relation_members.empty? + json.members(old_relation.relation_members) do |m| + json.type m.member_type.downcase + json.ref m.member_id + json.role m.member_role + end +end + +json.tags old_relation.tags unless old_relation.tags.empty? diff --git a/app/views/api/old_relations/history.json.jbuilder b/app/views/api/old_relations/history.json.jbuilder new file mode 100644 index 000000000..c5dacae0a --- /dev/null +++ b/app/views/api/old_relations/history.json.jbuilder @@ -0,0 +1,5 @@ +json.partial! "api/map/root_attributes" + +json.elements(@elems) do |old_relation| + json.partial! old_relation +end diff --git a/app/views/api/old_relations/version.json.jbuilder b/app/views/api/old_relations/version.json.jbuilder new file mode 100644 index 000000000..b4018429b --- /dev/null +++ b/app/views/api/old_relations/version.json.jbuilder @@ -0,0 +1,5 @@ +json.partial! "api/map/root_attributes" + +json.elements([@old_element]) do |old_relation| + json.partial! old_relation +end diff --git a/app/views/api/old_ways/_old_way.json.jbuilder b/app/views/api/old_ways/_old_way.json.jbuilder new file mode 100644 index 000000000..b2e79f879 --- /dev/null +++ b/app/views/api/old_ways/_old_way.json.jbuilder @@ -0,0 +1,13 @@ +json.type "way" +json.id old_way.way_id +json.timestamp old_way.timestamp.xmlschema +json.version old_way.version +json.changeset old_way.changeset_id +json.user old_way.changeset.user.display_name +json.uid old_way.changeset.user_id + +json.visible old_way.visible unless old_way.visible + +json.nodes old_way.nds unless old_way.nds.empty? + +json.tags old_way.tags unless old_way.tags.empty? diff --git a/app/views/api/old_ways/history.json.jbuilder b/app/views/api/old_ways/history.json.jbuilder new file mode 100644 index 000000000..b231d7de1 --- /dev/null +++ b/app/views/api/old_ways/history.json.jbuilder @@ -0,0 +1,5 @@ +json.partial! "api/map/root_attributes" + +json.elements(@elems) do |old_way| + json.partial! old_way +end diff --git a/app/views/api/old_ways/version.json.jbuilder b/app/views/api/old_ways/version.json.jbuilder new file mode 100644 index 000000000..bb261c1c5 --- /dev/null +++ b/app/views/api/old_ways/version.json.jbuilder @@ -0,0 +1,5 @@ +json.partial! "api/map/root_attributes" + +json.elements([@old_element]) do |old_way| + json.partial! old_way +end diff --git a/app/views/api/relations/_relation.json.jbuilder b/app/views/api/relations/_relation.json.jbuilder new file mode 100644 index 000000000..52263ce37 --- /dev/null +++ b/app/views/api/relations/_relation.json.jbuilder @@ -0,0 +1,19 @@ +json.type "relation" +json.id relation.id +json.timestamp relation.timestamp.xmlschema +json.version relation.version +json.changeset relation.changeset_id +json.user relation.changeset.user.display_name +json.uid relation.changeset.user_id + +json.visible relation.visible unless relation.visible + +unless relation.relation_members.empty? + json.members(relation.relation_members) do |m| + json.type m.member_type.downcase + json.ref m.member_id + json.role m.member_role + end +end + +json.tags relation.tags unless relation.tags.empty? diff --git a/app/views/api/relations/full.json.jbuilder b/app/views/api/relations/full.json.jbuilder new file mode 100644 index 000000000..268de01ca --- /dev/null +++ b/app/views/api/relations/full.json.jbuilder @@ -0,0 +1,7 @@ +json.partial! "api/map/root_attributes" + +all = @nodes + @ways + @relations + +json.elements(all) do |obj| + json.partial! obj +end diff --git a/app/views/api/relations/index.json.jbuilder b/app/views/api/relations/index.json.jbuilder new file mode 100644 index 000000000..25c54fc6f --- /dev/null +++ b/app/views/api/relations/index.json.jbuilder @@ -0,0 +1,5 @@ +json.partial! "api/map/root_attributes" + +json.elements(@relations) do |relation| + json.partial! relation +end diff --git a/app/views/api/relations/relations_for_node.json.jbuilder b/app/views/api/relations/relations_for_node.json.jbuilder new file mode 100644 index 000000000..25c54fc6f --- /dev/null +++ b/app/views/api/relations/relations_for_node.json.jbuilder @@ -0,0 +1,5 @@ +json.partial! "api/map/root_attributes" + +json.elements(@relations) do |relation| + json.partial! relation +end diff --git a/app/views/api/relations/relations_for_relation.json.jbuilder b/app/views/api/relations/relations_for_relation.json.jbuilder new file mode 100644 index 000000000..25c54fc6f --- /dev/null +++ b/app/views/api/relations/relations_for_relation.json.jbuilder @@ -0,0 +1,5 @@ +json.partial! "api/map/root_attributes" + +json.elements(@relations) do |relation| + json.partial! relation +end diff --git a/app/views/api/relations/relations_for_way.json.jbuilder b/app/views/api/relations/relations_for_way.json.jbuilder new file mode 100644 index 000000000..25c54fc6f --- /dev/null +++ b/app/views/api/relations/relations_for_way.json.jbuilder @@ -0,0 +1,5 @@ +json.partial! "api/map/root_attributes" + +json.elements(@relations) do |relation| + json.partial! relation +end diff --git a/app/views/api/relations/show.json.jbuilder b/app/views/api/relations/show.json.jbuilder new file mode 100644 index 000000000..682ef0a24 --- /dev/null +++ b/app/views/api/relations/show.json.jbuilder @@ -0,0 +1,5 @@ +json.partial! "api/map/root_attributes" + +json.elements([@relation]) do |relation| + json.partial! relation +end diff --git a/app/views/api/ways/_way.json.jbuilder b/app/views/api/ways/_way.json.jbuilder new file mode 100644 index 000000000..11e796bc8 --- /dev/null +++ b/app/views/api/ways/_way.json.jbuilder @@ -0,0 +1,13 @@ +json.type "way" +json.id way.id +json.timestamp way.timestamp.xmlschema +json.version way.version +json.changeset way.changeset_id +json.user way.changeset.user.display_name +json.uid way.changeset.user_id + +json.visible way.visible unless way.visible + +json.nodes way.nodes.ids unless way.nodes.ids.empty? + +json.tags way.tags unless way.tags.empty? diff --git a/app/views/api/ways/full.json.jbuilder b/app/views/api/ways/full.json.jbuilder new file mode 100644 index 000000000..9bd234fbe --- /dev/null +++ b/app/views/api/ways/full.json.jbuilder @@ -0,0 +1,7 @@ +json.partial! "api/map/root_attributes" + +all = @nodes + [@way] + +json.elements(all) do |obj| + json.partial! obj +end diff --git a/app/views/api/ways/index.json.jbuilder b/app/views/api/ways/index.json.jbuilder new file mode 100644 index 000000000..e1dbd4347 --- /dev/null +++ b/app/views/api/ways/index.json.jbuilder @@ -0,0 +1,5 @@ +json.partial! "api/map/root_attributes" + +json.elements(@ways) do |way| + json.partial! way +end diff --git a/app/views/api/ways/show.json.jbuilder b/app/views/api/ways/show.json.jbuilder new file mode 100644 index 000000000..e5cd31fed --- /dev/null +++ b/app/views/api/ways/show.json.jbuilder @@ -0,0 +1,5 @@ +json.partial! "api/map/root_attributes" + +json.elements([@way]) do |way| + json.partial! way +end diff --git a/app/views/api/ways/ways_for_node.json.jbuilder b/app/views/api/ways/ways_for_node.json.jbuilder new file mode 100644 index 000000000..e1dbd4347 --- /dev/null +++ b/app/views/api/ways/ways_for_node.json.jbuilder @@ -0,0 +1,5 @@ +json.partial! "api/map/root_attributes" + +json.elements(@ways) do |way| + json.partial! way +end diff --git a/test/controllers/api/map_controller_test.rb b/test/controllers/api/map_controller_test.rb index db83aaa9f..5df2dc6a3 100644 --- a/test/controllers/api/map_controller_test.rb +++ b/test/controllers/api/map_controller_test.rb @@ -23,6 +23,10 @@ module Api { :path => "/api/0.6/map", :method => :get }, { :controller => "api/map", :action => "index" } ) + assert_routing( + { :path => "/api/0.6/map.json", :method => :get }, + { :controller => "api/map", :action => "index", :format => "json" } + ) end # ------------------------------------- @@ -63,6 +67,54 @@ module Api end end + def test_map_json + node = create(:node, :lat => 7, :lon => 7) + tag = create(:node_tag, :node => node) + way1 = create(:way_node, :node => node).way + way2 = create(:way_node, :node => node).way + relation = create(:relation_member, :member => node).relation + + # Need to split the min/max lat/lon out into their own variables here + # so that we can test they are returned later. + minlon = node.lon - 0.1 + minlat = node.lat - 0.1 + maxlon = node.lon + 0.1 + maxlat = node.lat + 0.1 + bbox = "#{minlon},#{minlat},#{maxlon},#{maxlat}" + get :index, :params => { :bbox => bbox, :format => "json" } + if $VERBOSE + print @request.to_yaml + print @response.body + end + assert_response :success, "Expected success with the map call" + js = ActiveSupport::JSON.decode(@response.body) + assert_not_nil js + + assert_equal Settings.api_version, js["version"] + assert_equal Settings.generator, js["generator"] + assert_equal "#{format('%.7f', minlon)}", js["bounds"]["minlon"] + assert_equal "#{format('%.7f', minlat)}", js["bounds"]["minlat"] + assert_equal "#{format('%.7f', maxlon)}", js["bounds"]["maxlon"] + assert_equal "#{format('%.7f', maxlat)}", js["bounds"]["maxlat"] + + result_nodes = js["elements"].select { |a| a["type"] == "node" } + .select { |a| a["id"] == node.id } + .select { |a| a["lat"] == "#{format('%.7f', node.lat)}" } + .select { |a| a["lon"] == "#{format('%.7f', node.lon)}" } + .select { |a| a["version"] == node.version } + .select { |a| a["changeset"] == node.changeset_id } + .select { |a| a["timestamp"] == node.timestamp.xmlschema } + assert_equal result_nodes.count, 1 + result_node = result_nodes.first + + assert_equal result_node["tags"], tag.k => tag.v + assert_equal 2, (js["elements"].count { |a| a["type"] == "way" }) + assert_equal 1, (js["elements"].count { |a| a["type"] == "way" && a["id"] == way1.id }) + assert_equal 1, (js["elements"].count { |a| a["type"] == "way" && a["id"] == way2.id }) + assert_equal 1, (js["elements"].count { |a| a["type"] == "relation" }) + assert_equal 1, (js["elements"].count { |a| a["type"] == "relation" && a["id"] == relation.id }) + end + # This differs from the above test in that we are making the bbox exactly # the same as the node we are looking at def test_map_inclusive From 45a1d298a941939d4a5e7da273d9394a16d8c5f1 Mon Sep 17 00:00:00 2001 From: mmd-osm Date: Mon, 30 Dec 2019 17:26:00 +0100 Subject: [PATCH 02/10] Move default_format_xml to api_controller --- app/controllers/api/map_controller.rb | 5 ----- app/controllers/api/nodes_controller.rb | 5 ----- app/controllers/api/old_controller.rb | 5 ----- app/controllers/api/relations_controller.rb | 5 ----- app/controllers/api/ways_controller.rb | 5 ----- app/controllers/api_controller.rb | 5 +++++ 6 files changed, 5 insertions(+), 25 deletions(-) diff --git a/app/controllers/api/map_controller.rb b/app/controllers/api/map_controller.rb index 34257fc99..4b233c995 100644 --- a/app/controllers/api/map_controller.rb +++ b/app/controllers/api/map_controller.rb @@ -7,11 +7,6 @@ module Api before_action :default_format_xml - # Set format to xml unless client requires a specific format - def default_format_xml - request.format = "xml" unless params[:format] - end - # This is probably the most common call of all. It is used for getting the # OSM data for a specified bounding box, usually for editing. First the # bounding box (bbox) is checked to make sure that it is sane. All nodes diff --git a/app/controllers/api/nodes_controller.rb b/app/controllers/api/nodes_controller.rb index 1827603a7..08f19008f 100644 --- a/app/controllers/api/nodes_controller.rb +++ b/app/controllers/api/nodes_controller.rb @@ -15,11 +15,6 @@ module Api before_action :default_format_xml - # Set format to xml unless client requires a specific format - def default_format_xml - request.format = "xml" unless params[:format] - end - # Create a node from XML. def create assert_method :put diff --git a/app/controllers/api/old_controller.rb b/app/controllers/api/old_controller.rb index 92c6d9295..b68fde6b5 100644 --- a/app/controllers/api/old_controller.rb +++ b/app/controllers/api/old_controller.rb @@ -18,11 +18,6 @@ module Api before_action :default_format_xml - # Set format to xml unless client requires a specific format - def default_format_xml - request.format = "xml" unless params[:format] - end - def history # the .where() method used in the lookup_old_element_versions # call won't throw an error if no records are found, so we have diff --git a/app/controllers/api/relations_controller.rb b/app/controllers/api/relations_controller.rb index c33840978..566efe0eb 100644 --- a/app/controllers/api/relations_controller.rb +++ b/app/controllers/api/relations_controller.rb @@ -13,11 +13,6 @@ module Api before_action :default_format_xml - # Set format to xml unless client requires a specific format - def default_format_xml - request.format = "xml" unless params[:format] - end - def create assert_method :put diff --git a/app/controllers/api/ways_controller.rb b/app/controllers/api/ways_controller.rb index a721f82f7..95f36df53 100644 --- a/app/controllers/api/ways_controller.rb +++ b/app/controllers/api/ways_controller.rb @@ -13,11 +13,6 @@ module Api before_action :default_format_xml - # Set format to xml unless client requires a specific format - def default_format_xml - request.format = "xml" unless params[:format] - end - def create assert_method :put diff --git a/app/controllers/api_controller.rb b/app/controllers/api_controller.rb index 44efdc071..ca09e5807 100644 --- a/app/controllers/api_controller.rb +++ b/app/controllers/api_controller.rb @@ -3,6 +3,11 @@ class ApiController < ApplicationController private + # Set format to xml unless client requires a specific format + def default_format_xml + request.format = "xml" unless params[:format] + end + def authorize(realm = "Web Password", errormessage = "Couldn't authenticate you") # make the current_user object from any auth sources we have setup_user_auth From b54333fa6ddf9f6ede89e4da345a2ae93703ee7d Mon Sep 17 00:00:00 2001 From: mmd-osm Date: Thu, 2 Jan 2020 14:37:27 +0100 Subject: [PATCH 03/10] Unify lat/lon formatting for json output --- app/models/concerns/geo_record.rb | 4 ++++ app/views/api/map/_bounds.json.jbuilder | 8 ++++---- app/views/api/nodes/_node.json.jbuilder | 4 ++-- app/views/api/old_nodes/_old_node.json.jbuilder | 4 ++-- test/controllers/api/map_controller_test.rb | 12 ++++++------ 5 files changed, 18 insertions(+), 14 deletions(-) diff --git a/app/models/concerns/geo_record.rb b/app/models/concerns/geo_record.rb index 91533ece4..447ee19df 100644 --- a/app/models/concerns/geo_record.rb +++ b/app/models/concerns/geo_record.rb @@ -12,6 +12,10 @@ module GeoRecord def to_s format("%.7f", self) end + + def as_json(_) + format("%.7f", self).to_f + end end # This scaling factor is used to convert between the float lat/lon that is diff --git a/app/views/api/map/_bounds.json.jbuilder b/app/views/api/map/_bounds.json.jbuilder index 98755900e..16fdbeb0f 100644 --- a/app/views/api/map/_bounds.json.jbuilder +++ b/app/views/api/map/_bounds.json.jbuilder @@ -1,6 +1,6 @@ json.bounds do - json.minlat format("%.7f", @bounds.min_lat) - json.minlon format("%.7f", @bounds.min_lon) - json.maxlat format("%.7f", @bounds.max_lat) - json.maxlon format("%.7f", @bounds.max_lon) + json.minlat GeoRecord::Coord.new(@bounds.min_lat) + json.minlon GeoRecord::Coord.new(@bounds.min_lon) + json.maxlat GeoRecord::Coord.new(@bounds.max_lat) + json.maxlon GeoRecord::Coord.new(@bounds.max_lon) end diff --git a/app/views/api/nodes/_node.json.jbuilder b/app/views/api/nodes/_node.json.jbuilder index 12cf2fb95..e48d5f17c 100644 --- a/app/views/api/nodes/_node.json.jbuilder +++ b/app/views/api/nodes/_node.json.jbuilder @@ -1,8 +1,8 @@ json.type "node" json.id node.id if node.visible - json.lat format("%.7f", node.lat.to_f) - json.lon format("%.7f", node.lon.to_f) + json.lat GeoRecord::Coord.new(node.lat) + json.lon GeoRecord::Coord.new(node.lon) end json.timestamp node.timestamp.xmlschema json.version node.version diff --git a/app/views/api/old_nodes/_old_node.json.jbuilder b/app/views/api/old_nodes/_old_node.json.jbuilder index 5042c84a7..211d50332 100644 --- a/app/views/api/old_nodes/_old_node.json.jbuilder +++ b/app/views/api/old_nodes/_old_node.json.jbuilder @@ -1,8 +1,8 @@ json.type "node" json.id old_node.node_id if old_node.visible - json.lat format("%.7f", old_node.lat.to_f) - json.lon format("%.7f", old_node.lon.to_f) + json.lat GeoRecord::Coord.new(old_node.lat) + json.lon GeoRecord::Coord.new(old_node.lon) end json.timestamp old_node.timestamp.xmlschema json.version old_node.version diff --git a/test/controllers/api/map_controller_test.rb b/test/controllers/api/map_controller_test.rb index 5df2dc6a3..54461868b 100644 --- a/test/controllers/api/map_controller_test.rb +++ b/test/controllers/api/map_controller_test.rb @@ -92,15 +92,15 @@ module Api assert_equal Settings.api_version, js["version"] assert_equal Settings.generator, js["generator"] - assert_equal "#{format('%.7f', minlon)}", js["bounds"]["minlon"] - assert_equal "#{format('%.7f', minlat)}", js["bounds"]["minlat"] - assert_equal "#{format('%.7f', maxlon)}", js["bounds"]["maxlon"] - assert_equal "#{format('%.7f', maxlat)}", js["bounds"]["maxlat"] + assert_equal GeoRecord::Coord.new(minlon), js["bounds"]["minlon"] + assert_equal GeoRecord::Coord.new(minlat), js["bounds"]["minlat"] + assert_equal GeoRecord::Coord.new(maxlon), js["bounds"]["maxlon"] + assert_equal GeoRecord::Coord.new(maxlat), js["bounds"]["maxlat"] result_nodes = js["elements"].select { |a| a["type"] == "node" } .select { |a| a["id"] == node.id } - .select { |a| a["lat"] == "#{format('%.7f', node.lat)}" } - .select { |a| a["lon"] == "#{format('%.7f', node.lon)}" } + .select { |a| a["lat"] == GeoRecord::Coord.new(node.lat) } + .select { |a| a["lon"] == GeoRecord::Coord.new(node.lon) } .select { |a| a["version"] == node.version } .select { |a| a["changeset"] == node.changeset_id } .select { |a| a["timestamp"] == node.timestamp.xmlschema } From 97036c181e307dae10bb019dbcbd7a50d30bb3b8 Mon Sep 17 00:00:00 2001 From: mmd-osm Date: Thu, 2 Jan 2020 15:17:11 +0100 Subject: [PATCH 04/10] JSON output, handle Accept header --- app/controllers/api_controller.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/controllers/api_controller.rb b/app/controllers/api_controller.rb index ca09e5807..7599568af 100644 --- a/app/controllers/api_controller.rb +++ b/app/controllers/api_controller.rb @@ -5,7 +5,9 @@ class ApiController < ApplicationController # Set format to xml unless client requires a specific format def default_format_xml - request.format = "xml" unless params[:format] + unless params[:format] + request.format = "xml" unless request.format.symbol == :json + end end def authorize(realm = "Web Password", errormessage = "Couldn't authenticate you") From 70702d565abcf785d4fd94349accb5b7023933f0 Mon Sep 17 00:00:00 2001 From: mmd-osm Date: Wed, 8 Jan 2020 18:38:45 +0100 Subject: [PATCH 05/10] Remove no longer needed rubocop exemptions --- .rubocop_todo.yml | 3 --- 1 file changed, 3 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 080b7b3a5..06a9c76f0 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -199,9 +199,6 @@ Style/FormatStringToken: - 'test/controllers/api/relations_controller_test.rb' - 'test/controllers/api/ways_controller_test.rb' - 'test/lib/bounding_box_test.rb' - - 'app/views/api/map/_bounds.json.jbuilder' - - 'app/views/api/nodes/_node.json.jbuilder' - - 'app/views/api/old_nodes/_old_node.json.jbuilder' # Offense count: 539 # Cop supports --auto-correct. From 91b31f03de0143c4e207fb3a001b32859f560388 Mon Sep 17 00:00:00 2001 From: mmd-osm Date: Wed, 8 Jan 2020 18:49:45 +0100 Subject: [PATCH 06/10] Added unit tests --- test/controllers/api/nodes_controller_test.rb | 21 +++++++++++ .../api/old_nodes_controller_test.rb | 8 +++++ .../api/old_relations_controller_test.rb | 8 +++++ .../api/old_ways_controller_test.rb | 8 +++++ .../api/relations_controller_test.rb | 36 +++++++++++++++++++ test/controllers/api/ways_controller_test.rb | 24 +++++++++++++ 6 files changed, 105 insertions(+) diff --git a/test/controllers/api/nodes_controller_test.rb b/test/controllers/api/nodes_controller_test.rb index aee41d515..e2174a126 100644 --- a/test/controllers/api/nodes_controller_test.rb +++ b/test/controllers/api/nodes_controller_test.rb @@ -13,6 +13,10 @@ module Api { :path => "/api/0.6/node/1", :method => :get }, { :controller => "api/nodes", :action => "show", :id => "1" } ) + assert_routing( + { :path => "/api/0.6/node/1.json", :method => :get }, + { :controller => "api/nodes", :action => "show", :id => "1", :format => "json" } + ) assert_routing( { :path => "/api/0.6/node/1", :method => :put }, { :controller => "api/nodes", :action => "update", :id => "1" } @@ -25,6 +29,10 @@ module Api { :path => "/api/0.6/nodes", :method => :get }, { :controller => "api/nodes", :action => "index" } ) + assert_routing( + { :path => "/api/0.6/nodes.json", :method => :get }, + { :controller => "api/nodes", :action => "index", :format => "json" } + ) end def test_create @@ -464,6 +472,19 @@ module Api assert_select "node[id='#{node5.id}'][visible='false']", :count => 1 end + # test a working call with json format + get :index, :params => { :nodes => "#{node1.id},#{node2.id},#{node3.id},#{node4.id},#{node5.id}", :format => "json" } + + js = ActiveSupport::JSON.decode(@response.body) + assert_not_nil js + assert_equal 5, js["elements"].count + assert_equal 5, (js["elements"].count { |a| a["type"] == "node" }) + assert_equal 1, (js["elements"].count { |a| a["id"] == node1.id && a["visible"].nil? }) + assert_equal 1, (js["elements"].count { |a| a["id"] == node2.id && a["visible"] == false }) + assert_equal 1, (js["elements"].count { |a| a["id"] == node3.id && a["visible"].nil? }) + assert_equal 1, (js["elements"].count { |a| a["id"] == node4.id && a["visible"].nil? }) + assert_equal 1, (js["elements"].count { |a| a["id"] == node5.id && a["visible"] == false }) + # check error when a non-existent node is included get :index, :params => { :nodes => "#{node1.id},#{node2.id},#{node3.id},#{node4.id},#{node5.id},0" } assert_response :not_found diff --git a/test/controllers/api/old_nodes_controller_test.rb b/test/controllers/api/old_nodes_controller_test.rb index 941787d0a..76e78ffad 100644 --- a/test/controllers/api/old_nodes_controller_test.rb +++ b/test/controllers/api/old_nodes_controller_test.rb @@ -17,6 +17,14 @@ module Api { :path => "/api/0.6/node/1/2", :method => :get }, { :controller => "api/old_nodes", :action => "version", :id => "1", :version => "2" } ) + assert_routing( + { :path => "/api/0.6/node/1/history.json", :method => :get }, + { :controller => "api/old_nodes", :action => "history", :id => "1", :format => "json" } + ) + assert_routing( + { :path => "/api/0.6/node/1/2.json", :method => :get }, + { :controller => "api/old_nodes", :action => "version", :id => "1", :version => "2", :format => "json" } + ) assert_routing( { :path => "/api/0.6/node/1/2/redact", :method => :post }, { :controller => "api/old_nodes", :action => "redact", :id => "1", :version => "2" } diff --git a/test/controllers/api/old_relations_controller_test.rb b/test/controllers/api/old_relations_controller_test.rb index 40a80248c..19e8ae357 100644 --- a/test/controllers/api/old_relations_controller_test.rb +++ b/test/controllers/api/old_relations_controller_test.rb @@ -13,6 +13,14 @@ module Api { :path => "/api/0.6/relation/1/2", :method => :get }, { :controller => "api/old_relations", :action => "version", :id => "1", :version => "2" } ) + assert_routing( + { :path => "/api/0.6/relation/1/history.json", :method => :get }, + { :controller => "api/old_relations", :action => "history", :id => "1", :format => "json" } + ) + assert_routing( + { :path => "/api/0.6/relation/1/2.json", :method => :get }, + { :controller => "api/old_relations", :action => "version", :id => "1", :version => "2", :format => "json" } + ) assert_routing( { :path => "/api/0.6/relation/1/2/redact", :method => :post }, { :controller => "api/old_relations", :action => "redact", :id => "1", :version => "2" } diff --git a/test/controllers/api/old_ways_controller_test.rb b/test/controllers/api/old_ways_controller_test.rb index 73e968f30..cd676a960 100644 --- a/test/controllers/api/old_ways_controller_test.rb +++ b/test/controllers/api/old_ways_controller_test.rb @@ -13,6 +13,14 @@ module Api { :path => "/api/0.6/way/1/2", :method => :get }, { :controller => "api/old_ways", :action => "version", :id => "1", :version => "2" } ) + assert_routing( + { :path => "/api/0.6/way/1/history.json", :method => :get }, + { :controller => "api/old_ways", :action => "history", :id => "1", :format => "json" } + ) + assert_routing( + { :path => "/api/0.6/way/1/2.json", :method => :get }, + { :controller => "api/old_ways", :action => "version", :id => "1", :version => "2", :format => "json" } + ) assert_routing( { :path => "/api/0.6/way/1/2/redact", :method => :post }, { :controller => "api/old_ways", :action => "redact", :id => "1", :version => "2" } diff --git a/test/controllers/api/relations_controller_test.rb b/test/controllers/api/relations_controller_test.rb index b7c17a176..da18099bd 100644 --- a/test/controllers/api/relations_controller_test.rb +++ b/test/controllers/api/relations_controller_test.rb @@ -13,10 +13,18 @@ module Api { :path => "/api/0.6/relation/1/full", :method => :get }, { :controller => "api/relations", :action => "full", :id => "1" } ) + assert_routing( + { :path => "/api/0.6/relation/1/full.json", :method => :get }, + { :controller => "api/relations", :action => "full", :id => "1", :format => "json" } + ) assert_routing( { :path => "/api/0.6/relation/1", :method => :get }, { :controller => "api/relations", :action => "show", :id => "1" } ) + assert_routing( + { :path => "/api/0.6/relation/1.json", :method => :get }, + { :controller => "api/relations", :action => "show", :id => "1", :format => "json" } + ) assert_routing( { :path => "/api/0.6/relation/1", :method => :put }, { :controller => "api/relations", :action => "update", :id => "1" } @@ -29,6 +37,10 @@ module Api { :path => "/api/0.6/relations", :method => :get }, { :controller => "api/relations", :action => "index" } ) + assert_routing( + { :path => "/api/0.6/relations.json", :method => :get }, + { :controller => "api/relations", :action => "index", :format => "json" } + ) assert_routing( { :path => "/api/0.6/node/1/relations", :method => :get }, @@ -42,6 +54,18 @@ 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" } + ) + assert_routing( + { :path => "/api/0.6/relation/1/relations.json", :method => :get }, + { :controller => "api/relations", :action => "relations_for_relation", :id => "1", :format => "json" } + ) end # ------------------------------------- @@ -187,6 +211,18 @@ module Api assert_select "relation[id='#{relation4.id}'][visible='true']", :count => 1 end + # test a working call with json format + get :index, :params => { :relations => "#{relation1.id},#{relation2.id},#{relation3.id},#{relation4.id}", :format => "json" } + + js = ActiveSupport::JSON.decode(@response.body) + assert_not_nil js + assert_equal 4, js["elements"].count + assert_equal 4, (js["elements"].count { |a| a["type"] == "relation" }) + assert_equal 1, (js["elements"].count { |a| a["id"] == relation1.id && a["visible"].nil? }) + assert_equal 1, (js["elements"].count { |a| a["id"] == relation2.id && a["visible"] == false }) + assert_equal 1, (js["elements"].count { |a| a["id"] == relation3.id && a["visible"].nil? }) + assert_equal 1, (js["elements"].count { |a| a["id"] == relation4.id && a["visible"].nil? }) + # check error when a non-existent relation is included get :index, :params => { :relations => "#{relation1.id},#{relation2.id},#{relation3.id},#{relation4.id},0" } assert_response :not_found diff --git a/test/controllers/api/ways_controller_test.rb b/test/controllers/api/ways_controller_test.rb index 433b43f27..e4b9f3663 100644 --- a/test/controllers/api/ways_controller_test.rb +++ b/test/controllers/api/ways_controller_test.rb @@ -13,10 +13,18 @@ module Api { :path => "/api/0.6/way/1/full", :method => :get }, { :controller => "api/ways", :action => "full", :id => "1" } ) + assert_routing( + { :path => "/api/0.6/way/1/full.json", :method => :get }, + { :controller => "api/ways", :action => "full", :id => "1", :format => "json" } + ) assert_routing( { :path => "/api/0.6/way/1", :method => :get }, { :controller => "api/ways", :action => "show", :id => "1" } ) + assert_routing( + { :path => "/api/0.6/way/1.json", :method => :get }, + { :controller => "api/ways", :action => "show", :id => "1", :format => "json" } + ) assert_routing( { :path => "/api/0.6/way/1", :method => :put }, { :controller => "api/ways", :action => "update", :id => "1" } @@ -29,6 +37,10 @@ module Api { :path => "/api/0.6/ways", :method => :get }, { :controller => "api/ways", :action => "index" } ) + assert_routing( + { :path => "/api/0.6/ways.json", :method => :get }, + { :controller => "api/ways", :action => "index", :format => "json" } + ) end # ------------------------------------- @@ -104,6 +116,18 @@ module Api assert_select "way[id='#{way4.id}'][visible='true']", :count => 1 end + # test a working call with json format + get :index, :params => { :ways => "#{way1.id},#{way2.id},#{way3.id},#{way4.id}", :format => "json" } + + js = ActiveSupport::JSON.decode(@response.body) + assert_not_nil js + assert_equal 4, js["elements"].count + assert_equal 4, (js["elements"].count { |a| a["type"] == "way" }) + assert_equal 1, (js["elements"].count { |a| a["id"] == way1.id && a["visible"].nil? }) + assert_equal 1, (js["elements"].count { |a| a["id"] == way2.id && a["visible"] == false }) + assert_equal 1, (js["elements"].count { |a| a["id"] == way3.id && a["visible"].nil? }) + assert_equal 1, (js["elements"].count { |a| a["id"] == way4.id && a["visible"].nil? }) + # check error when a non-existent way is included get :index, :params => { :ways => "#{way1.id},#{way2.id},#{way3.id},#{way4.id},0" } assert_response :not_found From 03ca0b2c697052c80b25f2111ab6ab718478a95a Mon Sep 17 00:00:00 2001 From: mmd-osm Date: Thu, 9 Jan 2020 21:09:33 +0100 Subject: [PATCH 07/10] Added Accept header unit tests --- app/controllers/api/map_controller.rb | 2 +- app/controllers/api/nodes_controller.rb | 2 +- app/controllers/api/old_controller.rb | 2 +- app/controllers/api/relations_controller.rb | 2 +- app/controllers/api/ways_controller.rb | 2 +- app/controllers/api_controller.rb | 45 ++++++++++++- test/controllers/api/map_controller_test.rb | 72 +++++++++++++++++++++ test/test_helper.rb | 6 ++ 8 files changed, 125 insertions(+), 8 deletions(-) diff --git a/app/controllers/api/map_controller.rb b/app/controllers/api/map_controller.rb index 4b233c995..113554c72 100644 --- a/app/controllers/api/map_controller.rb +++ b/app/controllers/api/map_controller.rb @@ -5,7 +5,7 @@ module Api before_action :check_api_readable around_action :api_call_handle_error, :api_call_timeout - before_action :default_format_xml + before_action :set_default_request_format # This is probably the most common call of all. It is used for getting the # OSM data for a specified bounding box, usually for editing. First the diff --git a/app/controllers/api/nodes_controller.rb b/app/controllers/api/nodes_controller.rb index 08f19008f..336aebc91 100644 --- a/app/controllers/api/nodes_controller.rb +++ b/app/controllers/api/nodes_controller.rb @@ -13,7 +13,7 @@ module Api before_action :check_api_readable, :except => [:create, :update, :delete] around_action :api_call_handle_error, :api_call_timeout - before_action :default_format_xml + before_action :set_default_request_format, :except => [:create, :update, :delete] # Create a node from XML. def create diff --git a/app/controllers/api/old_controller.rb b/app/controllers/api/old_controller.rb index b68fde6b5..f672e7c49 100644 --- a/app/controllers/api/old_controller.rb +++ b/app/controllers/api/old_controller.rb @@ -16,7 +16,7 @@ module Api before_action :lookup_old_element, :except => [:history] before_action :lookup_old_element_versions, :only => [:history] - before_action :default_format_xml + before_action :set_default_request_format, :except => [:redact] def history # the .where() method used in the lookup_old_element_versions diff --git a/app/controllers/api/relations_controller.rb b/app/controllers/api/relations_controller.rb index 566efe0eb..329c5e29c 100644 --- a/app/controllers/api/relations_controller.rb +++ b/app/controllers/api/relations_controller.rb @@ -11,7 +11,7 @@ module Api before_action :check_api_readable, :except => [:create, :update, :delete] around_action :api_call_handle_error, :api_call_timeout - before_action :default_format_xml + before_action :set_default_request_format, :except => [:create, :update, :delete] def create assert_method :put diff --git a/app/controllers/api/ways_controller.rb b/app/controllers/api/ways_controller.rb index 95f36df53..a7c876710 100644 --- a/app/controllers/api/ways_controller.rb +++ b/app/controllers/api/ways_controller.rb @@ -11,7 +11,7 @@ module Api before_action :check_api_readable, :except => [:create, :update, :delete] around_action :api_call_handle_error, :api_call_timeout - before_action :default_format_xml + before_action :set_default_request_format, :except => [:create, :update, :delete] def create assert_method :put diff --git a/app/controllers/api_controller.rb b/app/controllers/api_controller.rb index 7599568af..c0534a6fa 100644 --- a/app/controllers/api_controller.rb +++ b/app/controllers/api_controller.rb @@ -3,10 +3,49 @@ class ApiController < ApplicationController private - # Set format to xml unless client requires a specific format - def default_format_xml + ## + # Set default request format to xml unless a client requests a specific format, + # which can be done via (a) URL suffix and/or (b) HTTP Accept header, where + # the URL suffix always takes precedence over the Accept header. + def set_default_request_format unless params[:format] - request.format = "xml" unless request.format.symbol == :json + accept_header = request.headers["HTTP_ACCEPT"] + if accept_header.nil? + # e.g. unit tests don't set an Accept: header by default, force XML in this case + request.format = "xml" + return + end + + req_mimetypes = [] + + # Some clients (JOSM) send Accept headers which cannot be parsed by Rails, example: *; q=.2 + # To be fair, JOSM's Accept header doesn't adhere to RFC 7231, section 5.3.1, et al. either + # As a workaround for backwards compatibility, we're assuming XML format + begin + req_mimetypes = Mime::Type.parse(accept_header) + rescue Mime::Type::InvalidMimeType + request.format = "xml" + return + end + + # req_mimetypes contains all Accept header MIME types with descending priority + req_mimetypes.each do |mime| + if mime.symbol == :xml + request.format = "xml" + break + end + + if mime.symbol == :json + request.format = "json" + break + end + + # Any format, not explicitly requesting XML or JSON -> assume XML as default + if mime == "*/*" + request.format = "xml" + break + end + end end end diff --git a/test/controllers/api/map_controller_test.rb b/test/controllers/api/map_controller_test.rb index 54461868b..c5392c723 100644 --- a/test/controllers/api/map_controller_test.rb +++ b/test/controllers/api/map_controller_test.rb @@ -29,6 +29,78 @@ module Api ) end + ## + # test http accept headers + def test_http_accept_header + node = create(:node, :lat => 7, :lon => 7) + + minlon = node.lon - 0.1 + minlat = node.lat - 0.1 + maxlon = node.lon + 0.1 + maxlat = node.lat + 0.1 + bbox = "#{minlon},#{minlat},#{maxlon},#{maxlat}" + + # Accept: XML format -> use XML + http_accept_format("text/xml") + get :index, :params => { :bbox => bbox } + assert_response :success, "Expected success with the map call" + assert_equal "application/xml; charset=utf-8", @response.header["Content-Type"] + + # Accept: Any format -> use XML + http_accept_format("*/*") + get :index, :params => { :bbox => bbox } + assert_response :success, "Expected success with the map call" + assert_equal "application/xml; charset=utf-8", @response.header["Content-Type"] + + # Accept: Any format, .json URL suffix -> use json + http_accept_format("*/*") + get :index, :params => { :bbox => bbox, :format => "json" } + assert_response :success, "Expected success with the map call" + assert_equal "application/json; charset=utf-8", @response.header["Content-Type"] + + # Accept: Firefox header -> use XML + http_accept_format("text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8") + get :index, :params => { :bbox => bbox } + assert_response :success, "Expected success with the map call" + assert_equal "application/xml; charset=utf-8", @response.header["Content-Type"] + + # Accept: JOSM header text/html, image/gif, image/jpeg, *; q=.2, */*; q=.2 -> use XML + # Note: JOSM's header does not comply with RFC 7231, section 5.3.1 + http_accept_format("text/html, image/gif, image/jpeg, *; q=.2, */*; q=.2") + get :index, :params => { :bbox => bbox } + assert_response :success, "Expected success with the map call" + assert_equal "application/xml; charset=utf-8", @response.header["Content-Type"] + + # Accept: text/plain, */* -> use XML + http_accept_format("text/plain, */*") + get :index, :params => { :bbox => bbox } + assert_response :success, "Expected success with the map call" + assert_equal "application/xml; charset=utf-8", @response.header["Content-Type"] + + # Accept: text/* -> use XML + http_accept_format("text/*") + get :index, :params => { :bbox => bbox } + assert_response :success, "Expected success with the map call" + assert_equal "application/xml; charset=utf-8", @response.header["Content-Type"] + + # Accept: json, */* format -> use json + http_accept_format("application/json, */*") + get :index, :params => { :bbox => bbox } + assert_response :success, "Expected success with the map call" + assert_equal "application/json; charset=utf-8", @response.header["Content-Type"] + + # Accept: json format -> use json + http_accept_format("application/json") + get :index, :params => { :bbox => bbox } + assert_response :success, "Expected success with the map call" + assert_equal "application/json; charset=utf-8", @response.header["Content-Type"] + + # text/json is in invalid format, ActionController::UnknownFormat error is expected + http_accept_format("text/json") + get :index, :params => { :bbox => bbox } + assert_response :internal_server_error, "text/json should fail" + end + # ------------------------------------- # Test reading a bounding box. # ------------------------------------- diff --git a/test/test_helper.rb b/test/test_helper.rb index 4d9372148..5b4d0d28f 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -115,6 +115,12 @@ module ActiveSupport @request.env["HTTP_AUTHORIZATION"] = format("Basic %{auth}", :auth => Base64.encode64("#{user}:#{pass}")) end + ## + # set request header for HTTP Accept + def http_accept_format(format) + @request.env["HTTP_ACCEPT"] = format + end + ## # set request readers to ask for a particular error format def error_format(format) From b556b054d1ac66ba5f8deebe27c60d14ea968b6d Mon Sep 17 00:00:00 2001 From: mmd-osm Date: Wed, 22 Jan 2020 17:38:28 +0100 Subject: [PATCH 08/10] JSON: added clarification for non-supported Accept header formats --- app/controllers/api_controller.rb | 4 ++++ test/controllers/api/map_controller_test.rb | 8 +++++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/app/controllers/api_controller.rb b/app/controllers/api_controller.rb index c0534a6fa..64514a432 100644 --- a/app/controllers/api_controller.rb +++ b/app/controllers/api_controller.rb @@ -45,6 +45,10 @@ class ApiController < ApplicationController request.format = "xml" break end + + # In case the client requests some other format besides XML, JSON and */*, + # we deliberately don't set request.format. The framework will return an + # ActionController::UnknownFormat error to the client later on in this case. end end end diff --git a/test/controllers/api/map_controller_test.rb b/test/controllers/api/map_controller_test.rb index c5392c723..d5cbfb4ef 100644 --- a/test/controllers/api/map_controller_test.rb +++ b/test/controllers/api/map_controller_test.rb @@ -32,7 +32,7 @@ module Api ## # test http accept headers def test_http_accept_header - node = create(:node, :lat => 7, :lon => 7) + node = create(:node) minlon = node.lon - 0.1 minlat = node.lat - 0.1 @@ -99,6 +99,12 @@ module Api http_accept_format("text/json") get :index, :params => { :bbox => bbox } assert_response :internal_server_error, "text/json should fail" + + # image/jpeg is a format which we don't support, ActionController::UnknownFormat error is expected + # HTTP 406 Not acceptable would be the correct response error code. That's outside of our control though. + http_accept_format("image/jpeg") + get :index, :params => { :bbox => bbox } + assert_response :internal_server_error, "text/json should fail" end # ------------------------------------- From 3159c3c3f715250ef97e468eb155b868322b0426 Mon Sep 17 00:00:00 2001 From: mmd-osm Date: Wed, 22 Jan 2020 22:33:30 +0100 Subject: [PATCH 09/10] JSON: return HTTP 406 for unsupported formats --- app/controllers/application_controller.rb | 2 ++ test/controllers/api/map_controller_test.rb | 9 ++++----- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index bee5e8169..f419460ee 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -185,6 +185,8 @@ class ApplicationController < ActionController::Base def api_call_handle_error yield + rescue ActionController::UnknownFormat + head :not_acceptable rescue ActiveRecord::RecordNotFound => e head :not_found rescue LibXML::XML::Error, ArgumentError => e diff --git a/test/controllers/api/map_controller_test.rb b/test/controllers/api/map_controller_test.rb index d5cbfb4ef..9f14cc7fd 100644 --- a/test/controllers/api/map_controller_test.rb +++ b/test/controllers/api/map_controller_test.rb @@ -95,16 +95,15 @@ module Api assert_response :success, "Expected success with the map call" assert_equal "application/json; charset=utf-8", @response.header["Content-Type"] - # text/json is in invalid format, ActionController::UnknownFormat error is expected + # text/json is in invalid format, return HTTP 406 Not acceptable http_accept_format("text/json") get :index, :params => { :bbox => bbox } - assert_response :internal_server_error, "text/json should fail" + assert_response :not_acceptable, "text/json should fail" - # image/jpeg is a format which we don't support, ActionController::UnknownFormat error is expected - # HTTP 406 Not acceptable would be the correct response error code. That's outside of our control though. + # image/jpeg is a format which we don't support, return HTTP 406 Not acceptable http_accept_format("image/jpeg") get :index, :params => { :bbox => bbox } - assert_response :internal_server_error, "text/json should fail" + assert_response :not_acceptable, "text/json should fail" end # ------------------------------------- From 7fcb76aa9a627ef18bfdecf77dfb096ba03d0490 Mon Sep 17 00:00:00 2001 From: mmd-osm Date: Wed, 22 Jan 2020 22:47:47 +0100 Subject: [PATCH 10/10] JSON: move root_attributes to shared folder --- app/views/api/{map => }/_root_attributes.json.jbuilder | 0 app/views/api/nodes/index.json.jbuilder | 2 +- app/views/api/nodes/show.json.jbuilder | 2 +- app/views/api/old_nodes/history.json.jbuilder | 2 +- app/views/api/old_nodes/version.json.jbuilder | 2 +- app/views/api/old_relations/history.json.jbuilder | 2 +- app/views/api/old_relations/version.json.jbuilder | 2 +- app/views/api/old_ways/history.json.jbuilder | 2 +- app/views/api/old_ways/version.json.jbuilder | 2 +- app/views/api/relations/full.json.jbuilder | 2 +- app/views/api/relations/index.json.jbuilder | 2 +- app/views/api/relations/relations_for_node.json.jbuilder | 2 +- app/views/api/relations/relations_for_relation.json.jbuilder | 2 +- app/views/api/relations/relations_for_way.json.jbuilder | 2 +- app/views/api/relations/show.json.jbuilder | 2 +- app/views/api/ways/full.json.jbuilder | 2 +- app/views/api/ways/index.json.jbuilder | 2 +- app/views/api/ways/show.json.jbuilder | 2 +- app/views/api/ways/ways_for_node.json.jbuilder | 2 +- 19 files changed, 18 insertions(+), 18 deletions(-) rename app/views/api/{map => }/_root_attributes.json.jbuilder (100%) diff --git a/app/views/api/map/_root_attributes.json.jbuilder b/app/views/api/_root_attributes.json.jbuilder similarity index 100% rename from app/views/api/map/_root_attributes.json.jbuilder rename to app/views/api/_root_attributes.json.jbuilder diff --git a/app/views/api/nodes/index.json.jbuilder b/app/views/api/nodes/index.json.jbuilder index f42f7563b..3e3ceb4cc 100644 --- a/app/views/api/nodes/index.json.jbuilder +++ b/app/views/api/nodes/index.json.jbuilder @@ -1,4 +1,4 @@ -json.partial! "api/map/root_attributes" +json.partial! "api/root_attributes" json.elements(@nodes) do |node| json.partial! node diff --git a/app/views/api/nodes/show.json.jbuilder b/app/views/api/nodes/show.json.jbuilder index c87777a87..9974da82f 100644 --- a/app/views/api/nodes/show.json.jbuilder +++ b/app/views/api/nodes/show.json.jbuilder @@ -1,4 +1,4 @@ -json.partial! "api/map/root_attributes" +json.partial! "api/root_attributes" json.elements([@node]) do |node| json.partial! node diff --git a/app/views/api/old_nodes/history.json.jbuilder b/app/views/api/old_nodes/history.json.jbuilder index b0e97e832..96e8cca2d 100644 --- a/app/views/api/old_nodes/history.json.jbuilder +++ b/app/views/api/old_nodes/history.json.jbuilder @@ -1,4 +1,4 @@ -json.partial! "api/map/root_attributes" +json.partial! "api/root_attributes" json.elements(@elems) do |old_node| json.partial! old_node diff --git a/app/views/api/old_nodes/version.json.jbuilder b/app/views/api/old_nodes/version.json.jbuilder index 992230bc0..f63e07d43 100644 --- a/app/views/api/old_nodes/version.json.jbuilder +++ b/app/views/api/old_nodes/version.json.jbuilder @@ -1,4 +1,4 @@ -json.partial! "api/map/root_attributes" +json.partial! "api/root_attributes" json.elements([@old_element]) do |old_node| json.partial! old_node diff --git a/app/views/api/old_relations/history.json.jbuilder b/app/views/api/old_relations/history.json.jbuilder index c5dacae0a..311a80a86 100644 --- a/app/views/api/old_relations/history.json.jbuilder +++ b/app/views/api/old_relations/history.json.jbuilder @@ -1,4 +1,4 @@ -json.partial! "api/map/root_attributes" +json.partial! "api/root_attributes" json.elements(@elems) do |old_relation| json.partial! old_relation diff --git a/app/views/api/old_relations/version.json.jbuilder b/app/views/api/old_relations/version.json.jbuilder index b4018429b..5b33e4b85 100644 --- a/app/views/api/old_relations/version.json.jbuilder +++ b/app/views/api/old_relations/version.json.jbuilder @@ -1,4 +1,4 @@ -json.partial! "api/map/root_attributes" +json.partial! "api/root_attributes" json.elements([@old_element]) do |old_relation| json.partial! old_relation diff --git a/app/views/api/old_ways/history.json.jbuilder b/app/views/api/old_ways/history.json.jbuilder index b231d7de1..b5cf80d09 100644 --- a/app/views/api/old_ways/history.json.jbuilder +++ b/app/views/api/old_ways/history.json.jbuilder @@ -1,4 +1,4 @@ -json.partial! "api/map/root_attributes" +json.partial! "api/root_attributes" json.elements(@elems) do |old_way| json.partial! old_way diff --git a/app/views/api/old_ways/version.json.jbuilder b/app/views/api/old_ways/version.json.jbuilder index bb261c1c5..c66488526 100644 --- a/app/views/api/old_ways/version.json.jbuilder +++ b/app/views/api/old_ways/version.json.jbuilder @@ -1,4 +1,4 @@ -json.partial! "api/map/root_attributes" +json.partial! "api/root_attributes" json.elements([@old_element]) do |old_way| json.partial! old_way diff --git a/app/views/api/relations/full.json.jbuilder b/app/views/api/relations/full.json.jbuilder index 268de01ca..98cbbfc40 100644 --- a/app/views/api/relations/full.json.jbuilder +++ b/app/views/api/relations/full.json.jbuilder @@ -1,4 +1,4 @@ -json.partial! "api/map/root_attributes" +json.partial! "api/root_attributes" all = @nodes + @ways + @relations diff --git a/app/views/api/relations/index.json.jbuilder b/app/views/api/relations/index.json.jbuilder index 25c54fc6f..f170cb150 100644 --- a/app/views/api/relations/index.json.jbuilder +++ b/app/views/api/relations/index.json.jbuilder @@ -1,4 +1,4 @@ -json.partial! "api/map/root_attributes" +json.partial! "api/root_attributes" json.elements(@relations) do |relation| json.partial! relation diff --git a/app/views/api/relations/relations_for_node.json.jbuilder b/app/views/api/relations/relations_for_node.json.jbuilder index 25c54fc6f..f170cb150 100644 --- a/app/views/api/relations/relations_for_node.json.jbuilder +++ b/app/views/api/relations/relations_for_node.json.jbuilder @@ -1,4 +1,4 @@ -json.partial! "api/map/root_attributes" +json.partial! "api/root_attributes" json.elements(@relations) do |relation| json.partial! relation diff --git a/app/views/api/relations/relations_for_relation.json.jbuilder b/app/views/api/relations/relations_for_relation.json.jbuilder index 25c54fc6f..f170cb150 100644 --- a/app/views/api/relations/relations_for_relation.json.jbuilder +++ b/app/views/api/relations/relations_for_relation.json.jbuilder @@ -1,4 +1,4 @@ -json.partial! "api/map/root_attributes" +json.partial! "api/root_attributes" json.elements(@relations) do |relation| json.partial! relation diff --git a/app/views/api/relations/relations_for_way.json.jbuilder b/app/views/api/relations/relations_for_way.json.jbuilder index 25c54fc6f..f170cb150 100644 --- a/app/views/api/relations/relations_for_way.json.jbuilder +++ b/app/views/api/relations/relations_for_way.json.jbuilder @@ -1,4 +1,4 @@ -json.partial! "api/map/root_attributes" +json.partial! "api/root_attributes" json.elements(@relations) do |relation| json.partial! relation diff --git a/app/views/api/relations/show.json.jbuilder b/app/views/api/relations/show.json.jbuilder index 682ef0a24..7f85d0f25 100644 --- a/app/views/api/relations/show.json.jbuilder +++ b/app/views/api/relations/show.json.jbuilder @@ -1,4 +1,4 @@ -json.partial! "api/map/root_attributes" +json.partial! "api/root_attributes" json.elements([@relation]) do |relation| json.partial! relation diff --git a/app/views/api/ways/full.json.jbuilder b/app/views/api/ways/full.json.jbuilder index 9bd234fbe..bebad5e9f 100644 --- a/app/views/api/ways/full.json.jbuilder +++ b/app/views/api/ways/full.json.jbuilder @@ -1,4 +1,4 @@ -json.partial! "api/map/root_attributes" +json.partial! "api/root_attributes" all = @nodes + [@way] diff --git a/app/views/api/ways/index.json.jbuilder b/app/views/api/ways/index.json.jbuilder index e1dbd4347..19e59cfb8 100644 --- a/app/views/api/ways/index.json.jbuilder +++ b/app/views/api/ways/index.json.jbuilder @@ -1,4 +1,4 @@ -json.partial! "api/map/root_attributes" +json.partial! "api/root_attributes" json.elements(@ways) do |way| json.partial! way diff --git a/app/views/api/ways/show.json.jbuilder b/app/views/api/ways/show.json.jbuilder index e5cd31fed..acb93c1e7 100644 --- a/app/views/api/ways/show.json.jbuilder +++ b/app/views/api/ways/show.json.jbuilder @@ -1,4 +1,4 @@ -json.partial! "api/map/root_attributes" +json.partial! "api/root_attributes" json.elements([@way]) do |way| json.partial! way diff --git a/app/views/api/ways/ways_for_node.json.jbuilder b/app/views/api/ways/ways_for_node.json.jbuilder index e1dbd4347..19e59cfb8 100644 --- a/app/views/api/ways/ways_for_node.json.jbuilder +++ b/app/views/api/ways/ways_for_node.json.jbuilder @@ -1,4 +1,4 @@ -json.partial! "api/map/root_attributes" +json.partial! "api/root_attributes" json.elements(@ways) do |way| json.partial! way