Make fflib_QueryFactory.toSOQL deterministic

fflib/src/classes/fflib_QueryFactory.cls

@@ -131,7 +131,7 @@ public class fflib_QueryFactory { //No explicit sharing declaration - inherit fr
 	public boolean equals(Object obj){
 		if( !(obj instanceof fflib_QueryFactory) || ((fflib_QueryFactory)obj).table != this.table || ((fflib_QueryFactory)obj).fields.size() != this.fields.size() )
 			return false;
-		return ((fflib_QueryFactory)obj).toString() == this.toString();
+		return ((fflib_QueryFactory)obj).toSOQL() == this.toSOQL();
 	}
 
 	/**
@@ -478,7 +478,9 @@ public class fflib_QueryFactory { //No explicit sharing declaration - inherit fr
 			if (enforceFLS) fflib_SecurityUtils.checkFieldIsReadable(table, 'Id');
 			result += 'Id  ';
 		}else{
-			for(QueryField field:fields){
+			List<QueryField> fieldsToQuery = new List<QueryField>(fields);
+			fieldsToQuery.sort(); //delegates to QueryFilter's comparable implementation
+			for(QueryField field:fieldsToQuery){
 				result += field + ', ';
 			}
 		}	
@@ -490,11 +492,7 @@ public class fflib_QueryFactory { //No explicit sharing declaration - inherit fr
 		result = result.substring(0,result.length()-2) + ' FROM ' + (relationship != null ? relationship.getRelationshipName() : table.getDescribe().getName());
 		if(conditionExpression != null)
 			result += ' WHERE '+conditionExpression;
-		/**
-		 *  check both the order and sortexpressions for fields to order by.  the order list uses an older method of sorting (addOrdering method), but does not work well with sorting related object fields.
-		 *  newer method uses sortExpressions list (addOrdering method), which work with all fields and can handle multiple sorts expressions (ie. order by last name desc, first name asc)
-		 *  both methods may be used together on a single query factory 
-		**/
+
 		if(order.size() > 0){
 			result += ' ORDER BY ';
 			for(Ordering o:order)
@@ -559,7 +557,7 @@ public class fflib_QueryFactory { //No explicit sharing declaration - inherit fr
 	}
 
 
-	public class QueryField{
+	public class QueryField implements Comparable{
 		List<Schema.SObjectField> fields;
 
 		/**
@@ -615,6 +613,33 @@ public class fflib_QueryFactory { //No explicit sharing declaration - inherit fr
 			objFields.removeAll(this.fields);
 			return objFields.size() == 0;
 		}
+		/**
+		 * Allows sorting QueryField instances, which means we'll get deterministic field ordering by just sorting the parent
+		 * QueryFactory's array when toSOQL'ing.
+		 *
+		 * Returns:
+		 * - Objects that are not QueryField instances as -2, which functions as -1 but with more flair
+		 * - QueryField instances with less joins in their path as -1
+		 * - QueryField instances with an equal number of joins and alphabetically first as an undefined negative integer
+		 * - equals as 0
+		 * - anything else an undefined positive integer (usually, but not always 1)
+		 **/
+		public Integer compareTo(Object o){
+			if(!(o instanceof QueryField))
+				return -2; //We can't possibly do a sane comparison against an unknwon type, go athead and let it "win"
+			QueryField that = (QueryField) o;
+			if(this.fields.size() < that.fields.size()){
+				return -1;
+			}else if( this.fields.size() == that.fields.size() ){
+				if(this.equals(that)){
+					return 0;
+				}else{
+					return this.toString().compareTo(that.toString());
+				}
+			}else{
+				return 1;
+			}
+		}
 	}
 
 	public class InvalidFieldException extends Exception{

fflib/src/classes/fflib_QueryFactoryTest.cls

@@ -453,6 +453,57 @@ private class fflib_QueryFactoryTest {
 		System.assert(query.containsIgnoreCase('Id FROM'));
 	}  
 
+	@isTest
+	static void queryField_compareTo(){
+		String otherType = 'bob';
+		fflib_QueryFactory.QueryField qf = new fflib_QueryFactory.QueryField(Contact.SObjectType.fields.Name);
+		fflib_QueryFactory.QueryField joinQf = new fflib_QueryFactory.QueryField(new List<Schema.SObjectField>{
+			Contact.SObjectType.fields.LastModifiedById,
+			Account.SObjectType.fields.OwnerId,
+			User.SObjectType.fields.Name
+		});
+		fflib_QueryFactory.QueryField otherJoinQf = new fflib_QueryFactory.QueryField(new List<Schema.SObjectField>{
+			Contact.SObjectType.fields.AccountId,
+			Account.SObjectType.fields.CreatedById,
+			User.SObjectType.fields.Name
+		});
+		System.assertEquals(-2, qf.compareTo(otherType));
+		System.assertEquals(0, qf.compareTo(qf));
+		System.assertEquals(
+			0, 
+			qf.compareTo(new fflib_QueryFactory.QueryField(Contact.SObjectType.fields.Name)),
+			'An equal but non-identical instance should return 0'
+		);
+		System.assertEquals(-1 , qf.compareTo(joinQf));
+		System.assertEquals(1, joinQf.compareTo(qf));
+		System.assert(joinQf.compareTo(otherJoinQf) > 0);
+		System.assert(otherJoinQf.compareTo(joinQf) < 0);
+	}
+
+	@isTest
+	static void deterministic_toSOQL(){
+		fflib_QueryFactory qf1 = new fflib_QueryFactory(User.SObjectType);
+		fflib_QueryFactory qf2 = new fflib_QueryFactory(User.SObjectType);
+		for(fflib_QueryFactory qf:new Set<fflib_QueryFactory>{qf1,qf2}){
+			qf.selectFields(new List<String>{
+				'Id',
+				'FirstName',
+				'LastName',
+				'CreatedBy.Name',
+				'CreatedBy.Manager',
+				'LastModifiedBy.Email'
+			});
+		}
+		String expectedQuery = 
+			'SELECT '
+			+'FirstName, Id, LastName, ' //less joins come first, alphabetically
+			+'CreatedBy.ManagerId, CreatedBy.Name, LastModifiedBy.Email ' //alphabetical on the same number of joinrs'
+			+'FROM User';
+		System.assertEquals(qf1.toSOQL(), qf2.toSOQL());
+		System.assertEquals(expectedQuery, qf1.toSOQL());
+		System.assertEquals(expectedQuery, qf2.toSOQL());
+	}
+
 	public static User createTestUser_noAccess(){
 		User usr;
 		try {