From: Matt Amos Date: Tue, 4 Nov 2008 15:52:22 +0000 (+0000) Subject: Added first attempt at bounding box support in changesets and tests for the same. X-Git-Tag: live~7557^2~224 X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/02fbcf5f06917d7dea546b90932da82b8ed29d2a Added first attempt at bounding box support in changesets and tests for the same. --- diff --git a/app/models/changeset.rb b/app/models/changeset.rb index 446ca351d..b00dfa8af 100644 --- a/app/models/changeset.rb +++ b/app/models/changeset.rb @@ -15,6 +15,9 @@ class Changeset < ActiveRecord::Base validates_presence_of :user_id, :created_at validates_inclusion_of :open, :in => [ true, false ] + # over-expansion factor to use when updating the bounding box + EXPAND = 0.1 + # Use a method like this, so that we can easily change how we # determine whether a changeset is open, without breaking code in at # least 6 controllers @@ -46,6 +49,35 @@ class Changeset < ActiveRecord::Base return cs end + ## + # returns the bounding box of the changeset. it is possible that some + # or all of the values will be nil, indicating that they are undefined. + def bbox + @bbox ||= [ min_lon, min_lat, max_lon, max_lat ] + end + + ## + # expand the bounding box to include the given bounding box. also, + # expand a little bit more in the direction of the expansion, so that + # further expansions may be unnecessary. this is an optimisation + # suggested on the wiki page by kleptog. + def update_bbox!(array) + # ensure that bbox is cached and has no nils in it. if there are any + # nils, just use the bounding box update to write over them. + @bbox = bbox.zip(array).collect { |a, b| a.nil? ? b : a } + + # FIXME - this looks nasty and violates DRY... is there any prettier + # way to do this? + @bbox[0] = array[0] + EXPAND * (@bbox[0] - @bbox[2]) if array[0] < @bbox[0] + @bbox[1] = array[1] + EXPAND * (@bbox[1] - @bbox[3]) if array[1] < @bbox[1] + @bbox[2] = array[2] + EXPAND * (@bbox[2] - @bbox[0]) if array[2] > @bbox[2] + @bbox[3] = array[3] + EXPAND * (@bbox[3] - @bbox[1]) if array[3] > @bbox[3] + + # update active record. rails 2.1's dirty handling should take care of + # whether this object needs saving or not. + self.min_lon, self.min_lat, self.max_lon, self.max_lat = @bbox + end + def tags_as_hash return tags end @@ -124,9 +156,14 @@ class Changeset < ActiveRecord::Base el1['created_at'] = self.created_at.xmlschema el1['open'] = self.open.to_s - # FIXME FIXME FIXME: This does not include changes yet! There is - # currently no changeset_id column in the tables as far as I can tell, - # so this is just a scaffold to build on, not a complete to_xml + el1['min_lon'] = (bbox[0] / SCALE).to_s unless bbox[0].nil? + el1['min_lat'] = (bbox[1] / SCALE).to_s unless bbox[1].nil? + el1['max_lon'] = (bbox[2] / SCALE).to_s unless bbox[2].nil? + el1['max_lat'] = (bbox[3] / SCALE).to_s unless bbox[3].nil? + + # NOTE: changesets don't include the XML of the changes within them, + # they are just structures for tagging. to get the osmChange of a + # changeset, see the download method of the controller. return el1 end diff --git a/app/models/node.rb b/app/models/node.rb index 67efeca2c..e58a1d896 100644 --- a/app/models/node.rb +++ b/app/models/node.rb @@ -112,6 +112,12 @@ class Node < ActiveRecord::Base return node end + ## + # the bounding box around a node + def bbox + [ longitude, latitude, longitude, latitude ] + end + def save_with_history! t = Time.now Node.transaction do @@ -134,6 +140,9 @@ class Node < ActiveRecord::Base old_node = OldNode.from_node(self) old_node.timestamp = t old_node.save_with_dependencies! + + # save the changeset in case of bounding box updates + changeset.save! end end @@ -148,6 +157,10 @@ class Node < ActiveRecord::Base else self.changeset_id = new_node.changeset_id self.visible = 0 + + # update the changeset with the deleted position + changeset.update_bbox!(bbox) + save_with_history! end else @@ -158,12 +171,19 @@ class Node < ActiveRecord::Base def update_from(new_node, user) check_consistency(self, new_node, user) + # update changeset with *old* position first + changeset.update_bbox!(bbox); + # FIXME logic needs to be double checked self.changeset_id = new_node.changeset_id self.latitude = new_node.latitude self.longitude = new_node.longitude self.tags = new_node.tags self.visible = true + + # update changeset with *new* position + changeset.update_bbox!(bbox); + save_with_history! end @@ -171,6 +191,10 @@ class Node < ActiveRecord::Base check_create_consistency(self, user) self.version = 0 self.visible = true + + # update the changeset to include the new location + changeset.update_bbox!(bbox) + save_with_history! end diff --git a/app/models/relation.rb b/app/models/relation.rb index 81f178997..9836ef4f1 100644 --- a/app/models/relation.rb +++ b/app/models/relation.rb @@ -221,6 +221,9 @@ class Relation < ActiveRecord::Base old_relation = OldRelation.from_relation(self) old_relation.timestamp = t old_relation.save_with_dependencies! + + # update the bbox of the changeset and save it too. + # FIXME: what is the bounding box of a relation? end end diff --git a/app/models/way.rb b/app/models/way.rb index 9d4d8ba87..4143291c1 100644 --- a/app/models/way.rb +++ b/app/models/way.rb @@ -179,9 +179,24 @@ class Way < ActiveRecord::Base @tags[k] = v end + ## + # the integer coords (i.e: unscaled) bounding box of the way, assuming + # straight line segments. + def bbox + lons = nodes.collect { |n| n.longitude } + lats = nodes.collect { |n| n.latitude } + [ lons.min, lats.min, lons.max, lats.max ] + end + def save_with_history! t = Time.now + # update the bounding box, but don't save it as the controller knows the + # lifetime of the change better. note that this has to be done both before + # and after the save, so that nodes from both versions are included in the + # bbox. + changeset.update_bbox!(bbox) unless nodes.empty? + Way.transaction do self.version += 1 self.timestamp = t @@ -211,6 +226,11 @@ class Way < ActiveRecord::Base old_way = OldWay.from_way(self) old_way.timestamp = t old_way.save_with_dependencies! + + # update and commit the bounding box, now that way nodes + # have been updated and we're in a transaction. + changeset.update_bbox!(bbox) unless nodes.empty? + changeset.save! end end diff --git a/lib/diff_reader.rb b/lib/diff_reader.rb index 165e30e20..d793f63e7 100644 --- a/lib/diff_reader.rb +++ b/lib/diff_reader.rb @@ -99,9 +99,8 @@ class DiffReader # diff, so we must fix these before saving the element. new.fix_placeholders!(ids) - # set the initial version to zero and save (which increments it) - new.version = 0 - new.save_with_history! + # create element given user + new.create_with_history(@changeset.user) # save placeholder => allocated ID map ids[model.to_s.downcase.to_sym][placeholder_id] = new.id diff --git a/test/functional/changeset_controller_test.rb b/test/functional/changeset_controller_test.rb index 31ade9fce..b9c8c63d0 100644 --- a/test/functional/changeset_controller_test.rb +++ b/test/functional/changeset_controller_test.rb @@ -36,8 +36,16 @@ class ChangesetControllerTest < ActionController::TestCase assert_response :bad_request, "creating a invalid changeset should fail" end + ## + # check that the changeset can be read and returns the correct + # document structure. def test_read + changeset_id = changesets(:normal_user_first_change).id + get :read, :id => changeset_id + assert_response :success, "cannot get first changeset" + assert_select "osm[version=#{API_VERSION}][generator=\"OpenStreetMap server\"]", 1 + assert_select "osm>changeset[id=#{changeset_id}]", 1 end def test_close @@ -487,5 +495,80 @@ EOF assert_select "osmChange>modify>node", 1 assert_select "osmChange>modify>way", 1 end - + + ## + # check that the bounding box of a changeset gets updated correctly + def test_changeset_bbox + basic_authorization "test@openstreetmap.org", "test" + + # create a new changeset + content "" + put :create + assert_response :success, "Creating of changeset failed." + changeset_id = @response.body.to_i + + # add a single node to it + with_controller(NodeController.new) do + content "" + put :create + assert_response :success, "Couldn't create node." + end + + # get the bounding box back from the changeset + get :read, :id => changeset_id + assert_response :success, "Couldn't read back changeset." + assert_select "osm>changeset[min_lon=1]", 1 + assert_select "osm>changeset[max_lon=1]", 1 + assert_select "osm>changeset[min_lat=2]", 1 + assert_select "osm>changeset[max_lat=2]", 1 + + # add another node to it + with_controller(NodeController.new) do + content "" + put :create + assert_response :success, "Couldn't create second node." + end + + # get the bounding box back from the changeset + get :read, :id => changeset_id + assert_response :success, "Couldn't read back changeset for the second time." + assert_select "osm>changeset[min_lon=1]", 1 + assert_select "osm>changeset[max_lon=2]", 1 + assert_select "osm>changeset[min_lat=1]", 1 + assert_select "osm>changeset[max_lat=2]", 1 + + # add (delete) a way to it + with_controller(WayController.new) do + content update_changeset(current_ways(:visible_way).to_xml, + changeset_id) + put :delete, :id => current_ways(:visible_way).id + assert_response :success, "Couldn't delete a way." + end + + # get the bounding box back from the changeset + get :read, :id => changeset_id + assert_response :success, "Couldn't read back changeset for the third time." + assert_select "osm>changeset[min_lon=1]", 1 + assert_select "osm>changeset[max_lon=3]", 1 + assert_select "osm>changeset[min_lat=1]", 1 + assert_select "osm>changeset[max_lat=3]", 1 + end + + #------------------------------------------------------------ + # utility functions + #------------------------------------------------------------ + + ## + # update the changeset_id of a way element + def update_changeset(xml, changeset_id) + xml_attr_rewrite(xml, 'changeset', changeset_id) + end + + ## + # update an attribute in a way element + def xml_attr_rewrite(xml, name, value) + xml.find("//osm/way").first[name] = value.to_s + return xml + end + end diff --git a/test/unit/way_test.rb b/test/unit/way_test.rb new file mode 100644 index 000000000..cd565fd27 --- /dev/null +++ b/test/unit/way_test.rb @@ -0,0 +1,16 @@ +require File.dirname(__FILE__) + '/../test_helper' + +class WayTest < Test::Unit::TestCase + api_fixtures + + def test_bbox + node = current_nodes(:used_node_1) + [ :visible_way, + :invisible_way, + :used_way ].each do |way_symbol| + way = current_ways(way_symbol) + assert_equal node.bbox, way.bbox + end + end + +end