Skip to content

Commit

Permalink
start automating Media integration; stand as demo for future model re…
Browse files Browse the repository at this point in the history
…factorings changes (#1062)

* fixes #1058: add MediaContainerResourceTest & more

adds MediaContainerResourceTest and fixes bug said test newly reveals in
automatic root album creation logic during transmogrification.

nit: in nit-fixing doc bug (documentation was wrong on
`MediaContainerResource#ensureRootAlbum`) also just lifts conditional
logic being pushed down into helper function; taking liberty here with
my opinion - that Single Responsibility (SOLID) would remove this
complexity to begin with (`ensureRootAlbum` should not care about what a
transmogrification is or what its config is).

* noop(nit:docs): point TODO at a trackable issue number

* noop(readability) run Goog style via clang-format

* noop(new api) Media* APIs: generate lowlevel types

adds MediaContainerResource (and accompanying `MediaAlbum` under the
hood) static APIs that admit these classes just wrap or maintain 1:1
correspondence to Photo* types.

Unfortunately only adds test coverage via the top-level usage
(`MediaContainerResourceTest`) because there's no unit test coverage in
place for MediaAlbum and I'm already spinning my wheels a bit adding
missing test infra. But splitting some of the MediaAlbum==PhotoAlbum
comparison unit test sanity-checking logic (into non-extant
MediaAlbumTest.java) would be the correct thing to do here.

motivation: this unblocks continued `MediaVertical` work without the need
to have (for example) a `AcmecoPhotoExporter` manually forked copy-pasta
style into `AcmecoMediaExporter` and then cause immediate techdebt for
us (through code drift).

longer-term: This change _does_ bring into question some of the
intermediate type planning we're doing here, but we've determined this
change is not going to be throw away work (eg: to unblock continued
Media vertical expansion) while we revisit and discuss intermediate
types a bit more. Those discussions will change _how_ this code is
called but are very unlikely to _remove_ callers to this code.

* WIP: experimental (not even compiling) draft of converter logic with some TODOs

* WIP: nitfixes; cleanups & renames

renames variables taht were backwards

cleans up code since MediaContainer APIs[^1] were pulled out of this
work stream to get committed in parallel.

[^1]: commit 8963426 (of branch `kate-jon-pair-utils`)

* WIP: fixes compilation errors with my generics setup

* noop: revert accidental formatting during merge

* noop(refactor) moves container copy logic into ExportResult; unfortunately there are no unit tests of this class

* noop(refactor) clang-format & add javadoc

* noop(refactor) pull ExportResult<...> construction out

* noop(docs) point to a tracked TODO: #1065

* WIP: merge fix: bad merge kept both versions of the same code

* adds Media* importer by conversion of Photo* one

also some nitpicks on some javadoc.

* fix: 2c2c4a3 = uncompiling copy-pasta

* fix incorrect marking of Import auto-support for Media

Co-authored-by: William Morland <[email protected]>
  • Loading branch information
jzacsh and wmorland authored Jun 22, 2022
1 parent 3054a60 commit d0a0bef
Show file tree
Hide file tree
Showing 4 changed files with 110 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import org.datatransferproject.spi.transfer.extension.TransferExtension;
import org.datatransferproject.spi.transfer.provider.Exporter;
import org.datatransferproject.spi.transfer.provider.Importer;
import org.datatransferproject.spi.transfer.provider.converter.PhotoToMediaConversionExporter;
import org.datatransferproject.transfer.microsoft.calendar.MicrosoftCalendarExporter;
import org.datatransferproject.transfer.microsoft.calendar.MicrosoftCalendarImporter;
import org.datatransferproject.transfer.microsoft.common.MicrosoftCredentialFactory;
Expand All @@ -36,11 +37,16 @@ public class MicrosoftTransferExtension implements TransferExtension {
private static final String CONTACTS = "CONTACTS";
private static final String CALENDAR = "CALENDAR";
private static final String PHOTOS = "PHOTOS";
private static final String MEDIA = "MEDIA";
private static final String OFFLINE_DATA = "OFFLINE-DATA";

// TODO(#1065) don't keep adding here - just have the converters invoked automatically when Media
// isn't supported on one or the other side of this equation; this is just a WIP prototype to show
// the concept of converters at play.
private static final ImmutableList<String> SUPPORTED_IMPORT_SERVICES =
ImmutableList.of(CALENDAR, CONTACTS, PHOTOS);
private static final ImmutableList<String> SUPPORTED_EXPORT_SERVICES =
ImmutableList.of(CALENDAR, CONTACTS, PHOTOS, OFFLINE_DATA);
ImmutableList.of(CALENDAR, CONTACTS, PHOTOS, MEDIA, OFFLINE_DATA);
private ImmutableMap<String, Importer> importerMap;
private ImmutableMap<String, Exporter> exporterMap;

Expand Down Expand Up @@ -135,8 +141,10 @@ PHOTOS, new MicrosoftPhotosImporter(BASE_GRAPH_URL, client, mapper, jobStore, mo
exporterBuilder.put(
CALENDAR,
new MicrosoftCalendarExporter(BASE_GRAPH_URL, client, mapper, transformerService));
exporterBuilder.put(
PHOTOS, new MicrosoftPhotosExporter(credentialFactory, jsonFactory, monitor));
// TODO(#1065) don't require this manual mapping; just do this automatically inside DTP
MicrosoftPhotosExporter photosExporter = new MicrosoftPhotosExporter(credentialFactory, jsonFactory, monitor);
exporterBuilder.put(PHOTOS, photosExporter);
exporterBuilder.put(MEDIA, new PhotoToMediaConversionExporter(photosExporter));
exporterBuilder.put(
OFFLINE_DATA, new MicrosoftOfflineDataExporter(BASE_GRAPH_URL, client, mapper));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
* The result of an item export operation, after retries.
*/
public class ExportResult<T extends DataModel> {

public static final ExportResult CONTINUE = new ExportResult(ResultType.CONTINUE);
public static final ExportResult END = new ExportResult(ResultType.CONTINUE);

Expand Down Expand Up @@ -41,6 +40,14 @@ public ExportResult(ResultType type, T exportedData) {
this.exportedData = exportedData;
}

/**
* Builds a new ExportResult. copying the current one but replacing only the data with
* `replacementData`.
*/
public <R extends DataModel> ExportResult<R> copyWithExportedData(R replacementData) {
return new ExportResult<>(this.getType(), replacementData, this.getContinuationData());
}

/**
* Ctor.
*
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package org.datatransferproject.spi.transfer.provider.converter;

import org.datatransferproject.spi.transfer.idempotentexecutor.IdempotentImportExecutor;
import org.datatransferproject.spi.transfer.provider.ImportResult;
import org.datatransferproject.spi.transfer.provider.Importer;
import org.datatransferproject.types.common.ExportInformation;
import org.datatransferproject.types.common.models.media.MediaContainerResource;
import org.datatransferproject.types.common.models.photos.PhotosContainerResource;
import org.datatransferproject.types.transfer.auth.AuthData;
import org.datatransferproject.types.transfer.auth.TokensAndUrlAuthData;

import java.util.Optional;
import java.util.UUID;

/**
* Automatically produces a "PHOTO" importer from a more sophisticated "MEDIA" importer.
*
* Produces an `Importer` that can handle import jobs for `PhotosContainerResource` (aka "PHOTOS"
* jobs) based on an existing importer that can handle the broader jobs for `MediaContainerResource`
* (aka "MEDIA" jobs).
*
* This is intended for providers who do not support "MEDIA" as a special case.
*/
// TODO(#1065) fix primitives-obession causing us to key Providers on "PHOTOS" string rather
// than underlying file types.
public class MediaToPhotoConversionImporter<
A extends AuthData,
PCR extends PhotosContainerResource,
WrappedImporter extends Importer<A, MediaContainerResource>> implements Importer<A, PCR> {
private final WrappedImporter wrappedMediaImporter;

public MediaToPhotoConversionImporter(WrappedImporter wrappedMediaImporter) {
this.wrappedMediaImporter = wrappedMediaImporter;
}

public ImportResult importItem(
UUID jobId,
IdempotentImportExecutor idempotentExecutor,
A authData,
PCR photosContainerResource) throws Exception {
MediaContainerResource mediaContainerResource = MediaContainerResource.photoToMedia(photosContainerResource);
return wrappedMediaImporter.importItem(jobId, idempotentExecutor, authData, mediaContainerResource);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
package org.datatransferproject.spi.transfer.provider.converter;

import java.util.Optional;
import java.util.UUID;
import org.datatransferproject.spi.transfer.provider.ExportResult;
import org.datatransferproject.spi.transfer.provider.Exporter;
import org.datatransferproject.types.common.ExportInformation;
import org.datatransferproject.types.common.models.media.MediaContainerResource;
import org.datatransferproject.types.common.models.photos.PhotosContainerResource;
import org.datatransferproject.types.transfer.auth.AuthData;
import org.datatransferproject.types.transfer.auth.TokensAndUrlAuthData;

/**
* Automatically produces a "MEDIA" importer from a "PHOTO" importer.
*
* This is intended for providers who do not support video, but should be allowed to continue using
* the common model of "Photo" without rewriting their DTP integration. In such cases this class
* allows the provider to export Media objects with only the relevant photo fields populated and
* anything else (like the video fields) ignored.
*
* This is intended for providers who do not support "PHOTOS" as a special case.
*/
// TODO(#1065) fix primitives-obession causing us to key Providers on "PHOTOS" string rather
// than underlying file types.
public class PhotoToMediaConversionExporter<
A extends AuthData,
PCR extends PhotosContainerResource,
WrappedExporter extends Exporter<A, PCR>
> implements Exporter<A, MediaContainerResource> {
private final WrappedExporter wrappedPhotoExporter;

public PhotoToMediaConversionExporter(WrappedExporter wrappedPhotoExporter) {
this.wrappedPhotoExporter = wrappedPhotoExporter;
}

public ExportResult<MediaContainerResource> export(
UUID jobId,
A authData,
Optional<ExportInformation> exportInformation) throws Exception {
ExportResult<PCR> originalExportResult =
wrappedPhotoExporter.export(jobId, authData, exportInformation);
PCR photosContainerResource = originalExportResult.getExportedData();
MediaContainerResource mediaContainerResource =
MediaContainerResource.photoToMedia(photosContainerResource);
return originalExportResult.copyWithExportedData(mediaContainerResource);
}
}

0 comments on commit d0a0bef

Please sign in to comment.