feat(offline-demo)!: add exporter and drop Microsoft dependency - #1509
Merged
Merged
Conversation
OfflineDemoTransferExtension.getExporter() returned a literal null, so the one extension in the repo that bypasses OAuth entirely was import-only and could not drive a transfer. Implement it, so offline-demo -> offline-demo becomes a complete, credential-free path. Replace MicrosoftOfflineData with a local DemoOfflineData. That type is a 20-line String wrapper, and depending on it pulled the entire Microsoft adapter -- okhttp plus every Graph exporter and importer -- into a demo module. Exporter<A, T extends DataModel> bounds T on DataModel rather than ContainerResource, so a plain value class is type-legal here. Also gate both getExporter and getImporter on OFFLINE_DATA. getImporter previously returned the importer for any vertical; unreachable today, since the auth extension advertises only OFFLINE_DATA, but TransferCompatibilityProvider probes extensions with MEDIA, PHOTOS and VIDEOS and would take the answer. Adds the module's first tests. BREAKING CHANGE: OfflineDemoImporter's type parameter changes from MicrosoftOfflineData to DemoOfflineData. This module is published to Maven Central, so it is a binary-compatibility break for any external consumer -- unlikely for a demo importer that prints to stdout, but worth a release note.
2 tasks
alexeyqu
added a commit
that referenced
this pull request
Aug 18, 2026
## Summary
`ExportResult` exposes two convenience constants. The second one is
wrong:
```java
public static final ExportResult CONTINUE = new ExportResult(ResultType.CONTINUE);
public static final ExportResult END = new ExportResult(ResultType.CONTINUE); // <-- END, built with CONTINUE
```
One character: `END` now carries `ResultType.END`.
## Why it matters, and why it doesn't (yet)
The constant has **no call sites anywhere in the repo** — every exporter
constructs `new ExportResult<>(ResultType.END, ...)` directly — so this
is latent, not a live bug. Nothing changes for existing code.
It is worth fixing anyway because it is the obvious thing for a new
exporter to reach for, and the resulting failure is quiet rather than
loud. `PortabilityInMemoryDataCopier#copyHelper` terminates on
`continuationData == null`, not on `ResultType`:
```java
ContinuationData continuationData = exportResult.getContinuationData();
if (null != continuationData) { ... recurse ... }
```
So an exporter returning `ExportResult.END` would *not* spin — the
constant also carries null `exportedData`, and `copyIteration` skips the
import entirely when that is null. The job would run to `COMPLETE`
having imported nothing at all. That is a much harder thing to notice
than a hang.
I hit this while writing an exporter for the offline-demo module (#1509)
and worked around it there by using the enum directly. Splitting the fix
out so it lands as a `fix:` in its own right rather than being buried in
a `feat:` — this repo squash-merges, so the PR title is what commitizen
sees.
## Test plan
- [x] `docker compose run --rm test --no-daemon build` — full repo
compiles and tests pass
- [x] `grep -rn "ExportResult.END" --include=*.java .` — confirms the
constant has no consumers, so no behaviour changes for existing callers
## Note for downstream consumers
`portability-spi-transfer` is published to Maven Central. Any external
caller relying on the current value of `ExportResult.END` would be
relying on it reporting `CONTINUE`, which is a bug rather than a
contract — but it is a behaviour change for them, so it is worth a line
in the release notes.
alexeyqu
changed the title
ameya9
approved these changes
Aug 18, 2026
alexeyqu
commented
Aug 25, 2026
This was referenced Sep 8, 2026
alexeyqu
added a commit
that referenced
this pull request
Sep 14, 2026
Adds a `docker compose` harness that runs a **real** `offline-demo` → `offline-demo` transfer — job creation, auth, worker claim, export, import — against the packaged `demo-server` jar, and asserts that the exported payload actually arrives. ```bash ./e2e/run.sh ``` No provider credentials, no local JDK, no local Python. ~35s with a warm Gradle cache. Nothing in the repo runs a transfer end to end today. The closest is `JobProcessorTest`, which stands up `JobProcessor` against sixteen mocks — useful, but it never moves data between two services. ### Stack | PR | Scope | |---|---| | #1509 (`feat/offline-demo-exporter`) | offline-demo becomes a complete export/import pair — **base of this PR** | | **this** | the harness that runs it | Based on #1509 rather than `master` because the transfer is only possible once offline-demo has an exporter. Retarget to `master` after #1509 merges. ### Shape Three services in the root `docker-compose.yml`; two share the repo's existing `Dockerfile`. **No new Dockerfile, no `build.gradle`, no `settings.gradle` entry, no Java.** | Service | What it is | |---|---| | `gradle` | the Gradle build container (renamed from `test`, see below) | | `dtp` | the same image, entrypoint overridden to `java -jar` the shadowJar | | `e2e` | stock `python:3.12-slim` + pytest, no Dockerfile | The jar is built through the `gradle` service, whose `ENTRYPOINT` is already `./gradlew`, so arguments pass straight through. That keeps the pinned Gradle 6.9.2 / JDK 11 toolchain single-sourced from the root `Dockerfile` and reuses the warm `gradle-cache` volume — which a `docker build` stage could not mount. `shadowJar` has no path to `copyWebApp`, so no Node or Angular toolchain is involved. `dtp` deliberately defines **no image of its own**. The repo publishes no server image today, so the harness pins a *contract* — an API on 8080 plus a readable log stream — that a future real image can satisfy by replacing three keys on one service. Meanwhile it does run the real packaged artifact, including the `mergeServiceFiles()` `META-INF/services` merge that `ServiceLoader` extension discovery depends on. ### Why the driver is Python, not Java Fair question on a Java project, so stating it plainly rather than leaving it to review. The decision that actually mattered was **whether the driver should be a Gradle module** — and a Java driver has to be one. In this repo that is not inert: root `build.gradle` selects subprojects by `new File(it.projectDir, 'src').exists()`, and `configure(sourceProjects())` then applies the `java` plugin, `group`/`version` from `RELEASE_VERSION`, six test dependencies, and **`sourcesJar` + `javadocJar`** to whatever it finds. A test-only module would silently acquire publication-shaped artifacts in a repo that publishes to Maven Central. It would also join `./gradlew build` (or need an opt-in gate), and resolve its dependencies through the pinned Gradle 6.9.2 / JDK 11 toolchain. An earlier in-process JUnit version of this harness was built and hit exactly that; its `build.gradle` needed a comment explaining that JUnit and Truth were arriving from a block it never asked to join. Out of the build, a container is the natural unit, and Python fits it: `pytest` + `requests`, pinned, isolated, invisible to the shipped dependency graph. It is also legible to people evaluating DTP who will not read Java — the same audience the credential-free path is for. And once real provider adapters are covered, their mock backends have to be containers anyway, so the driver's language stays invisible to DTP either way. **The cost, stated honestly:** the Java driver built requests from the real `portability-types-client` DTOs, so it *could not drift* from the API — rename a field and it stops compiling. The Python driver hand-writes those field names, so the same rename surfaces as a runtime 400 instead. CI still catches it, but later and less precisely. That is a real regression and the most likely thing to bite first. Not a line-count argument: both versions are ~400 lines. If the contract-drift risk is judged too high, the fallback is **JUnit + Testcontainers** — it keeps the typed DTOs and drops the in-process server, at the price of bringing the Gradle module back. ### Included rename: `test` → `gradle` The build container was called `test` while the service that actually runs tests was called something else. `Documentation/Developer.md` had the tell: ```bash docker compose run --rm test test --tests SomeTest # service name and Gradle task, same word ``` `gradle` is exact — the service's `ENTRYPOINT` *is* `./gradlew`. **The name `test` is left unused rather than reassigned to the pytest service.** It is documented and hardcoded in `docker-build.yml`, so a `test` that survived but meant something else would silently run the e2e suite for anyone with the old command in their fingers — and pass. It now fails with `no such service`. `Developer.md` and `docker-build.yml` are updated to match. Also corrects a stale claim in `Developer.md` that the root `Dockerfile` pins `gradle:8.10.2-jdk11`; it pins `eclipse-temurin:11-jdk-jammy`. ### Why completion is read from the server log Because there is no alternative. `TransferJob.state` is a hardcoded `State.CREATED` with **no getter**, so Jackson never emits it and `GET /api/transfer/{id}` reports no progress at all — `GetTransferJobAction` already fetches the real `PortabilityJob` and discards everything but the ids (`TODO(#553)`). Giving `TransferJob` a real nullable `state` would let the harness poll HTTP instead of grepping. That is a change to shipped code — and a binary-compatibility one, since `portability-types-client` is published — so it is deliberately **not** bundled here. Worth noting for anyone evaluating the endpoint: `GET /api/transfer/{id}` currently has **no callers at all**. `client-rest` has no `getTransferJob` method, so the route is effectively dead surface today. ### One finding worth reviewer attention `Finished processing … with 0 error(s).` is **necessary but not sufficient**. That count is `copier.getErrors().size()`, so a job that never copied anything reports zero too. Building the jar with `-PencryptionScheme=jwe` while the driver posts `cleartext` reproduces it exactly: ``` SEVERE No auth decrypter found for scheme cleartext while processing job: <uuid> DEBUG Finished processing jobId: <uuid> with 0 error(s). ``` The success marker fires on a transfer that moved nothing. So failure is detected from `SEVERE` lines naming the job — a clean run emits none — and the delivered payload is asserted **separately** from job completion. This generalises past this PR: any future assertion built on "the job finished cleanly" inherits the flaw. ### Verification Three negative checks were run by hand; none is committed: 1. **Scheme mismatch** → fails in ~5s on the `SEVERE` line with the server's own message. This is the check that exposed the trap above. 2. **Corrupted expected payload** → the payload assertion fails while completion still passes, proving the two are independent. 3. **Unsupported `dataType`** → both tests fail; the harness does not pass on "nothing crashed". `docker compose run --rm gradle` (the pre-existing build) confirmed still green. ### What a green run does not prove - **No real provider adapter is covered.** `offline-demo` has no HTTP surface, no pagination and no sub-resources, so this exercises the machinery rather than a data path. Covering an adapter that actually paginates — Imgur is the cheapest candidate — is the intended next step. - **Only `cleartext` is exercised.** JWE cannot be a runtime toggle: `SecurityExtensionLoader` asserts exactly one security extension on the classpath, so the scheme is fixed at build time and covering JWE means a second jar. - **Failures are diagnosable only from a log file**, not from structured received payloads. Pointing a real adapter at a mock HTTP backend is what would make received data directly assertable. ### CI Runs non-blocking (`continue-on-error: true`) to start with, uploading the server log as an artifact whether the run passes or fails. A flaky e2e that blocks merges is worse than no e2e; drop the flag once it has been green for a while.
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
2 participants
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.
Summary
OfflineDemoTransferExtension.getExporter()has been a literalreturn nullsince the module was added — the last touch was the mechanicalDataVerticalsignature change in8a0e3e73. That made the one extension in the repo that bypasses OAuth entirely import-only, so it could receive a transfer but never drive one.This implements it, so
offline-demo→offline-demois a complete, credential-free transfer path.It also replaces
MicrosoftOfflineDatawith a localDemoOfflineData. That type is a 20-lineStringwrapper, and depending on it pulled the entire Microsoft adapter — okhttp plus every Graph exporter and importer — into a demo module.Exporter<A, T extends DataModel>boundsTonDataModelrather thanContainerResource, so a plain value class is type-legal here.Notable decisions / tradeoffs
ResultType.ENDwith null continuation data.PortabilityInMemoryDataCopier#copyHelperrecurses oncontinuationData, not onResultType, so the null continuation is what actually ends the copy. A follow-up PR fixesExportResult.END— the static constant, distinct from the enum used here — which is currently built withResultType.CONTINUE. That fix is independent; nothing in this PR depends on it.No pagination. A demo exporter faking multiple pages via
IntPaginationTokenwould test the copier against a fixture written to satisfy it. Copier recursion is better covered by a real adapter that genuinely paginates.Both
getExporterandgetImporternow gate onOFFLINE_DATA.getImporterpreviously returned the importer for any vertical. That is unreachable today —OfflineDemoAuthServiceExtensionadvertises onlyOFFLINE_DATA, so no other vertical can produce a job for this service — butTransferCompatibilityProviderprobes extensions withgetImporterOrNull(extension, MEDIA/PHOTOS/VIDEOS)and would take the answer if that ever changed.The importer still prints to stdout. It is the demo's only observable output. Routing it through the
Monitorthatinitialize(context)already receives and discards is a reasonable follow-up, not part of this diff.The
"OFFLINE-DEMO"/"offline-demo"service-id mismatch is left alone.PortabilityAuthServiceProviderRegistrydoes an exact-matchMapBinderlookup whileTransferExtension.supportsServicelowercases both sides, so both spellings work as they are. Changing a published service id has a wider blast radius than the inconsistency warrants.OfflineDemoImporter's type parameter changes fromMicrosoftOfflineDatatoDemoOfflineData. This module is published to Maven Central, so this is a binary-compatibility break for any external consumer — unlikely for a demo importer that prints to stdout, but it warrants a release note.MicrosoftOfflineDataandMicrosoftOfflineDataExporterthemselves are untouched.Test plan
docker compose run --rm test :extensions:data-transfer:portability-data-transfer-offline-demo:test— 5 new tests, all passing (the module had none)docker compose run --rm test --no-daemon build— full repo, catches anything depending on the retyped importerdocker compose run --rm test --no-daemon -PofflineData=true :distributions:demo-server:shadowJar— the only in-repo consumer isdistributions/demo-server/build.gradle:105, behind that flag, and it references no offline-demo type directly, so a plainbuildwould not exercise itStack
This is the first of three PRs building toward a credential-free end-to-end transfer harness:
distributions/e2e-server— a slim headlessSingleVMMainbuild with no Angular toolchaine2e/docker-compose.e2e.yml+ a transfer driver