From eef9066184dbeb406ef96c2c2fc46118e12346d3 Mon Sep 17 00:00:00 2001
From: Mark Paluch <mpaluch@pivotal.io>
Date: Tue, 3 Jan 2017 10:39:33 +0100
Subject: [PATCH] DATACASS-352 - Compare persistent properties using column
 names in CassandraPersistentPropertyComparator.

We now compare persistent properties using their column names when both columns are regular columns. Column names either respect a defined column name or determine a column name based on the property name.

Previously, the comparison extracted the column name itself and in case the other column was not annotated, the column compared with itself, the column name with its own property name which breaks the comparator contract.
---
 ...CassandraPersistentPropertyComparator.java | 56 +++++++++----------
 ...PersistentPropertyComparatorUnitTests.java | 53 +++++++++++++-----
 2 files changed, 66 insertions(+), 43 deletions(-)

diff --git a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/mapping/CassandraPersistentPropertyComparator.java b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/mapping/CassandraPersistentPropertyComparator.java
index 2c8f9e553..e9cc2ad9d 100644
--- a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/mapping/CassandraPersistentPropertyComparator.java
+++ b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/mapping/CassandraPersistentPropertyComparator.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2013-2016 the original author or authors
+ * Copyright 2013-2017 the original author or authors
  * 
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -17,10 +17,19 @@ package org.springframework.data.cassandra.mapping;
 
 import java.util.Comparator;
 
+import org.springframework.cassandra.core.cql.CqlIdentifier;
+
 /**
  * {@link Comparator} implementation that orders {@link CassandraPersistentProperty} instances.
- * <p/>
- * Composite primary key properties and primary key properties sort before non-primary key properties.
+ * <p>
+ * Composite primary key properties and primary key properties sort before non-primary key properties. Ordering rules:
+ * <ul>
+ * <li>Composite primary keys first (equal if both {@link CassandraPersistentProperty} are a composite primary key)</li>
+ * <li>Primary key columns (see {@link CassandraPrimaryKeyColumnAnnotationComparator}, compare by ordinal/name/ordering)
+ * </li>
+ * <li>Regular columns, compared by column name (see
+ * {@link org.springframework.cassandra.core.cql.CqlIdentifier#compareTo(CqlIdentifier)})</li>
+ * </ul>
  *
  * @author Alex Shvid
  * @author Matthew T. Adams
@@ -30,6 +39,16 @@ import java.util.Comparator;
  * @see org.springframework.data.cassandra.mapping.CassandraPersistentProperty
  */
 public enum CassandraPersistentPropertyComparator implements Comparator<CassandraPersistentProperty> {
+
+	/**
+	 * Comparator instance.
+	 */
+	INSTANCE,
+
+	/**
+	 * @deprecated as of 1.5, use {@link #INSTANCE}
+	 */
+	@Deprecated
 	IT;
 
 	@Override
@@ -37,14 +56,11 @@ public enum CassandraPersistentPropertyComparator implements Comparator<Cassandr
 
 		if (left == null && right == null) {
 			return 0;
-		}
-		else if (left != null && right == null) {
+		} else if (left != null && right == null) {
 			return 1;
-		}
-		else if (left == null) {
+		} else if (left == null) {
 			return -1;
-		}
-		else if (left.equals(right)) {
+		} else if (left.equals(right)) {
 			return 0;
 		}
 
@@ -60,7 +76,7 @@ public enum CassandraPersistentPropertyComparator implements Comparator<Cassandr
 
 		if (leftIsPrimaryKey && rightIsPrimaryKey) {
 			return CassandraPrimaryKeyColumnAnnotationComparator.IT.compare(left.findAnnotation(PrimaryKeyColumn.class),
-				right.findAnnotation(PrimaryKeyColumn.class));
+					right.findAnnotation(PrimaryKeyColumn.class));
 		}
 
 		boolean leftIsKey = leftIsCompositePrimaryKey || leftIsPrimaryKey;
@@ -68,27 +84,11 @@ public enum CassandraPersistentPropertyComparator implements Comparator<Cassandr
 
 		if (leftIsKey && !rightIsKey) {
 			return -1;
-		}
-		else if (!leftIsKey && rightIsKey) {
+		} else if (!leftIsKey && rightIsKey) {
 			return 1;
 		}
 
 		// else, neither property is a composite primary key nor a primary key; compare @Column annotations
-
-		Column leftColumn = left.findAnnotation(Column.class);
-		Column rightColumn = right.findAnnotation(Column.class);
-
-		if (leftColumn != null && rightColumn != null) {
-			return CassandraColumnAnnotationComparator.IT.compare(leftColumn, rightColumn);
-		}
-		else if (leftColumn != null) {
-			return leftColumn.value().compareTo(left.getName());
-		}
-		else if (rightColumn != null) {
-			return left.getName().compareTo(rightColumn.value());
-		}
-		else {
-			return left.getName().compareTo(right.getName());
-		}
+		return left.getColumnName().compareTo(right.getColumnName());
 	}
 }
diff --git a/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/mapping/CassandraPersistentPropertyComparatorUnitTests.java b/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/mapping/CassandraPersistentPropertyComparatorUnitTests.java
index de3b082bd..20cb06c83 100644
--- a/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/mapping/CassandraPersistentPropertyComparatorUnitTests.java
+++ b/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/mapping/CassandraPersistentPropertyComparatorUnitTests.java
@@ -24,10 +24,11 @@ import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.Mock;
 import org.mockito.runners.MockitoJUnitRunner;
+import org.springframework.cassandra.core.cql.CqlIdentifier;
 
 /**
- * The CassandraPersistentPropertyComparatorUnitTests class is a test suite of test cases testing the contract
- * and functionality of the {@link CassandraPersistentPropertyComparator} class.
+ * The CassandraPersistentPropertyComparatorUnitTests class is a test suite of test cases testing the contract and
+ * functionality of the {@link CassandraPersistentPropertyComparator} class.
  *
  * @author John Blum
  * @see org.springframework.data.cassandra.mapping.CassandraPersistentPropertyComparator
@@ -36,11 +37,9 @@ import org.mockito.runners.MockitoJUnitRunner;
 @RunWith(MockitoJUnitRunner.class)
 public class CassandraPersistentPropertyComparatorUnitTests {
 
-	@Mock
-	private CassandraPersistentProperty left;
+	@Mock private CassandraPersistentProperty left;
 
-	@Mock
-	private CassandraPersistentProperty right;
+	@Mock private CassandraPersistentProperty right;
 
 	@Test
 	public void leftAndRightAreNullReturnsZero() {
@@ -144,24 +143,48 @@ public class CassandraPersistentPropertyComparatorUnitTests {
 
 	@Test
 	public void compareLeftAndRightNamesReturnsNegativeValue() {
+
 		when(left.isCompositePrimaryKey()).thenReturn(false);
 		when(left.isPrimaryKeyColumn()).thenReturn(true);
 		when(right.isCompositePrimaryKey()).thenReturn(true);
 		when(right.isPrimaryKeyColumn()).thenReturn(false);
-		when(left.findAnnotation(eq(Column.class))).thenReturn(null);
-		when(right.findAnnotation(eq(Column.class))).thenReturn(null);
-		when(left.getName()).thenReturn("left");
-		when(right.getName()).thenReturn("right");
+		when(left.getColumnName()).thenReturn(CqlIdentifier.cqlId("left"));
+		when(right.getColumnName()).thenReturn(CqlIdentifier.cqlId("right"));
 
-		assertThat(CassandraPersistentPropertyComparator.IT.compare(left, right), is(lessThan(0)));
+		assertThat(CassandraPersistentPropertyComparator.INSTANCE.compare(left, right), is(lessThan(0)));
 
 		verify(left, times(1)).isCompositePrimaryKey();
 		verify(left, times(1)).isPrimaryKeyColumn();
-		verify(left, times(1)).findAnnotation(eq(Column.class));
-		verify(left, times(1)).getName();
+		verify(left, times(1)).getColumnName();
 		verify(right, times(1)).isCompositePrimaryKey();
 		verify(right, times(1)).isPrimaryKeyColumn();
-		verify(right, times(1)).findAnnotation(eq(Column.class));
-		verify(right, times(1)).getName();
+		verify(right, times(1)).getColumnName();
+	}
+
+	/**
+	 * @see DATACASS-352
+	 */
+	@Test
+	public void columnNameComparisonShouldHonorContract() throws Exception {
+
+		BasicCassandraMappingContext context = new BasicCassandraMappingContext();
+		CassandraPersistentEntity<?> persistentEntity = context.getPersistentEntity(TwoColumns.class);
+
+		CassandraPersistentProperty annotated = persistentEntity.getPersistentProperty("annotated");
+		CassandraPersistentProperty another = persistentEntity.getPersistentProperty("anotherAnnotated");
+		CassandraPersistentProperty plain = persistentEntity.getPersistentProperty("plain");
+
+		assertThat(CassandraPersistentPropertyComparator.IT.compare(annotated, plain), is(lessThanOrEqualTo(-1)));
+		assertThat(CassandraPersistentPropertyComparator.IT.compare(plain, annotated), is(greaterThanOrEqualTo(1)));
+		assertThat(CassandraPersistentPropertyComparator.IT.compare(another, another), is(0));
+	}
+
+	static class TwoColumns {
+
+		@Column("annotated") String annotated;
+
+		@Column("another") String anotherAnnotated;
+
+		String plain;
 	}
 }