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

🚀 [Feature]: s3: support setting with checksum #970

Closed
3 tasks done
iredmail opened this issue Sep 4, 2023 · 3 comments · Fixed by #978
Closed
3 tasks done

🚀 [Feature]: s3: support setting with checksum #970

iredmail opened this issue Sep 4, 2023 · 3 comments · Fixed by #978

Comments

@iredmail
Copy link
Contributor

iredmail commented Sep 4, 2023

Feature Description

I understand current s3 implementation is designed for gofiber Storage interface, but it could be used as a s3 library too.

My question is, do you accept PR to add a new function alternative to Set() to accept checksum (e.g. sha256)? so that S3 server will verify the data integrity on server side.

Current Set() doesn't accept any checksum:

https://github.com/gofiber/storage/blob/main/s3/s3.go#L95C39-L99

The plan is adding new functions to accept checksum like below (since theses functions are not designed for Storage interface, i removed exp time.Duration):

SetWithChecksumCRC32(key string, val []byte, checksum string)
SetWithChecksumCRC32C(key string, val []byte, checksum string)
SetWithChecksumSHA1(key string, val []byte, checksum string)
SetWithChecksumSHA256(key string, val []byte, checksum string)

Or, one function to support all (checksum map[string]string, key is checksum algorithm like CRC32, SHA256, value is the checksum):

SetWithChecksum(key string, val []byte, checksum map[string]string)

Your opinion?

Additional Context (optional)

No response

Code Snippet (optional)

No response

Checklist:

  • I agree to follow Fiber's Code of Conduct.
  • I have checked for existing issues that describe my suggestion prior to opening this one.
  • I understand that improperly formatted feature requests may be closed without explanation.
@iredmail iredmail changed the title 🚀 [Feature]: s3: supporting set checksum (sha256) 🚀 [Feature]: s3: supporting set with checksum Sep 4, 2023
@iredmail iredmail changed the title 🚀 [Feature]: s3: supporting set with checksum 🚀 [Feature]: s3: support setting with checksum Sep 4, 2023
@gaby
Copy link
Member

gaby commented Sep 4, 2023

@ReneWerner87 How do we handle adding functions outside the Storage.go interface?

Can they be allowed as long as the main functions are implemented as expected?

@gaby
Copy link
Member

gaby commented Sep 4, 2023

The PR for minio is already adding a func to the interface CheckBucket(). #962

@iredmail To me it makes more sense to add: SetWithChecksum()

@ReneWerner87
Copy link
Member

@ReneWerner87 How do we handle adding functions outside the Storage.go interface?

Can they be allowed as long as the main functions are implemented as expected?

yes, as with the templates it is only important that the core is the same, the rest can vary

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants