Fix prefix matching, change semantics to ContentKey.startsWith()

api/model/src/main/java/org/projectnessie/api/v2/doc/ApiDoc.java

@@ -123,16 +123,19 @@ public interface ApiDoc {
 
   String KEY_MIN_PARAMETER_DESCRIPTION =
       "The lower bound of the content key range to retrieve (inclusive). "
-          + "The content keys of all returned entries will be greater than or equal to the min-value.\n\n"
+          + "The content keys of all returned entries will be greater than or equal to the min-value. "
+          + "Content-keys are compared as a 'whole', unlike prefix-keys.\n\n"
           + KEY_ELEMENTS_DESCRIPTION;
 
   String KEY_MAX_PARAMETER_DESCRIPTION =
       "The upper bound of the content key range to retrieve (inclusive). "
-          + "The content keys of all returned entries will be less than or equal to the max-value.\n\n"
+          + "The content keys of all returned entries will be less than or equal to the max-value. "
+          + "Content-keys are compared as a 'whole', unlike prefix-keys.\n\n"
           + KEY_ELEMENTS_DESCRIPTION;
 
   String KEY_PREFIX_PARAMETER_DESCRIPTION =
-      "The content key prefix to retrieve (inclusive). A content key matches a given prefix, if it starts with the prefix value. "
+      "The content key prefix to retrieve (inclusive). "
+          + "A content key matches a given prefix, a content key's elements starts with all elements of the prefix-key. "
           + "It is not restricted to key-element boundaries.\n\n"
           + "Must not be combined with min/max-key parameters.\n\n"
           + KEY_ELEMENTS_DESCRIPTION;

versioned/storage/common/src/main/java/org/projectnessie/versioned/storage/common/indexes/StoreIndexImpl.java

@@ -430,7 +430,7 @@ public int estimatedSerializedSize() {
     ByteBuffer target = ByteBuffer.allocate(estimatedSerializedSize());
 
     // Serialized segment index version
-    target.put((byte) 1);
+    target.put((byte) 2);
 
     ByteBuffer previousKey = null;
 
@@ -479,7 +479,23 @@ private ByteBuffer serializeKey(
 
   static <V> StoreIndex<V> deserializeStoreIndex(ByteBuffer serialized, ElementSerializer<V> ser) {
     byte version = serialized.get();
-    checkArgument(version == 1, "Unsupported serialized representation of KeyIndexSegment");
+    boolean needSort = false;
+    switch (version) {
+      case 1:
+        // In PR #6743, introducing min/max/prefix-key arguments for "get entries" and "get diff",
+        // StoreKey.compareTo() was updated, which might lead to a different order in the elements
+        // list. The "current" version has been bumped, version=1 serialized indexes are sorted
+        // after they have been loaded.
+        needSort = true;
+        break;
+      case 2:
+        // No change in the serialization format compared to version 1, but element order might be
+        // different.
+        break;
+      default:
+        throw new IllegalArgumentException(
+            "Unsupported serialized representation of KeyIndexSegment");
+    }
 
     int posPre = serialized.position();
 
@@ -508,6 +524,10 @@ static <V> StoreIndex<V> deserializeStoreIndex(ByteBuffer serialized, ElementSer
       elements.add(indexElement(key, value));
     }
 
+    if (needSort) {
+      elements.sort(KEY_COMPARATOR);
+    }
+
     int len = serialized.position() - posPre;
     return new StoreIndexImpl<>(elements, len, ser, false);
   }

versioned/storage/common/src/main/java/org/projectnessie/versioned/storage/common/indexes/StoreKey.java

@@ -51,7 +51,70 @@ public String rawString() {
 
   @Override
   public int compareTo(StoreKey that) {
-    return key.compareTo(that.key);
+    StringBuilder blk1 = new StringBuilder();
+    StringBuilder blk2 = new StringBuilder();
+    int blkIdx1 = 0;
+    int blkIdx2 = 0;
+    int blkLen1 = key.length();
+    int blkLen2 = that.key.length();
+
+    StringBuilder part1 = new StringBuilder();
+    StringBuilder part2 = new StringBuilder();
+    int partIdx1;
+    int partIdx2;
+    int partLen1;
+    int partLen2;
+
+    while (true) {
+      boolean blkEof1 = blkIdx1 >= blkLen1;
+      boolean blkEof2 = blkIdx2 >= blkLen2;
+      if (blkEof1) {
+        return blkEof2 ? 0 : -1;
+      } else if (blkEof2) {
+        return 1;
+      }
+
+      blkIdx1 = nextBlock(key, (char) 0, blkIdx1, blkLen1, blk1);
+      blkIdx2 = nextBlock(that.key, (char) 0, blkIdx2, blkLen2, blk2);
+
+      // Inner parts
+
+      partIdx1 = partIdx2 = 0;
+      partLen1 = blk1.length();
+      partLen2 = blk2.length();
+      while (true) {
+        boolean partEof1 = partIdx1 >= partLen1;
+        boolean partEof2 = partIdx2 >= partLen2;
+        if (partEof1) {
+          if (partEof2) {
+            break;
+          }
+          return -1;
+        } else if (partEof2) {
+          return 1;
+        }
+
+        partIdx1 = nextBlock(blk1, (char) 1, partIdx1, partLen1, part1);
+        partIdx2 = nextBlock(blk2, (char) 1, partIdx2, partLen2, part2);
+
+        int cmp = part1.compareTo(part2);
+        if (cmp != 0) {
+          return cmp;
+        }
+      }
+    }
+  }
+
+  private static int nextBlock(CharSequence k, char sep, int i, int l, StringBuilder sb) {
+    sb.setLength(0);
+    for (; i < l; i++) {
+      char c = k.charAt(i);
+      if (c == sep) {
+        return i + 1;
+      }
+      sb.append(c);
+    }
+    return i;
   }
 
   @Override

versioned/storage/common/src/test/java/org/projectnessie/versioned/storage/common/indexes/TestStoreIndexImpl.java

@@ -103,7 +103,7 @@ public void keyIndexSegment() {
     StoreKey keyExC = key("e", "x", "C");
     StoreKey keyNotExist = key("does", "not", "exist");
 
-    String serializationFormatVersion = "01";
+    String serializationFormatVersion = "02";
 
     String serializedA =
         "61007800410000"
@@ -149,7 +149,7 @@ public void keyIndexSegment() {
     Function<StoreIndex<ObjId>, StoreIndex<ObjId>> reSerialize =
         seg -> deserializeStoreIndex(seg.serialize(), OBJ_ID_SERIALIZER);
 
-    soft.assertThat(asHex(segment.serialize())).isEqualTo("01");
+    soft.assertThat(asHex(segment.serialize())).isEqualTo(serializationFormatVersion);
     soft.assertThat(reSerialize.apply(segment)).isEqualTo(segment);
     soft.assertThat(segment.asKeyList()).isEmpty();
     soft.assertThat(segment.elementCount()).isEqualTo(0);

versioned/storage/common/src/test/java/org/projectnessie/versioned/storage/common/indexes/TestStoreKey.java

@@ -19,7 +19,6 @@
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static java.util.Arrays.asList;
 import static java.util.Collections.singletonList;
-import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.assertj.core.api.InstanceOfAssertFactories.INTEGER;
 import static org.junit.jupiter.params.provider.Arguments.arguments;
@@ -96,18 +95,26 @@ void keyElementsGood(int elements) {
   @ParameterizedTest
   @MethodSource("compare")
   void compare(StoreKey a, StoreKey b, int expectedCompare) {
-    assertThat(a)
+    soft.assertThat(a)
         .describedAs("Compare of %s to %s expect %d", a, b, expectedCompare)
         .extracting(k -> Integer.signum(k.compareTo(b)))
         .asInstanceOf(INTEGER)
         .isEqualTo(expectedCompare);
+    soft.assertThat(a)
+        .describedAs("Reverse compare of %s to %s expect %d", a, b, expectedCompare)
+        .extracting(k -> Integer.signum(b.compareTo(k)))
+        .asInstanceOf(INTEGER)
+        .isEqualTo(-expectedCompare);
   }
 
   static Stream<Arguments> compare() {
     return Stream.of(
+        arguments(key("M", "k2\u0001k3", "C"), key("M", "k2\u0001πa", "C"), -1), // UNICODE CHAR
         arguments(key("a"), key("a"), 0),
         arguments(key("a"), key("a", "b"), -1),
         arguments(key("a"), key("a", "a"), -1),
+        arguments(key("a"), key("aπ", "a"), -1), // UNICODE CHAR
+        arguments(key("aa"), key("aπ"), -1), // UNICODE CHAR
         arguments(key("a", "a"), key("a"), 1),
         arguments(key("a"), key("abcdef"), -1),
         arguments(key("abcdef"), key("a"), 1),
@@ -116,10 +123,8 @@ static Stream<Arguments> compare() {
         arguments(key("0"), key("0123", "123", "123"), -1),
         arguments(key("abcdef", "abc", "abc"), key("a"), 1),
         arguments(key("key.0"), key("key.1"), -1),
-        arguments(key("key.1"), key("key.0"), 1),
         arguments(key("key.42"), key("key.42"), 0),
         arguments(key("key", "0"), key("key", "1"), -1),
-        arguments(key("key", "1"), key("key", "0"), 1),
         arguments(key("key", "42"), key("key", "42"), 0));
   }
 

versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/KeyRanges.java

@@ -20,8 +20,6 @@
 import static org.projectnessie.versioned.storage.common.logic.PagingToken.fromString;
 import static org.projectnessie.versioned.storage.versionstore.TypeMapping.keyToStoreKeyMax;
 import static org.projectnessie.versioned.storage.versionstore.TypeMapping.keyToStoreKeyMin;
-import static org.projectnessie.versioned.storage.versionstore.TypeMapping.keyToStoreKeyPrefixEnd;
-import static org.projectnessie.versioned.storage.versionstore.TypeMapping.keyToStoreKeyPrefixMin;
 
 import javax.annotation.Nullable;
 import org.immutables.value.Value;
@@ -63,10 +61,7 @@ StoreKey beginStoreKey() {
       return keyFromString(fromString(paging).token().toStringUtf8());
     }
     ContentKey prefix = prefixKey();
-    if (prefix != null) {
-      return keyToStoreKeyPrefixMin(prefix);
-    }
-    ContentKey min = minKey();
+    ContentKey min = (prefix != null) ? prefix : minKey();
     return min != null ? keyToStoreKeyMin(min) : null;
   }
 
@@ -78,11 +73,8 @@ PagingToken pagingTokenObj() {
 
   @Value.NonAttribute
   StoreKey endStoreKey() {
-    ContentKey prefix = prefixKey();
     ContentKey max = maxKey();
-    return prefix != null
-        ? keyToStoreKeyPrefixEnd(prefix)
-        : (max != null ? keyToStoreKeyMax(max) : null);
+    return max != null ? keyToStoreKeyMax(max) : null;
   }
 
   @Value.Check

versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/TypeMapping.java

@@ -154,23 +154,6 @@ public static StoreKey keyToStoreKeyMax(@Nonnull @jakarta.annotation.Nonnull Con
     return StoreKey.keyFromString(sb.toString());
   }
 
-  @Nonnull
-  @jakarta.annotation.Nonnull
-  public static StoreKey keyToStoreKeyPrefixMin(
-      @Nonnull @jakarta.annotation.Nonnull ContentKey key) {
-    StringBuilder sb = keyToStoreKeyPrepare(key);
-    return StoreKey.keyFromString(sb.toString());
-  }
-
-  @Nonnull
-  @jakarta.annotation.Nonnull
-  public static StoreKey keyToStoreKeyPrefixEnd(
-      @Nonnull @jakarta.annotation.Nonnull ContentKey key) {
-    StringBuilder sb = keyToStoreKeyPrepare(key);
-    sb.append((char) 255);
-    return StoreKey.keyFromString(sb.toString());
-  }
-
   @Nonnull
   @jakarta.annotation.Nonnull
   private static StringBuilder keyToStoreKeyPrepare(

versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/VersionStoreImpl.java

@@ -506,7 +506,10 @@ public PaginationIterator<KeyEntry> getKeys(
         index.iterator(keyRanges.beginStoreKey(), keyRanges.endStoreKey(), false);
     ContentMapping contentMapping = new ContentMapping(persist);
 
-    Predicate<ContentKey> keyPred = contentKeyPredicate != null ? contentKeyPredicate : x -> true;
+    Predicate<ContentKey> prefixPred =
+        prefixKey != null ? key -> key.startsWith(prefixKey) : x -> true;
+    Predicate<ContentKey> keyPred =
+        prefixPred.and(contentKeyPredicate != null ? contentKeyPredicate : x -> true);
 
     return new FilteringPaginationIterator<StoreIndexElement<CommitOp>, KeyEntry>(
         result,
@@ -779,7 +782,10 @@ public PaginationIterator<Diff> getDiffs(
 
     ContentMapping contentMapping = new ContentMapping(persist);
 
-    Predicate<ContentKey> keyPred = contentKeyPredicate != null ? contentKeyPredicate : x -> true;
+    Predicate<ContentKey> prefixPred =
+        prefixKey != null ? key -> key.startsWith(prefixKey) : x -> true;
+    Predicate<ContentKey> keyPred =
+        prefixPred.and(contentKeyPredicate != null ? contentKeyPredicate : x -> true);
 
     return new FilteringPaginationIterator<DiffEntry, Diff>(
         diffIter,

versioned/storage/store/src/test/java/org/projectnessie/versioned/storage/versionstore/TestKeyRanges.java

@@ -21,7 +21,6 @@
 import static org.projectnessie.versioned.storage.versionstore.KeyRanges.keyRanges;
 import static org.projectnessie.versioned.storage.versionstore.TypeMapping.MAIN_UNIVERSE;
 import static org.projectnessie.versioned.storage.versionstore.TypeMapping.keyToStoreKey;
-import static org.projectnessie.versioned.storage.versionstore.TypeMapping.keyToStoreKeyPrefixMin;
 import static org.projectnessie.versioned.storage.versionstore.TypeMapping.keyToStoreKeyVariant;
 
 import org.assertj.core.api.SoftAssertions;
@@ -78,6 +77,8 @@ public void max() {
     soft.assertThat(keyRange.endStoreKey()).isLessThan(keyToStoreKey(ContentKey.of("bar", "baz")));
     soft.assertThat(keyRange.endStoreKey()).isLessThan(keyToStoreKey(ContentKey.of("bar", "0")));
     soft.assertThat(keyRange.endStoreKey()).isLessThan(keyToStoreKey(ContentKey.of("barbaz")));
+    soft.assertThat(keyRange.endStoreKey())
+        .isLessThan(keyToStoreKey(ContentKey.of("barπ"))); //  UNICODE CHAR
     soft.assertThat(keyRange.endStoreKey()).isLessThan(keyToStoreKey(ContentKey.of("bar0")));
 
     soft.assertThat(keyRange.endStoreKey()).isLessThan(keyToStoreKey(ContentKey.of("bar0")));
@@ -88,28 +89,4 @@ public void max() {
     soft.assertThat(keyRange.endStoreKey())
         .isLessThan(keyFromString(storeKeyRawPrefix + (char) 1 + "baz"));
   }
-
-  @Test
-  public void prefix() {
-    ContentKey bar = ContentKey.of("bar");
-    KeyRanges keyRange = keyRanges(null, null, null, bar);
-
-    soft.assertThat(keyRange.beginStoreKey()).isEqualTo(keyToStoreKeyPrefixMin(bar));
-
-    soft.assertThat(keyRange.endStoreKey()).isGreaterThan(keyToStoreKey(bar));
-    soft.assertThat(keyRange.endStoreKey()).isGreaterThan(keyToStoreKeyVariant(bar, "Z"));
-    soft.assertThat(keyRange.endStoreKey()).isGreaterThan(keyToStoreKeyVariant(bar, "A"));
-
-    soft.assertThat(keyRange.endStoreKey())
-        .isGreaterThan(keyToStoreKey(ContentKey.of("bar", "baz")));
-    soft.assertThat(keyRange.endStoreKey()).isGreaterThan(keyToStoreKey(ContentKey.of("bar", "0")));
-    soft.assertThat(keyRange.endStoreKey()).isGreaterThan(keyToStoreKey(ContentKey.of("barbaz")));
-    soft.assertThat(keyRange.endStoreKey()).isGreaterThan(keyToStoreKey(ContentKey.of("bar0")));
-
-    String storeKeyRawPrefix = MAIN_UNIVERSE + (char) 0 + "bar";
-
-    soft.assertThat(keyRange.endStoreKey()).isGreaterThan(keyFromString(storeKeyRawPrefix));
-    soft.assertThat(keyRange.endStoreKey())
-        .isGreaterThan(keyFromString(storeKeyRawPrefix + (char) 1 + "baz"));
-  }
 }

versioned/tests/src/main/java/org/projectnessie/versioned/tests/AbstractDiff.java

@@ -135,11 +135,11 @@ protected void checkDiff() throws VersionStoreException {
 
       soft.assertThat(diffAsList(initial, secondCommit, null, null, ContentKey.of("k1"), null))
           .extracting(Diff::getKey)
-          .containsExactlyInAnyOrder(k1, k1a);
+          .containsExactlyInAnyOrder(k1);
 
       soft.assertThat(diffAsList(initial, secondCommit, null, null, ContentKey.of("k"), null))
           .extracting(Diff::getKey)
-          .containsExactlyInAnyOrder(k1, k2, k3, k1a);
+          .isEmpty();
 
       soft.assertThat(diffAsList(initial, secondCommit, null, null, ContentKey.of("x"), null))
           .isEmpty();