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.

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

Task/rfr 1057 add attachments #8

Merged
merged 7 commits into from
Jul 15, 2024
Merged

Task/rfr 1057 add attachments #8

merged 7 commits into from
Jul 15, 2024

Conversation

hb0
Copy link
Contributor

@hb0 hb0 commented Jun 4, 2024

Uploaded
This PR:

  • removes the serialization dependency as the upload functionality should be independent from the serialization functionality
  • adds a copy of the Uploadable class which replaces the old RequestMetaData construct

... and adds support for the new attachment metadata fields:

  • attachmentId
  • log-/image-/videoCount and fileSize for all uploads, but to be downward compatible with current client it's allowed to not set these, they default to 0. filesSize is the attachmentsFileSize.

@hb0 hb0 requested a review from muthenberg June 4, 2024 15:46
@hb0 hb0 self-assigned this Jun 4, 2024
build.gradle.kts Outdated Show resolved Hide resolved
Copy link
Member

@muthenberg muthenberg left a comment

Choose a reason for hiding this comment

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

See my comments inlinbe

src/main/kotlin/de/cyface/uploader/DefaultUploader.kt Outdated Show resolved Hide resolved
@hb0 hb0 force-pushed the task/RFR-1057_add-attachments branch from 8530f75 to dd6c1b6 Compare June 24, 2024 12:39
hb0

This comment was marked as resolved.

@hb0 hb0 marked this pull request as ready for review June 25, 2024 13:36
Copy link
Contributor Author

@hb0 hb0 left a comment

Choose a reason for hiding this comment

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

pre-reviewed

Comment on lines 86 to +93
override fun uploadAttachment(
jwtToken: String,
metaData: RequestMetaData,
measurementId: Long,
uploadable: Attachment,
file: File,
fileName: String,
progressListener: UploadProgressListener
progressListener: UploadProgressListener,
): Result {
val endpoint = attachmentsEndpoint(measurementId)
return uploadFile(jwtToken, metaData, file, endpoint, progressListener)
val measurementId = uploadable.identifier.measurementIdentifier
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to keep the PRs as small as possible, so I didn't try to refactor further here.

@@ -171,81 +171,98 @@ class DefaultUploader(private val apiEndpoint: String) : Uploader {
* We wrap errors with [UploadFailed] so that the caller can handle this without crashing.
* This way the SDK's `SyncPerformer` can determine if the sync should be repeated.
*/
@Suppress("ComplexMethod")
private fun handleUploadException(exception: Exception): Nothing {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to reduce the complexity of this method, so I extracted the network specific exception into an own method

Comment on lines -407 to -413
/**
* Assembles a `HttpContent` object which contains the metadata.
*
* @param metaData The metadata to convert.
* @return The meta data as `HttpContent`.
*/
fun preRequestBody(metaData: RequestMetaData): Map<String, String> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This moved to Uploadable.toMap() which seemed cleaner to me.

@@ -547,24 +520,20 @@ class DefaultUploader(private val apiEndpoint: String) : Uploader {
* @return the [String] read from the InputStream. If an I/O error occurs while reading from the stream, the
* already read string is returned which might my empty or cut short.
*/
@Suppress("MemberVisibilityCanBePrivate", "NestedBlockDepth") // Part of the API
@Suppress("MemberVisibilityCanBePrivate") // Part of the API
@JvmStatic
fun readInputStream(inputStream: InputStream): String {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just reduced the unnecessary double-nesting of the try catch blocks here.

file: File,
fileName: String,
progressListener: UploadProgressListener
progressListener: UploadProgressListener,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started putting a comma after the last parameter in such multi-line formats, so when we add or remove a parameter only the added line is shown in git as changed, not the previously last line where we would just have to add a , at the end.

just fyi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a copy form collector

@hb0 hb0 merged commit ac1b07c into main Jul 15, 2024
1 check passed
@hb0 hb0 deleted the task/RFR-1057_add-attachments branch July 15, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants