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

JsonObject: custom merge with MergeFunction #5454

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tsegismont
Copy link
Contributor

In some cases, users may want more flexibility when merging values of JSON objects.

In this case, they can provide an implementation of MergeFunction. Because we can't tell if a null result means null or skip, implementation can choose to return MergeFunction.SKIP.

In some cases, users may want more flexibility when merging values of JSON objects.

In this case, they can provide an implementation of MergeFunction.
Because we can't tell if a null result means null or skip, implementation can choose to return MergeFunction.SKIP.

Signed-off-by: Thomas Segismont <[email protected]>
@tsegismont
Copy link
Contributor Author

Any comment @vietj ?

* @see #mergeIn(JsonObject, int, MergeFunction)
*/
@FunctionalInterface
public interface MergeFunction extends BiFunction<Object, Object, Object> {
Copy link
Member

@vietj vietj Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer avoiding to introduce a new contract only for that purpose, BiFunction is already a functionnal interface

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we need to skip some entries then an extra predicate should be provided

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe a BiFunction<Object, Object, Optional<Object>> would be more expressive here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe a BiFunction<Object, Object, Optional<Object>> would be more expressive here

That was my initial thought. And then, considering in most cases we won't need to skip, I tried to avoid Optional by using a signal Object.

I created MergeFunction to encapsulate that signal object.

I would be fine with a BiFunction without Optional.

@vietj
Copy link
Member

vietj commented Jan 29, 2025

I think we should not rush this one and see if we can provide something different that would work better

@vietj
Copy link
Member

vietj commented Jan 29, 2025

where are the requirement coming from, specially the skip part

the current implementation defines the behavior:

  • not exist + exist ⇾ put
  • exist + not exist ⇾ no-op
  • exist + exist ⇾ overwrite or recursive merge (two json objects)

none of these cases actually deletes anything, it seems that handling the possibility of a case that would skip introduce the need for a more complex API that it might be needed. In other words using a BiFunction considers that two entries (whether they are null or not) always produce a new entry (which can be null).

@tsegismont
Copy link
Contributor Author

where are the requirement coming from

#5453

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.

2 participants