Merge pull request #9013 from CartoDB/8958-arcgis-oids

Fix incorrect querying of object ids in ArcGIS connector

Author: jgoizueta

services/datasources/lib/datasources/url/arcgis.rb

@@ -44,6 +44,9 @@ class ArcGIS < BaseDirectStream
         # Used to display more data only (for local debugging purposes)
         DEBUG = false
 
+        VECTOR_LAYER_TYPE = 'Feature Layer'.freeze
+        OID_FIELD_TYPE    = 'esriFieldTypeOID'.freeze
+
         attr_reader :metadata
 
         # Constructor
@@ -249,8 +252,15 @@ def get_subresource_metadata(id, subresource_id)
           # non-rails symbolize keys
           data = ::JSON.parse(response.body).inject({}){|memo,(k,v)| memo[k.to_sym] = v; memo}
 
+          raise ResponseError.new("Invalid layer type: '#{data[:type]}'") if data[:type] != VECTOR_LAYER_TYPE
           raise ResponseError.new("Missing data: 'fields'") if data[:fields].nil?
 
+          if data[:supportedQueryFormats].present?
+            supported_formats = data.fetch(:supportedQueryFormats).gsub(' ', '').split(',')
+          else
+            supported_formats = []
+          end
+
           begin
             @metadata = {
               arcgis_version:             data.fetch(:currentVersion),
@@ -259,14 +269,14 @@ def get_subresource_metadata(id, subresource_id)
               type:                       data.fetch(:type),
               geometry_type:              data.fetch(:geometryType),
               copyright:                  data.fetch(:copyrightText, ''),
-              fields:                     data.fetch(:fields).map{ |field|
+              fields:                     data.fetch(:fields).try(:map) { |field|
                 {
                   name: field['name'],
                   type: field['type']
                 }
               },
               max_records_per_query:      data.fetch(:maxRecordCount, 500),
-              supported_formats:          data.fetch(:supportedQueryFormats).gsub(' ', '').split(','),
+              supported_formats:          supported_formats,
               advanced_queries_supported: data.fetch(:supportsAdvancedQueries, false)
             }
           rescue => exception
@@ -337,6 +347,9 @@ def get_layers_list(url)
             raise ResponseError.new("Missing data: #{exception.to_s} #{request_url} #{exception.backtrace}")
           end
 
+          # We only support vector layers (not raster layers)
+          data = data.reject { |layer| layer['type'] != VECTOR_LAYER_TYPE }
+
           raise ResponseError.new("Empty layers list #{request_url}") if data.length == 0
 
           begin
@@ -388,10 +401,18 @@ def get_by_ids(url, ids, fields)
           raise InvalidInputDataError.new("'ids' empty or invalid") if (ids.nil? || ids.length == 0)
           raise InvalidInputDataError.new("'fields' empty or invalid") if (fields.nil? || fields.length == 0)
 
+          oid_field = fields.find { |field| field[:type] == OID_FIELD_TYPE }
+
           if ids.length == 1
-            ids_field = {objectIds: ids.first}
+            ids_field = { objectIds: ids.first }
           else
-            ids_field = {where: "OBJECTID >=#{ids.first} AND OBJECTID <=#{ids.last}"}
+            if oid_field
+              # Note that ids is sorted
+              ids_field = { where: "#{oid_field[:name]} >=#{ids.first} AND #{oid_field[:name]} <=#{ids.last}" }
+            else
+              # This could be innefficient with large number of ids, but it is limited to MAX_BLOCK_SIZE
+              ids_field = { objectIds: ids.join(',') }
+            end
           end
 
           prepared_fields = Addressable::URI.encode(fields.map { |field| "#{field[:name]}" }.join(','))

services/datasources/spec/fixtures/arcgis_layers.json

@@ -1,3 +1,3 @@
 {"layers":[
-	{"id":0, "name":"first layer"}
-]}
+	{"id":0, "name":"first layer", "type":"Feature Layer"}
+]}

services/datasources/spec/unit/arcgis_spec.rb

@@ -1,5 +1,7 @@
 # encoding: utf-8
 
+require 'active_support/core_ext'
+
 require_relative '../../lib/datasources'
 require_relative '../doubles/user'
 
@@ -519,4 +521,3 @@
     end
   end
 end
-