colin-kiegel / rust-derive-builder

derive builder implementation for rust structs
https://colin-kiegel.github.io/rust-derive-builder/
Apache License 2.0
1.35k stars 89 forks source link

Question: shorthand fields #299

Closed flrgh closed 12 months ago

flrgh commented 1 year ago

:wave:

So I have a struct like this:

#[derive(Default, Builder)]
#[builder(derive(Deserialize)]
#[builder_struct_attr(serde(deny_unknown_fields))]
pub struct Foo {
  pub expires: DateTime<Utc>,
  pub comment: String,

  // ...and many more fields
}

FooBuilder exposed in an HTTP API where the type can be deserialized from a JSON request payload:

{
  "comment": "This object expires a year from now",
  "expires": "2024-12-01T00:00:00Z"
}

I also have a convenience method for setting expires from a relative offset:

impl FooBuilder {
    pub fn ttl(&mut self, ttl: std::time::Duration) -> &mut Self {
        self.expires(chrono::Utc::now() + ttl)
    }
}

Now, it would be nice if this could extend all the way to the HTTP API so that I could accept payloads like this:

{
  "comment": "This object expires 60 seconds from now",
  "ttl": 60
}

Of course, that doesn't work, as there is no underlying ttl field on FooBuilder that can be deserialized:

Json deserialize error: unknown field ttl

Is there anything clever I can do to make this work without needing to create some kind of additional FooWithTtl struct with all of Foo's fields + ttl?

TedDriggs commented 1 year ago

You're looking for a way to cause a number field named ttl to deserialize into the expires: DateTime<Utc> field, which is a serde question rather than a derive_builder one.

You could use a field alias and a custom field deserializer, e.g. serde(alias = "ttl", deserialize_with = "..."). If the field was a number, you'd interpret it as a TTL; if it was a string you'd interpret it as a DateTime<Utc>.

This approach isn't perfect, since it'd allow someone to say expires: 60 and have that be interpreted as a TTL. If that isn't acceptable, then you will need to declare a separate struct to represent the fields that are settable via deserialization or the builder.

You could limit the amount of exposed weirdness from this by doing something to hide the interim struct...

#[derive(Builder)]
#[builder(
  public, 
  rename = "FooBuilder", 
  build_fn(private, rename = "build_internal", error = "FooBuilderError"),
  derive(Deserialize)
)]
struct InternalFoo {
  // ...
}

impl TryFrom<InternalFoo> for Foo {
  type Error = FooBuilderError;

  fn try_from(v: InternalFoo) -> Result<Self, Self::Error> {
    // ...elided...
    // this would be where you compute `expires` from `ttl` if it was specified, or error if both fields
    // were specified. 
  }
}

impl FooBuilder {
  pub fn build(&self) -> Result<Foo, FooBuilderError> {
    self.build_internal().and_then(Foo::try_from)
  }
}

This would make the InternalFoo struct invisible from outside the module, so people would only see Foo and FooBuilder.

You could use another crate of mine, field_names, to add a test that would make sure field changes to Foo were reflected in InternalFoo; there's an example of that here.

(Apologies if I got some of the property names wrong; had to write the code directly in the GitHub issue text box)

flrgh commented 12 months ago

Hey @TedDriggs,

[...] which is a serde question rather than a derive_builder one.

That's a good point. I guess my thought process was "if derive_builder had this feature, it would automatically extend to the Builder::deserialize() -> Builder.build() code path since the extra field would get included in builder(derive(Deserialize))."

Of course, no argument from me if y'all don't see this as a good feature idea. I don't think I would be able to offer up a PR for it right now even if there was interest.

Anyhow, thanks for taking the time to reply! This was all really helpful, and I'm checking out your field_names crate for inspiration.

TedDriggs commented 12 months ago

Of course, no argument from me if y'all don't see this as a good feature idea.

I see the rationale for the request, and have been in similar situations myself in the past. In my head, the line is the addition of a new field that both serde and derive_builder are expected to operate on; at that point, this is no longer deriving a deserializer or a builder of Foo, but instead of some intermediate structure that itself is convertible to a Foo. The degree of similarity between InterimFoo and Foo will dictate how annoying it is to have to repeat yourself, but they are ultimately distinct things.