noop refactor as a pre-step to landing a new importer that needs some of this video logic #1229
Conversation
This file contains 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
…he mediaItemresult into a function
…w/ copies for the video functions
most of the actual diffs from this branch (or rather from
GoogleMediaImporter branch that this branch picks up from) that needed
to be updated were tiny things - specifically:
1. some methods in GooglePhotosInterface were made public by this
branch, but otherwise untouched (and so hit conflicts with more
substantive changes in the upstream branch).
2. this branch renamed GooglePhotosInterface#uploadPhotoContent to
GooglePhotosInterface#uploadMediaContent.
3. PhotoResultSerializationTest had its PhotoResult object replaced with
a MediaResult object in this branch.
here's the diff i used to understand the intended changes (to just the
conflicting files):
```sh
$ git diff 2597c2b..upstream/GoogleMediaImporter \
extensions/data-transfer/portability-data-transfer-google/src/main/java/org/datatransferproject/datatransfer/google/photos/GooglePhotosI{nterface,mporter}.java \
extensions/data-transfer/portability-data-transfer-google/src/test/java/org/datatransferproject/datatransfer/google/photos/{GooglePhotosImporter,PhotoResultSerialization}Test.java
diff --git a/extensions/data-transfer/portability-data-transfer-google/src/main/java/org/datatransferproject/datatransfer/google/photos/GooglePhotosImporter.java b/extensions/data-transfer/portability-data-transfer-google/src/main/java/org/datatransferproject/datatransfer/google/photos/GooglePhotosImporter.java
index 877295a4..cf6ea6a7 100644
--- a/extensions/data-transfer/portability-data-transfer-google/src/main/java/org/datatransferproject/datatransfer/google/photos/GooglePhotosImporter.java
+++ b/extensions/data-transfer/portability-data-transfer-google/src/main/java/org/datatransferproject/datatransfer/google/photos/GooglePhotosImporter.java
@@ -217,7 +217,7 @@ public class GooglePhotosImporter
try {
Pair<InputStream, Long> inputStreamBytesPair =
getInputStreamForUrl(jobId, photo.getFetchableUrl(), photo.isInTempStore());
-
+
try (InputStream s = inputStreamBytesPair.getFirst()) {
String uploadToken = getOrCreatePhotosInterface(jobId, authData).uploadPhotoContent(s);
mediaItems.add(new NewMediaItem(cleanDescription(photo.getDescription()), uploadToken));
diff --git a/extensions/data-transfer/portability-data-transfer-google/src/main/java/org/datatransferproject/datatransfer/google/photos/GooglePhotosInterface.java b/extensions/data-transfer/portability-data-transfer-google/src/main/java/org/datatransferproject/datatransfer/google/photos/GooglePhotosInterface.java
index 2976c9d7..47703653 100644
--- a/extensions/data-transfer/portability-data-transfer-google/src/main/java/org/datatransferproject/datatransfer/google/photos/GooglePhotosInterface.java
+++ b/extensions/data-transfer/portability-data-transfer-google/src/main/java/org/datatransferproject/datatransfer/google/photos/GooglePhotosInterface.java
@@ -88,7 +88,7 @@ public class GooglePhotosInterface {
private final GoogleCredentialFactory credentialFactory;
private final RateLimiter writeRateLimiter;
- GooglePhotosInterface(
+ public GooglePhotosInterface(
GoogleCredentialFactory credentialFactory,
Credential credential,
JsonFactory jsonFactory,
@@ -139,7 +139,7 @@ public class GooglePhotosInterface {
BASE_URL + "mediaItems:search", Optional.empty(), content, MediaItemSearchResponse.class);
}
- GoogleAlbum createAlbum(GoogleAlbum googleAlbum)
+ public GoogleAlbum createAlbum(GoogleAlbum googleAlbum)
throws IOException, InvalidTokenException, PermissionDeniedException {
Map<String, Object> albumMap = createJsonMap(googleAlbum);
Map<String, Object> contentMap = ImmutableMap.of("album", albumMap);
@@ -148,7 +148,7 @@ public class GooglePhotosInterface {
return makePostRequest(BASE_URL + "albums", Optional.empty(), content, GoogleAlbum.class);
}
- String uploadPhotoContent(InputStream inputStream)
+ public String uploadMediaContent(InputStream inputStream)
throws IOException, InvalidTokenException, PermissionDeniedException {
// TODO: add filename
InputStreamContent content = new InputStreamContent(null, inputStream);
@@ -165,7 +165,7 @@ public class GooglePhotosInterface {
BASE_URL + "uploads/", Optional.of(PHOTO_UPLOAD_PARAMS), httpContent, String.class);
}
- BatchMediaItemResponse createPhotos(NewMediaItemUpload newMediaItemUpload)
+ public BatchMediaItemResponse createPhotos(NewMediaItemUpload newMediaItemUpload)
throws IOException, InvalidTokenException, PermissionDeniedException {
HashMap<String, Object> map = createJsonMap(newMediaItemUpload);
HttpContent httpContent = new JsonHttpContent(new JacksonFactory(), map);
diff --git a/extensions/data-transfer/portability-data-transfer-google/src/test/java/org/datatransferproject/datatransfer/google/photos/GooglePhotosImporterTest.java b/extensions/data-transfer/portability-data-transfer-google/src/test/java/org/datatransferproject/datatransfer/google/photos/GooglePhotosImporterTest.java
index 78d996fa..a578404e 100644
--- a/extensions/data-transfer/portability-data-transfer-google/src/test/java/org/datatransferproject/datatransfer/google/photos/GooglePhotosImporterTest.java
+++ b/extensions/data-transfer/portability-data-transfer-google/src/test/java/org/datatransferproject/datatransfer/google/photos/GooglePhotosImporterTest.java
@@ -141,7 +141,7 @@ public class GooglePhotosImporterTest {
OLD_ALBUM_ID,
false);
- Mockito.when(googlePhotosInterface.uploadPhotoContent(any())).thenReturn("token1", "token2");
+ Mockito.when(googlePhotosInterface.uploadMediaContent(any())).thenReturn("token1", "token2");
BatchMediaItemResponse batchMediaItemResponse =
new BatchMediaItemResponse(
new NewMediaItemResult[] {
@@ -199,7 +199,7 @@ public class GooglePhotosImporterTest {
OLD_ALBUM_ID,
false);
- Mockito.when(googlePhotosInterface.uploadPhotoContent(any())).thenReturn("token1", "token2");
+ Mockito.when(googlePhotosInterface.uploadMediaContent(any())).thenReturn("token1", "token2");
BatchMediaItemResponse batchMediaItemResponse =
new BatchMediaItemResponse(
new NewMediaItemResult[] {
@@ -293,7 +293,7 @@ public class GooglePhotosImporterTest {
OLD_ALBUM_ID,
true);
- Mockito.when(googlePhotosInterface.uploadPhotoContent(any())).thenReturn("token1");
+ Mockito.when(googlePhotosInterface.uploadMediaContent(any())).thenReturn("token1");
JobStore jobStore = Mockito.mock(LocalJobStore.class);
Mockito.when(jobStore.getStream(any(), any()))
.thenReturn(
@@ -338,7 +338,7 @@ public class GooglePhotosImporterTest {
OLD_ALBUM_ID,
true);
- Mockito.when(googlePhotosInterface.uploadPhotoContent(any()))
+ Mockito.when(googlePhotosInterface.uploadMediaContent(any()))
.thenThrow(new IOException("Unit Testing"));
JobStore jobStore = Mockito.mock(LocalJobStore.class);
Mockito.when(jobStore.getStream(any(), any()))
diff --git a/extensions/data-transfer/portability-data-transfer-google/src/test/java/org/datatransferproject/datatransfer/google/photos/PhotoResultSerializationTest.java b/extensions/data-transfer/portability-data-transfer-google/src/test/java/org/datatransferproject/datatransfer/google/photos/PhotoResultSerializationTest.java
index 477007bf..83f730f7 100644
--- a/extensions/data-transfer/portability-data-transfer-google/src/test/java/org/datatransferproject/datatransfer/google/photos/PhotoResultSerializationTest.java
+++ b/extensions/data-transfer/portability-data-transfer-google/src/test/java/org/datatransferproject/datatransfer/google/photos/PhotoResultSerializationTest.java
@@ -12,14 +12,14 @@ public class PhotoResultSerializationTest {
public void testSerialization() throws IOException, ClassNotFoundException {
String photoId = "photoId";
long bytes = 97397L;
- PhotoResult result = new PhotoResult(photoId, bytes);
+ MediaResult result = new MediaResult(photoId, bytes);
ByteArrayOutputStream out = new ByteArrayOutputStream();
new ObjectOutputStream(out).writeObject(result);
ByteArrayInputStream bais = new ByteArrayInputStream(out.toByteArray());
Object readObject = new ObjectInputStream(bais).readObject();
- assertThat(readObject, instanceOf(PhotoResult.class));
- PhotoResult deserialized = (PhotoResult) readObject;
+ assertThat(readObject, instanceOf(MediaResult.class));
+ MediaResult deserialized = (MediaResult) readObject;
assertThat(deserialized.getId(), equalTo(photoId));
assertThat(deserialized.getBytes(), equalTo(bytes));
}
```
Whereas, in contrast, upstream had more changes to the same lines -
primarily the addition of exceptions to some signatures and a switch to
new equality library usages in unit tests. Making sense of that diff was
done with: `git diff 2597c2b..af54b17`.
re-forking Photos code (as that's how this branch was originally
developed) because a lot of upstream changes have landed in video code
since then.
while I'm at it I'm also doing what I did with
MicrosoftMedia{Importer,Exporter} and trying to make the MediaImporter
not repeat itself, but at least factor our the common logic looping over
video vs. looping over photos objects internally.
this refactoring isn't perfect, but it's a baby step: in the future we
can use the abstractions added internally to this class to let _all_ the
adapters in the google package reuse this same code (both the Photos and
the Videos adapters).
state of this commit: doesn't compile quite yet:
```
$ gradle test
> Task :extensions:data-transfer:portability-data-transfer-google:compileJava FAILED
/usr/local/google/home/zacsh/pub/src/dtp/extensions/data-transfer/portability-data-transfer-google/src/main/java/org/datatransferproject/datatransfer/google/media/GoogleMediaImporter.java:188: error: incompatible types: invalid method reference
this::importPhotoBatch);
^
incompatible types: List<T> cannot be converted to List<PhotoModel>
where T is a type-variable:
T extends Object declared in method <T>importToAlbum(UUID,TokensAndUrlAuthData,List<T>,IdempotentImportExecutor,String)
Note: /usr/local/google/home/zacsh/pub/src/dtp/extensions/data-transfer/portability-data-transfer-google/src/main/java/org/datatransferproject/datatransfer/google/videos/GoogleVideosInterface.java uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
Note: Some messages have been simplified; recompile with -Xdiags:verbose to get full output
1 error
FAILURE: Build failed with an exception.
```
had one too many generic declarations (shouldn't have had `<T>` on the interface's method declaration itself, but rather strictly on the _interface_) note: this commit is also not compiling, but for difference reasons now.
not done as we've not yet updated the test for new functionality
this is a microstep because it's simply making the existing logic static, right were it is, before we move it out (in a later commit).
TBD fast-follows for myself and gphotos folks:
1. more unit test coverage; how are we going to do that constructively?
we want to probably move a lot of existing test logic into
GoogleVideoInterface instead, and mock those APIs in our tests here.
2. more manual testing
3. long-term cleanups of gphotos things (eg: see all TODOs landed in
this work)
4. long-term reconiling of gphotos upstream SDK interactions; eg:
- sometimes we use raw HTTP - as with GooglePhotos* adapters
- sometimes we use raw PhotosLibraryClient, as with GoogleVideos*
adapters
- reconciling these two will mean _one_ approach to gphotos SDKs can
be taken and maintained, and further simplification of logic sharing
across the adapters will naturally follow.
seehamrun
approved these changes
Apr 14, 2023
This was referenced Apr 14, 2023
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.
for context, see #1226 (comment)