Nugine / s3s

S3 Service Adapter
Apache License 2.0
148 stars 34 forks source link

derive Default for dto input types? #48

Closed simon-mo closed 1 year ago

simon-mo commented 1 year ago

Thanks for this great project.

Is there a reason that there isn't #derive[Default] for the input API types like PutObjectInput? One of my use case is to test the proxy implementation which requires constructing such object, without default it's very verbose to construct them.

Nugine commented 1 year ago

Here is the current generator:

https://github.com/Nugine/s3s/blob/14ec3e56d25ecbcc7d6f34c53e9f2a928678d088/codegen/src/dto.rs#L387-L401

Some required fields (BucketName, ObjectKey) don't have a reasonable default value. For example, an empty BucketName is not a valid bucket name. I'm not sure whether the input type should guarantee that its fields are either valid or optional.

Possible solutions:

simon-mo commented 1 year ago

Ah i see. In this case, I would say either just accepting empty values or add a builder for only the required types should be useful? Adding builders for all types seem like it will blow up the code size.

Nugine commented 1 year ago

For simplicity, I choose the first one: use empty values as default.

S3 trait implementation can still assume that the input is valid. It is a logic error if the caller passes invalid input to S3 operation.


After some tries, I find that empty defaults may be very misleading. And there are too many "strong types" which don't have space for an invalid default.

I don't feel good about such changes. So adding builders is the better choice.

Nugine commented 1 year ago

Example: https://github.com/Nugine/s3s/blob/a42f0a3f39539b2566d61abf3e07e2241bb7d732/crates/s3s/tests/dto.rs#L3-L14

simon-mo commented 1 year ago

Thank you!

simon-mo commented 1 year ago

Hey @Nugine, what's your thought on fluent style builder? I'm happy to provide a patch if that's okay with you. One minor thing is that I don't quite know off the top of my head if it is possible to make both patterns co-exist in rust with the ownership system.

Nugine commented 1 year ago

I'm okay with fluent style builder.

The setters of a fluent style builder can take &mut self or self and return the same. I prefer the former personally but we can support both styles.

Currently all the setters have a set_ prefix. It's ok if we add different setters without the prefix, like what aws-sdk-s3 does.

For example:

The codegen function is here https://github.com/Nugine/s3s/blob/e8bba54fe291664f73566235354002eb02002a75/codegen/src/dto.rs#L606-L625

BTW the codegen for data structures is some what redundant. I'll refactor it one day.