From 1a8bb079c32f0b143892884d989a917e91047cb7 Mon Sep 17 00:00:00 2001 From: Ryan Wallace Date: Thu, 7 May 2026 20:36:53 -0700 Subject: [PATCH 1/3] Download remote URLs via open-uri before passing to ogr2ogr GDAL's curl-based fetcher fails with "Empty reply from server" against some ArcGIS endpoints (e.g. Cloudflare-fronted servers that only offer HTTPS 1.1). Fetch remote URLs with Ruby's open-uri and hand ogr2ogr a local tempfile instead. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../importers/esri_geo_json.rb | 28 ++++++++++++++++--- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/lib/spatial_features/importers/esri_geo_json.rb b/lib/spatial_features/importers/esri_geo_json.rb index 35dac882..18477528 100644 --- a/lib/spatial_features/importers/esri_geo_json.rb +++ b/lib/spatial_features/importers/esri_geo_json.rb @@ -1,5 +1,7 @@ require 'digest/md5' require 'open3' +require 'open-uri' +require 'tempfile' require 'spatial_features/importers/geo_json' module SpatialFeatures @@ -15,10 +17,28 @@ def geojson private - def esri_json_to_geojson(url) - args = ['ogr2ogr', '-t_srs', 'EPSG:4326', '-f', 'GeoJSON', '/dev/stdout', url] - args << 'OGRGeoJSON' unless URI.parse(url).relative? # A relative URL is a local file path - Open3.capture2(*args).first + def esri_json_to_geojson(path_or_url) + return ogr2ogr_to_geojson(path_or_url) if URI.parse(path_or_url).relative? # It is a local file path + + # Download the URL ourselves rather than letting GDAL's curl fetch it. Servers that only + # offer HTTPS 1.1 may cause GDAL's curl to fail, but Ruby's open-uri can handle them. + with_downloaded_file(path_or_url) do |path| + ogr2ogr_to_geojson(path) + end + end + + # Returns the GeoJSON OGR reads out of the file at `path`, reprojected to EPSG:4326. + def ogr2ogr_to_geojson(path) + Open3.capture2('ogr2ogr', '-t_srs', 'EPSG:4326', '-f', 'GeoJSON', '/dev/stdout', path).first + end + + def with_downloaded_file(url) + Tempfile.create(['esri_geojson', '.json']) do |tempfile| + tempfile.binmode + URI.open(url) { |io| IO.copy_stream(io, tempfile) } + tempfile.close + return yield(tempfile.path) + end end end end From eea6ea30cd3aa6f2b7cec9e87d7102eda194e29c Mon Sep 17 00:00:00 2001 From: Ryan Wallace Date: Thu, 7 May 2026 20:45:31 -0700 Subject: [PATCH 2/3] Paginate ArcGIS query responses past maxRecordCount ArcGIS query endpoints truncate each response at the service's maxRecordCount (commonly 1000 or 2000 features) and set exceededTransferLimit when more results exist. Walk the pages with resultOffset and merge them into a single FeatureCollection before handing to ogr2ogr. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../importers/esri_geo_json.rb | 43 ++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/lib/spatial_features/importers/esri_geo_json.rb b/lib/spatial_features/importers/esri_geo_json.rb index 18477528..3e67d43d 100644 --- a/lib/spatial_features/importers/esri_geo_json.rb +++ b/lib/spatial_features/importers/esri_geo_json.rb @@ -35,11 +35,52 @@ def ogr2ogr_to_geojson(path) def with_downloaded_file(url) Tempfile.create(['esri_geojson', '.json']) do |tempfile| tempfile.binmode - URI.open(url) { |io| IO.copy_stream(io, tempfile) } + download_paginated(url, tempfile) tempfile.close return yield(tempfile.path) end end + + # ArcGIS query endpoints cap each response at the service's maxRecordCount + # (commonly 1000 or 2000 features) and signal exceededTransferLimit when + # more results are available. Walk the pages with resultOffset and merge + # them into a single FeatureCollection before handing to ogr2ogr. + def download_paginated(url, io) + combined = nil + offset = 0 + + loop do + page = JSON.parse(URI.open(paginated_url(url, offset)).read) + page_features = page['features'] || [] + + if combined.nil? + combined = page + else + combined['features'].concat(page_features) + end + + break if page_features.empty? || !exceeded_transfer_limit?(page) + offset += page_features.length + end + + combined&.delete('exceededTransferLimit') + combined&.fetch('properties', {})&.delete('exceededTransferLimit') + io.write(JSON.dump(combined)) if combined + end + + def exceeded_transfer_limit?(page) + page['exceededTransferLimit'] || page.dig('properties', 'exceededTransferLimit') + end + + def paginated_url(url, offset) + return url if offset.zero? + uri = URI.parse(url) + params = URI.decode_www_form(uri.query || '') + params.reject! { |k, _| k == 'resultOffset' } + params << ['resultOffset', offset.to_s] + uri.query = URI.encode_www_form(params) + uri.to_s + end end end end From 232983b3a9e944f5c9ec69f1c587402f130f8e01 Mon Sep 17 00:00:00 2001 From: Nicholas Jakobsen Date: Sun, 16 Aug 2026 11:23:45 -0700 Subject: [PATCH 3/3] feat: Bound and cover the ArcGIS query pagination `ESRIGeoJSON` walks an ArcGIS query endpoint's pages until one comes back without `exceededTransferLimit`. A server that does not honour `resultOffset` answers every request with the same first page and that flag still set, so the walk never reaches a last page and the collection grows until the process runs out of memory. The walk now stops after `max_pages` requests and raises, rather than writing a collection that is missing whatever came after the cap. A page that does not parse as JSON, and an endpoint that cannot be reached at all, both raise `ImportError` naming what happened instead of surfacing `JSON::ParserError` or `OpenURI::HTTPError`. Each request is bounded by `request_timeout`, since a hung endpoint would otherwise hold an import worker open indefinitely. `max_pages` and `request_timeout` are `class_attribute`s, defaulting to 500 and 60 seconds, which a deployment with a larger layer or a slower service can raise. Adds the importer's first specs, covering a single page, a walk across several pages, the `resultOffset` each request asks for, the flag nested under `properties`, a runaway server, an unparseable reply, an unreachable one, the request timeout, a local path read without downloading, and a local path holding shell metacharacters. Both response shapes are exercised: the GeoJSON an `f=geojson` query returns and the ESRI JSON an `f=json` query returns, the latter being the format the class is named for and the one OGR has to sniff from the content. They stub `URI` the way `kml_file_arcgis_spec.rb` stubs `Download`, so no new test dependency. Co-Authored-By: Claude Opus 5 (1M context) --- lib/spatial_features.rb | 1 + lib/spatial_features/download.rb | 107 ++++++--- lib/spatial_features/gdal.rb | 42 ++++ .../importers/esri_geo_json.rb | 82 +++++-- lib/spatial_features/importers/shapefile.rb | 5 +- spec/lib/spatial_features/download_spec.rb | 62 ++++++ .../importers/esri_geojson_spec.rb | 205 ++++++++++++++++++ 7 files changed, 458 insertions(+), 46 deletions(-) create mode 100644 lib/spatial_features/gdal.rb create mode 100644 spec/lib/spatial_features/download_spec.rb create mode 100644 spec/lib/spatial_features/importers/esri_geojson_spec.rb diff --git a/lib/spatial_features.rb b/lib/spatial_features.rb index fd6393c8..53d827d3 100644 --- a/lib/spatial_features.rb +++ b/lib/spatial_features.rb @@ -10,6 +10,7 @@ require 'spatial_features/uncached_result' require 'spatial_features/venn_polygons' require 'spatial_features/controller_helpers/spatial_extensions' +require 'spatial_features/gdal' require 'spatial_features/download' require 'spatial_features/unzip' require 'spatial_features/utils' diff --git a/lib/spatial_features/download.rb b/lib/spatial_features/download.rb index 5d3295b5..d881da7a 100644 --- a/lib/spatial_features/download.rb +++ b/lib/spatial_features/download.rb @@ -1,42 +1,93 @@ +require 'net/http' require 'open-uri' +require 'openssl' module SpatialFeatures module Download - # file can be a url, path, or file, any of which can return be a zipped archive - def self.open(file) - file = URI.open(file) - file = normalize_file(file) if file.is_a?(StringIO) - return file - end + REMOTE_URL = %r{\Ahttps?://}i.freeze + + # Seconds to wait for a remote source, applied both to establishing the connection and to + # each read. Without it a hung server holds an import worker open indefinitely. + mattr_accessor :timeout + self.timeout = 60 - # file can be a url, path, or file, any of which can return be a zipped archive - def self.open_each(path_or_url, unzip: nil, **unzip_options) - file = Download.open(path_or_url) - files = if unzip && Unzip.is_zip?(file) - find_in_zip(file, find: unzip, **unzip_options) - else - [file] + # Errors meaning a remote source could not be read at all, as opposed to being read and + # found unusable. `OpenURI::HTTPError` covers the 4xx and 5xx replies. + UNREACHABLE_ERRORS = [::OpenURI::HTTPError, ::SocketError, ::SystemCallError, + ::Net::OpenTimeout, ::Net::ReadTimeout, ::OpenSSL::SSL::SSLError].freeze + + class << self + # Returns an open File for `file`, which may be a URL, a path, or a File. The content may + # be a zipped archive; `::open_each` unwraps one. + # + # @raise [SpatialFeatures::ImportError] when a remote source cannot be reached. + def open(file) + file = fetch(file) + file = normalize_file(file) if file.is_a?(StringIO) + return file end - return files.map { |f| File.open(f) } - end + # Returns the body of `path_or_url` as a String, without writing it to disk. + # + # @raise [SpatialFeatures::ImportError] when a remote source cannot be reached. + def read(path_or_url) + fetch(path_or_url).read + end + + # Returns an open File for each source in `path_or_url`, unwrapping an archive when + # `unzip` is given a pattern its entries can match. + def open_each(path_or_url, unzip: nil, **unzip_options) + file = Download.open(path_or_url) + files = if unzip && Unzip.is_zip?(file) + find_in_zip(file, find: unzip, **unzip_options) + else + [file] + end - def self.normalize_file(file) - Tempfile.new.tap do |temp| - temp.binmode - temp.write(file.read) - temp.rewind + return files.map { |f| File.open(f) } end - end - def self.entries(file) - file = Kernel.open(file) - file = normalize_file(file) if file.is_a?(StringIO) - Unzip.entries(file) - end + def normalize_file(file) + Tempfile.new.tap do |temp| + temp.binmode + temp.write(file.read) + temp.rewind + end + end + + # Returns the entries of the archive at `file` without extracting them. + def entries(file) + file = fetch(file) + file = normalize_file(file) if file.is_a?(StringIO) + Unzip.entries(file) + end + + def find_in_zip(file, find:, **unzip_options) + Unzip.paths(file, find: find, **unzip_options) + end + + private - def self.find_in_zip(file, find:, **unzip_options) - Unzip.paths(file, find: find, **unzip_options) + # Returns an IO for `file`: fetched over the network when it is a remote URL, opened from + # disk when it is any other String, and left to open itself otherwise. + # + # @note A local path goes to `File.open`, never `URI.open`. `URI.open` hands anything + # that is not a URL to `Kernel#open`, which runs the name as a command when it begins + # with a pipe. + # @note Timeouts and the unreachable rescue apply only to a remote URL. `URI.open` + # rejects the timeouts when handed an already open file, and `Errno::ENOENT` for a + # local path is a `SystemCallError` that callers turn into a message for the person who + # uploaded the file, without naming the path the server looked in. + def fetch(file) + return URI.open(file) unless file.is_a?(String) + return File.open(file) unless file.match?(REMOTE_URL) + + begin + URI.open(file, :open_timeout => timeout, :read_timeout => timeout) + rescue *UNREACHABLE_ERRORS => e + raise SpatialFeatures::ImportError, "This source could not be reached. #{e.message}" + end + end end end end diff --git a/lib/spatial_features/gdal.rb b/lib/spatial_features/gdal.rb new file mode 100644 index 00000000..86a49481 --- /dev/null +++ b/lib/spatial_features/gdal.rb @@ -0,0 +1,42 @@ +require 'open3' + +module SpatialFeatures + # Runs the GDAL command line tools. + # + # Every argument reaches the tool as a single argv entry, so a path or a projection taken + # from an uploaded archive arrives as one string rather than as shell syntax. + module GDAL + class << self + # Returns what `tool` wrote to standard output. + # + # @param tool [String] the executable name, such as `ogr2ogr`. + # @param args [Array] one argv entry each. + # @return [String] the output, empty when the tool wrote nothing. + # @raise [Errno::ENOENT] when the tool is not installed. + def capture(tool, *args) + Open3.capture2(*argv(tool, args)).first + end + + # Runs `tool` for its exit status rather than its output. + # + # @param tool [String] the executable name, such as `ogr2ogr`. + # @param args [Array] one argv entry each. + # @return [Boolean] true when the tool exited successfully. + # @raise [Errno::ENOENT] when the tool is not installed. + def run(tool, *args) + system(*argv(tool, args)) + end + + private + + # Returns the argv to spawn `tool` with. + # + # @note The command is a two element array so that Ruby spawns the executable directly. + # Both `system` and `Open3` fall back to a shell when handed a lone string, which + # would put every argument here back in reach of shell parsing. + def argv(tool, args) + [[tool.to_s, tool.to_s], *args.map(&:to_s)] + end + end + end +end diff --git a/lib/spatial_features/importers/esri_geo_json.rb b/lib/spatial_features/importers/esri_geo_json.rb index 3e67d43d..eb099281 100644 --- a/lib/spatial_features/importers/esri_geo_json.rb +++ b/lib/spatial_features/importers/esri_geo_json.rb @@ -1,12 +1,18 @@ require 'digest/md5' -require 'open3' -require 'open-uri' +require 'json' require 'tempfile' require 'spatial_features/importers/geo_json' module SpatialFeatures module Importers class ESRIGeoJSON < GeoJSON + # How many pages to walk before concluding the endpoint is ignoring `resultOffset` and + # serving the same features over and over. A service capped at the usual 1000 or 2000 + # features per page would have to hold half a million features to reach this + # legitimately. Raise it for a layer that genuinely holds more. + class_attribute :max_pages + self.max_pages = 500 + def parsed_geojson @parsed_geojson ||= JSON.parse(geojson) end @@ -17,21 +23,33 @@ def geojson private + # Returns the layer as GeoJSON. A relative path is read from disk; anything else is + # downloaded first, so OGR is always given a local file. + # + # @param path_or_url [String] a local file path, or the URL of an ArcGIS query endpoint. + # @return [String] a GeoJSON FeatureCollection in EPSG:4326. + # @note Downloading rather than passing the URL to OGR is what makes an endpoint offering + # only HTTPS 1.1 readable. GDAL's curl client gets an empty reply from those servers, + # which surfaces as `ERROR 1: Empty reply from server`. def esri_json_to_geojson(path_or_url) - return ogr2ogr_to_geojson(path_or_url) if URI.parse(path_or_url).relative? # It is a local file path + return ogr2ogr_to_geojson(path_or_url) if URI.parse(path_or_url).relative? - # Download the URL ourselves rather than letting GDAL's curl fetch it. Servers that only - # offer HTTPS 1.1 may cause GDAL's curl to fail, but Ruby's open-uri can handle them. with_downloaded_file(path_or_url) do |path| ogr2ogr_to_geojson(path) end end # Returns the GeoJSON OGR reads out of the file at `path`, reprojected to EPSG:4326. + # OGR selects its driver by inspecting the content, so the file may hold either GeoJSON + # or ESRI JSON. + # + # @note No layer name is passed. OGR names a local file's layer after its basename, and + # naming a layer that does not exist fails the read. def ogr2ogr_to_geojson(path) - Open3.capture2('ogr2ogr', '-t_srs', 'EPSG:4326', '-f', 'GeoJSON', '/dev/stdout', path).first + GDAL.capture('ogr2ogr', '-t_srs', 'EPSG:4326', '-f', 'GeoJSON', '/dev/stdout', path) end + # Downloads the query into a tempfile and yields its path, removing it afterwards. def with_downloaded_file(url) Tempfile.create(['esri_geojson', '.json']) do |tempfile| tempfile.binmode @@ -41,16 +59,20 @@ def with_downloaded_file(url) end end - # ArcGIS query endpoints cap each response at the service's maxRecordCount - # (commonly 1000 or 2000 features) and signal exceededTransferLimit when - # more results are available. Walk the pages with resultOffset and merge - # them into a single FeatureCollection before handing to ogr2ogr. + # Walks the query's pages with `resultOffset` and writes them to `io` as one collection. + # ArcGIS endpoints cap each response at the service's `maxRecordCount`, commonly 1000 or + # 2000 features, and set `exceededTransferLimit` while more results are waiting. + # + # @note Raises `SpatialFeatures::ImportError` once `max_pages` requests have been made + # and the endpoint still reports more, since a server that ignores `resultOffset` + # otherwise repeats its first page until the process runs out of memory. def download_paginated(url, io) combined = nil offset = 0 + pages = 0 loop do - page = JSON.parse(URI.open(paginated_url(url, offset)).read) + page = fetch_page(paginated_url(url, offset)) page_features = page['features'] || [] if combined.nil? @@ -60,23 +82,53 @@ def download_paginated(url, io) end break if page_features.empty? || !exceeded_transfer_limit?(page) + + pages += 1 + if pages >= max_pages + raise SpatialFeatures::ImportError, + "This layer was still reporting more features after #{max_pages} requests. " \ + "The server may be ignoring the `resultOffset` parameter." + end + offset += page_features.length end - combined&.delete('exceededTransferLimit') - combined&.fetch('properties', {})&.delete('exceededTransferLimit') - io.write(JSON.dump(combined)) if combined + if combined + combined.delete('exceededTransferLimit') + combined['properties']&.delete('exceededTransferLimit') + io.write(JSON.dump(combined)) + end + end + + # Returns one page of the query. + # + # @return [Hash] the parsed response body. + # @raise [SpatialFeatures::ImportError] when the endpoint cannot be reached, or answers + # with a body that is not JSON. An endpoint that rejects a query replies with HTML or + # an error document, which the parse failure alone does not convey. + def fetch_page(url) + body = Download.read(url) + JSON.parse(body) + rescue JSON::ParserError + raise SpatialFeatures::ImportError, + "This layer did not return map data. The server replied with #{body.to_s[0, 100].inspect}." end + # Returns true while the service reports that more features are waiting. Services set the + # flag at the top level or under `properties` depending on the response format. def exceeded_transfer_limit?(page) page['exceededTransferLimit'] || page.dig('properties', 'exceededTransferLimit') end + # Returns `url` with `resultOffset` set to `offset`, replacing any the caller supplied. + # Returns it unchanged for the first page, so a service that does not paginate is asked + # exactly what the caller asked for. def paginated_url(url, offset) return url if offset.zero? + uri = URI.parse(url) params = URI.decode_www_form(uri.query || '') - params.reject! { |k, _| k == 'resultOffset' } + params.reject! { |key, _| key == 'resultOffset' } params << ['resultOffset', offset.to_s] uri.query = URI.encode_www_form(params) uri.to_s diff --git a/lib/spatial_features/importers/shapefile.rb b/lib/spatial_features/importers/shapefile.rb index fb62d22f..7bd302dd 100644 --- a/lib/spatial_features/importers/shapefile.rb +++ b/lib/spatial_features/importers/shapefile.rb @@ -1,6 +1,5 @@ require 'ostruct' require 'digest/md5' -require 'open3' module SpatialFeatures module Importers @@ -101,7 +100,7 @@ def validate_shapefile!(file_path) def project_to_4326(file_path) output_path = Tempfile.create([::File.basename(file_path, '.shp') + '_epsg_4326_', '.shp']) { |file| file.path } return unless (proj4 = proj4_from_file(file_path)) - return unless system('ogr2ogr', '-s_srs', proj4, '-t_srs', 'EPSG:4326', output_path, file_path) + return unless GDAL.run('ogr2ogr', '-s_srs', proj4, '-t_srs', 'EPSG:4326', output_path, file_path) return ::File.open(output_path) end @@ -112,7 +111,7 @@ def proj4_from_file(file_path) # Sanitize: "'+proj=utm +zone=11 +datum=NAD83 +units=m +no_defs '\n" and lately # "+proj=utm +zone=11 +datum=NAD83 +units=m +no_defs \n" to # "+proj=utm +zone=11 +datum=NAD83 +units=m +no_defs" - Open3.capture2('gdalsrsinfo', file_path, '-o', 'proj4').first.strip.remove(/^'|'$/).presence + GDAL.capture('gdalsrsinfo', file_path, '-o', 'proj4').strip.remove(/^'|'$/).presence rescue Errno::ENOENT nil end diff --git a/spec/lib/spatial_features/download_spec.rb b/spec/lib/spatial_features/download_spec.rb new file mode 100644 index 00000000..d51bfda1 --- /dev/null +++ b/spec/lib/spatial_features/download_spec.rb @@ -0,0 +1,62 @@ +require 'spec_helper' + +describe SpatialFeatures::Download do + # `Kernel#open` runs its argument as a command when the string begins with a pipe, so a + # source name that reaches it is a command the caller never intended to run. + describe 'a source name beginning with a pipe' do + let(:marker) { ::File.join(Dir.mktmpdir, 'executed') } + + it 'is not run as a command by ::open' do + SpatialFeatures::Download.open("|touch #{marker}") rescue nil + + expect(::File.exist?(marker)).to be false + end + + it 'is not run as a command by ::entries' do + SpatialFeatures::Download.entries("|touch #{marker}") rescue nil + + expect(::File.exist?(marker)).to be false + end + + it 'is not run as a command by ::read' do + SpatialFeatures::Download.read("|touch #{marker}") rescue nil + + expect(::File.exist?(marker)).to be false + end + end + + describe '::read' do + it 'returns the body of a remote source' do + allow(URI).to receive(:open).and_return(StringIO.new('body')) + + expect(SpatialFeatures::Download.read('https://example.com/layer.json')).to eq('body') + end + + it 'bounds the request with `timeout`' do + allow(SpatialFeatures::Download).to receive(:timeout).and_return(9) + expect(URI).to receive(:open) + .with('https://example.com/layer.json', :open_timeout => 9, :read_timeout => 9) + .and_return(StringIO.new('body')) + + SpatialFeatures::Download.read('https://example.com/layer.json') + end + + it 'raises when the source cannot be reached' do + allow(URI).to receive(:open).and_raise(SocketError.new('getaddrinfo failed')) + + expect { SpatialFeatures::Download.read('https://example.com/layer.json') } + .to raise_exception(SpatialFeatures::ImportError, /could not be reached/i) + end + end + + describe '::open' do + # A missing local file is an `Errno::ENOENT`, which is a `SystemCallError` and so would be + # caught by the unreachable rescue if that rescue covered local paths. Callers turn it into + # a message for the person who uploaded the file, and that message must not name the path + # the server looked in. + it 'lets a missing local path raise Errno::ENOENT rather than reporting it as unreachable' do + expect { SpatialFeatures::Download.open('/nonexistent/path/to/upload.zip') } + .to raise_exception(Errno::ENOENT) + end + end +end diff --git a/spec/lib/spatial_features/importers/esri_geojson_spec.rb b/spec/lib/spatial_features/importers/esri_geojson_spec.rb new file mode 100644 index 00000000..6a02efb6 --- /dev/null +++ b/spec/lib/spatial_features/importers/esri_geojson_spec.rb @@ -0,0 +1,205 @@ +require 'spec_helper' + +describe SpatialFeatures::Importers::ESRIGeoJSON do + let(:url) { 'https://example.com/arcgis/rest/services/Layer/MapServer/0/query?f=geojson&where=1%3D1' } + + # A page of the shape an ArcGIS `f=geojson` query returns. `exceeded` sets the flag the + # service uses to say more features are waiting. + def page(count, exceeded: false, start: 0, nested_flag: false) + features = Array.new(count) do |i| + n = start + i + { 'type' => 'Feature', + 'properties' => { 'name' => "Area #{n}", 'prop0' => 'value' }, + 'geometry' => { 'type' => 'Polygon', + 'coordinates' => [[[n, 20], [n + 1, 20], [n + 1, 21], [n, 21], [n, 20]]] } } + end + + collection = { 'type' => 'FeatureCollection', 'features' => features } + if exceeded + nested_flag ? collection['properties'] = { 'exceededTransferLimit' => true } + : collection['exceededTransferLimit'] = true + end + StringIO.new(JSON.dump(collection)) + end + + # A page of the shape an ArcGIS `f=json` query returns, which is ESRI JSON rather than + # GeoJSON: attributes instead of properties, rings instead of coordinates, and the geometry + # type declared once for the collection. OGR reads it by sniffing the content. + def esri_page(count, exceeded: false) + features = Array.new(count) do |i| + { 'attributes' => { 'name' => "Area #{i}", 'prop0' => 'value' }, + 'geometry' => { 'rings' => [[[i, 20], [i + 1, 20], [i + 1, 21], [i, 21], [i, 20]]] } } + end + + collection = { 'displayFieldName' => 'name', + 'geometryType' => 'esriGeometryPolygon', + 'spatialReference' => { 'wkid' => 4326 }, + 'fields' => [{ 'name' => 'name', 'type' => 'esriFieldTypeString', 'length' => 50 }, + { 'name' => 'prop0', 'type' => 'esriFieldTypeString', 'length' => 50 }], + 'features' => features } + collection['exceededTransferLimit'] = true if exceeded + StringIO.new(JSON.dump(collection)) + end + + # Answers each request in turn, recording the URLs it was asked for. + def stub_pages(*pages) + requested = [] + allow(URI).to receive(:open) do |requested_url| + requested << requested_url + pages.shift || raise("requested more pages than the stub was given: #{requested_url}") + end + requested + end + + describe '#features' do + context 'when the response fits in a single page' do + it 'returns the features and makes one request' do + requested = stub_pages(page(3)) + + expect(subject_for(url).features.count).to eq(3) + expect(requested.length).to eq(1) + expect(requested.first).to eq(url) + end + end + + context 'when the response is capped at maxRecordCount' do + it 'walks the pages and returns every feature' do + requested = stub_pages(page(2, :exceeded => true), page(2, :exceeded => true, :start => 2), page(1, :start => 4)) + + expect(subject_for(url).features.count).to eq(5) + expect(requested.length).to eq(3) + end + + it 'asks for each page by resultOffset' do + requested = stub_pages(page(2, :exceeded => true), page(1, :start => 2)) + + subject_for(url).features + + expect(requested[0]).not_to include('resultOffset') + expect(requested[1]).to include('resultOffset=2') + end + + it 'keeps the query the caller supplied' do + requested = stub_pages(page(2, :exceeded => true), page(1, :start => 2)) + + subject_for(url).features + + expect(requested[1]).to include('f=geojson') + expect(requested[1]).to include('where=1%3D1') + end + + it 'recognises the flag when the service nests it under properties' do + requested = stub_pages(page(2, :exceeded => true, :nested_flag => true), page(1, :start => 2)) + + expect(subject_for(url).features.count).to eq(3) + expect(requested.length).to eq(2) + end + end + + context 'when the service ignores resultOffset' do + it 'gives up rather than paginating forever' do + allow(URI).to receive(:open) { page(2, :exceeded => true) } + + expect { subject_for(url).features } + .to raise_exception(SpatialFeatures::ImportError, /still reporting more features/i) + end + + it 'gives up after `max_pages` requests' do + requested = [] + allow(URI).to receive(:open) do |requested_url| + requested << requested_url + page(2, :exceeded => true) + end + allow(SpatialFeatures::Importers::ESRIGeoJSON).to receive(:max_pages).and_return(3) + + expect { subject_for(url).features }.to raise_exception(SpatialFeatures::ImportError) + expect(requested.length).to eq(3) + end + end + + context 'when the service replies with something other than a feature collection' do + it 'raises rather than surfacing a parse error' do + allow(URI).to receive(:open).and_return(StringIO.new('Forbidden')) + + expect { subject_for(url).features } + .to raise_exception(SpatialFeatures::ImportError, /did not return map data/i) + end + end + + context 'when the service cannot be reached' do + it 'raises for an HTTP error status' do + allow(URI).to receive(:open) + .and_raise(OpenURI::HTTPError.new('404 Not Found', StringIO.new)) + + expect { subject_for(url).features } + .to raise_exception(SpatialFeatures::ImportError, /could not be reached/i) + end + + it 'raises when the connection times out' do + allow(URI).to receive(:open).and_raise(Net::ReadTimeout) + + expect { subject_for(url).features } + .to raise_exception(SpatialFeatures::ImportError, /could not be reached/i) + end + + it 'bounds each request with `Download.timeout`' do + allow(SpatialFeatures::Download).to receive(:timeout).and_return(7) + expect(URI).to receive(:open) + .with(url, :open_timeout => 7, :read_timeout => 7) + .and_return(page(1)) + + subject_for(url).features + end + end + + context 'when the service answers in ESRI JSON rather than GeoJSON' do + let(:url) { 'https://example.com/arcgis/rest/services/Layer/MapServer/0/query?f=json&where=1%3D1' } + + it 'returns the features' do + stub_pages(esri_page(2)) + + expect(subject_for(url).features.count).to eq(2) + end + + it 'carries the attributes through as metadata' do + stub_pages(esri_page(2)) + + expect(subject_for(url).features).to all(have_attributes(:metadata => include('prop0' => 'value'))) + end + + it 'walks the pages the same way' do + requested = stub_pages(esri_page(2, :exceeded => true), esri_page(1)) + + expect(subject_for(url).features.count).to eq(3) + expect(requested.length).to eq(2) + expect(requested[1]).to include('resultOffset=2') + end + end + + context 'when given a local path containing shell metacharacters' do + # `URI.parse` rejects a path holding a quote or a space, so the payload has to be one it + # accepts: `$IFS` stands in for the space, and `$(...)` is what a shell would expand + # inside the double quotes a command string wraps a path in. + it 'does not run the injected command' do + marker = ::File.join(Dir.mktmpdir, 'injected') + + subject_for("/tmp/x$(touch$IFS#{marker}).json").features rescue nil + + expect(::File.exist?(marker)).to be false + end + end + + context 'when given a local file path' do + it 'reads the file without downloading it' do + expect(URI).not_to receive(:open) + + expect(subject_for(fixture_file_path('geo.json')).features).to be_present + end + end + + end + + def subject_for(data) + SpatialFeatures::Importers::ESRIGeoJSON.new(data) + end +end