chore: normalize query param keys to lowercase in KeyRecipe and fix lock contention in KeyRangeCache (#4380)

* chore: normalize query param keys to lowercase in KeyRecipe

* add tests

Author: rahul2393

google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/KeyRangeCache.java

@@ -559,29 +559,68 @@ synchronized void update(Group groupIn) {
       tablets = newTablets;
     }
 
-    synchronized ChannelEndpoint fillRoutingHint(
+    ChannelEndpoint fillRoutingHint(
         boolean preferLeader,
         DirectedReadOptions directedReadOptions,
         RoutingHint.Builder hintBuilder) {
       boolean hasDirectedReadOptions =
           directedReadOptions.getReplicasCase()
               != DirectedReadOptions.ReplicasCase.REPLICAS_NOT_SET;
+
+      // Fast path: pick a tablet while holding the lock. If the endpoint is already
+      // cached on the tablet, return it immediately without releasing the lock.
+      // If the endpoint needs to be created (blocking network dial), release the
+      // lock first so other threads are not blocked during channel creation.
+      CachedTablet selected;
+      synchronized (this) {
+        selected =
+            selectTabletLocked(
+                preferLeader, hasDirectedReadOptions, hintBuilder, directedReadOptions);
+        if (selected == null) {
+          return null;
+        }
+        if (selected.endpoint != null || selected.serverAddress.isEmpty()) {
+          return selected.pick(hintBuilder);
+        }
+        // Slow path: endpoint not yet created. Capture the address and release the
+        // lock before calling endpointCache.get(), which may block on network dial.
+        hintBuilder.setTabletUid(selected.tabletUid);
+      }
+
+      String serverAddress = selected.serverAddress;
+      ChannelEndpoint endpoint = endpointCache.get(serverAddress);
+
+      synchronized (this) {
+        // Only update if the tablet's address hasn't changed since we released the lock.
+        if (selected.endpoint == null && selected.serverAddress.equals(serverAddress)) {
+          selected.endpoint = endpoint;
+        }
+        // Re-set tabletUid with the latest value in case update() ran concurrently.
+        hintBuilder.setTabletUid(selected.tabletUid);
+        return selected.endpoint;
+      }
+    }
+
+    private CachedTablet selectTabletLocked(
+        boolean preferLeader,
+        boolean hasDirectedReadOptions,
+        RoutingHint.Builder hintBuilder,
+        DirectedReadOptions directedReadOptions) {
       if (preferLeader
           && !hasDirectedReadOptions
           && hasLeader()
           && leader().distance <= MAX_LOCAL_REPLICA_DISTANCE
           && !leader().shouldSkip(hintBuilder)) {
-        return leader().pick(hintBuilder);
+        return leader();
       }
-
       for (CachedTablet tablet : tablets) {
         if (!tablet.matches(directedReadOptions)) {
           continue;
         }
         if (tablet.shouldSkip(hintBuilder)) {
           continue;
         }
-        return tablet.pick(hintBuilder);
+        return tablet;
       }
       return null;
     }

google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/KeyRecipe.java

@@ -30,7 +30,11 @@
 import java.time.format.ResolverStyle;
 import java.util.ArrayList;
 import java.util.Base64;
+import java.util.Collections;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Locale;
+import java.util.Map;
 import java.util.concurrent.ThreadLocalRandom;
 import java.util.function.BiFunction;
 import java.util.stream.Collectors;
@@ -780,10 +784,26 @@ public TargetRange keySetToTargetRange(KeySet in) {
   }
 
   public TargetRange queryParamsToTargetRange(Struct in) {
+    // toLowerCase(Locale.ROOT) is safe for query parameter names, even for non-ASCII
+    // characters such as the Turkish upper-case İ (U+0130). Query parameter names cannot
+    // be quoted in Spanner SQL (the @paramName syntax imposes an unquoted identifier
+    // grammar), so both the identifier sent by the server in the KeyRecipe and the
+    // parameter name bound by the user must follow the same syntax rules. Applying the
+    // same Locale.ROOT case-folding to both sides guarantees a consistent match.
+    // If the server were to normalize identifiers differently, the only consequence is
+    // a routing miss and graceful fallback to the default endpoint — not a query failure.
+    //
+    // Sort field names before inserting into the map so that when two param names
+    // collide after case-folding (e.g. "Id" vs "ID") the winner is deterministic,
+    // matching the Go implementation.
+    List<String> fieldNames = new ArrayList<>(in.getFieldsMap().keySet());
+    Collections.sort(fieldNames);
+    final Map<String, Value> lowercaseFields = new HashMap<>(fieldNames.size());
+    for (String fieldName : fieldNames) {
+      lowercaseFields.put(fieldName.toLowerCase(Locale.ROOT), in.getFieldsMap().get(fieldName));
+    }
     return encodeKeyInternal(
-        (index, identifier) -> {
-          return in.getFieldsMap().get(identifier);
-        },
+        (index, identifier) -> lowercaseFields.get(identifier.toLowerCase(Locale.ROOT)),
         KeyType.FULL_KEY);
   }
 

google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/KeyRecipeTest.java

@@ -17,6 +17,7 @@
 package com.google.cloud.spanner.spi.v1;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
 import com.google.protobuf.ByteString;
@@ -76,6 +77,87 @@ public void queryParamsUsesConstantValue() throws Exception {
     assertTrue(target.limit.isEmpty());
   }
 
+  @Test
+  public void queryParamsCaseInsensitiveFallback() throws Exception {
+    com.google.spanner.v1.KeyRecipe recipeProto =
+        createRecipe(
+            "part { tag: 1 }\n"
+                + "part {\n"
+                + "  order: ASCENDING\n"
+                + "  null_order: NULLS_FIRST\n"
+                + "  type { code: STRING }\n"
+                + "  identifier: \"id\"\n"
+                + "}\n");
+
+    Struct params =
+        parseStruct(
+            "fields {\n" + "  key: \"Id\"\n" + "  value { string_value: \"foo\" }\n" + "}\n");
+
+    KeyRecipe recipe = KeyRecipe.create(recipeProto);
+    TargetRange target = recipe.queryParamsToTargetRange(params);
+    assertEquals(expectedKey("foo"), target.start);
+    assertTrue(target.limit.isEmpty());
+  }
+
+  @Test
+  public void queryParamsCaseInsensitiveDuplicateUsesLastValue() throws Exception {
+    com.google.spanner.v1.KeyRecipe recipeProto =
+        createRecipe(
+            "part { tag: 1 }\n"
+                + "part {\n"
+                + "  order: ASCENDING\n"
+                + "  null_order: NULLS_FIRST\n"
+                + "  type { code: STRING }\n"
+                + "  identifier: \"ID\"\n"
+                + "}\n");
+
+    // Both "Id" and "id" normalize to "id"; the last one ("id"→"bar") wins.
+    Struct params =
+        parseStruct(
+            "fields {\n"
+                + "  key: \"Id\"\n"
+                + "  value { string_value: \"foo\" }\n"
+                + "}\n"
+                + "fields {\n"
+                + "  key: \"id\"\n"
+                + "  value { string_value: \"bar\" }\n"
+                + "}\n");
+
+    KeyRecipe recipe = KeyRecipe.create(recipeProto);
+    TargetRange target = recipe.queryParamsToTargetRange(params);
+    assertEquals(expectedKey("bar"), target.start);
+    assertFalse(target.approximate);
+    assertTrue(target.limit.isEmpty());
+  }
+
+  @Test
+  public void queryParamsCaseInsensitiveSafeForTurkishDotI() throws Exception {
+    // Turkish upper-case İ (U+0130) lower-cases to two characters under Locale.ROOT
+    // (i + combining dot above), so "SİCİL".length() != "SİCİL".toLowerCase(ROOT).length()
+    // and "SİCİL".equalsIgnoreCase("SİCİL".toLowerCase(ROOT)) is false.
+    // This is still safe because both the recipe identifier (server-sent) and the user's
+    // bound parameter name go through the same Locale.ROOT lower-casing before the
+    // HashMap lookup, so they produce the same string on both sides and the match succeeds.
+    com.google.spanner.v1.KeyRecipe recipeProto =
+        createRecipe(
+            "part { tag: 1 }\n"
+                + "part {\n"
+                + "  order: ASCENDING\n"
+                + "  null_order: NULLS_FIRST\n"
+                + "  type { code: STRING }\n"
+                + "  identifier: \"SİCİL\"\n"
+                + "}\n");
+
+    Struct params =
+        parseStruct(
+            "fields {\n" + "  key: \"SİCİL\"\n" + "  value { string_value: \"test\" }\n" + "}\n");
+
+    KeyRecipe recipe = KeyRecipe.create(recipeProto);
+    TargetRange target = recipe.queryParamsToTargetRange(params);
+    assertEquals(expectedKey("test"), target.start);
+    assertTrue(target.limit.isEmpty());
+  }
+
   private static com.google.spanner.v1.KeyRecipe createRecipe(String text)
       throws TextFormat.ParseException {
     com.google.spanner.v1.KeyRecipe.Builder builder = com.google.spanner.v1.KeyRecipe.newBuilder();