Problem/Motivation

The access token is stored in the base module (mapillary.settings:access_token), but the Graph API client that uses it, MapillaryApiClient, lives in mapillary_field, together with MapillaryApiCallEvent and MissingAccessTokenException. This causes several problems:

  • The base module cannot use the client. ApiSettingsForm builds the Graph API URL and the Authorization header by hand to check a new token.
  • mapillary_detection, mapillary_map_feature and mapillary_logs depend on mapillary_field partly or only for the client and its event.
  • The client only tells a 404 apart; every other failure is a plain \RuntimeException. MapillaryThumbnailRefresher::isServiceWideFailure() and isBadIdAnswer() unwrap the Guzzle exception with getPrevious() and parse the JSON body again to find the HTTP status and the Graph API error code.
  • A 429 answer and its Retry-After header are not handled; the caller cannot tell a rate limit from any other failure without the same unwrapping.
  • The client logs failed requests to the mapillary_field channel at error level, including expected answers such as 400 with error code 100 (a deleted or unknown image). The callers that run unattended (queue workers, the thumbnail refresher, Drush) log the same failure again, and Mapillary Logs logs it a third time when "Log errors" is on.
  • When the token is missing, the client throws before dispatching MapillaryApiCallEvent, so Mapillary Logs never records it.
  • The event is dispatched in four places in get(), one per exit path.
  • IDs are not validated before they are put into the request path. Only the Leaflet widgets (MapillaryLeafletWidgetBase) check the ID format. The Image, Sequence and Map Feature ID widgets, the image and sequence import forms and MapillaryDetectionImportForm send any input as '/' . $id, so an input such as me or 123/detections reaches another Graph API endpoint with the server token.

Steps to reproduce

  1. Enable mapillary_field, mapillary_logs and dblog, set a valid access token, and turn on "Log API calls" at /admin/config/services/mapillary/logs.
  2. Go to /admin/content/mapillary/images/import and import the image ID me.
  3. Open /admin/reports/dblog. Mapillary Logs shows a request to /me, the account of the token owner.
  4. Clear the token: drush cset mapillary.settings access_token '', and import any image ID. The form shows "Mapillary access token is not configured.", and there is no mapillary_logs entry for it.

Proposed resolution

  • Move MapillaryApiClient, MapillaryApiCallEvent and MissingAccessTokenException to the base module, and register the client as mapillary.api_client.
  • Add MapillaryApiException (extends \RuntimeException), thrown for every failed request. It carries the HTTP status, the Graph API error code and, for a 429, the Retry-After value, and answers isNotFound(), isBadId() (404, or 400 with error code 100) and isServiceWide() (missing or rejected token, missing permissions, rate limits). MissingAccessTokenException extends it.
  • Add typed methods that validate the ID before any request is sent: getImage(), getImages() (the multi-ID lookup), getSequenceImageIds(), getMapFeature(), getDetections(). Image, map feature and detection IDs are digits only; sequence IDs are letters, digits, _ and -, as in the Leaflet widgets. An invalid ID throws a MapillaryApiException for which isBadId() is TRUE. The importers, the thumbnail refresher and mapillary_field_update_11102() use these methods. get() stays for other paths.
  • Add checkToken() for ApiSettingsForm, so the form no longer builds the request itself.
  • Dispatch MapillaryApiCallEvent once, in a finally block, also when the token is missing.
  • Stop logging in the client. The exception goes to the caller, which knows whether the failure is expected: the queue workers, the thumbnail refresher and Drush already log it, and the widgets and forms show it. Mapillary Logs records every failed request when "Log errors" is on. This removes the double logging left open in #3629730: Mapillary Logs: error logging only works when API call logging is on.
  • MapillaryThumbnailRefresher uses the exception methods instead of unwrapping Guzzle exceptions.
  • mapillary_logs subscribes to the event under its new class. It keeps its mapillary_field dependency for now, because MapillaryViewerRenderEvent stays there.

Removing the mapillary_field dependency of mapillary_detection and mapillary_map_feature is left to a follow-up that moves the image and sequence importer to mapillary_entity; both modules still need that importer.

Remaining tasks

  • Unit tests for the client: each status and Graph API error code maps to the right exception and methods; Retry-After is read as seconds and as an HTTP date; an invalid ID sends no request; the event is dispatched exactly once on every path, including a missing token.
  • Tests that the old service ID and the old class names still work, including catching the old MissingAccessTokenException.
  • Update ApiSettingsFormTest, MapillaryThumbnailRefresherFailureTest, the importer tests and the Mapillary Logs tests for the new classes.
  • An empty post_update in mapillary so that drush updb rebuilds the container.
  • CHANGELOG: the client no longer logs to the mapillary_field channel; failed requests are logged by the caller and by Mapillary Logs.

User interface changes

  • An invalid ID in a widget or an import form is rejected with "... is not a valid Mapillary ... ID." without a request to Mapillary.

API changes

  • New service mapillary.api_client. mapillary_field.api_client stays as a deprecated alias.
  • New classes in mapillary: MapillaryApiClient, MapillaryApiCallEvent, MissingAccessTokenException, MapillaryApiException. The old class names in mapillary_field stay as deprecated aliases, so type hints, event listeners and catch blocks that use them keep working.
  • The event name mapillary_field.api_call does not change.
  • Failures are MapillaryApiException, a \RuntimeException, so code that catches \RuntimeException keeps working.
  • MapillaryThumbnailRefresher::isServiceWideFailure() is deprecated in favour of MapillaryApiException::isServiceWide().
  • The mapillary_field logger channel no longer receives client errors.

Data model changes

  • None.

Issue fork mapillary-3629746

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

orkutmuratyilmaz created an issue. See original summary.

  • orkutmuratyilmaz committed 94c83aee on 1.0.x
    task: #3629746: Move the Graph API client to the base module and add a...
orkutmuratyilmaz’s picture

Status: Active » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.