From 887dab1616988f7abf4b54ce5888ddf42bf134e3 Mon Sep 17 00:00:00 2001
From: Chapman Flack <chap@anastigmatix.net>
Date: Tue, 27 May 2025 20:11:40 -0400
Subject: [PATCH] Tighten implementation of SnippetTiebreaker

The comparator could return a premature result if one Snippet or both
being compared has a null implementor name, resulting in an output
ordering that could depend on the sequence of comparisons performed.

The case is unlikely to be seen in practice, as an implementor name
can only be made null by explicit implementor="" in an annotation or
-Addr.implementor=- when invoking the Java compiler.

Nonetheless, the tiebreaker method can be made more simple and convincing
by taking advantage of the Java 8 additions to Comparator.

Addresses #527.

For consistency's sake, also Comparator-ize TypeTiebreaker.
---
 .../processing/SnippetTiebreaker.java         | 49 ++++++++-----------
 .../annotation/processing/TypeTiebreaker.java | 26 +++++-----
 2 files changed, 35 insertions(+), 40 deletions(-)

diff --git a/pljava-api/src/main/java/org/postgresql/pljava/annotation/processing/SnippetTiebreaker.java b/pljava-api/src/main/java/org/postgresql/pljava/annotation/processing/SnippetTiebreaker.java
index dd562f72c..d21e59e84 100644
--- a/pljava-api/src/main/java/org/postgresql/pljava/annotation/processing/SnippetTiebreaker.java
+++ b/pljava-api/src/main/java/org/postgresql/pljava/annotation/processing/SnippetTiebreaker.java
@@ -12,9 +12,13 @@
  */
 package org.postgresql.pljava.annotation.processing;
 
+import java.util.Arrays;
 import java.util.Comparator;
+import static java.util.Comparator.comparing;
+import static java.util.Comparator.naturalOrder;
+import static java.util.Comparator.nullsFirst;
 
-import org.postgresql.pljava.sqlgen.Lexicals.Identifier;
+import org.postgresql.pljava.sqlgen.Lexicals.Identifier.Simple;
 
 /**
  * Resolve ties in {@code Snippet} ordering in an arbitrary but deterministic
@@ -22,34 +26,23 @@
  */
 class SnippetTiebreaker implements Comparator<Vertex<Snippet>>
 {
+	private static final Comparator<Vertex<Snippet>> VCMP;
+
+	static
+	{
+		Comparator<Snippet> scmp =
+			comparing(Snippet::implementorName,
+				nullsFirst(comparing(Simple::pgFolded, naturalOrder()))
+			)
+			.thenComparing(Snippet::deployStrings,   Arrays::compare)
+			.thenComparing(Snippet::undeployStrings, Arrays::compare);
+
+		VCMP = comparing(v -> v.payload, scmp);
+	}
+
 	@Override
-	public int compare( Vertex<Snippet> o1, Vertex<Snippet> o2)
+	public int compare(Vertex<Snippet> o1, Vertex<Snippet> o2)
 	{
-		Snippet s1 = o1.payload;
-		Snippet s2 = o2.payload;
-		int diff;
-		Identifier.Simple s1imp = s1.implementorName();
-		Identifier.Simple s2imp = s2.implementorName();
-		if ( null != s1imp  &&  null != s2imp )
-		{
-			diff = s1imp.pgFolded().compareTo( s2imp.pgFolded());
-			if ( 0 != diff )
-				return diff;
-		}
-		else
-			return null == s1imp ? -1 : 1;
-		String[] ds1 = s1.deployStrings();
-		String[] ds2 = s2.deployStrings();
-		diff = ds1.length - ds2.length;
-		if ( 0 != diff )
-			return diff;
-		for ( int i = 0 ; i < ds1.length ; ++ i )
-		{
-			diff = ds1[i].compareTo( ds2[i]);
-			if ( 0 != diff )
-				return diff;
-		}
-		assert s1 == s2 : "Two distinct Snippets compare equal by tiebreaker";
-		return 0;
+		return VCMP.compare(o1, o2);
 	}
 }
diff --git a/pljava-api/src/main/java/org/postgresql/pljava/annotation/processing/TypeTiebreaker.java b/pljava-api/src/main/java/org/postgresql/pljava/annotation/processing/TypeTiebreaker.java
index 6f104b07f..04fa37f82 100644
--- a/pljava-api/src/main/java/org/postgresql/pljava/annotation/processing/TypeTiebreaker.java
+++ b/pljava-api/src/main/java/org/postgresql/pljava/annotation/processing/TypeTiebreaker.java
@@ -13,6 +13,7 @@
 package org.postgresql.pljava.annotation.processing;
 
 import java.util.Comparator;
+import static java.util.Comparator.comparing;
 import java.util.Map;
 
 import javax.lang.model.type.TypeMirror;
@@ -24,22 +25,23 @@
 class TypeTiebreaker
 implements Comparator<Vertex<Map.Entry<TypeMirror, DBType>>>
 {
+	private static final Comparator<Vertex<Map.Entry<TypeMirror, DBType>>> VCMP;
+
+	static
+	{
+		Comparator<Map.Entry<TypeMirror, DBType>> ecmp =
+			comparing(
+				(Map.Entry<TypeMirror, DBType> e) -> e.getValue().toString())
+			.thenComparing(e -> e.getKey().toString());
+
+		VCMP = comparing(v -> v.payload, ecmp);
+	}
+
 	@Override
 	public int compare(
 		Vertex<Map.Entry<TypeMirror, DBType>> o1,
 		Vertex<Map.Entry<TypeMirror, DBType>> o2)
 	{
-		Map.Entry<TypeMirror, DBType> m1 = o1.payload;
-		Map.Entry<TypeMirror, DBType> m2 = o2.payload;
-		int diff =
-			m1.getValue().toString().compareTo( m2.getValue().toString());
-		if ( 0 != diff )
-			return diff;
-		diff = m1.getKey().toString().compareTo( m2.getKey().toString());
-		if ( 0 != diff )
-			return diff;
-		assert
-			m1 == m2 : "Two distinct type mappings compare equal by tiebreaker";
-		return 0;
+		return VCMP.compare(o1, o2);
 	}
 }