From 3a3f0d4d61e83d8173b6b4a83fa21866fd3562fd Mon Sep 17 00:00:00 2001
From: Jonah Graham <jonah@kichwacoders.com>
Date: Fri, 7 Apr 2023 16:56:11 -0400
Subject: [PATCH] Only consider depth of content types when they are related

In the case that two content types match a given input, the depth
was being used to select between them. This was done top make sure
more specific types for related types were selected as appropriate.

The way content types are selected is by first sorting all the
candidate types, and then choosing the first one.

This change makes the sort only consider the depth of the content
type if the content types have a descendent-ancestor relationship.
If they are not in the same lineage, the depth is not a criteria
for sorting the content types.

New content type tests have been added to cover the case from #351,
specifically, conflict4 matches the conflict for *.hpp files between
the tm4e and cdt plug-ins.

The comment update on conflict2 test and the new conflict2a test
should probably have been done when Bug 86915 was completed because
in 9cbda7e4a42e2fd94e50d8fe700c0ad1574fb5a9 the order of content types
was changed, but the comment was not changed, and it wasn't apparent
to me if there was an explicit test that covered the conflict case
of described content types.

Fixes #151
---
 .../plugin.xml                                | 59 ++++++++++++++++--
 .../content/Conflict2aContentDescriber.java   | 50 +++++++++++++++
 .../content/IContentTypeManagerTest.java      | 61 +++++++++++++++++--
 .../internal/content/ContentTypeCatalog.java  | 56 +++++++++++------
 4 files changed, 199 insertions(+), 27 deletions(-)
 create mode 100644 resources/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/resources/content/Conflict2aContentDescriber.java

diff --git a/resources/tests/org.eclipse.core.tests.resources/plugin.xml b/resources/tests/org.eclipse.core.tests.resources/plugin.xml
index a65cbff3827..6696f68f9a5 100644
--- a/resources/tests/org.eclipse.core.tests.resources/plugin.xml
+++ b/resources/tests/org.eclipse.core.tests.resources/plugin.xml
@@ -306,10 +306,25 @@
             file-extensions="conflict2"
             name="Specialized Conflict 2"
             id="sub_conflict2"/>
-      <!-- 
-      	Two unrelated types (sub_conflict3 and unrelated conflict3) are in conflict. 
-      	Order will be arbitrary.
-      	-->
+      <!--
+        A base type and a sub-type (that restates file-spec) are in conflict.
+        Specialized type should be picked if both have describers returning valid
+      -->
+      <content-type
+            file-extensions="conflict2a"
+            name="Conflict 2a"
+            id="base_conflict2a"
+            describer="org.eclipse.core.tests.resources.content.Conflict2aContentDescriber"/>
+      <content-type
+            base-type="base_conflict2a"
+            file-extensions="conflict2a"
+            name="Specialized Conflict 2a"
+            id="sub_conflict2a"
+            describer="org.eclipse.core.tests.resources.content.Conflict2aContentDescriber"/>
+      <!--
+        Two unrelated types (sub_conflict3 and unrelated conflict3) with same priority are in conflict.
+        Order will be arbitrary.
+      -->
       <content-type            		
             id="base_conflict3"                  
             file-extensions="base_conflict3"
@@ -323,6 +338,42 @@
             id="unrelated_conflict3"      
             file-extensions="conflict3"
             name="Unrelated Conflict 3"/>                                              
+      <!--
+        Two unrelated types (sub_conflict4 and unrelated conflict4) with with deeper having higher priority.
+        Order will be by priority.
+      -->
+      <content-type
+            id="base_conflict4"
+            file-extensions="base_conflict4"
+            name="Conflict 4"/>
+      <content-type
+            id="sub_conflict4"
+            base-type="base_conflict4"
+            file-extensions="conflict4"
+            name="Deeper Conflict 4"/>
+      <content-type
+            id="unrelated_conflict4"
+            file-extensions="conflict4"
+            priority="low"
+            name="Unrelated Conflict 4"/>
+      <!--
+        Two unrelated types (sub_conflict5 and unrelated conflict5) with with deeper having lower priority.
+        Order will be by priority.
+      -->
+      <content-type
+            id="base_conflict5"
+            file-extensions="base_conflict5"
+            name="Conflict 5"/>
+      <content-type
+            id="sub_conflict5"
+            base-type="base_conflict5"
+            file-extensions="conflict5"
+            priority="low"
+            name="Deeper Conflict 5"/>
+      <content-type
+            id="unrelated_conflict5"
+            file-extensions="conflict5"
+            name="Unrelated Conflict 5"/>
       <!-- end of conflict-related content types -->
 	  <content-type
             file-extensions="tzt"
diff --git a/resources/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/resources/content/Conflict2aContentDescriber.java b/resources/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/resources/content/Conflict2aContentDescriber.java
new file mode 100644
index 00000000000..9efb3efd4ad
--- /dev/null
+++ b/resources/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/resources/content/Conflict2aContentDescriber.java
@@ -0,0 +1,50 @@
+/*******************************************************************************
+ * Copyright (c) 2005, 2023 IBM Corporation and others.
+ *
+ * This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License 2.0
+ * which accompanies this distribution, and is available at
+ * https://www.eclipse.org/legal/epl-2.0/
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ *
+ * Contributors:
+ *     IBM Corporation - initial API and implementation
+ *******************************************************************************/
+package org.eclipse.core.tests.resources.content;
+
+import java.io.*;
+import java.nio.charset.StandardCharsets;
+import java.util.Arrays;
+import org.eclipse.core.runtime.QualifiedName;
+import org.eclipse.core.runtime.content.IContentDescription;
+import org.eclipse.core.runtime.content.ITextContentDescriber;
+
+/**
+ * A content describer that looks for "conflict2a" at the beginning of the
+ * stream as used by
+ * org.eclipse.core.tests.resources.content.IContentTypeManagerTest.testFileSpecConflicts()
+ */
+public class Conflict2aContentDescriber implements ITextContentDescriber {
+
+	@Override
+	public int describe(InputStream contents, IContentDescription description) throws IOException {
+		final byte[] expected = "conflict2a".getBytes(StandardCharsets.UTF_8);
+		byte[] actual = new byte[expected.length];
+		int read = contents.read(actual);
+		if (read == actual.length && Arrays.equals(expected, actual)) {
+			return VALID;
+		}
+		return INVALID;
+	}
+
+	@Override
+	public int describe(Reader contents, IContentDescription description) throws IOException {
+		throw new UnsupportedOperationException("Not used by test");
+	}
+
+	@Override
+	public QualifiedName[] getSupportedOptions() {
+		return IContentDescription.ALL;
+	}
+}
diff --git a/resources/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/resources/content/IContentTypeManagerTest.java b/resources/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/resources/content/IContentTypeManagerTest.java
index d3b11417601..dceb81acaa8 100644
--- a/resources/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/resources/content/IContentTypeManagerTest.java
+++ b/resources/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/resources/content/IContentTypeManagerTest.java
@@ -895,13 +895,33 @@ public void testFileSpecConflicts() throws IOException {
 		IContentType conflict2sub = manager.getContentType(PI_RESOURCES_TESTS + ".sub_conflict2");
 		assertNotNull("2.0", conflict2base);
 		assertNotNull("2.1", conflict2sub);
-		// when submitting contents, for related types, descendant comes first
 
+		// when submitting contents, for related types with indeterminate match, general
+		// comes first
 		IContentType[] selectedConflict2 = manager.findContentTypesFor(getRandomContents(), "test.conflict2");
 		assertEquals("2.2", 2, selectedConflict2.length);
 		assertEquals("2.3", selectedConflict2[0], conflict2base);
 		assertEquals("2.4", selectedConflict2[1], conflict2sub);
 
+		IContentType conflict2abase = manager.getContentType(PI_RESOURCES_TESTS + ".base_conflict2a");
+		IContentType conflict2asub = manager.getContentType(PI_RESOURCES_TESTS + ".sub_conflict2a");
+		assertNotNull("2.5", conflict2abase);
+		assertNotNull("2.6", conflict2asub);
+
+		// when submitting contents, for related types with valid match, specific
+		// comes first
+		IContentType[] selectedConflict2a = manager
+				.findContentTypesFor(getInputStream("conflict2a", StandardCharsets.UTF_8), "test.conflict2a");
+		assertEquals("2.7", 2, selectedConflict2a.length);
+		assertEquals("2.8", selectedConflict2a[0], conflict2asub);
+		assertEquals("2.9", selectedConflict2a[1], conflict2abase);
+
+		// when not submitting contents, for related types, most general type prevails
+		selectedConflict2a = manager.findContentTypesFor("test.conflict2a");
+		assertEquals("2.10", 2, selectedConflict2a.length);
+		assertEquals("2.11", selectedConflict2a[0], conflict2abase);
+		assertEquals("2.12", selectedConflict2a[1], conflict2asub);
+
 		IContentType conflict3base = manager.getContentType(PI_RESOURCES_TESTS + ".base_conflict3");
 		IContentType conflict3sub = manager.getContentType(PI_RESOURCES_TESTS + ".sub_conflict3");
 		IContentType conflict3unrelated = manager.getContentType(PI_RESOURCES_TESTS + ".unrelated_conflict3");
@@ -910,13 +930,44 @@ public void testFileSpecConflicts() throws IOException {
 		assertNotNull("3.0.3", conflict3unrelated);
 
 		// Two unrelated types (sub_conflict3 and unrelated conflict3) are in conflict.
-		// Order will be based on depth (more general first since they don't have
-		// describers)
+		// Order will be arbitrary (lexicographically).
 
 		IContentType[] selectedConflict3 = manager.findContentTypesFor(getRandomContents(), "test.conflict3");
 		assertEquals("4.0", 2, selectedConflict3.length);
-		assertEquals("4.1", selectedConflict3[0], conflict3unrelated);
-		assertEquals("4.2", selectedConflict3[1], conflict3sub);
+		assertEquals("4.1", selectedConflict3[0], conflict3sub);
+		assertEquals("4.2", selectedConflict3[1], conflict3unrelated);
+
+		IContentType conflict4base = manager.getContentType(PI_RESOURCES_TESTS + ".base_conflict4");
+		IContentType conflict4sub = manager.getContentType(PI_RESOURCES_TESTS + ".sub_conflict4");
+		IContentType conflict4unrelated_lowPriority = manager.getContentType(PI_RESOURCES_TESTS + ".unrelated_conflict4");
+		assertNotNull("5.0.1", conflict4base);
+		assertNotNull("5.0.2", conflict4sub);
+		assertNotNull("5.0.4", conflict4unrelated_lowPriority);
+
+		// Two unrelated types (sub_conflict4 and unrelated conflict4) are in conflict,
+		// but with different priorities
+		// Order will be based on priority
+
+		IContentType[] selectedConflict4 = manager.findContentTypesFor(getRandomContents(), "test.conflict4");
+		assertEquals("6.0", 2, selectedConflict4.length);
+		assertEquals("6.1", selectedConflict4[0], conflict4sub);
+		assertEquals("6.2", selectedConflict4[1], conflict4unrelated_lowPriority);
+
+		IContentType conflict5base = manager.getContentType(PI_RESOURCES_TESTS + ".base_conflict5");
+		IContentType conflict5sub_lowPriority = manager.getContentType(PI_RESOURCES_TESTS + ".sub_conflict5");
+		IContentType conflict5unrelated = manager.getContentType(PI_RESOURCES_TESTS + ".unrelated_conflict5");
+		assertNotNull("6.0.1", conflict5base);
+		assertNotNull("6.0.2", conflict5sub_lowPriority);
+		assertNotNull("6.0.5", conflict5unrelated);
+
+		// Two unrelated types (sub_conflict5 and unrelated conflict5) are in conflict,
+		// but with different priorities
+		// Order will be based on priority
+
+		IContentType[] selectedConflict5 = manager.findContentTypesFor(getRandomContents(), "test.conflict5");
+		assertEquals("7.0", 2, selectedConflict5.length);
+		assertEquals("7.1", selectedConflict5[0], conflict5unrelated);
+		assertEquals("7.2", selectedConflict5[1], conflict5sub_lowPriority);
 	}
 
 	@Test
diff --git a/runtime/bundles/org.eclipse.core.contenttype/src/org/eclipse/core/internal/content/ContentTypeCatalog.java b/runtime/bundles/org.eclipse.core.contenttype/src/org/eclipse/core/internal/content/ContentTypeCatalog.java
index fa61841144f..3904fcbd459 100644
--- a/runtime/bundles/org.eclipse.core.contenttype/src/org/eclipse/core/internal/content/ContentTypeCatalog.java
+++ b/runtime/bundles/org.eclipse.core.contenttype/src/org/eclipse/core/internal/content/ContentTypeCatalog.java
@@ -41,16 +41,30 @@ public final class ContentTypeCatalog {
 	private ContentTypeManager manager;
 
 	/**
-	 * A sorting policy where the more generic content type wins. Lexicographical comparison is done
-	 * as a last resort when all other criteria fail.
+	 * Return true if type1 is an ancestor of type2 or if type2 is an ancestor of
+	 * type1
+	 *
+	 * @param type1
+	 * @param type2
+	 * @return true type1 is ancestor or type2, or vice versa. false otherwise
+	 */
+	private static boolean isAncestor(ContentType type1, ContentType type2) {
+		return type1.isKindOf(type2) || type2.isKindOf(type1);
+	}
+
+	/**
+	 * A sorting policy where the more generic content type wins. Lexicographical
+	 * comparison is done as a last resort when all other criteria fail.
 	 */
 	private final Comparator<IContentType> policyConstantGeneralIsBetter = (IContentType o1, IContentType o2) -> {
 		ContentType type1 = (ContentType) o1;
 		ContentType type2 = (ContentType) o2;
-		// first criteria: depth - the lower, the better
-		int depthCriteria = type1.getDepth() - type2.getDepth();
-		if (depthCriteria != 0)
-			return depthCriteria;
+		if (isAncestor(type1, type2)) {
+			// first criteria: depth - the lower, the better
+			int depthCriteria = type1.getDepth() - type2.getDepth();
+			if (depthCriteria != 0)
+				return depthCriteria;
+		}
 		// second criteria: priority - the higher, the better
 		int priorityCriteria = type1.getPriority() - type2.getPriority();
 		if (priorityCriteria != 0)
@@ -66,10 +80,12 @@ public final class ContentTypeCatalog {
 	private Comparator<IContentType> policyConstantSpecificIsBetter = (IContentType o1, IContentType o2) -> {
 		ContentType type1 = (ContentType) o1;
 		ContentType type2 = (ContentType) o2;
-		// first criteria: depth - the higher, the better
-		int depthCriteria = type1.getDepth() - type2.getDepth();
-		if (depthCriteria != 0)
-			return -depthCriteria;
+		if (isAncestor(type1, type2)) {
+			// first criteria: depth - the higher, the better
+			int depthCriteria = type1.getDepth() - type2.getDepth();
+			if (depthCriteria != 0)
+				return -depthCriteria;
+		}
 		// second criteria: priority - the higher, the better
 		int priorityCriteria = type1.getPriority() - type2.getPriority();
 		if (priorityCriteria != 0)
@@ -84,10 +100,12 @@ public final class ContentTypeCatalog {
 	private Comparator<IContentType> policyGeneralIsBetter = (IContentType o1, IContentType o2) -> {
 		ContentType type1 = (ContentType) o1;
 		ContentType type2 = (ContentType) o2;
-		// first criteria: depth - the lower, the better
-		int depthCriteria = type1.getDepth() - type2.getDepth();
-		if (depthCriteria != 0)
-			return depthCriteria;
+		if (isAncestor(type1, type2)) {
+			// first criteria: depth - the lower, the better
+			int depthCriteria = type1.getDepth() - type2.getDepth();
+			if (depthCriteria != 0)
+				return depthCriteria;
+		}
 		// second criteria: priority - the higher, the better
 		int priorityCriteria = type1.getPriority() - type2.getPriority();
 		if (priorityCriteria != 0)
@@ -109,10 +127,12 @@ public final class ContentTypeCatalog {
 	private Comparator<IContentType> policySpecificIsBetter = (IContentType o1, IContentType o2) -> {
 		ContentType type1 = (ContentType) o1;
 		ContentType type2 = (ContentType) o2;
-		// first criteria: depth - the higher, the better
-		int depthCriteria = type1.getDepth() - type2.getDepth();
-		if (depthCriteria != 0)
-			return -depthCriteria;
+		if (isAncestor(type1, type2)) {
+			// first criteria: depth - the higher, the better
+			int depthCriteria = type1.getDepth() - type2.getDepth();
+			if (depthCriteria != 0)
+				return -depthCriteria;
+		}
 		// second criteria: priority - the higher, the better
 		int priorityCriteria = type1.getPriority() - type2.getPriority();
 		if (priorityCriteria != 0)