Add automatic media job type compatibility #1136

incjo · 2022-08-09T12:54:16Z

makes progress against #1065 by building on top of #1062 by further expanding support for automatically handling media jobs.

It allows not writing dedicated media adapters in cases where the existing photo/video functionality is satisfactory. It also provides automatic interoperability with destinations that only provide a Media adapter.

Main contributions:

replaced MediaToPhotoConversionImporter and MediaToPhotoConversionExporter with generic versions that can support conversion between any two adapters: AnyToAnyImporter and AnyToAnyExporter
added special case adapters MediaExporterDecorator and MediaImporterDecorator which combine existing photo and video adapters to seamlessly cover the media job type.
added a compatibility layer TransferCompatibilityProvider (perhaps rename to AdapterCompatibilityProvider?) which attempts to build a substitute adapter upon not finding a real one. E.g. when a media job can't find a media importer, it will combine existing photo and video importers to perform the job.

jzacsh

sorry for the delay - I hadn't realized I hadn't submitted or finished my comments. Feel free to ping me in chat if my next replies aren't within a day.

jzacsh · 2022-08-17T15:50:31Z

...c/main/java/org/datatransferproject/spi/transfer/provider/TransferCompatibilityProvider.java

+ */
+public class TransferCompatibilityProvider {
+
+  private static final String PHOTOS = "PHOTOS";


since you're tackling this, please setup a new enum (just to make some micro-progress against #1065's wish to kill these magic strings). eg: org.datatransferproject.spi.transfer.DataVertical; fwiw I think spi.transfer is the right place (and not in types.common.models) because these are have historically been used and developed as "verticals" which are not strictly the same things as models but more of a meta concept. Pulling the enum out this way has two benefits:

implicitly documents a bit of that distinction I just described

lets us eventually update the transferextension's maps to use these documented enums as keys instead of the magic strings they take now

Cool, I'll pick it up. I've been frustrated by this issue as well.

jzacsh · 2022-08-17T15:58:54Z

.../src/main/java/org/datatransferproject/spi/transfer/provider/converter/AnyToAnyExporter.java

+   *                       sufficient.
+   */
+  public AnyToAnyExporter(Exporter<AD, From> exporter, Function<From, To> converter,
+      Function<ContainerResource, ContainerResource> inputConverter) {


should inputConverter's parameterizations be To and From respectively?

Unfortunately, no. That's because an Exporter<PhotosContainerResource> takes ExportInformation<ContainerResource> as input to its export method. This container resource can be anything at all, e.g. IdOnlyContainerResource, DateRangeContainerResource, PhotosContainerResource, or even some private resource etc. For some reason, we're not enforcing strict typing in this case.

The From and To exporters might support different inputs and still be made compatible via the AnyToAnyExporter. For example, converting from PhotosExporter to MediaExporter might involve passing Id and DateRange resources unchanged and converting the PhotosContainerResource to MediaContainerResource.

jzacsh · 2022-08-17T16:01:04Z

.../src/main/java/org/datatransferproject/spi/transfer/provider/converter/AnyToAnyExporter.java

+  private final Exporter<AD, From> exporter;
+  private final Function<From, To> converter;
+  private final Function<ContainerResource, ContainerResource> inputConverter;


Suggested change

private final Exporter<AD, From> exporter;

private final Function<From, To> converter;

private final Function<ContainerResource, ContainerResource> inputConverter;

private final Exporter<AD, From> exporter;

private final Function<From, To> containerResourceConverter;

private final Function<ContainerResource, ContainerResource> exportInformationConverter;

jzacsh · 2022-08-17T16:02:21Z

.../src/main/java/org/datatransferproject/spi/transfer/provider/converter/AnyToAnyExporter.java

+   *                       export call. It's required because exporters often support various
+   *                       container resources as part of export information. E.g. photo exporters
+   *                       support DateRangeContainerResource, while media adapters might only
+   *                       support MediaContainerResource. Oftentimes, Function.identity() is


Oftentimes, Function.identity() is sufficient.

Maybe we just provide an overloaded constructor for that case then? (eg only takes two params, and automatically uses Function.identity() for the third arg?)

That is fair, I just don't have any such usages at the moment. I actually had that for the MicrosoftTransferExtension while it didn't have a dedicated media adapter.

jzacsh · 2022-08-22T15:28:56Z

.../org/datatransferproject/spi/transfer/provider/converter/MediaToPhotoConversionImporter.java

@@ -23,6 +23,7 @@
 */
 // TODO(#1065) fix primitives-obession causing us to key Providers on "PHOTOS" string rather
 // than underlying file types.
+@Deprecated


everywhere you add this annotation, can you comment also what the recommended alternative is, as an inline comment? eg:

@Deprecated // prefer FooBar#baz

jzacsh · 2022-08-22T15:33:17Z

...java/org/datatransferproject/spi/transfer/provider/converter/MediaImporterDecoratorTest.java

+
+    videos = List.of(
+        new VideoModel("v1", "", null, null, "d4", null, false),
+        new VideoModel("v2", "", null, null, "d5", "a3", false),


should the album params on these photo or video models match the params of the media albums above? Or are they intentionally mismatched for testing purpose?

Ah, no reason. The particular data isn't relevant to the test.

jzacsh · 2022-08-22T15:34:19Z

...java/org/datatransferproject/spi/transfer/provider/converter/MediaImporterDecoratorTest.java

+    videosImporter = (id, ex, ad, data) -> ImportResult.OK;
+    mediaImporter = new MediaImporterDecorator<>(photosImporter, videosImporter);
+
+    albums = List.of(new MediaAlbum("album1", "name1", "desc1"),


(optional nitpick) suggestion to make these lines consistent with your others below

Suggested change

albums = List.of(new MediaAlbum("album1", "name1", "desc1"),

albums = List.of(

new MediaAlbum("album1", "name1", "desc1"),

jzacsh · 2022-08-22T15:36:54Z

.../src/main/java/org/datatransferproject/types/common/models/media/MediaContainerResource.java

+   * Extracts a new VideosContainerResource from the relevant matching fields in a given
+   * MediaContainerResource.
+   */
+  public static VideosContainerResource mediaToVideo(MediaContainerResource mediaContainer) {


nit: consider adding test coverage of these two new static methods

jzacsh · 2022-08-22T15:42:46Z

Do you have a pending PR somewhere that shows the final use of this? (eg: a PR that modifies a transfer extension?)

incjo · 2022-08-23T14:25:44Z

Do you have a pending PR somewhere that shows the final use of this? (eg: a PR that modifies a transfer extension?)

In our case, we're creating Media jobs without adding dedicated Media adapters. Essentially, we're leveraging the TransferCompatibilityProvider to automatically separate a MediaContainerResource into photos and videos, then use existing video and photo exporters and importers to do the remainder of the work while having the whole transfer happen under a single job.

This way we get to seamlessly support Media jobs without adding any new adapters, in our codebase or the destinations.

jzacsh · 2022-08-29T19:09:45Z

hey @incjo did you accidentally revert this PR via a force-push? I'm not able to find the code we reviewed here and indeed if I click "compare" on this "incjo force-pushed the master branch from 6e97d8d to ed1f42b" it looks like your new code is gone (like adding @Deprecated lines, or your new classes); screenshot inlined below:

incjo · 2022-08-30T13:05:06Z

hey @incjo did you accidentally revert this PR via a force-push? I'm not able to find the code we reviewed here and indeed if I click compare it looks like your new code is gone

Yep, for some reason Github decides to close the PR if you force push the wrong thing which is exactly what I did. Thanks Github. I don't have permissions to reopen the PR. I've addressed your feedback and will probably just make a new PR with the changes.

xokker requested a review from jzacsh August 12, 2022 11:03

jzacsh requested changes Aug 22, 2022

View reviewed changes

incjo force-pushed the master branch from 11fc596 to 6e97d8d Compare August 24, 2022 16:23

incjo merged commit ed1f42b into dtinit:master Aug 24, 2022

incjo force-pushed the master branch from 6e97d8d to ed1f42b Compare August 24, 2022 16:23

jzacsh mentioned this pull request Aug 30, 2022

Add automatic media job type compatibility and introduce the DataVertical enum #1151

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add automatic media job type compatibility #1136

Add automatic media job type compatibility #1136

incjo commented Aug 9, 2022 •

edited by jzacsh

Loading

jzacsh left a comment

jzacsh Aug 17, 2022

incjo Aug 23, 2022 •

edited

Loading

jzacsh Aug 17, 2022

incjo Aug 23, 2022

jzacsh Aug 17, 2022

jzacsh Aug 17, 2022

incjo Aug 23, 2022

jzacsh Aug 22, 2022

jzacsh Aug 22, 2022

incjo Aug 23, 2022

jzacsh Aug 22, 2022

jzacsh Aug 22, 2022

jzacsh commented Aug 22, 2022

incjo commented Aug 23, 2022

jzacsh commented Aug 29, 2022

incjo commented Aug 30, 2022

	albums = List.of(new MediaAlbum("album1", "name1", "desc1"),
	albums = List.of(
	new MediaAlbum("album1", "name1", "desc1"),

Add automatic media job type compatibility #1136

Add automatic media job type compatibility #1136

Conversation

incjo commented Aug 9, 2022 • edited by jzacsh Loading

jzacsh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

incjo Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzacsh commented Aug 22, 2022

incjo commented Aug 23, 2022

jzacsh commented Aug 29, 2022

incjo commented Aug 30, 2022

incjo commented Aug 9, 2022 •

edited by jzacsh

Loading

incjo Aug 23, 2022 •

edited

Loading