From: Matt Amos Date: Mon, 17 Nov 2008 19:00:01 +0000 (+0000) Subject: Migration to add close-time to changesets. This replaces the boolean 'open' attribute... X-Git-Tag: live~7577^2~141 X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/a90be5e69a478e2b49ae676b649a78589f85a50e Migration to add close-time to changesets. This replaces the boolean 'open' attribute. Added checks to ensure that the maximum lifetime and number of changes in a changeset are enforced. Added some tests. --- diff --git a/app/controllers/changeset_controller.rb b/app/controllers/changeset_controller.rb index e16a4e9b3..181c827b3 100644 --- a/app/controllers/changeset_controller.rb +++ b/app/controllers/changeset_controller.rb @@ -53,7 +53,11 @@ class ChangesetController < ApplicationController raise OSM::APIUserChangesetMismatchError end - changeset.open = false + # to close the changeset, we'll just set its closed_at time to + # now. this might not be enough if there are concurrency issues, + # but we'll have to wait and see. + changeset.closed_at = DateTime.now + changeset.save! render :nothing => true rescue ActiveRecord::RecordNotFound @@ -361,10 +365,10 @@ class ChangesetController < ApplicationController raise OSM::APIBadUserInput.new("bad time range") if times.size != 2 from, to = times.collect { |t| DateTime.parse(t) } - return ['created_at > ? and created_at < ?', from, to] + return ['closed_at >= ? and created_at <= ?', from, to] else # if there is no comma, assume its a lower limit on time - return ['created_at > ?', DateTime.parse(time)] + return ['closed_at >= ?', DateTime.parse(time)] end else return nil @@ -380,7 +384,7 @@ class ChangesetController < ApplicationController ## # restrict changes to those which are open def conditions_open(open) - return open.nil? ? nil : ['open = ?', true] + return open.nil? ? nil : ['closed_at >= ?', DateTime.now] end end diff --git a/app/models/changeset.rb b/app/models/changeset.rb index 047569dca..f67d2118d 100644 --- a/app/models/changeset.rb +++ b/app/models/changeset.rb @@ -12,17 +12,31 @@ class Changeset < ActiveRecord::Base has_many :old_ways has_many :old_relations - validates_presence_of :user_id, :created_at - validates_inclusion_of :open, :in => [ true, false ] + validates_presence_of :user_id, :created_at, :closed_at # over-expansion factor to use when updating the bounding box EXPAND = 0.1 + # maximum number of elements allowed in a changeset + MAX_ELEMENTS = 50000 + + # maximum time a changeset is allowed to be open for (note that this + # is in days - so one hour is Rational(1,24)). + MAX_TIME_OPEN = 1 + + # idle timeout increment, one hour as a rational number of days. + IDLE_TIMEOUT = Rational(1,24) + # 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 def is_open? - return open + # a changeset is open (that is, it will accept further changes) when + # it has not yet run out of time and its capacity is small enough. + # note that this may not be a hard limit - due to timing changes and + # concurrency it is possible that some changesets may be slightly + # longer than strictly allowed or have slightly more changes in them. + return ((closed_at > DateTime.now) and (num_changes <= MAX_ELEMENTS)) end def self.from_xml(xml, create=false) @@ -36,6 +50,11 @@ class Changeset < ActiveRecord::Base doc.find('//osm/changeset').each do |pt| if create cs.created_at = Time.now + # initial close time is 1h ahead, but will be increased on each + # modification. + cs.closed_at = Time.now + IDLE_TIMEOUT + # initially we have no changes in a changeset + cs.num_changes = 0 end pt.find('tag').each do |tag| @@ -78,6 +97,14 @@ class Changeset < ActiveRecord::Base self.min_lon, self.min_lat, self.max_lon, self.max_lat = @bbox end + ## + # the number of elements is also passed in so that we can ensure that + # a single changeset doesn't contain too many elements. this, of course, + # destroys the optimisation described in the bbox method above. + def add_changes!(elements) + self.num_changes += elements + end + def tags_as_hash return tags end @@ -107,8 +134,14 @@ class Changeset < ActiveRecord::Base # do the changeset update and the changeset tags update in the # same transaction to ensure consistency. Changeset.transaction do - # fixme update modified_at time? - # FIXME there is no modified_at time, should it be added + # set the auto-close time to be one hour in the future unless + # that would make it more than 24h long, in which case clip to + # 24h, as this has been decided is a reasonable time limit. + if (closed_at - created_at) > (MAX_TIME_OPEN - IDLE_TIMEOUT) + self.closed_at = created_at + MAX_TIME_OPEN + else + self.closed_at = DateTime.now + IDLE_TIMEOUT + end self.save! tags = self.tags @@ -155,7 +188,8 @@ class Changeset < ActiveRecord::Base end el1['created_at'] = self.created_at.xmlschema - el1['open'] = self.open.to_s + el1['closed_at'] = self.closed_at.xmlschema unless is_open? + el1['open'] = is_open?.to_s el1['min_lon'] = (bbox[0].to_f / GeoRecord::SCALE).to_s unless bbox[0].nil? el1['min_lat'] = (bbox[1].to_f / GeoRecord::SCALE).to_s unless bbox[1].nil? @@ -180,7 +214,7 @@ class Changeset < ActiveRecord::Base end # can't change a closed changeset - unless open + unless is_open? raise OSM::APIChangesetAlreadyClosedError end diff --git a/app/models/node.rb b/app/models/node.rb index faba4ed66..e926e06a2 100644 --- a/app/models/node.rb +++ b/app/models/node.rb @@ -141,6 +141,9 @@ class Node < ActiveRecord::Base old_node.timestamp = t old_node.save_with_dependencies! + # tell the changeset we updated one element only + changeset.add_changes! 1 + # save the changeset in case of bounding box updates changeset.save! end diff --git a/app/models/relation.rb b/app/models/relation.rb index b94aef9ae..2607e7f2f 100644 --- a/app/models/relation.rb +++ b/app/models/relation.rb @@ -307,6 +307,9 @@ class Relation < ActiveRecord::Base end end + # tell the changeset we updated one element only + changeset.add_changes! 1 + # save the (maybe updated) changeset bounding box changeset.save! end diff --git a/app/models/way.rb b/app/models/way.rb index 0c27e5460..3b3f92432 100644 --- a/app/models/way.rb +++ b/app/models/way.rb @@ -233,6 +233,10 @@ class Way < ActiveRecord::Base # 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? + + # tell the changeset we updated one element only + changeset.add_changes! 1 + changeset.save! end end diff --git a/db/migrate/023_add_end_time_to_changesets.rb b/db/migrate/023_add_end_time_to_changesets.rb new file mode 100644 index 000000000..b87ce3fde --- /dev/null +++ b/db/migrate/023_add_end_time_to_changesets.rb @@ -0,0 +1,34 @@ +class AddEndTimeToChangesets < ActiveRecord::Migration + def self.up + # swap the boolean closed-or-not for a time when the changeset will + # close or has closed. + add_column(:changesets, :closed_at, :datetime, :null => false) + + # it appears that execute will only accept string arguments, so + # this is an ugly, ugly hack to get some sort of mysql/postgres + # independence. now i have to go wash my brain with bleach. + execute("update changesets set closed_at=(now()-'1 hour') where open=(1=0)") + execute("update changesets set closed_at=(now()+'1 hour') where open=(1=1)") + + # remove the open column as it is unnecessary now and denormalises + # the table. + remove_column :changesets, :open + + # add a column to keep track of the number of changes in a changeset. + # could probably work out how many changes there are here, but i'm not + # sure its actually important. + add_column(:changesets, :num_changes, :integer, + :null => false, :default => 0) + end + + def self.down + # in the reverse direction, we can look at the closed_at to figure out + # if changesets are closed or not. + add_column(:changesets, :open, :boolean, :null => false, :default => true) + execute("update changesets set open=(closed_at > now())") + remove_column :changesets, :closed_at + + # remove the column for tracking number of changes + remove_column :changesets, :num_changes + end +end diff --git a/test/fixtures/changesets.yml b/test/fixtures/changesets.yml index ed0df0228..defd691d2 100644 --- a/test/fixtures/changesets.yml +++ b/test/fixtures/changesets.yml @@ -7,33 +7,37 @@ normal_user_first_change: id: 1 user_id: 1 created_at: "2007-01-01 00:00:00" - open: true + closed_at: <%= DateTime.now + Rational(1,24) %> min_lon: <%= 1 * SCALE %> min_lat: <%= 1 * SCALE %> max_lon: <%= 5 * SCALE %> max_lat: <%= 5 * SCALE %> + num_changes: 11 second_user_first_change: id: 2 user_id: 2 created_at: "2008-05-01 01:23:45" - open: true + closed_at: <%= DateTime.now + Rational(1,24) %> + num_changes: 0 normal_user_closed_change: id: 3 user_id: 1 created_at: "2007-01-01 00:00:00" - open: false + closed_at: "2007-01-02 00:00:00" + num_changes: 0 normal_user_version_change: id: 4 user_id: 1 created_at: "2008-01-01 00:00:00" - open: true + closed_at: <%= DateTime.now + Rational(1,24) %> min_lon: <%= 1 * SCALE %> min_lat: <%= 1 * SCALE %> max_lon: <%= 4 * SCALE %> max_lat: <%= 4 * SCALE %> + num_changes: 8 # changeset to contain all the invalid stuff that is in the # fixtures (nodes outside the world, etc...), but needs to have @@ -42,5 +46,5 @@ invalid_changeset: id: 5 user_id: 3 created_at: "2008-01-01 00:00:00" - open: false - \ No newline at end of file + closed_at: "2008-01-02 00:00:00" + num_changes: 9 diff --git a/test/functional/changeset_controller_test.rb b/test/functional/changeset_controller_test.rb index 8ccdec889..3d7531cfb 100644 --- a/test/functional/changeset_controller_test.rb +++ b/test/functional/changeset_controller_test.rb @@ -618,15 +618,19 @@ EOF get :query, :time => '2007-12-31' assert_response :success, "can't get changesets by time-since" - assert_changesets [2,4,5] + assert_changesets [1,2,4,5] get :query, :time => '2008-01-01T12:34Z' assert_response :success, "can't get changesets by time-since with hour" - assert_changesets [2] + assert_changesets [1,2,4,5] get :query, :time => '2007-12-31T23:59Z,2008-01-01T00:01Z' assert_response :success, "can't get changesets by time-range" - assert_changesets [4,5] + assert_changesets [1,4,5] + + get :query, :open => 'true' + assert_response :success, "can't get changesets by open-ness" + assert_changesets [1,2,4] end ## @@ -700,6 +704,62 @@ EOF assert_response :conflict end + ## + # check that a changeset can contain a certain max number of changes. + def test_changeset_limits + basic_authorization "test@openstreetmap.org", "test" + + # open a new changeset + content "" + put :create + assert_response :success, "can't create a new changeset" + cs_id = @response.body.to_i + + # start the counter just short of where the changeset should finish. + offset = 10 + # alter the database to set the counter on the changeset directly, + # otherwise it takes about 6 minutes to fill all of them. + changeset = Changeset.find(cs_id) + changeset.num_changes = Changeset::MAX_ELEMENTS - offset + changeset.save! + + with_controller(NodeController.new) do + # create a new node + content "" + put :create + assert_response :success, "can't create a new node" + node_id = @response.body.to_i + + get :read, :id => node_id + assert_response :success, "can't read back new node" + node_doc = XML::Parser.string(@response.body).parse + node_xml = node_doc.find("//osm/node").first + + # loop until we fill the changeset with nodes + offset.times do |i| + node_xml['lat'] = rand.to_s + node_xml['lon'] = rand.to_s + node_xml['version'] = (i+1).to_s + + content node_doc + put :update, :id => node_id + assert_response :success, "attempt #{i} should have succeeded" + end + + # trying again should fail + node_xml['lat'] = rand.to_s + node_xml['lon'] = rand.to_s + node_xml['version'] = offset.to_s + + content node_doc + put :update, :id => node_id + assert_response :conflict, "final attempt should have failed" + end + + changeset = Changeset.find(cs_id) + assert_equal Changeset::MAX_ELEMENTS + 1, changeset.num_changes + end + #------------------------------------------------------------ # utility functions #------------------------------------------------------------