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] Add Serialization Optimization for Primitive Collection Types #2386

Open
wants to merge 1 commit into
base: bmarcaurele/small-conjure-collections-refactor
Choose a base branch
from
Open
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
5 changes: 5 additions & 0 deletions changelog/@unreleased/pr-2386.v2.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
type: improvement
improvement:
description: '[FR] Add Serialization Optimization for Primitive Collection Types'
links:
- https://github.com/palantir/conjure-java/pull/2386

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -22,15 +22,19 @@
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.exc.InvalidNullException;
import com.palantir.conjure.java.lib.SafeLong;
import com.palantir.conjure.java.serialization.ObjectMappers;
import com.palantir.product.CovariantListExample;
import com.palantir.product.ListExample;
import com.palantir.product.PrimitiveExample;
import java.util.Collections;
import java.util.List;
import java.util.Optional;
import org.junit.jupiter.api.Test;

public class NonNullCollectionsTest {
private static final ObjectMapper objectMapper = ObjectMappers.newClientObjectMapper();
private static final ObjectMapper clientMapper = ObjectMappers.newClientObjectMapper();
private static final ObjectMapper serverMapper = ObjectMappers.newServerJsonMapper();

@Test
public void throwsNpe() {
@@ -55,7 +59,7 @@ public void testOptionalSerialization() throws JsonProcessingException {
.optionalItems(Collections.singleton(Optional.empty()))
.build();

assertThat(objectMapper.readValue(objectMapper.writeValueAsString(listExample), ListExample.class))
assertThat(clientMapper.readValue(clientMapper.writeValueAsString(listExample), ListExample.class))
.isEqualTo(listExample);

// non-null collections will add "contentNulls = Nulls.FAIL" to the JsonSetter annotation. This will cause deser
@@ -64,12 +68,31 @@ public void testOptionalSerialization() throws JsonProcessingException {
.addAllItems(Collections.singleton(Optional.empty()))
.build();
assertThatExceptionOfType(InvalidNullException.class)
.isThrownBy(() -> objectMapper.readValue(
objectMapper.writeValueAsString(covariantListExample), CovariantListExample.class));
.isThrownBy(() -> clientMapper.readValue(
clientMapper.writeValueAsString(covariantListExample), CovariantListExample.class));

// Similarly, setting a null in the builder also breaks
assertThatExceptionOfType(NullPointerException.class).isThrownBy(() -> CovariantListExample.builder()
.addAllItems(Collections.singleton(null))
.build());
}

@Test
public void testSerDeOptimizationRespectsConjureEmptyCollections() throws JsonProcessingException {
PrimitiveExample expected = PrimitiveExample.builder().build();
assertThat(clientMapper.writeValueAsString(expected))
.describedAs("Does not serialize any empty collections, even when optimizing for primitives")
.isEqualTo("{}");
}

@Test
public void testSerializationRoundtrip() throws JsonProcessingException {
PrimitiveExample expected = PrimitiveExample.builder()
.ints(List.of(1, 2, 3))
.doubles(List.of(1.1, 2.2, 3.3))
.longs(List.of(SafeLong.of(1L), SafeLong.of(2L)))
.build();
String serialized = serverMapper.writeValueAsString(expected);
assertThat(expected).isEqualTo(clientMapper.readValue(serialized, PrimitiveExample.class));
}
}
Original file line number Diff line number Diff line change
@@ -168,6 +168,21 @@ public void testObjectGenerator_excludeEmptyCollections() throws IOException {
assertThatFilesAreTheSame(files, REFERENCE_FILES_FOLDER);
}

@Test
public void testObjectGenerator_primitiveCollections() throws IOException {
ConjureDefinition def =
Conjure.parse(ImmutableList.of(new File("src/test/resources/primitive-collections.yml")));
List<Path> files = new GenerationCoordinator(
MoreExecutors.directExecutor(),
ImmutableSet.of(new ObjectGenerator(Options.builder()
.excludeEmptyCollections(true)
.nonNullCollections(true)
.build())))
.emit(def, tempDir);

assertThatFilesAreTheSame(files, REFERENCE_FILES_FOLDER);
}

@Test
public void testConjureImports() throws IOException {
ConjureDefinition conjure = Conjure.parse(ImmutableList.of(
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
types:
definitions:
default-package: com.palantir.product
objects:
PrimitiveExample:
fields:
ints: list<integer>
doubles: list<double>
longs: list<safelong>
Original file line number Diff line number Diff line change
@@ -38,8 +38,33 @@ private ConjureCollections() {
// cannot instantiate
}

/*
* This is bizarre. Allow me to explain...
*
* We do _not_ want to expose the Conjure*List types externally
* but we also want the optimizations they provide to make it thru
* to jackson for serialization. So the runtime type needs to be
* preserved while also not exposing the type :phew:.
*
* To achieve this we have to do some gymnastics surrounding the type
* system. We need this to return the type of the list given, but also
* return specific Conjure types when detected. This requires that we
* erase the type info, but we know this is safe because we are directly
* returning the same type which is by definition the identity function.
* Therefore the input List<T> is the same types as the output List<T>.
*/
public static <T> List<T> unmodifiableList(List<T> list) {
return Collections.unmodifiableList(list);
// Return the unmodifiable version of the Eclipse types
if (list instanceof ConjureIntegerList) {
return (List<T>) ((ConjureIntegerList) list).asUnmodifiable();
} else if (list instanceof ConjureDoubleList) {
return (List<T>) ((ConjureDoubleList) list).asUnmodifiable();
} else if (list instanceof ConjureSafeLongList) {
return (List<T>) ((ConjureSafeLongList) list).asUnmodifiable();
} else {
// Otherwise use the JDK types
return Collections.unmodifiableList(list);
}
}

@SuppressWarnings("unchecked")
Original file line number Diff line number Diff line change
@@ -16,20 +16,21 @@

package com.palantir.conjure.java.lib.internal;

import com.fasterxml.jackson.annotation.JsonValue;
import java.util.AbstractList;
import java.util.Collection;
import java.util.RandomAccess;
import org.eclipse.collections.impl.list.mutable.primitive.DoubleArrayList;
import org.eclipse.collections.api.list.primitive.MutableDoubleList;
import org.eclipse.collections.impl.utility.Iterate;

/**
* ConjureDoubleList is a boxed list wrapper for the eclipse-collections DoubleArrayList. In eclipse-collections 12,
* a BoxedMutableDoubleList will be released. Once available, ConjureDoubleList should be replaced with that.
*/
final class ConjureDoubleList extends AbstractList<Double> implements RandomAccess {
private final DoubleArrayList delegate;
private final MutableDoubleList delegate;
Copy link
Member Author

Choose a reason for hiding this comment

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

Changing this type so that I can wrap both DoubleArrayList and UnmodifiableDoubleList which share this interface as the lowest common denominator.


ConjureDoubleList(DoubleArrayList delegate) {
ConjureDoubleList(MutableDoubleList delegate) {
this.delegate = delegate;
}

@@ -69,4 +70,15 @@ public void clear() {
public Double set(int index, Double element) {
return delegate.set(index, element);
}

public ConjureDoubleList asUnmodifiable() {
return new ConjureDoubleList(delegate.asUnmodifiable());
}

// Cannot be named 'toArray' as that conflicts with the #toArray in AbstractList
// This is a serialization optimization that avoids boxing, but does copy
@JsonValue
double[] jacksonSerialize() {
return delegate.toArray();
Copy link
Member Author

Choose a reason for hiding this comment

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

This is how we avoid boxing out the door (I checked the Jackson code paths manually, it does infact operate on the array), while also respecting the various Jackson flags we use like excludeEmptyCollections. It does make a copy, but implementing a version that avoided this copy was very complex as it required leaking details of various Jackson settings.

}
}
Loading