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:
-
query()emitted an invalid query whenever no filter was set. The wrapper wrote$options->filteron its own line, so an empty filter became a bare;statement. Overpass rejects that withparse error: Unknown type ";"— including for the exact call in the module's own README. Covered byOverpassApiTest::testWrapOmitsAnEmptyFilterStatement. -
Connect timeouts were never retried.
ConnectExceptionextendsTransferException, notRequestException, in Guzzle 7, so thecatch (RequestException | BadResponseException $e)inrawQuery()never saw one — and theif ($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 nowTransferException. Covered bytestRawQueryRetriesConnectException, which failed before the fix. -
Runtime errors were reported as success. Overpass answers a runtime error or its own timeout with HTTP 200, a
remark, and an empty or partialelementsarray.query()returned that partial array, so a caller stored truncated data believing the query had succeeded.execute()now throwsOverpassApiRuntimeException, carrying the remark and the partial count; advisory remarks are logged instead of thrown. -
The query builder's
{{bbox}}substitution broke queries pasted from Overpass Turbo. It substituted(south,west,north,east), parentheses included, sonode["amenity"="cafe"]({{bbox}})— how Turbo writes it — becamenode[...]((s,w,n,e)), which Overpass rejects withparse error: Unknown query clause. The token now expands to bare coordinates, as in Turbo. -
rawQuery()builtAcceptandContent-Typeheaders and never passed them, called\Drupal::httpClient()instead of the injected client, and fetchedlock.persistentstatically. All three fixed. See "Deviations" below on the header values. -
Utils::returnBytes()returned the string"-"for amemory_limitof-1, because it stripped the1as 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. -
A genuine 500's body never reached the caller. The
switchtreats 500 as a proxy's gateway timeout and retries it, but after the last attempt onlyClientExceptionwas wrapped, so aServerExceptionwas thrown bare and Guzzle's message truncates the body to 120 characters. Server errors are now wrapped the same way. -
[out:count]is not valid Overpass syntax — the correct form isout 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. -
The preset duplicate form would have saved over the preset it was opened from, since
DefaultHtmlRouteProviderhands the form the persisted original. It duplicates first now. (New code in this branch, caught in review before it shipped.) -
tests/src/PHPCodeSniffer/Standard.phpwas dead and broken — it extendsPHP_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 logicquery()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) intonode(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.jsonno longer requiresdrupal/geocoder,drupal/leafletorgeocoder-php/nominatim-provider. drupal.org packaging reads only the root file, so every consumer ofoverpass_apiwas installing Leaflet, Geocoder and a Nominatim client whether or not it used the GUI. They move tosuggest, alongsidedrupal/feeds, and torequire-devso CI can still install the submodules' tests.halaxa/json-machineis added torequire. core_version_requirement: ^10.3 || ^11andphp: 8.1on 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.distandphpstan.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: OpenStreetMapon all three.info.ymlfiles, replacingOSM. 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:
query()has no: arrayreturn type. The spec gives it one, but the existing$options->returnRawmakes it return a string. Typing itarraywould break that silently, so the interface leaves it untyped with an@return array|stringdocblock and deprecatesreturnRawinstead.- The request headers are
Content-Type: text/plain; charset=utf-8, and there is noAccept. The spec says to pass the headersrawQuery()was building. But the body of a POST to/interpreteris Overpass QL, not JSON, soContent-Type: application/jsonwould misinform any proxy in front of the endpoint; and the response format is chosen by the query's own[out:...]setting, so a hard-codedAccept: application/jsonwould be wrong for an xml or csv query. The wrong headers are dropped and an accurate one is sent. - The Feeds plugin uses the
@FeedsFetcherannotation, not the#[FeedsFetcher]attribute. Feeds only gainedDrupal\feeds\Attribute\FeedsFetcherrecently and keeps the annotation for older releases (its plugin manager usesAttributeDiscoveryWithAnnotations), so the annotation works on every Feeds 3.x a site might have. - 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:
Add OverpassApiInterface, a result object and runtime-error detectionAdd batched element fetch and streamed resultsAdd the overpass_preset config entity and a preset executorAdd the overpass_api_feeds fetcher submodulePackaging: move UI dependencies to suggest, https endpoint, CIHardening: inject services, fail fast in web requests, bounded bodiesFix the preset duplicate form, and cover the Feeds fetcherFix the settings form fataling on Drupal 11Land the overpass_api_ui half of the query builder refactorGroup with the OpenStreetMap modules on the Extend page
Happy to split any of them into their own MR if that is easier to review.