From e6ade31ee150bd87b9d2aa746c8e376ad9890fa0 Mon Sep 17 00:00:00 2001 From: Will Nicholson Date: Tue, 20 Oct 2020 14:27:16 +0100 Subject: [PATCH 1/3] Stop using references to collections that won't be serialized. Types with collections are a bit hairy, what if it's first read as Set but later we serialize the same thing as SortedSet? Can't reuse the original value but instead need a copy constructor, and that's a lot of effort to code --- .../joda/beans/ser/bin/BeanReferences.java | 21 +++++++++++++------ .../org/joda/beans/ser/SerTestHelper.java | 13 ++++++++++++ .../ser/bin/TestSerializeReferencingBin.java | 13 ++++++++++++ 3 files changed, 41 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/joda/beans/ser/bin/BeanReferences.java b/src/main/java/org/joda/beans/ser/bin/BeanReferences.java index 339a178b..701a1fa1 100644 --- a/src/main/java/org/joda/beans/ser/bin/BeanReferences.java +++ b/src/main/java/org/joda/beans/ser/bin/BeanReferences.java @@ -132,13 +132,13 @@ private void findReferencesBean( return; } - // has this object been seen before, if so no need to check it again - int result = objects.compute(base, BeanReferences::incrementOrOne); - if (result > 1) { - return; - } - if (base instanceof Bean) { + // has this object been seen before, if so no need to check it again + int result = objects.compute(base, BeanReferences::incrementOrOne); + if (result > 1) { + return; + } + addClassInfo(base, declaredClass); Bean bean = (Bean) base; if (settings.getConverter().isConvertible(bean.getClass())) { @@ -166,11 +166,20 @@ private void findReferencesBean( } else if (parentIterator != null) { SerIterator childIterator = settings.getIteratorFactory().createChild(base, parentIterator); if (childIterator != null) { + // shouldn't try and reuse references to collections findReferencesIterable(childIterator, objects); } else { + int result = objects.compute(base, BeanReferences::incrementOrOne); + if (result > 1) { + return; + } addClassInfo(base, declaredClass); } } else { + int result = objects.compute(base, BeanReferences::incrementOrOne); + if (result > 1) { + return; + } addClassInfo(base, declaredClass); } } diff --git a/src/test/java/org/joda/beans/ser/SerTestHelper.java b/src/test/java/org/joda/beans/ser/SerTestHelper.java index f9429515..796fbf87 100644 --- a/src/test/java/org/joda/beans/ser/SerTestHelper.java +++ b/src/test/java/org/joda/beans/ser/SerTestHelper.java @@ -19,10 +19,12 @@ import java.util.Currency; import java.util.EnumSet; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Locale; import java.util.Map; import java.util.TimeZone; +import java.util.TreeSet; import org.joda.beans.sample.Address; import org.joda.beans.sample.Company; @@ -228,6 +230,17 @@ public static ImmGenericCollections testGenericInterfaces( .build(); } + public static ImmGenericCollections testGenericInterfacesCollections() { + return ImmGenericCollections.builder() + .map(ImmutableMap.of( + "First", Arrays.asList("A", "B"), + "First1", ImmutableList.of("A", "B"), + "Third1", new TreeSet<>(ImmutableList.of("A", "B")), + "Third", new HashSet<>(Arrays.asList("A", "B")), + "Second", testCollections())) + .build(); + } + public static ImmKeyList testIntermediateInterfaces() { // second serialized as JodaConvertInterface, non-bean // third and fourth are serialized as an intermediate Joda-Convert interface INamedKey diff --git a/src/test/java/org/joda/beans/ser/bin/TestSerializeReferencingBin.java b/src/test/java/org/joda/beans/ser/bin/TestSerializeReferencingBin.java index 928c5619..196776f0 100644 --- a/src/test/java/org/joda/beans/ser/bin/TestSerializeReferencingBin.java +++ b/src/test/java/org/joda/beans/ser/bin/TestSerializeReferencingBin.java @@ -87,6 +87,19 @@ public void test_writeJodaConvertInterface() { BeanAssert.assertBeanEquals(bean, array); } + @Test + public void test_writeJodaConvertInterfaceCollections() { + ImmGenericCollections array = SerTestHelper.testGenericInterfacesCollections(); + + byte[] bytes = JodaBeanSer.COMPACT.binWriterReferencing().write(array); + //System.out.println(JodaBeanBinReader.visualize(bytes)); + + ImmGenericCollections bean = + (ImmGenericCollections) JodaBeanSer.COMPACT.binReader().read(bytes); + //System.out.println(bean); + BeanAssert.assertBeanEquals(bean, array); + } + @Test public void test_writeIntermediateInterface() { ImmKeyList array = SerTestHelper.testIntermediateInterfaces(); From 6b34aa7491e9b53f075a2d7551337060e32042b6 Mon Sep 17 00:00:00 2001 From: Will Nicholson Date: Tue, 20 Oct 2020 14:41:09 +0100 Subject: [PATCH 2/3] Add child iterator --- src/main/java/org/joda/beans/ser/bin/BeanReferences.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/main/java/org/joda/beans/ser/bin/BeanReferences.java b/src/main/java/org/joda/beans/ser/bin/BeanReferences.java index 701a1fa1..33e40118 100644 --- a/src/main/java/org/joda/beans/ser/bin/BeanReferences.java +++ b/src/main/java/org/joda/beans/ser/bin/BeanReferences.java @@ -166,6 +166,9 @@ private void findReferencesBean( } else if (parentIterator != null) { SerIterator childIterator = settings.getIteratorFactory().createChild(base, parentIterator); if (childIterator != null) { + if (childIterator.metaTypeRequired()) { + objects.compute(childIterator.metaTypeName(), BeanReferences::incrementOrOne); + } // shouldn't try and reuse references to collections findReferencesIterable(childIterator, objects); } else { From bfb97278447e0e00639a2055b5650734e4e885a9 Mon Sep 17 00:00:00 2001 From: Stephen Colebourne Date: Mon, 30 Dec 2024 10:57:06 +0000 Subject: [PATCH 3/3] Adjust logic to avoid changing binary form --- src/changes/changes.xml | 9 ++++-- .../joda/beans/ser/bin/BeanReferences.java | 30 +++++++------------ .../org/joda/beans/ser/SerTestHelper.java | 14 ++++----- 3 files changed, 25 insertions(+), 28 deletions(-) diff --git a/src/changes/changes.xml b/src/changes/changes.xml index b5f9ce05..7cfbbc30 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -34,6 +34,11 @@ Potentially incompatible change: The standard and referencing binary formats now support null as a map key. + + Potentially incompatible change: + The referencing binary format no longer deduplicates collections. + The old format can still be parsed successfully. + Potentially incompatible change: The standard binary and simple JSON formats now handle `Iterable` as a collection type. @@ -44,7 +49,7 @@ Instead of a Joda-Convert formatted string, the array is output as a list of primitives. This also applies to multi-dimensional arrays. This is a much more natural JSON format. - The old format cam still be parsed successfully. + The old format can still be parsed successfully. Incompatible change: @@ -52,7 +57,7 @@ For example, where the type of the collection is Object, previously a String value was explicitly typed as a String, now the type is implicit. This is a much more natural JSON format. - The old format cam still be parsed successfully. + The old format can still be parsed successfully. Incompatible change: diff --git a/src/main/java/org/joda/beans/ser/bin/BeanReferences.java b/src/main/java/org/joda/beans/ser/bin/BeanReferences.java index 58401786..b86cd781 100644 --- a/src/main/java/org/joda/beans/ser/bin/BeanReferences.java +++ b/src/main/java/org/joda/beans/ser/bin/BeanReferences.java @@ -132,13 +132,18 @@ private void findReferencesBean( return; } - if (base instanceof Bean) { - // has this object been seen before, if so no need to check it again - var result = objects.compute(base, BeanReferences::incrementOrOne); - if (result > 1) { - return; + // has this object been seen before, if so no need to check it again + if (objects.compute(base, BeanReferences::incrementOrOne) > 1) { + // shouldn't try and reuse references to collections + if (!(base instanceof Bean) && parentIterator != null) { + var childIterator = settings.getIteratorFactory().createChild(base, parentIterator); + if (childIterator != null) { + findReferencesIterable(childIterator, objects); + } } - + return; + } + if (base instanceof Bean bean) { addClassInfo(base, declaredClass); if (settings.getConverter().isConvertible(bean.getClass())) { return; @@ -165,25 +170,12 @@ private void findReferencesBean( } else if (parentIterator != null) { var childIterator = settings.getIteratorFactory().createChild(base, parentIterator); if (childIterator != null) { - if (childIterator.metaTypeRequired()) { - objects.compute(childIterator.metaTypeName(), BeanReferences::incrementOrOne); - } // shouldn't try and reuse references to collections findReferencesIterable(childIterator, objects); } else { - // has this object been seen before, if so no need to check it again - int result = objects.compute(base, BeanReferences::incrementOrOne); - if (result > 1) { - return; - } addClassInfo(base, declaredClass); } } else { - // has this object been seen before, if so no need to check it again - int result = objects.compute(base, BeanReferences::incrementOrOne); - if (result > 1) { - return; - } addClassInfo(base, declaredClass); } } diff --git a/src/test/java/org/joda/beans/ser/SerTestHelper.java b/src/test/java/org/joda/beans/ser/SerTestHelper.java index e75f0384..659c08a6 100644 --- a/src/test/java/org/joda/beans/ser/SerTestHelper.java +++ b/src/test/java/org/joda/beans/ser/SerTestHelper.java @@ -260,13 +260,13 @@ public static ImmGenericCollections testGenericInterfaces( public static ImmGenericCollections testGenericInterfacesCollections() { return ImmGenericCollections.builder() - .map(ImmutableMap.of( - "First", Arrays.asList("A", "B"), - "First1", ImmutableList.of("A", "B"), - "Third1", new TreeSet<>(ImmutableList.of("A", "B")), - "Third", new HashSet<>(Arrays.asList("A", "B")), - "Second", testCollections())) - .build(); + .map(ImmutableMap.of( + "First", Arrays.asList("A", "B"), + "First1", ImmutableList.of("A", "B"), + "Third1", new TreeSet<>(ImmutableList.of("A", "B")), + "Third", new HashSet<>(Arrays.asList("A", "B")), + "Second", testCollections(true))) + .build(); } public static ImmKeyList testIntermediateInterfaces() {