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.

Sign up for GitHub

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

Equals compares collections last #1847

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft

Equals compares collections last #1847

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
33 changes: 31 additions & 2 deletions conjure-java-core/src/main/java/com/palantir/conjure/java/types/MethodSpecs.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static com.palantir.logsafe.Preconditions.checkState;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.palantir.conjure.java.util.JavaNameSanitizer;
import com.palantir.conjure.spec.FieldName;
Expand All @@ -27,6 +28,7 @@
import com.squareup.javapoet.FieldSpec;
import com.squareup.javapoet.MethodSpec;
import com.squareup.javapoet.ParameterSpec;
import com.squareup.javapoet.ParameterizedTypeName;
import com.squareup.javapoet.TypeName;
import com.squareup.javapoet.TypeSpec;
import java.time.OffsetDateTime;
Expand All @@ -38,6 +40,8 @@

public final class MethodSpecs {

private static final ImmutableSet<String> COLLECTION_TYPES =
ImmutableSet.of("java.util.List", "java.util.Set", "java.util.Map");
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we could pass through the original conjure type information so that we can avoid regressions if we eventually specialize the field types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could potentially pipe the List<EnrichedField> through and use a DefaultTypeVisitor to determine if field should be treated as a collection. It might be worth piping something like List<Pair<EnrichedField, FieldSpec>> through to ensure proper association between the two.

private static final String MEMOIZED_HASH_CODE = "memoizedHashCode";

public static MethodSpec createEquals(TypeName thisClass) {
Expand Down Expand Up @@ -70,8 +74,10 @@ public static MethodSpec createEqualTo(

CodeBlock equalsTo = fields.isEmpty()
? CodeBlock.of("$L", true)
: CodeBlocks.of(
fields.stream().map(MethodSpecs::createEqualsStatement).collect(joining(CodeBlock.of(" && "))));
: CodeBlocks.of(fields.stream()
.sorted(MethodSpecs::collectionsLastForEquals)
.map(MethodSpecs::createEqualsStatement)
.collect(joining(CodeBlock.of(" && "))));

return MethodSpec.methodBuilder("equalTo")
.addModifiers(Modifier.PRIVATE)
Expand All @@ -82,6 +88,29 @@ public static MethodSpec createEqualTo(
.build();
}

private static int collectionsLastForEquals(FieldSpec field1, FieldSpec field2) {
if (COLLECTION_TYPES.contains(getTypeForEquals(field1.type))) {
if (COLLECTION_TYPES.contains(getTypeForEquals(field2.type))) {
return 0; // both are collections, keep in place
}
return 1; // sort collections last for equals comparison
}
return 0; // keep in place
}

private static String getTypeForEquals(TypeName typeName) {
if (typeName instanceof ParameterizedTypeName) {
ParameterizedTypeName parameterizedTypeName = (ParameterizedTypeName) typeName;
String canonicalName = parameterizedTypeName.rawType.canonicalName();
if ("java.util.Optional".equals(canonicalName)) {
return getTypeForEquals(
parameterizedTypeName.typeArguments.iterator().next());
}
return canonicalName;
}
return typeName.toString();
}

private static CodeBlock createEqualsStatement(FieldSpec field) {
String thisField = "this." + field.name;
String otherField = "other." + field.name;
Expand Down