Add the API surface openstreetmap 2.0 depends on: service interface, result object, presets, Feeds fetcher, packaging

Branch: 3618910-osm-2.0-api-surface, off 1.0.x at 9c20cd53. Related: #3618910 (collaboration thread), #3614000 (preset config entity), #3614002 (Feeds).

Why

openstreetmap 2.0 is being built against this module rather than carrying its own Overpass transport, on the principle we agreed in #3618910: anything that manages the connection to Overpass lives in overpass_api; anything about OpenStreetMap data lives in openstreetmap. Overpass is one way to retrieve OSM data; the OSM API, planet extracts and exported files are others.

This branch adds what openstreetmap 2.0 needs to be built and tested against an interface rather than a concrete class, and picks up the fixes that turned up while reading the code to do it. Everything here changes the beta API, so it wants to land as one release.

What openstreetmap 2.0 calls

openstreetmap component API used
Add an OSM node by id getElement()
Sync one node getElements($type, [$id], 'meta geom qt')
Sync all / sync source run PresetExecutorInterface::execute() with a sink, then iterateElements()
Sync source validation OverpassPresetInterface::getQueryType(), buildQuery()
Feeds parser (openstreetmap_feeds) OverpassResultInterface::iterateElements() on the fetcher's file
Config dependencies overpass_api.preset.* config names
Tests doubles of OverpassApiInterface and PresetExecutorInterface

Bugs found and fixed

These were found by reading the code and confirmed against the live endpoint at https://overpass-api.de/api/interpreter or by a failing test, not inferred:

  1. query() emitted an invalid query whenever no filter was set. The wrapper wrote $options->filter on its own line, so an empty filter became a bare ; statement. Overpass rejects that with parse error: Unknown type ";" — including for the exact call in the module's own README. Covered by OverpassApiTest::testWrapOmitsAnEmptyFilterStatement.

  2. Connect timeouts were never retried. ConnectException extends TransferException, not RequestException, in Guzzle 7, so the catch (RequestException | BadResponseException $e) in rawQuery() never saw one — and the if ($code == 0 && $e instanceof ConnectException) branch inside it was unreachable. A DNS failure or connect timeout propagated raw on the first attempt. The catch is now TransferException. Covered by testRawQueryRetriesConnectException, which failed before the fix.

  3. Runtime errors were reported as success. Overpass answers a runtime error or its own timeout with HTTP 200, a remark, and an empty or partial elements array. query() returned that partial array, so a caller stored truncated data believing the query had succeeded. execute() now throws OverpassApiRuntimeException, carrying the remark and the partial count; advisory remarks are logged instead of thrown.

  4. The query builder's {{bbox}} substitution broke queries pasted from Overpass Turbo. It substituted (south,west,north,east), parentheses included, so node["amenity"="cafe"]({{bbox}}) — how Turbo writes it — became node[...]((s,w,n,e)), which Overpass rejects with parse error: Unknown query clause. The token now expands to bare coordinates, as in Turbo.

  5. rawQuery() built Accept and Content-Type headers and never passed them, called \Drupal::httpClient() instead of the injected client, and fetched lock.persistent statically. All three fixed. See "Deviations" below on the header values.

  6. Utils::returnBytes() returned the string "-" for a memory_limit of -1, because it stripped the 1 as a unit suffix. The memory guard was therefore dividing by a negative number on unlimited-memory hosts. It now returns -1, and the guard treats a non-positive limit as no limit.

  7. A genuine 500's body never reached the caller. The switch treats 500 as a proxy's gateway timeout and retries it, but after the last attempt only ClientException was wrapped, so a ServerException was thrown bare and Guzzle's message truncates the body to 120 characters. Server errors are now wrapped the same way.

  8. [out:count] is not valid Overpass syntax — the correct form is out count; as the out statement. The doc this branch implements suggested the former for the preset Test button; verified against the live endpoint (Unknown output format: count) and implemented as the latter.

  9. The preset duplicate form would have saved over the preset it was opened from, since DefaultHtmlRouteProvider hands the form the persisted original. It duplicates first now. (New code in this branch, caught in review before it shipped.)

  10. tests/src/PHPCodeSniffer/Standard.php was dead and broken — it extends PHP_CodeSniffer\Standards\AbstractSniff, which exists in no PHP_CodeSniffer release, is referenced by no ruleset, and was itself the source of 56 of the module's coding-standards errors. Deleted.

The public API

OverpassApiInterface

The service keeps its id and implements the interface; services.yml aliases Drupal\overpass_api\OverpassApiInterface to it so consumers can autowire.

  • execute($query, $options): OverpassResultInterface — sends a complete query verbatim, checks the remark, returns a decoded result.
  • wrap($statements, $options): string — the wrapping logic query() has always applied, exposed so a preset can produce a complete query with the caller's own out verbosity.
  • getElements($type, $ids, $out) — chunks ids (chunk_size, 200 by default) into node(id:1,2,3); statements and merges the results, replacing one-request-per-element loops.
  • rawQuery($query, $options), query($statements, $options), getElement($type, $id), getStatus(), getSlotAvailability() — kept.

OverpassResultInterface

getElements(), iterateElements(), getDataTimestamp(), getRemark(), getGenerator(), getFilePath(), getRaw(), Countable.

OverpassResult::fromString() decodes lazily, so a caller that only wants the remark does not pay for decoding the elements. OverpassResult::fromFile() reads a file-backed response with halaxa/json-machine (MIT, no dependencies, added to require), never holding more than one element at a time — which is what makes a 100k-element result workable for a queue-based synchroniser and for the Feeds fetcher/parser pair. It also lets a parser wrap a response it did not fetch itself, for example a file delivered by Feeds' own HTTP or upload fetcher; openstreetmap_feeds relies on that.

OverpassQueryOptions additions

Property Default Purpose
sink NULL Stream URI the body is written to with Guzzle's own sink, instead of being buffered. The result then exposes getFilePath().
maxAttempts NULL (config) Per-call override of repeat_attempts, so an interactive caller can fail fast.
userAgent NULL (config) Per-call override of the configured User-Agent.

returnRaw is deprecated in favour of rawQuery() and execute()->getRaw(). It still works and is removed in 2.0.0.

Exceptions

OverpassApiException extends \RuntimeException is the new base, so one catch will do. OverpassApiHttpException and OverpassApiMemoryLimitException now extend it (both previously extended \Exception, which \RuntimeException does too, so nothing that catches them breaks). New: OverpassApiRuntimeException (carries the remark and partial count) and OverpassApiConfigurationException.

Presets (#3614000)

The overpass_preset config entity is in the main module, not a submodule, so openstreetmap can depend on overpass_api:overpass_api and still reference presets on every site.

config_prefix: preset, admin permission administer overpass presets, list, add, edit, duplicate and delete forms under /admin/structure/overpass-preset, and a full config schema so other modules' config can declare dependencies: config: [overpass_api.preset.X] and kernel tests with strict schema checking pass.

It stores the parts of a query — query_type, osm_types, filters, spatial, timeout, raw_query — and generates the OQL from them, so an edit can never leave a stale generated copy behind. The geocoded area id and coordinates are stored, so a cron run never calls Nominatim.

OverpassPresetInterface::buildQuery() returns a complete query: builder modes wrap their statements with the preset's timeout and the caller's out verbosity; a raw preset is returned verbatim with only {{bbox}} substituted, and $options->out is ignored because the author owns the out statements. PresetExecutorInterface runs one without the caller touching the API service, and its count() backs the Test button.

UI: a "Load preset" select and a "Save as preset" field on the query builder, both gated on the new permission; saving captures the resolved geocoding results. A Test button on the preset edit form reporting the element count and the OSM data timestamp.

Feeds fetcher (#3614002, fetcher half)

overpass_api_feeds provides an "Overpass API" fetcher. fetch() streams to a sink and returns a FetcherResult for the file; nothing is decoded in the fetcher. A runtime error from Overpass becomes a FetchException and the partial file is removed, rather than being handed to the parser.

The parser — turning OSM elements into mappable sources — is about OSM data rather than about the connection to Overpass, so it belongs in the openstreetmap queue and ships there as openstreetmap_feeds. Suggest re-titling #3614002 to "Add overpass_api_feeds fetcher" with a note pointing at the sibling issue. Neither half requires the other: this fetcher hands its file to any parser, and osm_json will parse a file that arrived through Feeds' own HTTP or upload fetcher.

Packaging

  • Root composer.json no longer requires drupal/geocoder, drupal/leaflet or geocoder-php/nominatim-provider. drupal.org packaging reads only the root file, so every consumer of overpass_api was installing Leaflet, Geocoder and a Nominatim client whether or not it used the GUI. They move to suggest, alongside drupal/feeds, and to require-dev so CI can still install the submodules' tests. halaxa/json-machine is added to require.
  • core_version_requirement: ^10.3 || ^11 and php: 8.1 on every .info.yml.
  • Endpoint default becomes https://overpass-api.de/api/, with an update hook that moves a site still on the old http default across.
  • .gitlab-ci.yml, phpcs.xml.dist and phpstan.neon, so the tests here run on drupal.org CI.
  • README rewritten. It no longer claims Drupal 8 support and no longer mentions spatie/data-transfer-object, removed in 38b977e6.
  • package: OpenStreetMap on all three .info.yml files, replacing OSM. The Extend page groups modules by package and wraps each group in a <details>, so the two projects — which are meant to be installed together — were landing in separate collapsible sections.

Not something a commit can do: please opt in to security advisory coverage on the project page. openstreetmap is covered, is used by around 90 sites, and now depends on this module; a covered module depending on an uncovered one is something site owners notice.

Also changed: where the QL generation lives

The pure Overpass QL generation moves out of overpass_api_ui into a new overpass_api.query_builder service in the main module, because a preset has to build its query on a site where the GUI submodule is not installed. The submodule's QueryBuilder keeps its whole public API and its geocoding, and delegates QL generation to the new service — its 22 existing unit tests pass unchanged against the delegating implementation, which is the evidence that the move is behaviour-preserving. Fixing bug 4 above fell out of it.

Three tests were added alongside them, asserting the constructor's arity and parameter type and the behaviour of the two delegating methods. They guard a failure mode worth knowing about: PHP silently discards surplus arguments to a userland constructor, so a QueryBuilder built with a statement builder it did not accept would still have satisfied the existing tests.

Deviations from the plan in #3618910

Four, each for a reason:

  1. query() has no : array return type. The spec gives it one, but the existing $options->returnRaw makes it return a string. Typing it array would break that silently, so the interface leaves it untyped with an @return array|string docblock and deprecates returnRaw instead.
  2. The request headers are Content-Type: text/plain; charset=utf-8, and there is no Accept. The spec says to pass the headers rawQuery() was building. But the body of a POST to /interpreter is Overpass QL, not JSON, so Content-Type: application/json would misinform any proxy in front of the endpoint; and the response format is chosen by the query's own [out:...] setting, so a hard-coded Accept: application/json would be wrong for an xml or csv query. The wrong headers are dropped and an accurate one is sent.
  3. The Feeds plugin uses the @FeedsFetcher annotation, not the #[FeedsFetcher] attribute. Feeds only gained Drupal\feeds\Attribute\FeedsFetcher recently and keeps the annotation for older releases (its plugin manager uses AttributeDiscoveryWithAnnotations), so the annotation works on every Feeds 3.x a site might have.
  4. The preset Test button uses out count;, not [out:count]. See bug 8.

Two smaller additions beyond the plan: the CI scaffolding, so the new tests actually run; and rawQuery() taking OverpassQueryOptions, which is what lets the settings form and the query builder pass maxAttempts and fail fast in a web request without going through execute() and decoding a body they only want as text.

Retry defaults

With the old defaults (180 s timeout, 8 s lock, 10 attempts) a failing call could hold a PHP process for over half an hour, and raised max_execution_time to match. Fine for cron and queue workers, hostile in a web request.

The install defaults drop to 60 s and 3 attempts, and the two paths that run inside a web request — the settings form's test query and the query builder's "Run query" — pass maxAttempts explicitly. Existing sites keep their configured values; nothing rewrites them.

Testing

109 unit tests, 258 assertions, all passing on PHP 8.3 with Drupal 11.2 and PHPUnit 11. Guzzle's MockHandler plus a history middleware covers the transport: the retry sequence, connect-exception retries, remark detection on both buffered and streamed responses, getElements() chunking, sink handling, header and body contents, endpoint validation, and the exception hierarchy. QueryBuilder, the preset entity, PresetExecutor, OverpassResult (both backings) and the Feeds fetcher each have their own tests.

The module passes phpcs with Drupal and DrupalPractice with no errors and no warnings.

Not covered by automated tests: the preset and query-builder form flows and the list builder. Those want functional tests, which is worth a follow-up issue rather than holding this up.

That gap is not theoretical. While this branch was being reviewed on a real site, /admin/config/system/overpass-api turned out to be a hard ArgumentCountError on Drupal 11: the injected SettingsForm passed only the config factory to ConfigFormBase, whose second argument, the typed config manager, core made required in 11 after deprecating it as optional in 10.3. It worked on 10.3 and fataled on 11. A unit-only suite cannot see that — a constructor signature mismatch only surfaces once the container builds the form. Fixed in "Fix the settings form fataling on Drupal 11"; a functional test that simply requests the settings page would guard the whole class of problem.

Suggested review order

The commits are separable and each is self-consistent:

  1. Add OverpassApiInterface, a result object and runtime-error detection
  2. Add batched element fetch and streamed results
  3. Add the overpass_preset config entity and a preset executor
  4. Add the overpass_api_feeds fetcher submodule
  5. Packaging: move UI dependencies to suggest, https endpoint, CI
  6. Hardening: inject services, fail fast in web requests, bounded bodies
  7. Fix the preset duplicate form, and cover the Feeds fetcher
  8. Fix the settings form fataling on Drupal 11
  9. Land the overpass_api_ui half of the query builder refactor
  10. Group with the OpenStreetMap modules on the Extend page

Happy to split any of them into their own MR if that is easier to review.

Edited by Tyler Cifu Shuster

Merge request reports

Loading
Loading