Conversation
alexeyqu
marked this pull request as ready for review
September 8, 2026 01:08
alexeyqu
force-pushed
the
feat/imgur-configurable-urls
branch
from
September 9, 2026 01:19
c6d5b84 to
79e64aa
Compare
lisad
approved these changes
Sep 9, 2026
alexeyqu
force-pushed
the
feat/imgur-configurable-urls
branch
from
September 14, 2026 23:35
79e64aa to
d9b417f
Compare
ImgurTransferExtension hardcoded the API root and ImgurOAuthConfig
hardcoded the authorization and token endpoints, so pointing the adapter
at a staging environment or a test double meant rebuilding it.
Both now read from config/imgur.yaml on the classpath, falling back to
Imgur's own endpoints when no file is present:
serviceConfig:
baseUrl: https://api.imgur.com/3
authUrl: https://api.imgur.com/oauth2/authorize
tokenUrl: https://api.imgur.com/oauth2/token
This follows the convention Flickr, Deezer and Synology already use for
other per-service settings; Imgur had simply not adopted it. Both the
exporter and the importer already took baseUrl as a constructor
parameter, so only the extension-level constants needed to change.
One asymmetry worth noting for review: ImgurOAuthConfig calls the static
TransferServiceConfig.getForService rather than taking an injected
instance, because OAuth2ServiceExtension is never handed a
service-scoped one the way WorkerModule hands one to a TransferExtension.
Behaviour is unchanged when no config/imgur.yaml is on the classpath,
which is the case for every distribution in this repo today.
Adds the first tests for either class.
alexeyqu
force-pushed
the
feat/imgur-configurable-urls
branch
from
September 14, 2026 23:39
d9b417f to
68226e3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Makes
ImgurTransferExtension's API root andImgurOAuthConfig's authorization and token endpoints configurable, so the adapter can be pointed at a staging environment or a test double without rebuilding it.All three currently hardcode Imgur's own URLs. They now read
config/imgur.yamlfrom the classpath, falling back to exactly today's values when no such file exists:Behaviour is unchanged for every distribution in this repo — none ships a
config/imgur.yaml, so every code path falls through to the defaults.Stack
feat/offline-demo-exporter)test/e2e-offline-demo-compose)api.imgur.comBased on #1516 rather than
masterto keep the follow-up harness work off the merge queue. Retarget tomasteronce the parents land.Why this is a convention, not a new extension point
TransferServiceConfigalready exists for exactly this, andWorkerModulealready binds a per-extension instance viagetForService(ext.getServiceId()). Flickr (FlickrTransferExtension:98), Deezer (DeezerTransferExtension:75) and Synology all consume it; Synology and the generic adapter already read abaseUrlfrom it. Imgur simply had not adopted the pattern.Worth noting for anyone weighing this against other adapters: no adapter in the repo had a configurable URL on an export path before this. The two that read URLs from config —
GenericTransferExtensionand Synology — are import-only (getExporterthrows and returnsnullrespectively), which is presumably why they were the ones to need it. Flickr and Deezer consumeTransferServiceConfigonly forperUserRateLimit.Imgur is the cheapest place to start, because
baseUrlwas already a constructor parameter on bothImgurPhotosExporterandImgurPhotosImporter— only the extension-level constants needed to change. By contrastMicrosoftTransferExtension.BASE_GRAPH_URLis threaded into six importers.One asymmetry, flagged for review
ImgurTransferExtensiontakes its config from the injectedcontext.getService(TransferServiceConfig.class), as the convention expects.ImgurOAuthConfigcannot: anOAuth2ServiceExtensionis never handed a service-scopedTransferServiceConfigthe wayWorkerModulehands one to aTransferExtension. It therefore calls the staticTransferServiceConfig.getForService("Imgur")itself, once, at construction.That means an auth class reads a file named for the transfer side. Both halves genuinely describe the same service's endpoints, and this is the only classpath config mechanism that exists — the alternative was shadowing
config/common.yaml, which replaces the jar's copy wholesale rather than merging. Happy to restructure if you'd prefer a separate key or file.Tests
Adds the first tests for either class — 9 in total, covering defaults, per-key overrides, a config with no
serviceConfigsection, and aserviceConfigthat sets unrelated keys.There is also a test pinning
getServiceName()to"Imgur":PortabilityAuthServiceProviderRegistrydoes an exact-match lookup on that string, so changing it silently breaks every Imgur job at creation time.The five pre-existing
ImgurPhotoExporterTestcases still pass — verified in a clean worktree containing only this commit, since stacked PRs get no CI in this repo (pull_request: branches: [master]matches the base branch).