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

Feature/issue 185 WIP #227

Closed
wants to merge 9 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
@@ -4,4 +4,6 @@ out
.idea
*.iml
*.ipr
*.iws
*.iws
.vscode
bin/
10 changes: 10 additions & 0 deletions build.gradle
Original file line number Diff line number Diff line change
@@ -33,6 +33,8 @@ dependencies {
testImplementation "com.fasterxml.jackson.datatype:jackson-datatype-jsr310:${fasterxmlVersion}"
testImplementation "com.fasterxml.jackson.datatype:jackson-datatype-jdk8:${fasterxmlVersion}"
testImplementation "com.fasterxml.jackson.module:jackson-module-scala_2.11:${fasterxmlVersion}"
testImplementation "com.fasterxml.jackson.core:jackson-core:${fasterxmlVersion}"
testImplementation "com.fasterxml.jackson.core:jackson-annotations:${fasterxmlVersion}"
}

tasks.register('codeCoverageReport', JacocoReport) {
@@ -54,6 +56,14 @@ tasks.register('codeCoverageReport', JacocoReport) {

tasks.test {
finalizedBy tasks.codeCoverageReport
testLogging {
events "passed", "skipped", "failed"
showStandardStreams = true
exceptionFormat "full"
showExceptions true
showCauses true
showStackTraces true
}
}

jar {
2 changes: 1 addition & 1 deletion gradle.properties
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
junitJupiterVersion=5.11.2
vavrVersion=0.10.5
fasterxmlVersion=2.7.2
fasterxmlVersion=2.18.2
javapoetVersion=1.9.0
jaxbVersion=2.3.0
Original file line number Diff line number Diff line change
@@ -288,8 +288,8 @@ public static ComplexInnerClass build() {
42.42d,
42.42f,
'4',
new Integer(42).byteValue(),
new Integer(42).shortValue(),
Integer.valueOf(42).byteValue(),
Integer.valueOf(42).shortValue(),
false,
"42");
}
@@ -300,8 +300,8 @@ public static ComplexInnerClass buildAnother() {
87.87d,
87.87f,
'8',
new Integer(87).byteValue(),
new Integer(87).shortValue(),
Integer.valueOf(87).byteValue(),
Integer.valueOf(87).shortValue(),
false,
"87");
}
54 changes: 54 additions & 0 deletions src/test/java/io/vavr/jackson/issues/issue185/Child.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
package io.vavr.jackson.issues.issue185;

import java.util.Objects;

public class Child {
private String name;
private String description;

public Child() {
}

public Child(String name, String description) {
this.name = name;
this.description = description;
}

public String getName() {
return name;
}

public void setName(String name) {
this.name = name;
}

public String getDescription() {
return description;
}

public void setDescription(String description) {
this.description = description;
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
Child child = (Child) o;
return Objects.equals(name, child.name) &&
Objects.equals(description, child.description);
}

@Override
public int hashCode() {
return Objects.hash(name, description);
}

@Override
public String toString() {
return "Child{" +
"name='" + name + '\'' +
", description='" + description + '\'' +
'}';
}
}
67 changes: 67 additions & 0 deletions src/test/java/io/vavr/jackson/issues/issue185/Issue185Test.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
package io.vavr.jackson.issues.issue185;

import java.io.IOException;

import com.fasterxml.jackson.annotation.JsonInclude.Include;
import com.fasterxml.jackson.databind.DeserializationFeature;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.SerializationFeature;
import io.vavr.jackson.datatype.VavrModule;
import io.vavr.jackson.datatype.VavrModule.Settings;

import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

class Issue185Test {

private String loadJson(String path) throws IOException {
return new String(getClass().getResourceAsStream(path).readAllBytes());
}

@Test
void shouldMergeJavaTypes() throws Exception {
//Given
ObjectMapper mapper = new ObjectMapper()
.configure(SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS, true)
.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)
.setDefaultPropertyInclusion(Include.NON_EMPTY)
.registerModule(new VavrModule(new Settings().deserializeNullAsEmptyCollection(true)));

String expected = loadJson("/issue185/result.json");
ParentJava expectedObject = mapper.readerFor(ParentJava.class).readValue(expected);

//When
String parent = loadJson("/issue185/parent.json");
String toMerge = loadJson("/issue185/to_merge.json");

ParentJava parentObject = mapper.readerFor(ParentJava.class).readValue(parent);
ParentJava updated = mapper.readerForUpdating(parentObject).readValue(toMerge);

//Then
Assertions.assertEquals(updated, expectedObject);
}

@Test
void shouldMergeVavrTypes() throws Exception {
//Given
ObjectMapper mapper = new ObjectMapper()
.configure(SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS, true)
.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)
.setDefaultPropertyInclusion(Include.NON_EMPTY)
.registerModule(new VavrModule(new Settings().deserializeNullAsEmptyCollection(true)));

String expected = loadJson("/issue185/result.json");
ParentVavr expectedObject = mapper.readerFor(ParentVavr.class).readValue(expected);

//When
String parent = loadJson("/issue185/parent.json");
String toMerge = loadJson("/issue185/to_merge.json");

ParentVavr parentObject = mapper.readerFor(ParentVavr.class).readValue(parent);
ParentVavr updated = mapper.readerForUpdating(parentObject).readValue(toMerge);

//Then
Assertions.assertEquals(updated,expectedObject);
}

}
63 changes: 63 additions & 0 deletions src/test/java/io/vavr/jackson/issues/issue185/ParentJava.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
package io.vavr.jackson.issues.issue185;

import com.fasterxml.jackson.annotation.JsonMerge;
import com.fasterxml.jackson.annotation.OptBoolean;

import java.util.List;
import java.util.Map;

import java.util.Objects;

//TODO replace with record in the future
public class ParentJava {

@JsonMerge(OptBoolean.FALSE)
List<String> list;

@JsonMerge
Map<String, String> map;

@JsonMerge
Map<String, Map<String, String>> deepMap;

@JsonMerge
Child child;

private ParentJava() {
}

private ParentJava(
List<String> list, Map<String, String> map,
Map<String, Map<String, String>> deepMap, Child child) {
this.list = list;
this.map = map;
this.deepMap = deepMap;
this.child = child;
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
ParentJava that = (ParentJava) o;
return Objects.equals(list, that.list) &&
Objects.equals(map, that.map) &&
Objects.equals(deepMap, that.deepMap) &&
Objects.equals(child, that.child);
}

@Override
public int hashCode() {
return Objects.hash(list, map, deepMap, child);
}

@Override
public String toString() {
return "ParentJava{" +
"list=" + list +
", map=" + map +
", deepMap=" + deepMap +
", child=" + child +
'}';
}
}
78 changes: 78 additions & 0 deletions src/test/java/io/vavr/jackson/issues/issue185/ParentVavr.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
package io.vavr.jackson.issues.issue185;

import com.fasterxml.jackson.annotation.JsonMerge;
import com.fasterxml.jackson.annotation.OptBoolean;
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;

import io.vavr.collection.List;
import io.vavr.collection.Map;

import java.util.Objects;

//TODO replace with record in the future
public class ParentVavr {

@JsonMerge(OptBoolean.FALSE)
List<String> list;

@JsonMerge
@JsonDeserialize(using = ParentMapDeserializer.class)
Map<String, String> map;

@JsonMerge
@JsonDeserialize(using = ParentDeepMapDeserializer.class)
Map<String, Map<String, String>> deepMap;

@JsonMerge
Child child;

// Define specialized deserializers as static inner classes
private static class ParentMapDeserializer extends VavrJsonMergeMapDeserializer<ParentVavr, String, String> {
public ParentMapDeserializer() {
super(ParentVavr.class, String.class, String.class, parent -> parent.map);
}
}

private static class ParentDeepMapDeserializer extends VavrJsonMergeMapDeserializer<ParentVavr, String, Map<String, String>> {
public ParentDeepMapDeserializer() {
super(ParentVavr.class, String.class, (Class<Map<String, String>>)(Class<?>)Map.class, parent -> parent.deepMap);
}
}

private ParentVavr() {
}

private ParentVavr(List<String> list, Map<String, String> map,
Map<String, Map<String, String>> deepMap, Child child) {
this.list = list;
this.map = map;
this.deepMap = deepMap;
this.child = child;
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
ParentVavr that = (ParentVavr) o;
return Objects.equals(list, that.list) &&
Objects.equals(map, that.map) &&
Objects.equals(deepMap, that.deepMap) &&
Objects.equals(child, that.child);
}

@Override
public int hashCode() {
return Objects.hash(list, map, deepMap, child);
}

@Override
public String toString() {
return "ParentVavr{" +
"list=" + list +
", map=" + map +
", deepMap=" + deepMap +
", child=" + child +
'}';
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,265 @@
package io.vavr.jackson.issues.issue185;

import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.databind.DeserializationContext;
import com.fasterxml.jackson.databind.JsonDeserializer;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
import io.vavr.collection.HashMap;
import io.vavr.collection.Map;

import java.io.IOException;
import java.util.Iterator;
import java.util.function.Function;

/**
* A generic deserializer for VAVR Maps that supports JSON merge.
* Enhanced to handle both simple maps and nested maps through generic type detection.
*
* @param <T> The type of the parent/container object
* @param <K> The type of keys in the map
* @param <V> The type of values in the map
*/
public class VavrJsonMergeMapDeserializer<T, K, V> extends JsonDeserializer<Map<K, V>> {
Copy link
Member

Choose a reason for hiding this comment

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

We don't need two separate map deserializers, right? Just one that's JsonMerge-friendly - would be good to make sure that the new deserializer implementation passes all the existing tests as well - then we could simply swap the implementation with the new one

Copy link
Author

Choose a reason for hiding this comment

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

Hi @pivovarit,

as we discussed, the PR offer an example in case of an user require to interact with @ jsonMerge but implement a complete solution with support for that annotation is pretty expensive.

We should merge this PR and add an example for that scenario.

Copy link
Member

Choose a reason for hiding this comment

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

But why can't we integrate it with the existing Map deserializer?

Copy link
Author

Choose a reason for hiding this comment

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

Current VAVR Module support all Data Structures from the library for Serialize/Deserialize:

    //Example
    ObjectMapper mapper = new ObjectMapper()
      .configure(SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS, true)
      .configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)
      .setDefaultPropertyInclusion(Include.NON_EMPTY)
      .registerModule(new VavrModule(new Settings().deserializeNullAsEmptyCollection(true)));

But add JsonMerge support will create an extra effort to update the Module and not only to support that Types, if the library, accept JsonMerge support, it should be necessary to accept it for the rest of the types.

I consider that it is better to maintain the Module in current state and maybe in the future evolve current support using some specific subclasses like ContainerDeserializerBase<T>

For users who require this behaviour, the library add an example as a Workaround in the tests related to this issue.

As a summary:

  • It is possible to Merge adding a custom serializer but generilize the support, not only will have to be reviewed for that Types, it is necessary to be reviewed for all VAVR Data structures and in the another hand, how many users use that annotation? This is the reason that the example in the test it could be valuable for this tiny audience using that annotation but for the general audience with current Module implementation is enough IMHO.

Juan Antonio

Copy link
Member

Choose a reason for hiding this comment

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

The new serializer needs to be compatible with the existing one - otherwise, people won't find it helpful.

So, there are two options here:

  • we patch the existing one with JsonMerge support and refactor it later
  • the new serializer matches the behaviour of the original one +/- JsonMerge support (which means that it should be tested using existing tests)

I believe that it's much better for users to just roll forward with the first option until we're sure that the new one doesn't introduce breaking changes by accident. There's also no point in keeping a non-backward compatible serializer just as an example because if users try to use it and it doesn't work just the existing one, they will get mad.

First option is already implemented here: #238

Copy link
Author

Choose a reason for hiding this comment

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

Oki, you won :p

In that case, maybe it doesn´t have any sense to maintain opened this PR, but I will take a look the changes and I will review current test in order to see if something is possible to be reinforced.

Agree?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good!


private final Class<T> parentClass;
private final Class<K> keyClass;
private final Class<V> valueClass;
private final Function<T, Map<K, V>> mapGetter;

/**
* Creates a new VAVRMapDeserializer.
*
* @param parentClass The class of the parent/container object
* @param keyClass The class of the keys in the map
* @param valueClass The class of the values in the map
* @param mapGetter Function to extract the current map from the parent object
*
*/
public VavrJsonMergeMapDeserializer(
Class<T> parentClass, Class<K> keyClass, Class<V> valueClass,
Function<T, Map<K, V>> mapGetter) {
this.parentClass = parentClass;
this.keyClass = keyClass;
this.valueClass = valueClass;
this.mapGetter = mapGetter;
}

@Override
public Map<K, V> deserialize(JsonParser p, DeserializationContext ctxt) throws IOException {
ObjectMapper mapper = (ObjectMapper) p.getCodec();
JsonNode node = mapper.readTree(p);

// Get current map from the parent instance
Map<K, V> currentMap = HashMap.empty();
Object currentValue = p.currentValue();

if (parentClass.isInstance(currentValue)) {
@SuppressWarnings("unchecked")
T parent = (T) currentValue;
currentMap = mapGetter.apply(parent);
if (currentMap == null) {
currentMap = HashMap.empty();
}
}

// Create a mutable map to store all entries
java.util.Map<K, V> mergedMap = new java.util.HashMap<>();

// Add all entries from current map
currentMap.forEach(mergedMap::put);

// Add or update entries from the incoming JSON
if (node.isObject()) {
Iterator<String> fieldNames = node.fieldNames();
while (fieldNames.hasNext()) {
String fieldName = fieldNames.next();
JsonNode valueNode = node.get(fieldName);

// Convert the key from String to K
K key = convertToType(mapper, fieldName, keyClass);

// Convert the value from JsonNode to V, handling nested maps
V value = convertNodeToType(mapper, valueNode, valueClass, mergedMap.get(key));

mergedMap.put(key, value);
}
}

// Convert back to VAVR Map
return HashMap.ofAll(mergedMap);
}

/**
* Converts a String to the target type
*/
@SuppressWarnings("unchecked")
private <X> X convertToType(ObjectMapper mapper, String value, Class<X> targetClass) {
if (targetClass == String.class) {
return (X) value;
} else if (targetClass == Integer.class) {
return (X) Integer.valueOf(value);
} else if (targetClass == Long.class) {
return (X) Long.valueOf(value);
} else if (targetClass == Double.class) {
return (X) Double.valueOf(value);
} else if (targetClass == Boolean.class) {
return (X) Boolean.valueOf(value);
} else {
try {
return mapper.readValue("\"" + value + "\"", targetClass);
} catch (IOException e) {
throw new RuntimeException("Failed to convert " + value + " to " + targetClass.getName(), e);
}
}
}

/**
* Converts a JsonNode to the target type, with special handling for nested maps
*
* @param mapper ObjectMapper to use for conversion
* @param node JsonNode to convert
* @param targetClass Target class
* @param existingValue The existing value (if any) to merge with
* @return Converted value
*/
@SuppressWarnings("unchecked")
private <X> X convertNodeToType(ObjectMapper mapper, JsonNode node, Class<X> targetClass, Object existingValue) {
try {
// Handle primitive types directly
if (node.isTextual() && targetClass == String.class) {
return (X) node.asText();
} else if (node.isNumber()) {
if (targetClass == Integer.class) {
return (X) Integer.valueOf(node.asInt());
} else if (targetClass == Long.class) {
return (X) Long.valueOf(node.asLong());
} else if (targetClass == Double.class) {
return (X) Double.valueOf(node.asDouble());
}
} else if (node.isBoolean() && targetClass == Boolean.class) {
return (X) Boolean.valueOf(node.asBoolean());
}

// Special handling for nested maps
if (node.isObject() && Map.class.isAssignableFrom(targetClass)) {
// Get the existing nested map if available
Map<String, Object> existingNestedMap = null;
if (existingValue instanceof Map) {
existingNestedMap = (Map<String, Object>) existingValue;
}

// This is a nested map case - need to convert each entry recursively
java.util.Map<String, Object> nestedMap = new java.util.HashMap<>();

// First, add all existing entries to preserve them
if (existingNestedMap != null) {
existingNestedMap.forEach(nestedMap::put);
}

// Process each field in the nested object
Iterator<String> fieldNames = node.fieldNames();
while (fieldNames.hasNext()) {
String fieldName = fieldNames.next();
JsonNode nestedValueNode = node.get(fieldName);

// Get existing nested value if available (for deep nested maps)
Object existingNestedValue = null;
if (existingNestedMap != null) {
existingNestedValue = existingNestedMap.get(fieldName).getOrNull();
}

// For nested maps, we assume String keys and process values according to their JSON type
Object nestedValue;
if (nestedValueNode.isObject()) {
// Another level of nesting - handle recursively with merging
nestedValue = convertToVavrMap(mapper, nestedValueNode, existingNestedValue);
} else if (nestedValueNode.isTextual()) {
nestedValue = nestedValueNode.asText();
} else if (nestedValueNode.isNumber()) {
nestedValue = nestedValueNode.isInt() ? nestedValueNode.asInt() :
nestedValueNode.isLong() ? nestedValueNode.asLong() : nestedValueNode.asDouble();
} else if (nestedValueNode.isBoolean()) {
nestedValue = nestedValueNode.asBoolean();
} else {
// For other types, use Jackson's conversion
nestedValue = mapper.treeToValue(nestedValueNode, Object.class);
}

nestedMap.put(fieldName, nestedValue);
}

// Convert to VAVR Map
return (X) HashMap.ofAll(nestedMap);
}

// Default fallback to Jackson's conversion
return mapper.convertValue(node, targetClass);
} catch (Exception e) {
throw new RuntimeException("Failed to convert node to " + targetClass.getName(), e);
}
}

/**
* Default overload for convertNodeToType without existing value
*/
private <X> X convertNodeToType(ObjectMapper mapper, JsonNode node, Class<X> targetClass) {
return convertNodeToType(mapper, node, targetClass, null);
}

/**
* Helper method to convert a JSON object node to a VAVR Map with merging support
*/
private Map<String, Object> convertToVavrMap(ObjectMapper mapper, JsonNode node, Object existingMap) throws IOException {
java.util.Map<String, Object> javaMap = new java.util.HashMap<>();

// First, add all existing entries from the map to merge with
if (existingMap instanceof Map) {
@SuppressWarnings("unchecked")
Map<String, Object> existingVavrMap = (Map<String, Object>) existingMap;
existingVavrMap.forEach(javaMap::put);
}

// Then add/update entries from the incoming JSON
Iterator<String> fieldNames = node.fieldNames();
while (fieldNames.hasNext()) {
String key = fieldNames.next();
JsonNode valueNode = node.get(key);

// Get the existing value for this key if it exists
Object existingValue = null;
if (existingMap instanceof Map) {
@SuppressWarnings("unchecked")
Map<String, Object> existingVavrMap = (Map<String, Object>) existingMap;
existingValue = existingVavrMap.get(key).getOrNull();
}

Object value;
if (valueNode.isObject()) {
// Recursively handle nested objects with merging
value = convertToVavrMap(mapper, valueNode, existingValue);
} else if (valueNode.isTextual()) {
value = valueNode.asText();
} else if (valueNode.isNumber()) {
value = valueNode.isInt() ? valueNode.asInt() :
valueNode.isLong() ? valueNode.asLong() : valueNode.asDouble();
} else if (valueNode.isBoolean()) {
value = valueNode.asBoolean();
} else {
// For other types, use Jackson's conversion
value = mapper.treeToValue(valueNode, Object.class);
}

javaMap.put(key, value);
}

return HashMap.ofAll(javaMap);
}

/**
* Overload for convertToVavrMap without existing map
*/
private Map<String, Object> convertToVavrMap(ObjectMapper mapper, JsonNode node) throws IOException {
return convertToVavrMap(mapper, node, null);
}
}
22 changes: 22 additions & 0 deletions src/test/resources/issue185/parent.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
{
"list": [
"first",
"second"
],
"map": {
"first_key": "first_value",
"second_key": "second_value"
},
"deepMap": {
"first_key": {
"first_nested_key": "first_nested_value"
},
"second_key": {
"second_nested_key": "second_nested_value"
}
},
"child": {
"name": "original_name",
"description": "original_description"
}
}
27 changes: 27 additions & 0 deletions src/test/resources/issue185/result.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
{
"list": [
"first",
"third"
],
"map": {
"first_key": "first_overridden_value",
"second_key": "second_value",
"third_key": "third_value"
},
"deepMap": {
"first_key": {
"first_nested_key": "first_nested_value",
"first_overridden_nested_key": "first_overridden_nested_value"
},
"second_key": {
"second_nested_key": "second_nested_value"
},
"third_key": {
"third_nested_key": "third_nested_value"
}
},
"child": {
"name": "overridden_name",
"description": "original_description"
}
}
21 changes: 21 additions & 0 deletions src/test/resources/issue185/to_merge.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
{
"list": [
"first",
"third"
],
"map": {
"first_key": "first_overridden_value",
"third_key": "third_value"
},
"deepMap": {
"first_key": {
"first_overridden_nested_key": "first_overridden_nested_value"
},
"third_key": {
"third_nested_key": "third_nested_value"
}
},
"child": {
"name": "overridden_name"
}
}