From 24b21e4a29d28a3174965d449d36465ecda9ead7 Mon Sep 17 00:00:00 2001 From: Shaun McDonald Date: Sun, 12 Oct 2008 22:32:51 +0000 Subject: [PATCH 1/1] additional consistency checks. making the error message for the number of nodes use the configured number of nodes, rather than a hard coded number. minor improvements to the way controller functional tests. Not sure if they should be turned into integration tests. --- app/controllers/api_controller.rb | 2 +- app/controllers/way_controller.rb | 4 ++++ lib/geo_record.rb | 8 +++++++- lib/osm.rb | 7 +++++++ test/functional/way_controller_test.rb | 12 ++++++++++-- 5 files changed, 29 insertions(+), 4 deletions(-) diff --git a/app/controllers/api_controller.rb b/app/controllers/api_controller.rb index 2f040a92b..8b876d3a7 100644 --- a/app/controllers/api_controller.rb +++ b/app/controllers/api_controller.rb @@ -116,7 +116,7 @@ class ApiController < ApplicationController node_ids = @nodes.collect(&:id) if node_ids.length > APP_CONFIG['max_number_of_nodes'] - report_error("You requested too many nodes (limit is 50,000). Either request a smaller area, or use planet.osm") + report_error("You requested too many nodes (limit is #{APP_CONFIG['max_number_of_nodes']}). Either request a smaller area, or use planet.osm") return end if node_ids.length == 0 diff --git a/app/controllers/way_controller.rb b/app/controllers/way_controller.rb index 6f4704c77..08270094d 100644 --- a/app/controllers/way_controller.rb +++ b/app/controllers/way_controller.rb @@ -12,6 +12,10 @@ class WayController < ApplicationController way = Way.from_xml(request.raw_post, true) if way + # FIXME move some of this to the model. The controller shouldn't need to + # know about the fact that the first version number is 0 on creation + # it will also allow use to run a variation on the check_consistency + # so that we don't get exceptions thrown when the changesets are not right unless way.preconditions_ok? render :text => "", :status => :precondition_failed else diff --git a/lib/geo_record.rb b/lib/geo_record.rb index 3eab72b2d..273419757 100644 --- a/lib/geo_record.rb +++ b/lib/geo_record.rb @@ -45,10 +45,16 @@ module GeoRecord # Generic checks that are run for the updates and deletes of # node, ways and relations. This code is here to avoid duplication, # and allow the extention of the checks without having to modify the - # code in 6 places. This will throw an exception if there is an inconsistency + # code in 6 places for all the updates and deletes. Some of these tests are + # needed for creates, but are currently not run :-( + # This will throw an exception if there is an inconsistency def check_consistency(old, new, user) if new.version != old.version raise OSM::APIVersionMismatchError.new(new.version, old.version) + elsif new.changeset.nil? + raise OSM::APIChangesetMissingError.new + elsif new.changeset.empty? + raise OSM::APIChangesetMissingError.new elsif new.changeset.user_id != user.id raise OSM::APIUserChangesetMismatchError.new elsif not new.changeset.is_open? diff --git a/lib/osm.rb b/lib/osm.rb index e0e83845e..82fc835b4 100644 --- a/lib/osm.rb +++ b/lib/osm.rb @@ -46,6 +46,13 @@ module OSM { :text => "The supplied changeset has already been closed", :status => :conflict } end end + + # Raised when a change is expecting a changeset, but the changeset doesn't exist + class APIChangesetMissingError < APIError + def render_opts + { :text => "You need to supply a changeset to be able to make a change", :status => :conflict } + end + end # Raised when the provided version is not equal to the latest in the db. class APIVersionMismatchError < APIError diff --git a/test/functional/way_controller_test.rb b/test/functional/way_controller_test.rb index b4f4599a4..558e45489 100644 --- a/test/functional/way_controller_test.rb +++ b/test/functional/way_controller_test.rb @@ -109,6 +109,7 @@ class WayControllerTest < Test::Unit::TestCase def test_create_invalid basic_authorization "test@openstreetmap.org", "test" + # FIXME All of these will fail because they don't have a valid changeset # create a way with non-existing node content "" put :create @@ -137,9 +138,16 @@ class WayControllerTest < Test::Unit::TestCase # now set auth basic_authorization("test@openstreetmap.org", "test"); - # this should work + # this shouldn't work as with the 0.6 api we need pay load to delete delete :delete, :id => current_ways(:visible_way).id - assert_response :success + assert_response :bad_request + + # Now try without having a changeset + content "" + delete :delete, :id => current_ways(:visible_way).id + assert_response :bad_request + + # Now try and get a changeset # this won't work since the way is already deleted delete :delete, :id => current_ways(:invisible_way).id -- 2.43.2