From c11d961f624cb610c4b8184d24e522b2c093d1ea Mon Sep 17 00:00:00 2001 From: Tom Hughes Date: Tue, 19 Jun 2007 23:20:39 +0000 Subject: [PATCH 1/1] Close a number of holes in the API by making it validate changes more carefully. --- app/controllers/node_controller.rb | 40 +++++----- app/controllers/segment_controller.rb | 49 +++++++----- app/controllers/way_controller.rb | 47 ++++++------ app/models/node.rb | 87 +++++++++++---------- app/models/old_node.rb | 16 +++- app/models/old_segment.rb | 4 + app/models/old_way.rb | 8 +- app/models/segment.rb | 69 +++++++++-------- app/models/way.rb | 104 ++++++++++++++------------ 9 files changed, 240 insertions(+), 184 deletions(-) diff --git a/app/controllers/node_controller.rb b/app/controllers/node_controller.rb index cc29c09e5..f2a3ce329 100644 --- a/app/controllers/node_controller.rb +++ b/app/controllers/node_controller.rb @@ -4,16 +4,10 @@ class NodeController < ApplicationController before_filter :authorize after_filter :compress_output - def create + 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 diff --git a/app/controllers/segment_controller.rb b/app/controllers/segment_controller.rb index a75ff911d..92825115f 100644 --- a/app/controllers/segment_controller.rb +++ b/app/controllers/segment_controller.rb @@ -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 diff --git a/app/controllers/way_controller.rb b/app/controllers/way_controller.rb index 559438207..34f1ab5e7 100644 --- a/app/controllers/way_controller.rb +++ b/app/controllers/way_controller.rb @@ -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 diff --git a/app/models/node.rb b/app/models/node.rb index abf0c583e..8b09ffef7 100644 --- a/app/models/node.rb +++ b/app/models/node.rb @@ -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 diff --git a/app/models/old_node.rb b/app/models/old_node.rb index 0bb676ef5..d7a52c336 100644 --- a/app/models/old_node.rb +++ b/app/models/old_node.rb @@ -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 diff --git a/app/models/old_segment.rb b/app/models/old_segment.rb index 7eac278fe..c243c522d 100644 --- a/app/models/old_segment.rb +++ b/app/models/old_segment.rb @@ -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) diff --git a/app/models/old_way.rb b/app/models/old_way.rb index 4a5362bc1..062604253 100644 --- a/app/models/old_way.rb +++ b/app/models/old_way.rb @@ -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 diff --git a/app/models/segment.rb b/app/models/segment.rb index 246570b46..aa4e07887 100644 --- a/app/models/segment.rb +++ b/app/models/segment.rb @@ -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 diff --git a/app/models/way.rb b/app/models/way.rb index d5d2dc98e..5a26e2216 100644 --- a/app/models/way.rb +++ b/app/models/way.rb @@ -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? -- 2.43.2