Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(drive): upload and download file #3872

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(drive): upload and download file #3872

wants to merge 1 commit into from

Conversation

DenovVasil
Copy link
Contributor

@DenovVasil DenovVasil commented Jan 23, 2025 •
edited
Loading

Description

Added the ability to download and upload files.

Related issues

issue
element-template PR
doc PR
image

drive.mov

@DenovVasil DenovVasil requested a review from a team as a code owner January 23, 2025 09:06
@DenovVasil DenovVasil self-assigned this Jan 23, 2025
@DenovVasil DenovVasil added this to the 8.7.0-alpha4 milestone Jan 23, 2025
@DenovVasil DenovVasil force-pushed the 990-upload-and-download-files-for-drive branch from d4d176c to 4ccbc16 Compare January 23, 2025 10:38
@DenovVasil DenovVasil force-pushed the 990-upload-and-download-files-for-drive branch 3 times, most recently from 8f73cb5 to 602fb6a Compare January 23, 2025 16:43
Oleksiivanov
Oleksiivanov previously approved these changes Jan 23, 2025
View reviewed changes
@DenovVasil DenovVasil force-pushed the 990-upload-and-download-files-for-drive branch from 602fb6a to d750d5e Compare January 24, 2025 10:00
Oleksiivanov
Oleksiivanov previously approved these changes Jan 24, 2025
View reviewed changes
@johnBgood johnBgood self-requested a review January 27, 2025 10:34
johnBgood
johnBgood reviewed Jan 27, 2025
View reviewed changes
Copy link
Collaborator

@johnBgood johnBgood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor questions

...ectors/google/google-drive/src/main/java/io/camunda/connector/gdrive/GoogleDriveService.java Outdated
File file = createRequest.execute();
return new GoogleDriveResult(file.getId(), MimeTypeUrl.getFileUrl(file.getId()));
} catch (Exception e) {
throw new RuntimeException(e);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a log here before rethrowing? We could also add a message to this RuntimeException("the message", e)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

...ectors/google/google-drive/src/main/java/io/camunda/connector/gdrive/GoogleDriveService.java Outdated
return documentMapper.mapToDocument(outputStream.toByteArray(), fileMetaData);
}
} catch (IOException e) {
throw new RuntimeException(e);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

...ectors/google/google-drive/src/main/java/io/camunda/connector/gdrive/GoogleDriveService.java Show resolved Hide resolved
...ectors/google/google-drive/src/main/java/io/camunda/connector/gdrive/GoogleDriveService.java Outdated Show resolved Hide resolved
...ectors/google/google-drive/src/main/java/io/camunda/connector/gdrive/GoogleDriveService.java
Drive.Files.Create createRequest = drive.files().create(fileMetaData, content);

if (document.metadata().getSize() > MAX_DIRECT_UPLOAD_FILE_SIZE_BYTES) {
createRequest.getMediaHttpUploader().setProgressListener(new LoggerProgressListener());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get this part, we have a MAX SIZE, but we can still upload the file if the size is exceeded? If so, we should rename the variable because it might be confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, this is done to pay tribute to the documentation. Just to show the difference between the two types uploading (multipart and resumable). But from a code point of view, everything except the listener is identical. https://developers.google.com/drive/api/guides/manage-uploads
https://developers.google.com/api-client-library/java/google-api-java-client/media-upload#implementation

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I see, then maybe just a javadoc comment on MAX_DIRECT_UPLOAD_FILE_SIZE_BYTES explaining that beyond this size the upload is handled differently etc should be enough

...oogle/google-drive/src/main/java/io/camunda/connector/gdrive/model/request/DownloadData.java
public record DownloadData(
@TemplateProperty(
group = "operationDetails",
label = "File ID",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no description, is it excepted or can we guide the user somehow?

.../google/google-drive/src/main/java/io/camunda/connector/gdrive/model/request/UploadData.java Show resolved Hide resolved
@Szik
Copy link

Szik commented Jan 27, 2025

reviewing connector.
personally, i do not find it intuitive to deal with IDs for documents in google drive.
Someone not being familiar with it might need to google/research this.
Can we provide in the documentation of this connector a simple example on how to obtain what a user needs to enter in each filed (up-/download file)? I think that this enhances the understanding of the usage of the new document handling feature.

@DenovVasil DenovVasil force-pushed the 990-upload-and-download-files-for-drive branch from dd46ee2 to b4e2978 Compare January 28, 2025 09:15
johnBgood
johnBgood previously approved these changes Jan 28, 2025
View reviewed changes
Copy link
Collaborator

@johnBgood johnBgood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one last comment to add a javadoc comment, otherwise all green :)

@DenovVasil DenovVasil force-pushed the 990-upload-and-download-files-for-drive branch from b4e2978 to d2a54e3 Compare January 28, 2025 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnBgood johnBgood johnBgood approved these changes

@Oleksiivanov Oleksiivanov Oleksiivanov left review comments

Assignees

@DenovVasil DenovVasil

Labels
None yet
Projects
None yet
Milestone
8.7.0-alpha4
Development

Successfully merging this pull request may close these issues.
4 participants
@DenovVasil @Szik @johnBgood @Oleksiivanov