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

feat(storage_client): upload signed URL #495

Merged
merged 12 commits into from
Jun 7, 2023
Merged

feat(storage_client): upload signed URL #495

merged 12 commits into from
Jun 7, 2023

Conversation

dshukertjr
Copy link
Member

@dshukertjr dshukertjr commented May 31, 2023

What kind of change does this PR introduce?

Implements signed upload URL by adding Expose the createUploadSignedUrl, uploadToSignedUrl, and uploadToSignedUrl methods. This allows developers to first upload the file to their own server while attaching the token from createSignedUploadUrl() method, and then complete the upload process from their server to Supabase.

final response =
    await storage.from(newBucketName).createSignedUploadUrl(uploadPath);

// Uploads the file to their own server, and call the following to complete the upload to Supabase

await storage
    .from(newBucketName)
    .uploadToSignedUrl(response.path, response.token, file);

I didn't add the binary version of uploadToSignedUrl method, as it is intended to be used in a server environment.

Related supabase/storage-js#158, supabase/storage#282 (comment)

@Vinzent03
Copy link
Collaborator

I didn't add the binary version of uploadToSignedUrl method, as it is intended to be used in a server environment.

dart_edge for example runs on the server, but compiles Dart to js, so File won't work. I would binary as well.

@dshukertjr
Copy link
Member Author

dshukertjr commented Jun 1, 2023

@Vinzent03
Sure, thanks for the suggestion! I know you only had good intentions, but I might appreciate it if you could wait on commenting on PRs until they are out of the draft status.

@dshukertjr dshukertjr marked this pull request as ready for review June 5, 2023 14:56
retryController: retryController,
);

return cleanPath;
Copy link
Collaborator

Choose a reason for hiding this comment

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

So far we are using (response as Map)['Key'] as String to return the path in upload(), but I see that storage-js uses cleanPath everywhere. I guess they are always the same, so it doesn't matter.

Vinzent03
Vinzent03 previously approved these changes Jun 6, 2023
Comment on lines +115 to +116
assert(retryAttempts == null || retryAttempts >= 0,
'retryAttempts has to be greater or equal to 0');
Copy link
Collaborator

@bdlukaa bdlukaa Jun 6, 2023

Choose a reason for hiding this comment

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

Isn't it possible to make retryAttempts non nullable?

int retryAttempts = 0,

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yeah, giving it a default value would be so much better!

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I just remembered. There is a global settings for retryAttempt, and if the retryAttempt value passed to the individual methods are null, it will fall back to the global default, so we might need to keep it nullable here.

bdlukaa
bdlukaa previously approved these changes Jun 6, 2023
packages/storage_client/lib/src/types.dart Show resolved Hide resolved
Comment on lines +200 to +207
const SignedUploadURLResponse({
required String signedUrl,
required String path,
required this.token,
}) : super(
signedUrl: signedUrl,
path: path,
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't we use the new super constructors?

Suggested change
const SignedUploadURLResponse({
required String signedUrl,
required String path,
required this.token,
}) : super(
signedUrl: signedUrl,
path: path,
);
const SignedUploadURLResponse({
required super.signedUrl,
required super.path,
required this.token,
});

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a great point. Currently, the minimum Dart SDK version is 2.15.0, and I believe the new super constructor was introduced on v2.17.0, so let's actually do that in another PR. I think we can bump the minimum SDK version to 2.17.0 and minimum Flutter version to 3.0.0 soon, since it's been a while for those to be around.

@dshukertjr dshukertjr dismissed stale reviews from bdlukaa and Vinzent03 via deca03a June 7, 2023 00:07
@dshukertjr dshukertjr requested review from Vinzent03 and bdlukaa June 7, 2023 00:18
@Vinzent03
Copy link
Collaborator

Isn't it better to add the documentation to the field itself and not just to the class? So that for example vscode can show the doc on path when using signedUrl.path.

@dshukertjr
Copy link
Member Author

@Vinzent03
Yeah, I don't know what I was thinking 😂

@dshukertjr dshukertjr merged commit f330d19 into main Jun 7, 2023
@dshukertjr dshukertjr deleted the feat/upload-url branch June 7, 2023 11:07
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.

3 participants