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

Field annotation for interpreting item with user-provided function in AsChangeset #2527

Open
heyrict opened this issue Oct 5, 2020 · 5 comments · May be fixed by #2529
Open

Field annotation for interpreting item with user-provided function in AsChangeset #2527

heyrict opened this issue Oct 5, 2020 · 5 comments · May be fixed by #2529

Comments

@heyrict
Copy link
Contributor

heyrict commented Oct 5, 2020

Realted issue: #26

I'd like to build a struct that implements AsChangeset, but it is not possible to set some values to NULL while keeping the struct ignoring some None fields. Using Option<Option<T>> or another type e.g. enum { Value(T), Null, Unset } will add extra overhead to the struct and may limit its usability.

Instead of adding an extra type, I'd suggest adding a field annotation #[diesel(parse_with = "parse_func")] similar to what serde use to AsChangeset, so that users themselves can determine whether to set null or to ignore the field. parse_func is a function of type FnMut(T) -> Option<Option<impl AsChangset>>. This will not break the current behavior and is relatively simple to implement IMHO.

e.g.

#[derive(AsChangeset)]
struct UpdateUser {
  #[changeset_options (parse_with = "update_user_hobby_set")]
  hobby: Option<Option<String>>,
  #[changeset_options(parse_with = "Country::update_set")]
  country: Country,
}

fn<T> update_user_hobby_set(hobby: Option<Option<String>>) -> Option<Option<T>>
where:
  T: AsChangeset
{
  // Returns None to ignore the field, Some(None) to set null, Some(Some(T)) to set value to T.
  // Making the return value an enum is also fine for me.
  hobby
}

struct Country(JsonValue)

impl Country {
  fn update_set(&self) -> Option<Option<i32>> {
    match self.0 {
      JsonValue::Null => Some(None),
      JsonValue::Undefined => None,
      JsonValue::Integer(v) => *v as i32,
      _ => None,
    }
  }
}

Let me know if you have any other workarounds other than manually implementing AsChangeset. I can work on this when I have some spare time.


Edit: The issue is found in v1.4.x at the written time. After investigating the master branch, it still seems impossible to make a custom value (e.g. serde_json::Value) return NULL.

@heyrict heyrict changed the title Annotation for setting item to null in AsChangeset Field annotation for interpreting item with user-provided function in AsChangeset Oct 5, 2020
@weiznich
Copy link
Member

weiznich commented Oct 5, 2020

Using Option<Option> or another type e.g. enum { Value(T), Null, Unset } will add extra overhead to the struct and may limit its usability.

Can you please clarify what exactly you mean here? Have you any numbers proofing this claim? Otherwise I would say that this theoretical overhead is not measurable in practice, therefore I do not see any reason why this should be changed.

Let me know if you have any other workarounds other than manually implementing AsChangeset.

Just to clarify that: Implementing AsChangeset manually is not considered to be a workaround, that's something that is a valid use case. The derive is on purpose designed in such a way that is covers simple conversions well, but anything more involved not.

@heyrict
Copy link
Contributor Author

heyrict commented Oct 5, 2020

Sorry for the ambiguity.

Let's say we have a update struct

struct A {
  ...
  field: T
  ...
}

I would like to make T::Null set the field NULL, and T::Skip skips the field, but there is no way to make a custom variable T parsed as Null except I manually implement AsChangset for struct A, though other fields are fine with the derive macro. Implementing AsChangeset for T will not work, as the Option unwrapping logic is built in the derive macro.

There do have other workarounds, but it should be more convenient if a conversion function can be added for that field.

@weiznich
Copy link
Member

weiznich commented Oct 5, 2020

Have you tried to just implement AsChangeset for T? The derive internally calls the AsChangeset impl for every separate member, as far as I'm aware. That's in my opinion the correct place to handle this.

@heyrict
Copy link
Contributor Author

heyrict commented Oct 6, 2020

What the derive macro does internally is to generate the following code

struct A {
  ...
  field: T,
  field2: Option<U>,
  ...
}

impl AsChangeset for A {
  fn as_changeset(&self) -> Self::Changeset {
    (
      ...,
      table::field.eq(self.field),
      self.field2.map(|x| table::field2.eq(x)),
      ...,
    ).as_changeset()
  }
}

Using a user-defined struct will not work as Self::as_changeset() is called after Eq::<Left, Right>(table::field, field, where there is a constraint AppearsOnTable<Right> which will never be met.

@weiznich
Copy link
Member

weiznich commented Oct 6, 2020

That sounds like something that should be fixed in the derive itself. I would accept a PR doing that. (The Insertable derive already does something like that, so maybe look there for an implementation idea?)

@heyrict heyrict linked a pull request Oct 7, 2020 that will close this issue
@pksunkara pksunkara assigned pksunkara and unassigned pksunkara Jun 30, 2021
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 a pull request may close this issue.

3 participants