Merge branch 'p' of https://github.com/jfirebaugh/openstreetmap-website into jfirebaugh-p
authorAndy Allan <git@gravitystorm.co.uk>
Wed, 22 Nov 2017 10:47:18 +0000 (10:47 +0000)
committerAndy Allan <git@gravitystorm.co.uk>
Wed, 22 Nov 2017 10:47:18 +0000 (10:47 +0000)
Refs #139

1  2 
app/controllers/api_controller.rb
app/controllers/relation_controller.rb
app/controllers/search_controller.rb
app/models/relation.rb

@@@ -1,18 -1,20 +1,18 @@@
  class ApiController < ApplicationController
 -
 -  skip_before_filter :verify_authenticity_token
 -  before_filter :check_api_readable, :except => [:capabilities]
 -  before_filter :setup_user_auth, :only => [:permissions]
 -  after_filter :compress_output
 -  around_filter :api_call_handle_error, :api_call_timeout
 +  skip_before_action :verify_authenticity_token
 +  before_action :check_api_readable, :except => [:capabilities]
 +  before_action :setup_user_auth, :only => [:permissions]
 +  around_action :api_call_handle_error, :api_call_timeout
  
    # Get an XML response containing a list of tracepoints that have been uploaded
    # within the specified bounding box, and in the specified page.
    def trackpoints
 -    #retrieve the page number
 -    page = params['page'].to_s.to_i
 +    # retrieve the page number
 +    page = params["page"].to_s.to_i
  
      unless page >= 0
 -        report_error("Page number must be greater than or equal to 0")
 -        return
 +      report_error("Page number must be greater than or equal to 0")
 +      return
      end
  
      offset = page * TRACEPOINTS_PER_PAGE
@@@ -24,7 -26,7 +24,7 @@@
        bbox = BoundingBox.from_bbox_params(params)
        bbox.check_boundaries
        bbox.check_size
 -    rescue Exception => err
 +    rescue StandardError => err
        report_error(err.message)
        return
      end
  
      doc = XML::Document.new
      doc.encoding = XML::Encoding::UTF_8
 -    root = XML::Node.new 'gpx'
 -    root['version'] = '1.0'
 -    root['creator'] = 'OpenStreetMap.org'
 -    root['xmlns'] = "http://www.topografix.com/GPX/1/0"
 -    
 +    root = XML::Node.new "gpx"
 +    root["version"] = "1.0"
 +    root["creator"] = "OpenStreetMap.org"
 +    root["xmlns"] = "http://www.topografix.com/GPX/1/0"
 +
      doc.root = root
  
      # initialise these variables outside of the loop so that they
          gpx_file = Trace.find(gpx_id)
  
          if gpx_file.trackable?
 -          track = XML::Node.new 'trk'
 +          track = XML::Node.new "trk"
            doc.root << track
            timestamps = true
  
            if gpx_file.identifiable?
              track << (XML::Node.new("name") << gpx_file.name)
              track << (XML::Node.new("desc") << gpx_file.description)
 -            track << (XML::Node.new("url") << url_for(:controller => 'trace', :action => 'view', :display_name => gpx_file.user.display_name, :id => gpx_file.id))
 +            track << (XML::Node.new("url") << url_for(:controller => "trace", :action => "view", :display_name => gpx_file.user.display_name, :id => gpx_file.id))
            end
          else
            # use the anonymous track segment if the user hasn't allowed
            # their GPX points to be tracked.
            timestamps = false
 -          if anon_track.nil? 
 -            anon_track = XML::Node.new 'trk'
 +          if anon_track.nil?
 +            anon_track = XML::Node.new "trk"
              doc.root << anon_track
            end
            track = anon_track
          end
        end
 -      
 +
        if trackid != point.trackid
          if gpx_file.trackable?
 -          trkseg = XML::Node.new 'trkseg'
 +          trkseg = XML::Node.new "trkseg"
            track << trkseg
            trackid = point.trackid
          else
 -          if anon_trkseg.nil? 
 -            anon_trkseg = XML::Node.new 'trkseg'
 +          if anon_trkseg.nil?
 +            anon_trkseg = XML::Node.new "trkseg"
              anon_track << anon_trkseg
            end
            trkseg = anon_trkseg
  
      response.headers["Content-Disposition"] = "attachment; filename=\"tracks.gpx\""
  
 -    render :text => doc.to_s, :content_type => "text/xml"
 +    render :xml => doc.to_s
    end
  
 -  # This is probably the most common call of all. It is used for getting the 
 +  # This is probably the most common call of all. It is used for getting the
    # OSM data for a specified bounding box, usually for editing. First the
 -  # bounding box (bbox) is checked to make sure that it is sane. All nodes 
 +  # bounding box (bbox) is checked to make sure that it is sane. All nodes
    # are searched, then all the ways that reference those nodes are found.
    # All Nodes that are referenced by those ways are fetched and added to the list
    # of nodes.
    # Then all the relations that reference the already found nodes and ways are
 -  # fetched. All the nodes and ways that are referenced by those ways are then 
 +  # fetched. All the nodes and ways that are referenced by those ways are then
    # fetched. Finally all the xml is returned.
    def map
      # Figure out the bbox
        bbox = BoundingBox.from_bbox_params(params)
        bbox.check_boundaries
        bbox.check_size
 -    rescue Exception => err
 +    rescue StandardError => err
        report_error(err.message)
        return
      end
  
 -    @nodes = Node.bbox(bbox).where(:visible => true).includes(:node_tags).limit(MAX_NUMBER_OF_NODES+1)
 -    # get all the nodes, by tag not yet working, waiting for change from NickB
 -    # need to be @nodes (instance var) so tests in /spec can be performed
 -    #@nodes = Node.search(bbox, params[:tag])
 +    nodes = Node.bbox(bbox).where(:visible => true).includes(:node_tags).limit(MAX_NUMBER_OF_NODES + 1)
  
 -    node_ids = @nodes.collect(&:id)
 +    node_ids = nodes.collect(&:id)
      if node_ids.length > MAX_NUMBER_OF_NODES
        report_error("You requested too many nodes (limit is #{MAX_NUMBER_OF_NODES}). Either request a smaller area, or use planet.osm")
        return
      end
 -    if node_ids.length == 0
 -      render :text => "<osm version='#{API_VERSION}' generator='#{GENERATOR}'></osm>", :content_type => "text/xml"
 -      return
 -    end
  
      doc = OSM::API.new.get_xml_doc
  
      # add bounds
 -    doc.root << bbox.add_bounds_to(XML::Node.new 'bounds')
 +    doc.root << bbox.add_bounds_to(XML::Node.new("bounds"))
  
      # get ways
      # find which ways are needed
 -    ways = Array.new
 -    if node_ids.length > 0
 -      way_nodes = WayNode.find_all_by_node_id(node_ids)
 +    ways = []
 +    if node_ids.empty?
 +      list_of_way_nodes = []
 +    else
 +      way_nodes = WayNode.where(:node_id => node_ids)
        way_ids = way_nodes.collect { |way_node| way_node.id[0] }
 -      ways = Way.find(way_ids, :include => [:way_nodes, :way_tags])
 +      ways = Way.preload(:way_nodes, :way_tags).find(way_ids)
  
 -      list_of_way_nodes = ways.collect { |way|
 -        way.way_nodes.collect { |way_node| way_node.node_id }
 -      }
 +      list_of_way_nodes = ways.collect do |way|
 +        way.way_nodes.collect(&:node_id)
 +      end
        list_of_way_nodes.flatten!
 -
 -    else
 -      list_of_way_nodes = Array.new
      end
  
      # - [0] in case some thing links to node 0 which doesn't exist. Shouldn't actually ever happen but it does. FIXME: file a ticket for this
      nodes_to_fetch = (list_of_way_nodes.uniq - node_ids) - [0]
  
 -    if nodes_to_fetch.length > 0
 -      @nodes += Node.includes(:node_tags).find(nodes_to_fetch)
 +    unless nodes_to_fetch.empty?
 +      nodes += Node.includes(:node_tags).find(nodes_to_fetch)
      end
  
      visible_nodes = {}
      changeset_cache = {}
      user_display_name_cache = {}
  
 -    @nodes.each do |node|
 +    nodes.each do |node|
        if node.visible?
          doc.root << node.to_xml_node(changeset_cache, user_display_name_cache)
          visible_nodes[node.id] = node
        end
      end
  
 -    way_ids = Array.new
 +    way_ids = []
      ways.each do |way|
        if way.visible?
          doc.root << way.to_xml_node(visible_nodes, changeset_cache, user_display_name_cache)
          way_ids << way.id
        end
 -    end 
 +    end
  
      relations = Relation.nodes(visible_nodes.keys).visible +
                  Relation.ways(way_ids).visible
  
 -    # we do not normally return the "other" partners referenced by an relation, 
 -    # e.g. if we return a way A that is referenced by relation X, and there's 
 -    # another way B also referenced, that is not returned. But we do make 
 -    # an exception for cases where an relation references another *relation*; 
 +    # we do not normally return the "other" partners referenced by an relation,
 +    # e.g. if we return a way A that is referenced by relation X, and there's
 +    # another way B also referenced, that is not returned. But we do make
 +    # an exception for cases where an relation references another *relation*;
      # in that case we return that as well (but we don't go recursive here)
 -    relations += Relation.relations(relations.collect { |r| r.id }).visible
 +    relations += Relation.relations(relations.collect(&:id)).visible
  
      # this "uniq" may be slightly inefficient; it may be better to first collect and output
      # all node-related relations, then find the *not yet covered* way-related ones etc.
      relations.uniq.each do |relation|
-       doc.root << relation.to_xml_node(nil, changeset_cache, user_display_name_cache)
+       doc.root << relation.to_xml_node(changeset_cache, user_display_name_cache)
      end
  
      response.headers["Content-Disposition"] = "attachment; filename=\"map.osm\""
  
 -    render :text => doc.to_s, :content_type => "text/xml"
 +    render :xml => doc.to_s
    end
  
    # Get a list of the tiles that have changed within a specified time
    # period
    def changes
 -    zoom = (params[:zoom] || '12').to_i
 +    zoom = (params[:zoom] || "12").to_i
  
 -    if params.include?(:start) and params.include?(:end)
 +    if params.include?(:start) && params.include?(:end)
        starttime = Time.parse(params[:start])
        endtime = Time.parse(params[:end])
      else
 -      hours = (params[:hours] || '1').to_i.hours
 +      hours = (params[:hours] || "1").to_i.hours
        endtime = Time.now.getutc
        starttime = endtime - hours
      end
  
 -    if zoom >= 1 and zoom <= 16 and
 -       endtime > starttime and endtime - starttime <= 24.hours
 +    if zoom >= 1 && zoom <= 16 &&
 +       endtime > starttime && endtime - starttime <= 24.hours
        mask = (1 << zoom) - 1
  
 -      tiles = Node.where(:timestamp => starttime .. endtime).group("maptile_for_point(latitude, longitude, #{zoom})").count
 +      tiles = Node.where(:timestamp => starttime..endtime).group("maptile_for_point(latitude, longitude, #{zoom})").count
  
        doc = OSM::API.new.get_xml_doc
 -      changes = XML::Node.new 'changes'
 +      changes = XML::Node.new "changes"
        changes["starttime"] = starttime.xmlschema
        changes["endtime"] = endtime.xmlschema
  
          x = (tile.to_i >> zoom) & mask
          y = tile.to_i & mask
  
 -        t = XML::Node.new 'tile'
 +        t = XML::Node.new "tile"
          t["x"] = x.to_s
          t["y"] = y.to_s
          t["z"] = zoom.to_s
  
        doc.root << changes
  
 -      render :text => doc.to_s, :content_type => "text/xml"
 +      render :xml => doc.to_s
      else
 -      render :text => "Requested zoom is invalid, or the supplied start is after the end time, or the start duration is more than 24 hours", :status => :bad_request
 +      render :plain => "Requested zoom is invalid, or the supplied start is after the end time, or the start duration is more than 24 hours", :status => :bad_request
      end
    end
  
 -  # External apps that use the api are able to query the api to find out some 
 -  # parameters of the API. It currently returns: 
 +  # External apps that use the api are able to query the api to find out some
 +  # parameters of the API. It currently returns:
    # * minimum and maximum API versions that can be used.
    # * maximum area that can be requested in a bbox request in square degrees
    # * number of tracepoints that are returned in each tracepoints page
    def capabilities
      doc = OSM::API.new.get_xml_doc
  
 -    api = XML::Node.new 'api'
 -    version = XML::Node.new 'version'
 -    version['minimum'] = "#{API_VERSION}";
 -    version['maximum'] = "#{API_VERSION}";
 +    api = XML::Node.new "api"
 +    version = XML::Node.new "version"
 +    version["minimum"] = API_VERSION.to_s
 +    version["maximum"] = API_VERSION.to_s
      api << version
 -    area = XML::Node.new 'area'
 -    area['maximum'] = MAX_REQUEST_AREA.to_s;
 +    area = XML::Node.new "area"
 +    area["maximum"] = MAX_REQUEST_AREA.to_s
      api << area
 -    tracepoints = XML::Node.new 'tracepoints'
 -    tracepoints['per_page'] = TRACEPOINTS_PER_PAGE.to_s
 +    notearea = XML::Node.new "note_area"
 +    notearea["maximum"] = MAX_NOTE_REQUEST_AREA.to_s
 +    api << notearea
 +    tracepoints = XML::Node.new "tracepoints"
 +    tracepoints["per_page"] = TRACEPOINTS_PER_PAGE.to_s
      api << tracepoints
 -    waynodes = XML::Node.new 'waynodes'
 -    waynodes['maximum'] = MAX_NUMBER_OF_WAY_NODES.to_s
 +    waynodes = XML::Node.new "waynodes"
 +    waynodes["maximum"] = MAX_NUMBER_OF_WAY_NODES.to_s
      api << waynodes
 -    changesets = XML::Node.new 'changesets'
 -    changesets['maximum_elements'] = Changeset::MAX_ELEMENTS.to_s
 +    changesets = XML::Node.new "changesets"
 +    changesets["maximum_elements"] = Changeset::MAX_ELEMENTS.to_s
      api << changesets
 -    timeout = XML::Node.new 'timeout'
 -    timeout['seconds'] = API_TIMEOUT.to_s
 +    timeout = XML::Node.new "timeout"
 +    timeout["seconds"] = API_TIMEOUT.to_s
      api << timeout
 -    
 +    status = XML::Node.new "status"
 +    status["database"] = database_status.to_s
 +    status["api"] = api_status.to_s
 +    status["gpx"] = gpx_status.to_s
 +    api << status
      doc.root << api
 +    policy = XML::Node.new "policy"
 +    blacklist = XML::Node.new "imagery"
 +    IMAGERY_BLACKLIST.each do |url_regex|
 +      xnd = XML::Node.new "blacklist"
 +      xnd["regex"] = url_regex.to_s
 +      blacklist << xnd
 +    end
 +    policy << blacklist
 +    doc.root << policy
  
 -    render :text => doc.to_s, :content_type => "text/xml"
 +    render :xml => doc.to_s
    end
  
    # External apps that use the api are able to query which permissions
    # * if authenticated via basic auth all permissions are granted, so the list will contain all permissions.
    # * unauthenticated users have no permissions, so the list will be empty.
    def permissions
 -    @permissions = case
 -                   when current_token.present?
 +    @permissions = if current_token.present?
                       ClientApplication.all_permissions.select { |p| current_token.read_attribute(p) }
 -                   when @user
 +                   elsif current_user
                       ClientApplication.all_permissions
                     else
                       []
@@@ -1,31 -1,37 +1,31 @@@
  class RelationController < ApplicationController
 -  require 'xml/libxml'
 +  require "xml/libxml"
  
 -  skip_before_filter :verify_authenticity_token
 -  before_filter :authorize, :only => [:create, :update, :delete]
 -  before_filter :require_allow_write_api, :only => [:create, :update, :delete]
 -  before_filter :require_public_data, :only => [:create, :update, :delete]
 -  before_filter :check_api_writable, :only => [:create, :update, :delete]
 -  before_filter :check_api_readable, :except => [:create, :update, :delete]
 -  after_filter :compress_output
 -  around_filter :api_call_handle_error, :api_call_timeout
 +  skip_before_action :verify_authenticity_token
 +  before_action :authorize, :only => [:create, :update, :delete]
 +  before_action :require_allow_write_api, :only => [:create, :update, :delete]
 +  before_action :require_public_data, :only => [:create, :update, :delete]
 +  before_action :check_api_writable, :only => [:create, :update, :delete]
 +  before_action :check_api_readable, :except => [:create, :update, :delete]
 +  around_action :api_call_handle_error, :api_call_timeout
  
    def create
      assert_method :put
  
      relation = Relation.from_xml(request.raw_post, true)
 -    
 -    # We assume that an exception has been thrown if there was an error 
 -    # generating the relation
 -    #if relation
 -    relation.create_with_history @user
 -    render :text => relation.id.to_s, :content_type => "text/plain"
 -    #else
 -    # render :text => "Couldn't get turn the input into a relation.", :status => :bad_request
 -    #end
 +
 +    # Assume that Relation.from_xml has thrown an exception if there is an error parsing the xml
 +    relation.create_with_history current_user
 +    render :plain => relation.id.to_s
    end
  
    def read
      relation = Relation.find(params[:id])
      response.last_modified = relation.timestamp
      if relation.visible
 -      render :text => relation.to_xml.to_s, :content_type => "text/xml"
 +      render :xml => relation.to_xml.to_s
      else
 -      render :text => "", :status => :gone
 +      head :gone
      end
    end
  
  
      relation = Relation.find(params[:id])
      new_relation = Relation.from_xml(request.raw_post)
 -    
 -    if new_relation and new_relation.id == relation.id
 -      relation.update_from new_relation, @user
 -      render :text => relation.version.to_s, :content_type => "text/plain"
 -    else
 -      render :nothing => true, :status => :bad_request
 +
 +    unless new_relation && new_relation.id == relation.id
 +      raise OSM::APIBadUserInput, "The id in the url (#{relation.id}) is not the same as provided in the xml (#{new_relation.id})"
      end
 +
 +    relation.update_from new_relation, current_user
 +    render :plain => relation.version.to_s
    end
  
    def delete
      relation = Relation.find(params[:id])
      new_relation = Relation.from_xml(request.raw_post)
 -    if new_relation and new_relation.id == relation.id
 -      relation.delete_with_history!(new_relation, @user)
 -      render :text => relation.version.to_s, :content_type => "text/plain"
 +    if new_relation && new_relation.id == relation.id
 +      relation.delete_with_history!(new_relation, current_user)
 +      render :plain => relation.version.to_s
      else
 -      render :nothing => true, :status => :bad_request
 +      head :bad_request
      end
    end
  
    # -----------------------------------------------------------------
    # full
 -  # 
 +  #
    # input parameters: id
    #
    # returns XML representation of one relation object plus all its
    # -----------------------------------------------------------------
    def full
      relation = Relation.find(params[:id])
 -    
 +
      if relation.visible
 -      
 +
        # first find the ids of nodes, ways and relations referenced by this
        # relation - note that we exclude this relation just in case.
 -      
 -      node_ids = relation.members.select { |m| m[0] == 'Node' }.map { |m| m[1] }
 -      way_ids = relation.members.select { |m| m[0] == 'Way' }.map { |m| m[1] }
 -      relation_ids = relation.members.select { |m| m[0] == 'Relation' and m[1] != relation.id }.map { |m| m[1] }
 -      
 +
 +      node_ids = relation.members.select { |m| m[0] == "Node" }.map { |m| m[1] }
 +      way_ids = relation.members.select { |m| m[0] == "Way" }.map { |m| m[1] }
 +      relation_ids = relation.members.select { |m| m[0] == "Relation" && m[1] != relation.id }.map { |m| m[1] }
 +
        # next load the relations and the ways.
 -      
 +
        relations = Relation.where(:id => relation_ids).includes(:relation_tags)
        ways = Way.where(:id => way_ids).includes(:way_nodes, :way_tags)
 -      
 -      # now additionally collect nodes referenced by ways. Note how we 
 +
 +      # now additionally collect nodes referenced by ways. Note how we
        # recursively evaluate ways but NOT relations.
 -      
 -      way_node_ids = ways.collect { |way|
 -        way.way_nodes.collect { |way_node| way_node.node_id }
 -      }
 +
 +      way_node_ids = ways.collect do |way|
 +        way.way_nodes.collect(&:node_id)
 +      end
        node_ids += way_node_ids.flatten
        nodes = Node.where(:id => node_ids.uniq).includes(:node_tags)
 -      
 +
        # create XML.
        doc = OSM::API.new.get_xml_doc
        visible_nodes = {}
-       visible_members = { "Node" => {}, "Way" => {}, "Relation" => {} }
        changeset_cache = {}
        user_display_name_cache = {}
 -      
 +
        nodes.each do |node|
 -        if node.visible? # should be unnecessary if data is consistent.
 -          doc.root << node.to_xml_node(changeset_cache, user_display_name_cache)
 -          visible_nodes[node.id] = node
 -        end
 +        next unless node.visible? # should be unnecessary if data is consistent.
 +
 +        doc.root << node.to_xml_node(changeset_cache, user_display_name_cache)
 +        visible_nodes[node.id] = node
-         visible_members["Node"][node.id] = true
        end
 +
        ways.each do |way|
 -        if way.visible? # should be unnecessary if data is consistent.
 -          doc.root << way.to_xml_node(visible_nodes, changeset_cache, user_display_name_cache)
 -        end
 +        next unless way.visible? # should be unnecessary if data is consistent.
 +
 +        doc.root << way.to_xml_node(visible_nodes, changeset_cache, user_display_name_cache)
-         visible_members["Way"][way.id] = true
        end
 +
        relations.each do |rel|
 -        if rel.visible? # should be unnecessary if data is consistent.
 -          doc.root << rel.to_xml_node(changeset_cache, user_display_name_cache)
 -        end
 +        next unless rel.visible? # should be unnecessary if data is consistent.
 +
-         doc.root << rel.to_xml_node(nil, changeset_cache, user_display_name_cache)
-         visible_members["Relation"][rel.id] = true
++        doc.root << rel.to_xml_node(changeset_cache, user_display_name_cache)
        end
 +
        # finally add self and output
-       doc.root << relation.to_xml_node(visible_members, changeset_cache, user_display_name_cache)
+       doc.root << relation.to_xml_node(changeset_cache, user_display_name_cache)
 -      render :text => doc.to_s, :content_type => "text/xml"
 -      
 +      render :xml => doc.to_s
 +
      else
 -      render :nothing => true, :status => :gone
 +      head :gone
      end
    end
  
    def relations
 -    ids = params['relations'].split(',').collect { |w| w.to_i }
 +    unless params["relations"]
 +      raise OSM::APIBadUserInput, "The parameter relations is required, and must be of the form relations=id[,id[,id...]]"
 +    end
  
 -    if ids.length > 0
 -      doc = OSM::API.new.get_xml_doc
 +    ids = params["relations"].split(",").collect(&:to_i)
  
 -      Relation.find(ids).each do |relation|
 -        doc.root << relation.to_xml_node
 -      end
 +    if ids.empty?
 +      raise OSM::APIBadUserInput, "No relations were given to search for"
 +    end
  
 -      render :text => doc.to_s, :content_type => "text/xml"
 -    else
 -      render :text => "You need to supply a comma separated list of ids.", :status => :bad_request
 +    doc = OSM::API.new.get_xml_doc
 +
 +    Relation.find(ids).each do |relation|
 +      doc.root << relation.to_xml_node
      end
 +
 +    render :xml => doc.to_s
    end
  
    def relations_for_way
    end
  
    def relations_for_object(objtype)
 -    relationids = RelationMember.where(:member_type => objtype, :member_id => params[:id]).collect { |ws| ws.relation_id }.uniq
 +    relationids = RelationMember.where(:member_type => objtype, :member_id => params[:id]).collect(&:relation_id).uniq
  
      doc = OSM::API.new.get_xml_doc
  
        doc.root << relation.to_xml_node if relation.visible
      end
  
 -    render :text => doc.to_s, :content_type => "text/xml"
 +    render :xml => doc.to_s
    end
  end
@@@ -2,44 -2,43 +2,44 @@@ class SearchController < ApplicationCon
    # Support searching for nodes, ways, or all
    # Can search by tag k, v, or both (type->k,value->v)
    # Can search by name (k=name,v=....)
 -  skip_before_filter :verify_authenticity_token
 -  after_filter :compress_output
 +  skip_before_action :verify_authenticity_token
  
    def search_all
 -    do_search(true,true,true)
 +    do_search(true, true, true)
    end
  
    def search_ways
 -    do_search(true,false,false)
 +    do_search(true, false, false)
    end
 +
    def search_nodes
 -    do_search(false,true,false)
 +    do_search(false, true, false)
    end
 +
    def search_relations
 -    do_search(false,false,true)
 +    do_search(false, false, true)
    end
  
 -  def do_search(do_ways,do_nodes,do_relations)
 -    type = params['type']
 -    value = params['value']
 -    unless type or value
 -      name = params['name']
 +  def do_search(do_ways, do_nodes, do_relations)
 +    type = params["type"]
 +    value = params["value"]
 +    unless type || value
 +      name = params["name"]
        if name
 -        type = 'name'
 +        type = "name"
          value = name
        end
      end
  
      if do_nodes
 -      response.headers['Error'] = "Searching of nodes is currently unavailable"
 -      render :nothing => true, :status => :service_unavailable
 +      response.headers["Error"] = "Searching of nodes is currently unavailable"
 +      head :service_unavailable
        return false
      end
  
      unless value
 -      response.headers['Error'] = "Searching for a key without value is currently unavailable"
 -      render :nothing => true, :status => :service_unavailable
 +      response.headers["Error"] = "Searching for a key without value is currently unavailable"
 +      head :service_unavailable
        return false
      end
  
@@@ -50,7 -49,7 +50,7 @@@
        nodes = nodes.where(:current_node_tags => { :v => value }) if value
        nodes = nodes.limit(100)
      else
 -      nodes = Array.new
 +      nodes = []
      end
  
      # Matching for way tags table
@@@ -60,7 -59,7 +60,7 @@@
        ways = ways.where(:current_way_tags => { :v => value }) if value
        ways = ways.limit(100)
      else
 -      ways = Array.new
 +      ways = []
      end
  
      # Matching for relation tags table
        relations = relations.where(:current_relation_tags => { :v => value }) if value
        relations = relations.limit(2000)
      else
 -      relations = Array.new
 +      relations = []
      end
  
      # Fetch any node needed for our ways (only have matching nodes so far)
 -    nodes += Node.find(ways.collect { |w| w.nds }.uniq)
 +    nodes += Node.find(ways.collect(&:nds).uniq)
  
      # Print
      visible_nodes = {}
@@@ -91,9 -90,9 +91,9 @@@
      end
  
      relations.each do |rel|
-       doc.root << rel.to_xml_node(nil, changeset_cache, user_display_name_cache)
+       doc.root << rel.to_xml_node(changeset_cache, user_display_name_cache)
      end
  
 -    render :text => doc.to_s, :content_type => "text/xml"
 +    render :xml => doc.to_s
    end
  end
diff --combined app/models/relation.rb
 +# == Schema Information
 +#
 +# Table name: current_relations
 +#
 +#  id           :integer          not null, primary key
 +#  changeset_id :integer          not null
 +#  timestamp    :datetime         not null
 +#  visible      :boolean          not null
 +#  version      :integer          not null
 +#
 +# Indexes
 +#
 +#  current_relations_timestamp_idx  (timestamp)
 +#
 +# Foreign Keys
 +#
 +#  current_relations_changeset_id_fkey  (changeset_id => changesets.id)
 +#
 +
  class Relation < ActiveRecord::Base
 -  require 'xml/libxml'
 -  
 +  require "xml/libxml"
 +
    include ConsistencyValidations
    include NotRedactable
 +  include ObjectMetadata
  
    self.table_name = "current_relations"
  
    belongs_to :changeset
  
 -  has_many :old_relations, :order => 'version'
 +  has_many :old_relations, -> { order(:version) }
  
 -  has_many :relation_members, :order => 'sequence_id'
 +  has_many :relation_members, -> { order(:sequence_id) }
    has_many :relation_tags
  
    has_many :containing_relation_members, :class_name => "RelationMember", :as => :member
 -  has_many :containing_relations, :class_name => "Relation", :through => :containing_relation_members, :source => :relation, :extend => ObjectFinder
 -
 -  validates_presence_of :id, :on => :update
 -  validates_presence_of :timestamp,:version,  :changeset_id 
 -  validates_uniqueness_of :id
 -  validates_inclusion_of :visible, :in => [ true, false ]
 -  validates_numericality_of :id, :on => :update, :integer_only => true
 -  validates_numericality_of :changeset_id, :version, :integer_only => true
 -  validates_associated :changeset
 -  
 -  scope :visible, where(:visible => true)
 -  scope :invisible, where(:visible => false)
 -  scope :nodes, lambda { |*ids| joins(:relation_members).where(:current_relation_members => { :member_type => "Node", :member_id => ids }) }
 -  scope :ways, lambda { |*ids| joins(:relation_members).where(:current_relation_members => { :member_type => "Way", :member_id => ids }) }
 -  scope :relations, lambda { |*ids| joins(:relation_members).where(:current_relation_members => { :member_type => "Relation", :member_id => ids }) }
 -
 -  TYPES = ["node", "way", "relation"]
 -
 -  def self.from_xml(xml, create=false)
 -    begin
 -      p = XML::Parser.string(xml)
 -      doc = p.parse
 -
 -      doc.find('//osm/relation').each do |pt|
 -        return Relation.from_xml_node(pt, create)
 -      end
 -      raise OSM::APIBadXMLError.new("node", xml, "XML doesn't contain an osm/relation element.")
 -    rescue LibXML::XML::Error, ArgumentError => ex
 -      raise OSM::APIBadXMLError.new("relation", xml, ex.message)
 +  has_many :containing_relations, :class_name => "Relation", :through => :containing_relation_members, :source => :relation
 +
 +  validates :id, :uniqueness => true, :presence => { :on => :update },
 +                 :numericality => { :on => :update, :integer_only => true }
 +  validates :version, :presence => true,
 +                      :numericality => { :integer_only => true }
 +  validates :changeset_id, :presence => true,
 +                           :numericality => { :integer_only => true }
 +  validates :timestamp, :presence => true
 +  validates :changeset, :associated => true
 +  validates :visible, :inclusion => [true, false]
 +
 +  scope :visible, -> { where(:visible => true) }
 +  scope :invisible, -> { where(:visible => false) }
 +  scope :nodes, ->(*ids) { joins(:relation_members).where(:current_relation_members => { :member_type => "Node", :member_id => ids.flatten }) }
 +  scope :ways, ->(*ids) { joins(:relation_members).where(:current_relation_members => { :member_type => "Way", :member_id => ids.flatten }) }
 +  scope :relations, ->(*ids) { joins(:relation_members).where(:current_relation_members => { :member_type => "Relation", :member_id => ids.flatten }) }
 +
 +  TYPES = %w[node way relation].freeze
 +
 +  def self.from_xml(xml, create = false)
 +    p = XML::Parser.string(xml, :options => XML::Parser::Options::NOERROR)
 +    doc = p.parse
 +
 +    doc.find("//osm/relation").each do |pt|
 +      return Relation.from_xml_node(pt, create)
      end
 +    raise OSM::APIBadXMLError.new("node", xml, "XML doesn't contain an osm/relation element.")
 +  rescue LibXML::XML::Error, ArgumentError => ex
 +    raise OSM::APIBadXMLError.new("relation", xml, ex.message)
    end
  
 -  def self.from_xml_node(pt, create=false)
 +  def self.from_xml_node(pt, create = false)
      relation = Relation.new
  
 -    raise OSM::APIBadXMLError.new("relation", pt, "Version is required when updating") unless create or not pt['version'].nil?
 -    relation.version = pt['version']
 -    raise OSM::APIBadXMLError.new("relation", pt, "Changeset id is missing") if pt['changeset'].nil?
 -    relation.changeset_id = pt['changeset']
 -    
 +    raise OSM::APIBadXMLError.new("relation", pt, "Version is required when updating") unless create || !pt["version"].nil?
 +    relation.version = pt["version"]
 +    raise OSM::APIBadXMLError.new("relation", pt, "Changeset id is missing") if pt["changeset"].nil?
 +    relation.changeset_id = pt["changeset"]
 +
      unless create
 -      raise OSM::APIBadXMLError.new("relation", pt, "ID is required when updating") if pt['id'].nil?
 -      relation.id = pt['id'].to_i
 -      # .to_i will return 0 if there is no number that can be parsed. 
 +      raise OSM::APIBadXMLError.new("relation", pt, "ID is required when updating") if pt["id"].nil?
 +      relation.id = pt["id"].to_i
 +      # .to_i will return 0 if there is no number that can be parsed.
        # We want to make sure that there is no id with zero anyway
 -      raise OSM::APIBadUserInput.new("ID of relation cannot be zero when updating.") if relation.id == 0
 +      raise OSM::APIBadUserInput, "ID of relation cannot be zero when updating." if relation.id.zero?
      end
 -    
 +
      # We don't care about the timestamp nor the visibility as these are either
 -    # set explicitly or implicit in the action. The visibility is set to true, 
 +    # set explicitly or implicit in the action. The visibility is set to true,
      # and manually set to false before the actual delete.
      relation.visible = true
  
      # Start with no tags
 -    relation.tags = Hash.new
 +    relation.tags = {}
  
      # Add in any tags from the XML
 -    pt.find('tag').each do |tag|
 -      raise OSM::APIBadXMLError.new("relation", pt, "tag is missing key") if tag['k'].nil?
 -      raise OSM::APIBadXMLError.new("relation", pt, "tag is missing value") if tag['v'].nil?
 -      relation.add_tag_keyval(tag['k'], tag['v'])
 +    pt.find("tag").each do |tag|
 +      raise OSM::APIBadXMLError.new("relation", pt, "tag is missing key") if tag["k"].nil?
 +      raise OSM::APIBadXMLError.new("relation", pt, "tag is missing value") if tag["v"].nil?
 +      relation.add_tag_keyval(tag["k"], tag["v"])
      end
  
      # need to initialise the relation members array explicitly, as if this
 -    # isn't done for a new relation then @members attribute will be nil, 
 -    # and the members will be loaded from the database instead of being 
 +    # isn't done for a new relation then @members attribute will be nil,
 +    # and the members will be loaded from the database instead of being
      # empty, as intended.
 -    relation.members = Array.new
 -
 -    pt.find('member').each do |member|
 -      #member_type = 
 -      logger.debug "each member"
 -      raise OSM::APIBadXMLError.new("relation", pt, "The #{member['type']} is not allowed only, #{TYPES.inspect} allowed") unless TYPES.include? member['type']
 -      logger.debug "after raise"
 -      #member_ref = member['ref']
 -      #member_role
 -      member['role'] ||= "" # Allow  the upload to not include this, in which case we default to an empty string.
 -      logger.debug member['role']
 -      relation.add_member(member['type'].classify, member['ref'], member['role'])
 +    relation.members = []
 +
 +    pt.find("member").each do |member|
 +      # member_type =
 +      raise OSM::APIBadXMLError.new("relation", pt, "The #{member['type']} is not allowed only, #{TYPES.inspect} allowed") unless TYPES.include? member["type"]
 +      # member_ref = member['ref']
 +      # member_role
 +      member["role"] ||= "" # Allow  the upload to not include this, in which case we default to an empty string.
 +      relation.add_member(member["type"].classify, member["ref"], member["role"])
      end
 -    raise OSM::APIBadUserInput.new("Some bad xml in relation") if relation.nil?
 +    raise OSM::APIBadUserInput, "Some bad xml in relation" if relation.nil?
  
 -    return relation
 +    relation
    end
  
    def to_xml
      doc = OSM::API.new.get_xml_doc
 -    doc.root << to_xml_node()
 -    return doc
 +    doc.root << to_xml_node
 +    doc
    end
  
-   def to_xml_node(visible_members = nil, changeset_cache = {}, user_display_name_cache = {})
+   def to_xml_node(changeset_cache = {}, user_display_name_cache = {})
 -    el1 = XML::Node.new 'relation'
 -    el1['id'] = self.id.to_s
 -    el1['visible'] = self.visible.to_s
 -    el1['timestamp'] = self.timestamp.xmlschema
 -    el1['version'] = self.version.to_s
 -    el1['changeset'] = self.changeset_id.to_s
 -
 -    if changeset_cache.key?(self.changeset_id)
 -      # use the cache if available
 -    else
 -      changeset_cache[self.changeset_id] = self.changeset.user_id
 -    end
 -
 -    user_id = changeset_cache[self.changeset_id]
 +    el = XML::Node.new "relation"
 +    el["id"] = id.to_s
  
 -    if user_display_name_cache.key?(user_id)
 -      # use the cache if available
 -    elsif self.changeset.user.data_public?
 -      user_display_name_cache[user_id] = self.changeset.user.display_name
 -    else
 -      user_display_name_cache[user_id] = nil
 -    end
 +    add_metadata_to_xml_node(el, self, changeset_cache, user_display_name_cache)
  
 -    if not user_display_name_cache[user_id].nil?
 -      el1['user'] = user_display_name_cache[user_id]
 -      el1['uid'] = user_id.to_s
 +    relation_members.each do |member|
-       p = 0
-       if visible_members
-         # if there is a list of visible members then use that to weed out deleted segments
-         p = 1 if visible_members[member.member_type][member.member_id]
-       else
-         # otherwise, manually go to the db to check things
-         p = 1 if member.member.visible?
-       end
-       next unless p
 +      member_el = XML::Node.new "member"
 +      member_el["type"] = member.member_type.downcase
 +      member_el["ref"] = member.member_id.to_s
 +      member_el["role"] = member.member_role
 +      el << member_el
      end
  
 -    self.relation_members.each do |member|
 -      e = XML::Node.new 'member'
 -      e['type'] = member.member_type.downcase
 -      e['ref'] = member.member_id.to_s
 -      e['role'] = member.member_role
 -      el1 << e
 -    end
 +    add_tags_to_xml_node(el, relation_tags)
  
 -    self.relation_tags.each do |tag|
 -      e = XML::Node.new 'tag'
 -      e['k'] = tag.k
 -      e['v'] = tag.v
 -      el1 << e
 -    end
 -    return el1
 -  end 
 +    el
 +  end
  
 -  # FIXME is this really needed?
 +  # FIXME: is this really needed?
    def members
 -    @members ||= self.relation_members.map do |member|
 +    @members ||= relation_members.map do |member|
        [member.member_type, member.member_id, member.member_role]
      end
    end
  
    def tags
 -    unless @tags
 -      @tags = Hash.new
 -      self.relation_tags.each do |tag|
 -        @tags[tag.k] = tag.v
 -      end
 -    end
 -    @tags
 +    @tags ||= Hash[relation_tags.collect { |t| [t.k, t.v] }]
    end
  
 -  def members=(m)
 -    @members = m
 -  end
 +  attr_writer :members
  
 -  def tags=(t)
 -    @tags = t
 -  end
 +  attr_writer :tags
  
    def add_member(type, id, role)
      @members ||= []
    end
  
    def add_tag_keyval(k, v)
 -    @tags = Hash.new unless @tags
 +    @tags ||= {}
  
      # duplicate tags are now forbidden, so we can't allow values
      # in the hash to be overwritten.
 -    raise OSM::APIDuplicateTagsError.new("relation", self.id, k) if @tags.include? k
 +    raise OSM::APIDuplicateTagsError.new("relation", id, k) if @tags.include? k
  
      @tags[k] = v
    end
  
    ##
 -  # updates the changeset bounding box to contain the bounding box of 
 +  # updates the changeset bounding box to contain the bounding box of
    # the element with given +type+ and +id+. this only works with nodes
    # and ways at the moment, as they're the only elements to respond to
    # the :bbox call.
    def update_changeset_element(type, id)
      element = Kernel.const_get(type.capitalize).find(id)
      changeset.update_bbox! element.bbox
 -  end    
 +  end
  
    def delete_with_history!(new_relation, user)
 -    unless self.visible
 +    unless visible
        raise OSM::APIAlreadyDeletedError.new("relation", new_relation.id)
      end
  
 -    # need to start the transaction here, so that the database can 
 +    # need to start the transaction here, so that the database can
      # provide repeatable reads for the used-by checks. this means it
      # shouldn't be possible to get race conditions.
      Relation.transaction do
 -      self.lock!
 +      lock!
        check_consistency(self, new_relation, user)
        # This will check to see if this relation is used by another relation
 -      rel = RelationMember.joins(:relation).where("visible = ? AND member_type = 'Relation' and member_id = ? ", true, self.id).first
 -      raise OSM::APIPreconditionFailedError.new("The relation #{new_relation.id} is used in relation #{rel.relation.id}.") unless rel.nil?
 +      rel = RelationMember.joins(:relation).find_by("visible = ? AND member_type = 'Relation' and member_id = ? ", true, id)
 +      raise OSM::APIPreconditionFailedError, "The relation #{new_relation.id} is used in relation #{rel.relation.id}." unless rel.nil?
  
        self.changeset_id = new_relation.changeset_id
        self.tags = {}
  
    def update_from(new_relation, user)
      Relation.transaction do
 -      self.lock!
 +      lock!
        check_consistency(self, new_relation, user)
 -      unless new_relation.preconditions_ok?(self.members)
 -        raise OSM::APIPreconditionFailedError.new("Cannot update relation #{self.id}: data or member data is invalid.")
 +      unless new_relation.preconditions_ok?(members)
 +        raise OSM::APIPreconditionFailedError, "Cannot update relation #{id}: data or member data is invalid."
        end
        self.changeset_id = new_relation.changeset_id
        self.changeset = new_relation.changeset
        save_with_history!
      end
    end
 -  
 +
    def create_with_history(user)
      check_create_consistency(self, user)
 -    unless self.preconditions_ok?
 -      raise OSM::APIPreconditionFailedError.new("Cannot create relation: data or member data is invalid.")
 +    unless preconditions_ok?
 +      raise OSM::APIPreconditionFailedError, "Cannot create relation: data or member data is invalid."
      end
      self.version = 0
      self.visible = true
    end
  
    def preconditions_ok?(good_members = [])
 -    # These are hastables that store an id in the index of all 
 +    # These are hastables that store an id in the index of all
      # the nodes/way/relations that have already been added.
 -    # If the member is valid and visible then we add it to the 
 +    # If the member is valid and visible then we add it to the
      # relevant hash table, with the value true as a cache.
      # Thus if you have nodes with the ids of 50 and 1 already in the
      # relation, then the hash table nodes would contain:
      # => {50=>true, 1=>true}
 -    elements = { :node => Hash.new, :way => Hash.new, :relation => Hash.new }
 +    elements = { :node => {}, :way => {}, :relation => {} }
  
      # pre-set all existing members to good
      good_members.each { |m| elements[m[0].downcase.to_sym][m[1]] = true }
  
 -    self.members.each do |m|
 +    members.each do |m|
        # find the hash for the element type or die
 -      hash = elements[m[0].downcase.to_sym] or return false
 +      hash = elements[m[0].downcase.to_sym]
 +      return false unless hash
 +
        # unless its in the cache already
 -      unless hash.key? m[1]
 -        # use reflection to look up the appropriate class
 -        model = Kernel.const_get(m[0].capitalize)
 -        # get the element with that ID
 -        element = model.where(:id => m[1]).first
 -
 -        # and check that it is OK to use.
 -        unless element and element.visible? and element.preconditions_ok?
 -          raise OSM::APIPreconditionFailedError.new("Relation with id #{self.id} cannot be saved due to #{m[0]} with id #{m[1]}")
 -        end
 -        hash[m[1]] = true
 +      next if hash.key? m[1]
 +
 +      # use reflection to look up the appropriate class
 +      model = Kernel.const_get(m[0].capitalize)
 +      # get the element with that ID. and, if found, lock the element to
 +      # ensure it can't be deleted until after the current transaction
 +      # commits.
 +      element = model.lock("for share").find_by(:id => m[1])
 +
 +      # and check that it is OK to use.
 +      unless element && element.visible? && element.preconditions_ok?
 +        raise OSM::APIPreconditionFailedError, "Relation with id #{id} cannot be saved due to #{m[0]} with id #{m[1]}"
        end
 +      hash[m[1]] = true
      end
  
 -    return true
 -  end
 -
 -  # Temporary method to match interface to nodes
 -  def tags_as_hash
 -    return self.tags
 +    true
    end
  
    ##
    # if any members are referenced by placeholder IDs (i.e: negative) then
 -  # this calling this method will fix them using the map from placeholders 
 -  # to IDs +id_map+. 
 +  # this calling this method will fix them using the map from placeholders
 +  # to IDs +id_map+.
    def fix_placeholders!(id_map, placeholder_id = nil)
 -    self.members.map! do |type, id, role|
 +    members.map! do |type, id, role|
        old_id = id.to_i
        if old_id < 0
          new_id = id_map[type.downcase.to_sym][old_id]
 -        raise OSM::APIBadUserInput.new("Placeholder #{type} not found for reference #{old_id} in relation #{self.id.nil? ? placeholder_id : self.id}.") if new_id.nil?
 +        raise OSM::APIBadUserInput, "Placeholder #{type} not found for reference #{old_id} in relation #{self.id.nil? ? placeholder_id : self.id}." if new_id.nil?
          [type, new_id, role]
        else
          [type, id, role]
    end
  
    private
 -  
 +
    def save_with_history!
 +    t = Time.now.getutc
 +
 +    self.version += 1
 +    self.timestamp = t
 +
      Relation.transaction do
        # have to be a little bit clever here - to detect if any tags
        # changed then we have to monitor their before and after state.
        tags_changed = false
  
 -      t = Time.now.getutc
 -      self.version += 1
 -      self.timestamp = t
 -      self.save!
 +      # clone the object before saving it so that the original is
 +      # still marked as dirty if we retry the transaction
 +      clone.save!
  
        tags = self.tags.clone
 -      self.relation_tags.each do |old_tag|
 +      relation_tags.each do |old_tag|
          key = old_tag.k
          # if we can match the tags we currently have to the list
          # of old tags, then we never set the tags_changed flag. but
 -        # if any are different then set the flag and do the DB 
 +        # if any are different then set the flag and do the DB
          # update.
 -        if tags.has_key? key 
 +        if tags.key? key
            tags_changed |= (old_tag.v != tags[key])
  
            # remove from the map, so that we can expect an empty map
        end
        # if there are left-over tags then they are new and will have to
        # be added.
 -      tags_changed |= (not tags.empty?)
 -      RelationTag.delete_all(:relation_id => self.id)
 -      self.tags.each do |k,v|
 +      tags_changed |= !tags.empty?
 +      RelationTag.where(:relation_id => id).delete_all
 +      self.tags.each do |k, v|
          tag = RelationTag.new
 -        tag.relation_id = self.id
 +        tag.relation_id = id
          tag.k = k
          tag.v = v
          tag.save!
        end
 -      
 +
        # same pattern as before, but this time we're collecting the
        # changed members in an array, as the bounding box updates for
        # elements are per-element, not blanked on/off like for tags.
 -      changed_members = Array.new
 +      changed_members = []
        members = self.members.clone
 -      self.relation_members.each do |old_member|
 +      relation_members.each do |old_member|
          key = [old_member.member_type, old_member.member_id, old_member.member_role]
          i = members.index key
          if i.nil?
        # members may be in a different order and i don't feel like implementing
        # a longest common subsequence algorithm to optimise this.
        members = self.members
 -      RelationMember.delete_all(:relation_id => self.id)
 -      members.each_with_index do |m,i|
 +      RelationMember.where(:relation_id => id).delete_all
 +      members.each_with_index do |m, i|
          mem = RelationMember.new
 -        mem.relation_id = self.id
 +        mem.relation_id = id
          mem.sequence_id = i
          mem.member_type = m[0]
          mem.member_id = m[1]
        # bounding box. this is similar to how the map call does things and is
        # reasonable on the assumption that adding or removing members doesn't
        # materially change the rest of the relation.
 -      any_relations = 
 -        changed_members.collect { |id,type| type == "relation" }.
 -        inject(false) { |b,s| b or s }
 +      any_relations =
 +        changed_members.collect { |_id, type| type == "relation" }
 +                       .inject(false) { |acc, elem| acc || elem }
  
 -      update_members = if tags_changed or any_relations
 +      update_members = if tags_changed || any_relations
                           # add all non-relation bounding boxes to the changeset
                           # FIXME: check for tag changes along with element deletions and
                           # make sure that the deleted element's bounding box is hit.
                           self.members
 -                       else 
 +                       else
                           changed_members
                         end
 -      update_members.each do |type, id, role|
 -        if type != "Relation"
 -          update_changeset_element(type, id)
 -        end
 +      update_members.each do |type, id, _role|
 +        update_changeset_element(type, id) if type != "Relation"
        end
  
        # tell the changeset we updated one element only
        changeset.save!
      end
    end
 -
  end