From: Steve Coast <steve@asklater.com>
Date: Fri, 25 Jan 2008 14:32:45 +0000 (+0000)
Subject: dont eager load tags (false primary key fucks all sorts of things up) and move delete... 
X-Git-Tag: live~7909
X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/bbd769304cf29bbd9574fd3c7167feb7d5d0aa17

dont eager load tags (false primary key fucks all sorts of things up) and move delete way logic to model so that amf_controller can use it (plan is to do this with all of the methods. sigh.)
---

diff --git a/app/controllers/amf_controller.rb b/app/controllers/amf_controller.rb
index 876dbd10e..471e2fada 100644
--- a/app/controllers/amf_controller.rb
+++ b/app/controllers/amf_controller.rb
@@ -475,21 +475,21 @@ class AmfController < ApplicationController
   end
 
   # ----- getpoi
-  #		  read POI from database
+  # read POI from database
   #		  (only called on revert: POIs are usually read by whichways)
   #		  in:	[0] node id, [1] baselong, [2] basey, [3] masterscale
   #		  does: reads POI
   #		  out:	[0] id (unchanged), [1] projected long, [2] projected lat,
   #				[3] hash of tags
   def getpoi(args) #:doc:
-    id,baselong,basey,masterscale=args; id=id.to_i
-    poi=ActiveRecord::Base.connection.select_one("SELECT latitude*0.0000001 AS lat,longitude*0.0000001 AS lng,tags "+
-    "FROM current_nodes WHERE visible=1 AND id=#{id}")
-    if poi.nil? then return [nil,nil,nil,''] end
-    [id,
-      long2coord(poi['lng'].to_f,baselong,masterscale),
-      lat2coord(poi['lat'].to_f,basey,masterscale),
-      tag2array(poi['tags'])]
+    id,baselong,basey,masterscale = args
+    
+    n = Node.find(id.to_i)
+    if n
+      return [n.id, n.long_potlatch(baselong,masterscale), n.lat_potlatch(basey,masterscale), n.tags_as_hash]
+    else
+      return [nil,nil,nil,'']
+    end
   end
 
   # ----- deleteway
diff --git a/app/controllers/way_controller.rb b/app/controllers/way_controller.rb
index 25a1231db..c3640609a 100644
--- a/app/controllers/way_controller.rb
+++ b/app/controllers/way_controller.rb
@@ -69,30 +69,21 @@ class WayController < ApplicationController
     end
   end
 
+  # This is the API call to delete a way
   def delete
     begin
       way = Way.find(params[:id])
-
-      if way.visible
-        # omg FIXME
-        if RelationMember.find(:first, :joins => "INNER JOIN current_relations ON current_relations.id=current_relation_members.id", :conditions => [ "visible = 1 AND member_type='way' and member_id=?", params[:id]])
-          render :text => "", :status => :precondition_failed
-        else
-          way.user_id = @user.id
-          way.tags = []
-          way.nds = []
-          way.visible = false
-	  way.save_with_history!
-
-	  render :nothing => true
-        end
-      else
-        render :text => "", :status => :gone
-      end
+      way.delete_with_relations_and_history(@user)
+
+      # if we get here, all is fine, otherwise something will catch below.  
+      render :nothing => true
+      return
+    rescue OSM::APIAlreadyDeletedError
+      render :text => "", :status => :gone
+    rescue OSM::APIPreconditionFailedError
+      render :text => "", :status => :precondition_failed
     rescue ActiveRecord::RecordNotFound
       render :nothing => true, :status => :not_found
-    rescue => ex
-      puts ex
     end
   end
 
diff --git a/app/models/way.rb b/app/models/way.rb
index 2c9ce9f72..924fffe5c 100644
--- a/app/models/way.rb
+++ b/app/models/way.rb
@@ -52,7 +52,9 @@ class Way < ActiveRecord::Base
   
   # You can't pull in all the tags too unless we put a sequence_id on the way_tags table and have a multipart key
   def self.find_eager(id)
-    way = Way.find(id, :include => [:way_tags, {:way_nodes => :node}])
+    way = Way.find(id, :include => {:way_nodes => :node})
+    #If waytag had a multipart key that was real, you could do this:
+    #way = Way.find(id, :include => [:way_tags, {:way_nodes => :node}])
   end
 
   # Find a way given it's ID, and in a single SQL call also grab its nodes and tags
@@ -204,4 +206,22 @@ class Way < ActiveRecord::Base
     return true
   end
 
+  # Delete the way and it's relations, but don't really delete it - set its visibility to false and update the history etc to maintain wiki-like functionality.
+  def delete_with_relations_and_history(user)
+    if self.visible
+      # omg FIXME
+      if RelationMember.find(:first, :joins => "INNER JOIN current_relations ON current_relations.id=current_relation_members.id",
+                             :conditions => [ "visible = 1 AND member_type='way' and member_id=?", self.id])
+        raise OSM::APIPreconditionFailedError
+      else
+        self.user_id = user.id
+        self.tags = []
+        self.nds = []
+        self.visible = false
+        self.save_with_history!
+      end
+    else
+      raise OSM::APIAlreadyDeletedError
+    end
+  end
 end
diff --git a/app/models/way_tag.rb b/app/models/way_tag.rb
index 3f6ac8ce3..4548674d4 100644
--- a/app/models/way_tag.rb
+++ b/app/models/way_tag.rb
@@ -1,8 +1,9 @@
 class WayTag < ActiveRecord::Base
   set_table_name 'current_way_tags'
 
-  # false multipart key
-  set_primary_keys :id, :k, :v
+  # False multipart keys. The following would be a hack:
+  # set_primary_keys :id, :k, :v
+  # FIXME add a real multipart key to waytags so that we can do eager loadin
 
   belongs_to :way, :foreign_key => 'id'
 end
diff --git a/lib/osm.rb b/lib/osm.rb
index 24df7a35d..bd9351026 100644
--- a/lib/osm.rb
+++ b/lib/osm.rb
@@ -1,13 +1,6 @@
+# The OSM module provides support functions for OSM.
 module OSM
 
-  # This piece of magic reads a GPX with SAX and spits out
-  # lat/lng and stuff
-  #
-  # This would print every latitude value:
-  #
-  # gpx = OSM::GPXImporter.new('somefile.gpx')
-  # gpx.points {|p| puts p['latitude']}
-
   require 'time'
   require 'rexml/parsers/sax2parser'
   require 'rexml/text'
@@ -15,11 +8,27 @@ module OSM
   require 'digest/md5'
   require 'RMagick'
 
+  # The base class for API Errors.
+  class APIError < RuntimeError
+  end
+
+  # Raised when an API object is not found.
+  class APINotFoundError < APIError
+  end
+
+  # Raised when a precondition to an API action fails sanity check.
+  class APIPreconditionFailedError < APIError
+  end
+
+  # Raised when to delete an already-deleted object.
+  class APIAlreadyDeletedError < APIError
+  end
+
+  # Helper methods for going to/from mercator and lat/lng.
   class Mercator
     include Math
 
     #init me with your bounding box and the size of your image
-
     def initialize(min_lat, min_lon, max_lat, max_lon, width, height)
       xsize = xsheet(max_lon) - xsheet(min_lon)
       ysize = ysheet(max_lat) - ysheet(min_lat)
@@ -62,6 +71,13 @@ module OSM
   end
 
 
+  # This piece of magic reads a GPX with SAX and spits out
+  # lat/lng and stuff
+  #
+  # This would print every latitude value:
+  #
+  # gpx = OSM::GPXImporter.new('somefile.gpx')
+  # gpx.points {|p| puts p['latitude']}
   class GPXImporter
     # FIXME swap REXML for libXML
     attr_reader :possible_points