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

Allow extra fields in Patcher classes that don't exist in original #98

Closed

Conversation

codingismy11to7
Copy link

There was a specific check put in to guard against this behavior, but no tests ensuring this was the behavior - so thought maybe it could be changed.

Example scenario: we have an api, and want to update a user. The user can specify any of the user fields to update. I'd like to use most of the fields to patch the user, but there's some that need massaging first.

case class User(name: String, email: String, roleIds: Set[RoleId])
case class UserUpdate(name: Option[String], email: Option[String], roleNames: Option[Set[String]])

In this simple case, I want to use the UserUpdate class to patch the User class, even though I have to handle the verification/conversion/patching of roleNames into roleIds myself.

It may be that this should be handled through some flag or some enhanced patch api with transformers or something, just wanted to start the discussion.

@codecov-io
Copy link

codecov-io commented Jan 29, 2019

Codecov Report

Merging #98 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #98   +/-   ##
=======================================
  Coverage   93.64%   93.64%           
=======================================
  Files           7        7           
  Lines         362      362           
  Branches       59       59           
=======================================
  Hits          339      339           
  Misses         23       23

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f80ac4...c420bdb. Read the comment docs.

@krzemin
Copy link
Member

krzemin commented Jan 29, 2019

Thanks for great proposal!

The guard for fields existing in patch case class, but non-existing in target class was intentional and it was to increase type safety / confidence against stupid typos. I agree we could have some tests that explicitly express the intention ;)

Your proposal is kind of similar to proposal from #64 (comment) that assumed providing custom function to patchers that allows to combine target value from new one (from patch) and old one (from existing target). The conversion from Set[String] to Set[RoleId] could be handled by chimney transformers automatically (assuming RoleId is a value class). Do you think it would make sense to express your use case with operation from this proposal?

Do you think it's crucial to support re-named fields in patchers as well?

@codingismy11to7
Copy link
Author

To answer the last question first, in my specific case, renamed fields aren't crucial.

The proposal in #64 does seem very useful, and could be used for my particular case (although i'd be throwing away the new value because my Set[String] -> Set[RoleId] conversion is in a cats Validated, and by the time i've gotten to the patching step i've already computed the role id set). Regardless, anything that could work would be good.

Looking at it again, I'm assuming the types would have to be the same, so that might not work for my case after all. RoleId is not a value class in my case (was just making an example), it's a UUID that i've mapped from the (String) name given by the end user.

Anyway, not sure about exact details, but if types could be different or there was to be a new patching syntax and we could mark fields as ignored, I could work with it

@krzemin
Copy link
Member

krzemin commented Jan 29, 2019

I'm assuming the types would have to be the same, so that might not work for my case after all

I assumed types don't need to be literally equal, but there exists chimney transformation for them. But you're right in your case it would not work (unless you put your custom mapping logic into implicit transformer in scope which is... meh, not the best idea :P).

So we need to find an API that is able to express really custom logic.

What about this one?

user
  .with(userPatch)
  .enableIncompletePatches // don't emit compile error when you find a field that doesn't match the target
  .withFieldValue(_.roleIds, (old: Set[RoleId], patch: UserPatch) => ...)
  .patch

@codingismy11to7
Copy link
Author

Sorry - got busy.

That proposal looks great! I think it would solve all sorts of use cases.

@krzemin
Copy link
Member

krzemin commented Jan 21, 2020

Incomplete patches support was merged in #130.
For custom logic part I created #134.

@krzemin krzemin closed this Jan 21, 2020
@lokolte
Copy link

lokolte commented Jun 21, 2021

@krzemin is this functionality already implemented? or can we build some dsl that fit this?

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 this pull request may close these issues.

4 participants