Migration to add close-time to changesets. This replaces the boolean 'open' attribute...

diff --git a/app/controllers/changeset_controller.rb b/app/controllers/changeset_controller.rb
index e16a4e9b37a1f6fa1a537d809f7588421936163f..181c827b35a91e49657db56941c58e318c76507a 100644 (file)
--- a/app/controllers/changeset_controller.rb
+++ b/app/controllers/changeset_controller.rb
@@ -53,7 +53,11 @@ class ChangesetController < ApplicationController
       raise OSM::APIUserChangesetMismatchError 
     end
     
       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
     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) }
         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
       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
       end
     else
       return nil


@@ -380,7 +384,7 @@ class ChangesetController < ApplicationController
   ##
   # restrict changes to those which are open
   def conditions_open(open)
   ##
   # 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
   end
 
 end

diff --git a/app/models/changeset.rb b/app/models/changeset.rb
index 047569dca5cf15db682c9c1fbd8fe72a85b3a942..f67d2118d4382215c7b28460a993a6153f7de98e 100644 (file)
--- 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
   
   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
 
   
   # 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?
   # 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)
   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
       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|
         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
 
     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
   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
     # 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
       self.save!
 
       tags = self.tags


@@ -155,7 +188,8 @@ class Changeset < ActiveRecord::Base
     end
     
     el1['created_at'] = self.created_at.xmlschema
     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?
 
     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
     end
     
     # can't change a closed changeset

-    unless open
+    unless is_open?

       raise OSM::APIChangesetAlreadyClosedError
     end
 
       raise OSM::APIChangesetAlreadyClosedError
     end
 

diff --git a/app/models/node.rb b/app/models/node.rb
index faba4ed66d5e37817fc4821c4947d76aecbc03cc..e926e06a210baa326842920a2c113f39085aee05 100644 (file)
--- 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!
 
       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
       # 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 b94aef9ae9bb3946e5c4f4caad0971d37e0f7364..2607e7f2fd3c0f17ecb314730248839f494e7f52 100644 (file)
--- a/app/models/relation.rb
+++ b/app/models/relation.rb
@@ -307,6 +307,9 @@ class Relation < ActiveRecord::Base
         end
       end
 
         end
       end
 

+      # tell the changeset we updated one element only
+      changeset.add_changes! 1
+

       # save the (maybe updated) changeset bounding box
       changeset.save!
     end
       # save the (maybe updated) changeset bounding box
       changeset.save!
     end

diff --git a/app/models/way.rb b/app/models/way.rb
index 0c27e54606414e3e0986036a281955b25f13528c..3b3f9243283e64eee1ec27b7d34ac97e2db7d0df 100644 (file)
--- 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?
       # 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
       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 (file)
index 0000000..b87ce3f
--- /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 ed0df02288843f4fc8c5347ebd9d91661da5cc3c..defd691d2c773d67a4579cb37273fa72660fcdf0 100644 (file)
--- 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"
   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 %>
   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"
   
 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"
 
 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"
 
 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 %>
   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
 
 # 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"
   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 8ccdec889505f3166f9c846e8698abfd1a7249c8..3d7531cfba0c79c87a885379292af531af2b61a6 100644 (file)
--- 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"
 
     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"
 
     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"
 
     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
 
   ##
   end
 
   ##


@@ -700,6 +704,62 @@ EOF
     assert_response :conflict
   end
 
     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 "<osm><changeset/></osm>"
+    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 "<osm><node changeset='#{cs_id}' lat='0.0' lon='0.0'/></osm>"
+      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
   #------------------------------------------------------------
   #------------------------------------------------------------
   # utility functions
   #------------------------------------------------------------