Close a number of holes in the API by making it validate changes
authorTom Hughes <tom@compton.nu>
Tue, 19 Jun 2007 23:20:39 +0000 (23:20 +0000)
committerTom Hughes <tom@compton.nu>
Tue, 19 Jun 2007 23:20:39 +0000 (23:20 +0000)
more carefully.

app/controllers/node_controller.rb
app/controllers/segment_controller.rb
app/controllers/way_controller.rb
app/models/node.rb
app/models/old_node.rb
app/models/old_segment.rb
app/models/old_way.rb
app/models/segment.rb
app/models/way.rb

index cc29c09e52422f90c167fb89734a5067de384bc1..f2a3ce3291cd48e2f7203d5457eb742fbb150a4e 100644 (file)
@@ -4,16 +4,10 @@ class NodeController < ApplicationController
   before_filter :authorize
   after_filter :compress_output
 
-  def create\r
+  def create
     response.headers["Content-Type"] = 'text/xml'
     if request.put?
-      node = nil
-      begin
-        node = Node.from_xml(request.raw_post, true)
-      rescue
-        render :text => "XML didn't parse", :status => 400 # if we got here the doc didnt parse
-        return
-      end
+      node = Node.from_xml(request.raw_post, true)
 
       if node
         node.user_id = @user.id
@@ -61,9 +55,13 @@ class NodeController < ApplicationController
 
     when :delete
       if node.visible
-        node.visible = 0
-        node.save_with_history
-        render :nothing => true
+        if Segment.find(:first, :conditions => [ "visible = 1 and (node_a = ? or node_b = ?)", node.id, node.id])
+          render :nothing => true, :status => HTTP_PRECONDITION_FAILED
+        else
+          node.visible = 0
+          node.save_with_history
+          render :nothing => true
+        end
       else
         render :nothing => true, :status => 410
       end
@@ -71,17 +69,21 @@ class NodeController < ApplicationController
     when :put
       new_node = Node.from_xml(request.raw_post)
 
-      node.timestamp = Time.now
-      node.user_id = @user.id
+      if new_node
+        node.timestamp = Time.now
+        node.user_id = @user.id
 
-      node.latitude = new_node.latitude 
-      node.longitude = new_node.longitude
-      node.tags = new_node.tags
+        node.latitude = new_node.latitude 
+        node.longitude = new_node.longitude
+        node.tags = new_node.tags
 
-      if node.id == new_node.id and node.save_with_history
-        render :nothing => true, :status => 200
+        if node.id == new_node.id and node.save_with_history
+          render :nothing => true
+        else
+          render :nothing => true, :status => 500
+        end
       else
-        render :nothing => true, :status => 500
+        render :nothing => true, :status => 400 # if we got here the doc didnt parse
       end
       return
     end
index a75ff911d703255830be5c2e30707076ee3fa054..92825115f616a38d92e185f125ed556db619067b 100644 (file)
@@ -31,7 +31,6 @@ class SegmentController < ApplicationController
           render :nothing => true, :status => 500
         end
         return
-
       else
         render :nothing => true, :status => 400 # if we got here the doc didnt parse
         return
@@ -58,9 +57,13 @@ class SegmentController < ApplicationController
 
     when :delete
       if segment.visible
-        segment.visible = 0
-        segment.save_with_history
-        render :nothing => true
+        if WaySegment.find(:first, :joins => "INNER JOIN current_ways ON current_ways.id = current_way_segments.id", :conditions => [ "current_ways.visible = 1 AND current_way_segments.segment_id = ?", segment.id ])
+          render :nothing => true, :status => HTTP_PRECONDITION_FAILED
+        else
+          segment.visible = 0
+          segment.save_with_history
+          render :nothing => true
+        end
       else
         render :nothing => true, :status => 410
       end
@@ -68,26 +71,32 @@ class SegmentController < ApplicationController
     when :put
       new_segment = Segment.from_xml(request.raw_post)
 
-      segment.timestamp = Time.now
-      segment.user_id = @user.id
-
-      if new_segment.node_a == new_segment.node_b
-        render :nothing => true, :status => HTTP_EXPECTATION_FAILED
-        return
-      end
+      if new_segment
+        if new_segment.node_a == new_segment.node_b
+          render :nothing => true, :status => HTTP_EXPECTATION_FAILED
+          return
+        end
+        
+        unless new_segment.preconditions_ok? # are the nodes visible?
+          render :nothing => true, :status => HTTP_PRECONDITION_FAILED
+          return
+        end
 
-      segment.node_a = new_segment.node_a
-      segment.node_b = new_segment.node_b
-      
-      segment.tags = new_segment.tags
-      segment.visible = new_segment.visible
+        segment.timestamp = Time.now
+        segment.user_id = @user.id
+        segment.node_a = new_segment.node_a
+        segment.node_b = new_segment.node_b
+        segment.tags = new_segment.tags
+        segment.visible = new_segment.visible
 
-      if segment.id == new_segment.id and segment.save_with_history
-        render :nothing => true, :status => HTTP_OK
+        if segment.id == new_segment.id and segment.save_with_history
+          render :nothing => true
+        else
+          render :nothing => true, :status => 500
+        end
       else
-        render :nothing => true, :status => 500
+        render :nothing => true, :status => 400 # if we got here the doc didnt parse
       end
-      return
     end
 
   end
index 5594382074a22727fad6381cc5990d165cb8f179..34f1ab5e7d3818f582df5664e6d3b3728f9ab189 100644 (file)
@@ -11,17 +11,16 @@ class WayController < ApplicationController
 
       if way
         way.user_id = @user.id
+
         unless way.preconditions_ok? # are the segments (and their nodes) visible?
-          render :nothing => true, :status => 412
+          render :nothing => true, :status => HTTP_PRECONDITION_FAILED
           return
         end
 
         if way.save_with_history
           render :text => way.id.to_s
-          return
         else
           render :nothing => true, :status => 500
-          return
         end
         return
       else
@@ -87,38 +86,36 @@ class WayController < ApplicationController
       render :text => way.to_xml.to_s
 
     when :delete
-      unless way.visible
+      if way.visible
+        way.visible = false
+        way.save_with_history
+        render :nothing => true
+      else
         render :nothing => true, :status => 410
-        return
       end
 
-      way.visible = false
-      way.save_with_history
-      render :nothing => true
-      return
     when :put
-      way = Way.from_xml(request.raw_post)
+      new_way = Way.from_xml(request.raw_post)
 
-      if way
-        way_in_db = Way.find(way.id)
-        if way_in_db
-          way_in_db.user_id = @user.id
-          way_in_db.tags = way.tags
-          way_in_db.segs = way.segs
-          way_in_db.timestamp = way.timestamp
-          way_in_db.visible = true
-          if way_in_db.save_with_history
-            render :text => way.id
-          else
-            render :nothing => true, :status => 500
-          end
+      if new_way
+        unless new_way.preconditions_ok? # are the segments (and their nodes) visible?
+          render :nothing => true, :status => HTTP_PRECONDITION_FAILED
           return
+        end
+
+        way.user_id = @user.id
+        way.tags = new_way.tags
+        way.segs = new_way.segs
+        way.timestamp = new_way.timestamp
+        way.visible = true
+
+        if way.id == new_way.id and way.save_with_history
+          render :nothing => true
         else
-          render :nothing => true, :status => 404 # way doesn't exist yet
+          render :nothing => true, :status => 500
         end
       else
         render :nothing => true, :status => 400 # if we got here the doc didnt parse
-        return
       end
     end
   end
index abf0c583e14a97ced1aed1cd69d8bd932a4beaaa..8b09ffef77d8cd3f4131da9faab05be65c1ac750 100644 (file)
@@ -2,59 +2,69 @@ class Node < ActiveRecord::Base
   require 'xml/libxml'
   set_table_name 'current_nodes'
   
-
-  validates_numericality_of :latitude
-  validates_numericality_of :longitude
-  # FIXME validate lat and lon within the world
+  validates_presence_of :user_id, :timestamp
+  validates_inclusion_of :visible, :in => [ true, false ]
+  validates_numericality_of :latitude, :longitude
+  validate :validate_position
 
   has_many :old_nodes, :foreign_key => :id
   belongs_to :user
 
-  def self.from_xml(xml, create=false)
-    p = XML::Parser.new
-    p.string = xml
-    doc = p.parse
-
-    node = Node.new
+  def validate_position
+    errors.add_to_base("Node is not in the world") unless in_world?
+  end
 
-    doc.find('//osm/node').each do |pt|
+  def in_world?
+    return false if self.latitude < -90 or self.latitude > 90
+    return false if self.longitude < -180 or self.longitude > 180
+    return true
+  end
 
+  def self.from_xml(xml, create=false)
+    begin
+      p = XML::Parser.new
+      p.string = xml
+      doc = p.parse
+  
+      node = Node.new
 
-      node.latitude = pt['lat'].to_f
-      node.longitude = pt['lon'].to_f
+      doc.find('//osm/node').each do |pt|
+        node.latitude = pt['lat'].to_f
+        node.longitude = pt['lon'].to_f
 
-      if node.latitude > 90 or node.latitude < -90 or node.longitude > 180 or node.longitude < -180
-        return nil
-      end
+        return nil unless node.in_world?
 
-      unless create
-        if pt['id'] != '0'
-          node.id = pt['id'].to_i
+        unless create
+          if pt['id'] != '0'
+            node.id = pt['id'].to_i
+          end
         end
-      end
 
-      node.visible = pt['visible'] and pt['visible'] == 'true'
+        node.visible = pt['visible'] and pt['visible'] == 'true'
 
-      if create
-        node.timestamp = Time.now
-      else
-        if pt['timestamp']
-          node.timestamp = Time.parse(pt['timestamp'])
+        if create
+          node.timestamp = Time.now
+        else
+          if pt['timestamp']
+            node.timestamp = Time.parse(pt['timestamp'])
+          end
         end
-      end
 
-      tags = []
-
-      pt.find('tag').each do |tag|
-        tags << [tag['k'],tag['v']]
-      end
+        tags = []
 
-      tags = tags.collect { |k,v| "#{k}=#{v}" }.join(';')
-      tags = '' if tags.nil?
+        pt.find('tag').each do |tag|
+          tags << [tag['k'],tag['v']]
+        end
 
-      node.tags = tags
+        tags = tags.collect { |k,v| "#{k}=#{v}" }.join(';')
+        tags = '' if tags.nil?
 
+        node.tags = tags
+      end
+    rescue
+      node = nil
     end
+
     return node
   end
 
@@ -62,12 +72,13 @@ class Node < ActiveRecord::Base
     begin
       Node.transaction do
         self.timestamp = Time.now
-        self.save
+        self.save!
         old_node = OldNode.from_node(self)
-        old_node.save
+        old_node.save!
       end
+
       return true
-    rescue Exception => ex
+    rescue
       return nil
     end
   end
index 0bb676ef51f379c4b2177f13da96f5d148e86744..d7a52c33617aaaedeaa4a0b1387555a8a701ac1c 100644 (file)
@@ -1,8 +1,23 @@
 class OldNode < ActiveRecord::Base
   set_table_name 'nodes'
+  
+  validates_presence_of :user_id, :timestamp
+  validates_inclusion_of :visible, :in => [ true, false ]
+  validates_numericality_of :latitude, :longitude
+  validate :validate_position
 
   belongs_to :user
 
+  def validate_position
+    errors.add_to_base("Node is not in the world") unless in_world?
+  end
+
+  def in_world?
+    return false if self.latitude < -90 or self.latitude > 90
+    return false if self.longitude < -180 or self.longitude > 180
+    return true
+  end
+
   def self.from_node(node)
     old_node = OldNode.new
     old_node.latitude = node.latitude
@@ -26,5 +41,4 @@ class OldNode < ActiveRecord::Base
     el1['timestamp'] = self.timestamp.xmlschema
     return el1
   end
-
 end
index 7eac278fe2d78176aac3a50d47d4b6189d22cac3..c243c522db628bfd24628feaab9d000b18843ef1 100644 (file)
@@ -1,6 +1,10 @@
 class OldSegment < ActiveRecord::Base
   set_table_name 'segments'
 
+  validates_presence_of :user_id, :timestamp
+  validates_inclusion_of :visible, :in => [ true, false ]
+  validates_numericality_of :node_a, :node_b
+
   belongs_to :user
 
   def self.from_segment(segment)
index 4a5362bc161d486f4be84d4b7f2ef5432009dfb8..06260425389c2c8db9ea84cc23c18000f609e962 100644 (file)
@@ -13,7 +13,7 @@ class OldWay < ActiveRecord::Base
     return old_way
   end
 
-  def save_with_dependencies
+  def save_with_dependencies!
 
     # dont touch this unless you really have figured out why it's called
     # (Rails doesn't deal well with the old ways table (called 'ways') because
@@ -21,7 +21,7 @@ class OldWay < ActiveRecord::Base
     # id and get it back but we have that and we want to get the 'version' back
     # we could add another column but thats a lot of data. No, set_primary_key
     # doesn't work either.
-    save()
+    save!
     clear_aggregation_cache
     clear_association_cache
     @attributes.update(OldWay.find(:first, :conditions => ['id = ? AND timestamp = ?', self.id, self.timestamp]).instance_variable_get('@attributes'))
@@ -34,7 +34,7 @@ class OldWay < ActiveRecord::Base
       tag.v = v
       tag.id = self.id
       tag.version = self.version
-      tag.save
+      tag.save!
     end
 
     i = 0
@@ -43,7 +43,7 @@ class OldWay < ActiveRecord::Base
       seg.id = self.id
       seg.segment_id = n
       seg.version = self.version
-      seg.save
+      seg.save!
     end
   end
 
index 246570b46ae1ee3db2b8491308b8d29ff951b175..aa4e07887cca2751301954848029dd9be60ead18 100644 (file)
@@ -2,8 +2,9 @@ class Segment < ActiveRecord::Base
   require 'xml/libxml'
   set_table_name 'current_segments'
 
-  validates_numericality_of :node_a
-  validates_numericality_of :node_b
+  validates_presence_of :user_id, :timestamp
+  validates_inclusion_of :visible, :in => [ true, false ]
+  validates_numericality_of :node_a, :node_b
 
   has_many :old_segments, :foreign_key => :id
   belongs_to :user
@@ -13,43 +14,48 @@ class Segment < ActiveRecord::Base
   belongs_to :to_node, :class_name => 'Node', :foreign_key => 'node_b'
 
   def self.from_xml(xml, create=false)
-    p = XML::Parser.new
-    p.string = xml
-    doc = p.parse
-
-    segment = Segment.new
+    begin
+      p = XML::Parser.new
+      p.string = xml
+      doc = p.parse
 
-    doc.find('//osm/segment').each do |pt|
+      segment = Segment.new
 
-      segment.node_a = pt['from'].to_i
-      segment.node_b = pt['to'].to_i
+      doc.find('//osm/segment').each do |pt|
+        segment.node_a = pt['from'].to_i
+        segment.node_b = pt['to'].to_i
 
-      if pt['id'] != '0'
-        segment.id = pt['id'].to_i
-      end
+        unless create
+          if pt['id'] != '0'
+            segment.id = pt['id'].to_i
+          end
+        end
 
-      segment.visible = true
+        segment.visible = true
 
-      if create
-        segment.timestamp = Time.now
-      else
-        if pt['timestamp']
-          segment.timestamp = Time.parse(pt['timestamp'])
+        if create
+          segment.timestamp = Time.now
+        else
+          if pt['timestamp']
+            segment.timestamp = Time.parse(pt['timestamp'])
+          end
         end
-      end
 
-      tags = []
+        tags = []
 
-      pt.find('tag').each do |tag|
-        tags << [tag['k'],tag['v']]
-      end
-
-      tags = tags.collect { |k,v| "#{k}=#{v}" }.join(';')
-      tags = '' if tags.nil?
+        pt.find('tag').each do |tag|
+          tags << [tag['k'],tag['v']]
+        end
 
-      segment.tags = tags
+        tags = tags.collect { |k,v| "#{k}=#{v}" }.join(';')
+        tags = '' if tags.nil?
 
+        segment.tags = tags
+      end
+    rescue
+      segment = nil
     end
+
     return segment
   end
 
@@ -57,12 +63,13 @@ class Segment < ActiveRecord::Base
     begin
       Segment.transaction do
         self.timestamp = Time.now
-        self.save
+        self.save!
         old_segment = OldSegment.from_segment(self)
-        old_segment.save
+        old_segment.save!
       end
+
       return true
-    rescue Exception => ex
+    rescue
       return nil
     end
   end
index d5d2dc98ef8b11ee92173c090862ccf7ad8aab44..5a26e221684fbcd7b6ac87d37bee4811c2365391 100644 (file)
@@ -11,34 +11,37 @@ class Way < ActiveRecord::Base
   set_table_name 'current_ways'
 
   def self.from_xml(xml, create=false)
-    p = XML::Parser.new
-    p.string = xml
-    doc = p.parse
+    begin
+      p = XML::Parser.new
+      p.string = xml
+      doc = p.parse
 
-    way = Way.new
+      way = Way.new
 
-    doc.find('//osm/way').each do |pt|
-      if !create and pt['id'] != '0'
-        way.id = pt['id'].to_i
-      end
+      doc.find('//osm/way').each do |pt|
+        if !create and pt['id'] != '0'
+          way.id = pt['id'].to_i
+        end
 
-      if create
-        way.timestamp = Time.now
-        way.visible = true
-      else
-        if pt['timestamp']
-          way.timestamp = Time.parse(pt['timestamp'])
+        if create
+          way.timestamp = Time.now
+          way.visible = true
+        else
+          if pt['timestamp']
+            way.timestamp = Time.parse(pt['timestamp'])
+          end
         end
-      end
 
-      pt.find('tag').each do |tag|
-        way.add_tag_keyval(tag['k'], tag['v'])
-      end
+        pt.find('tag').each do |tag|
+          way.add_tag_keyval(tag['k'], tag['v'])
+        end
 
-      pt.find('seg').each do |seg|
-        way.add_seg_num(seg['id'])
+        pt.find('seg').each do |seg|
+          way.add_seg_num(seg['id'])
+        end
       end
-
+    rescue
+      way = nil
     end
 
     return way
@@ -140,38 +143,47 @@ class Way < ActiveRecord::Base
   end
 
   def save_with_history
-    t = Time.now
-    self.timestamp = t
-    self.save
+    begin
+      Way.transaction do
+        t = Time.now
+        self.timestamp = t
+        self.save!
     
-    WayTag.delete_all(['id = ?', self.id])
-
-    self.tags.each do |k,v|
-      tag = WayTag.new
-      tag.k = k
-      tag.v = v
-      tag.id = self.id
-      tag.save
-    end
+        WayTag.delete_all(['id = ?', self.id])
+
+        self.tags.each do |k,v|
+          tag = WayTag.new
+          tag.k = k
+          tag.v = v
+          tag.id = self.id
+          tag.save!
+        end
 
-    WaySegment.delete_all(['id = ?', self.id])
+        WaySegment.delete_all(['id = ?', self.id])
     
-    i = 0
-    self.segs.each do |n|
-      seg = WaySegment.new
-      seg.id = self.id
-      seg.segment_id = n
-      seg.sequence_id = i
-      seg.save
-      i += 1
-    end
+        i = 0
+        self.segs.each do |n|
+          seg = WaySegment.new
+          seg.id = self.id
+          seg.segment_id = n
+          seg.sequence_id = i
+          seg.save!
+          i += 1
+        end
+
+        old_way = OldWay.from_way(self)
+        old_way.timestamp = t
+        old_way.save_with_dependencies!
+      end
 
-    old_way = OldWay.from_way(self)
-    old_way.timestamp = t
-    old_way.save_with_dependencies
+      return true
+    rescue
+      return nil
+    end
   end
 
   def preconditions_ok?
+    return false if self.segs.empty?
     self.segs.each do |n|
       segment = Segment.find(n)
       unless segment and segment.visible and segment.preconditions_ok?