PUT semantics for patching

[luksow]

Hi,
First of all, thanks for all the great work!

Recently I came across use case that I believe is not covered by chimney.

I have
case class A(int: Int, stringOpt: Option[String], doesntChange: Boolean)
and then I'd like to do a PUT-like update with a case class
case class PatchA(int: Int, stringOpt: Option[String])
which is a subset of A.

Using patchWith would result in checking if stringOpt is defined in PatchA and appling change only if it's Some. However, I'd expect a direct assignment, so if stringOpt in PatchA is None, stringOpt in patched version of A should be None as well.

Use case: it's very useful if you're working on some REST service and you support both PATCH (partial update) and PUT.

[MateuszKubuszok]

I think we could consider some modifier like:

a.using[PatchA].allowNoneOverride.patch(patchA)

I think we didn't have requests for patching modifiers, but since we have one (and might have in the future) it might make sense to introduce a syntax for passing these modifiers.

@krzemin what do you think?

[krzemin]

Using patchWith would result in checking if stringOpt is defined in PatchA and appling change only if it's Some. However, I'd expect a direct assignment, so if stringOpt in PatchA is None, stringOpt in patched version of A should be None as well.

@luksow isn't it a default behavior as for now?

> case class A(int: Int, stringOpt: Option[String], doesntChange: Boolean)
defined class A

> case class PatchA(int: Int, stringOpt: Option[String])
defined class PatchA

> val x = A(0, Some("abc"), false)
x: A = A(0, Some("abc"), false)

> val y = x.patchWith(PatchA(10, None))
y: A = A(10, None, false)

> val z = y.patchWith(PatchA(20, Some("xyz")))
z: A = A(20, Some("xyz"), false)

We also support optional patches which allows you to optionally deliver a value in a patch. This is extremely useful when delivering JSON for HTTP PATCH request that may contains optional fields.

> case class OptPatchInt(int: Option[Int])
defined class OptPatchInt

> val u = y.patchWith(OptPatchInt(Some(30)))
u: A = A(30, None, false)

> val v = y.patchWith(OptPatchInt(None))
v: A = A(10, None, false)

[luksow]

Oh, ok, than I'm a victim of confusion... and also should check my codebase for bugs 😆

I expected patchWith to behave as partial patch, so whenever it encounters Option[T] in patching entity it equals to patching.orElse(beingPatched) or patching.getOrElse(beingPatched) (depending on type of beingPatched) but it seems as it's kind of a mix of PUT and PATCH semantics. Here's a longer example to show what I mean:

import java.util.UUID

import io.scalaland.chimney.dsl._

case class User(id: UUID, emailAddress: String, companyId: Option[UUID])

case class UserPatch(emailAddress: Option[String], companyId: Option[UUID])

case class UserPut(emailAddress: String, companyId: Option[UUID])

val testUser = User(UUID.fromString("5b2d5ece-e0c8-41d8-af58-07b11873eb6e"), "[email protected]", Some(UUID.fromString("5b6b0d88-9e6c-42e7-9fe2-203954fe5eaf")))

// PATCHing - only emailAddress changes
testUser.patchWith(UserPatch(Some("[email protected]"), None))
// res0: User = User(5b2d5ece-e0c8-41d8-af58-07b11873eb6e,[email protected],None)
// WRONG! I expected to (partial) patch only email, thus leaving companyId = 5b6b0d88-9e6c-42e7-9fe2-203954fe5eaf
// expected: User(5b2d5ece-e0c8-41d8-af58-07b11873eb6e,[email protected],5b6b0d88-9e6c-42e7-9fe2-203954fe5eaf)
// it'd be nice to hint expected behaviour here

// PUTing - both fields are replaced
testUser.patchWith(UserPut("[email protected]", None))
// res1: User = User(5b2d5ece-e0c8-41d8-af58-07b11873eb6e,[email protected],None)
// YEY!

So I'd love to have methods:

1. With PATCH semantics = getOrElse/``orElse`/replace in case of non-Option.
2. With PUT semantics = checks for direct type equality and replaces values.

Hope it's clear now.

[krzemin]

Oh, I see now. Let's summarize current semantics that consists of 2 rules:

1. if we have field f: T in patch that correspond to f: T in entity, just take f's value from patch
2. if we have field f: Option[T] in patch that correspond to f: T in entity, set entity's f to v only if patch's f is Some(v)

Btw, results of your examples follows it, so it's not a bug.

What might be problematic is the fact, that in case of companyId: Option[UUID] being present both in the entity and patch, you always end up being served by the 1st rule. I realize that switching to Option[Option[UUID]] may not be very convenient (however currently possible if you want to end up in 2nd rule).

It would be more convenient to have some dedicated support for cases where we have Option[T] on both sides (in patch and in the entity). But if we go with PATCH-like (3rd?) rule, then we allow only transitions from None to Some(v) in the entity - there's no way to clear it.

patch	entity	result
None	None	None
Some(u)	None	Some(u)
None	Some(v)	Some(v)
Some(u)	Some(v)	Some(u)

I don't think this should be a default mode for all cases.

Now there are few questions:

• should we introduce flags for patchers that enable additional rules? (like in case of transformers)
• should we introduce separate combinators like replaceWith (that use only 1st rule) and patchWith (1+2+3)?
• should we allow to enable/disable extra rules per field when patching?
• any other ideas for nice, flexible API that minimize confusion factor?

[MateuszKubuszok]

I believe that the default should be the current behavior. I expect, that some other people would come up with some modifiers case I cannot see today (so, 2 cases would not be enough). I think that leaving current behavior as a default and allowing modifiers (like with transformer) could be a good solution.

@luksow ?

[luksow]

Current behavior should not be changed for the sake of compatibility.

Having said that, since I was confused once, I'll be probably confused again, so I'd like to have some kind of "modifiers" that are very explicit and tells me if I'm PATCHing or PUTing. I guess the most flexible API is one that allows you to define strategies. A strategy would be defining functions:

// (entity value, patch value) => patched value
(A, A) => A
(A, Option[A]) => A
(Option[A], A) => Option[A]
(Option[A], Option[A]) => Option[A]

With some default strategies defined in the lib (current, patch, put, ...). However, since I'm not doing anything crazy with chimney, I don't know how it plays with current customization options.

[krzemin]

Tried to realize how many possible semantics/strategies of handling optional fields we may have and my reasoning is as follows:

• in the patched entity we may have field f of type either T or Option[T]
• in the patch object: f field may be missing, f may be of type T, f may be of type Option[T]

Let's draw a table of all reasonable semantics for all possible cases.

case	type of f in entity	f in patch	semantic result
1.	f: T	f missing	entity.f
2.	f: T	f: T	patch.f
3.	f: T	f: Option[T]	patch.f.getOrElse(entity.f)
4.	f: Option[T]	f missing	entity.f
5.	f: Option[T]	f: T	Some(patch.f)
6.	f: Option[T]	f: Option[T]	two possible semantics: patch.f (current) patch.f.orElse(entity.f) (requested)

I'm pretty sure that cases 1-5 are obvious and the only reasonable choices (pls correct mi if I missed something). The only real choice we have in case 6., where current semantics is to take a value from patch - so the semantics is the same as in case 2, without distinguishing Option[T].

However we can drive this semantics with DSL modifiers introduced in #130.

So little example how it could work:

case class User(name: Option[String], age: Option[Int])
case class UserPatch(name: Option[String], age: Option[Int])

val user = User(Some("John"), Some(30))
val userPatch = UserPatch(None, None)

user.patchUsing(userPatch) // current behavior: User(None, None)
user
  .using(userPatch)
  .ignoreNoneInPatch
  .patch // new behavior: User(Some("John"), Some(30))

Also realized that this proposal is about the same thing as #69.

[luksow]

@krzemin I like your table, it clarifies this problem.

TBH, I don't like your proposal, as it seems to be verbose while patchUsing is super simple - the perfect solution for me would be additional param to this method. However, I'm not totally against your idea as long as you can make both cases explicit (even if there's some default) to avoid confusion, so:

user
  .using(userPatch)
  .ignoreNoneInPatch
  .patch // new behavior: User(Some("John"), Some(30))

user
  .using(userPatch)
  .applyNoneInPatch // or sth similar
  .patch // old behavior: User(None, None)

Thanks for taking care of it!

[krzemin]

There are cases where you don't have Option fields in patch and I believe they shouldn't be bothered by deciding on what to do in case 6.

For the sake of making decision explicitly visible, theoretically we could add applyNoneInPatch to DSL, but it would be no-op. I don't really like this way of extending DSL, but I admit that stating semantics of 6th case explicitly is important.

For both transformers and patchers (since #130) DSLs we have set of boolean flags that drive semantics of generated code. Current convention is that these flags have their default values and users can change them using DSL. But for cases when they don't care - it's not required. Proposed ignoreNoneInPatch is no exception to this rule - just another flag with default value and ability to change. As for now I don't really see a reason why this flag should be treated specially (e.g. as a param to patchUsing instead of ignoreNoneInPatch DSL method).

[luksow]

There are cases where you don't have Option fields in patch and I believe they shouldn't be bothered by deciding on what to do in case 6.

That's true. However, I'd prefer to explicitly say what to do in case I update case classes later and don't really come back to chimney part 😉

For the sake of making decision explicitly visible, theoretically we could add applyNoneInPatch to DSL, but it would be no-op. I don't really like this way of extending DSL, but I admit that stating semantics of 6th case explicitly is important.

Exactly my point. No-op is fine for me. Maybe there's another way of being explicit?

Anyway, I can live with that, so that's not a big issue once we have ignoreNoneInPatch.

[krzemin]

However, I'd prefer to explicitly say what to do in case I update case classes later and don't really come back to chimney part

One can argue you can do it anyway - for option cleaning behavior you call patchUsing, for ignore behavior, you use a flag :)

Maybe there's another way of being explicit?

I'm pretty sure that better API that exposes boolean flags in more visible way can be proposed, but I would like to treat all the flags (including transformers' ones) in the same way. So it's rather a proposal for another ticket.

If you really require no-op combinator, you can simply define extension method on PatcherUsing class in your project.

[krzemin]

Resolved in #131 .