From cd9b0a770b327b0fb8409c7a9e24426948048f4d Mon Sep 17 00:00:00 2001 From: Rahul Yadav Date: Mon, 9 Mar 2026 12:17:06 +0530 Subject: [PATCH 1/2] chore: normalize query param keys to lowercase in KeyRecipe --- .../cloud/spanner/spi/v1/KeyRecipe.java | 11 ++-- .../cloud/spanner/spi/v1/KeyRecipeTest.java | 54 +++++++++++++++++++ 2 files changed, 62 insertions(+), 3 deletions(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/KeyRecipe.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/KeyRecipe.java index 876a68583e2..eb99d0984e1 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/KeyRecipe.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/KeyRecipe.java @@ -30,7 +30,10 @@ import java.time.format.ResolverStyle; import java.util.ArrayList; import java.util.Base64; +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 +783,12 @@ public TargetRange keySetToTargetRange(KeySet in) { } public TargetRange queryParamsToTargetRange(Struct in) { + final Map lowercaseFields = new HashMap<>(); + for (Map.Entry entry : in.getFieldsMap().entrySet()) { + lowercaseFields.put(entry.getKey().toLowerCase(Locale.ROOT), entry.getValue()); + } return encodeKeyInternal( - (index, identifier) -> { - return in.getFieldsMap().get(identifier); - }, + (index, identifier) -> lowercaseFields.get(identifier.toLowerCase(Locale.ROOT)), KeyType.FULL_KEY); } diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/KeyRecipeTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/KeyRecipeTest.java index 9f6387c5c4c..98e5799dc25 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/KeyRecipeTest.java +++ b/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,59 @@ 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()); + } + 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(); From cf4502ed91dfabb6a183d8097a4d968edc99d82a Mon Sep 17 00:00:00 2001 From: Rahul Yadav Date: Mon, 9 Mar 2026 15:27:48 +0530 Subject: [PATCH 2/2] add tests --- .../cloud/spanner/spi/v1/KeyRangeCache.java | 47 +++++++++++++++++-- .../cloud/spanner/spi/v1/KeyRecipe.java | 21 +++++++-- .../cloud/spanner/spi/v1/KeyRecipeTest.java | 28 +++++++++++ 3 files changed, 89 insertions(+), 7 deletions(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/KeyRangeCache.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/KeyRangeCache.java index a648594a657..bdbd495aa58 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/KeyRangeCache.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/KeyRangeCache.java @@ -559,21 +559,60 @@ 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; @@ -581,7 +620,7 @@ && leader().distance <= MAX_LOCAL_REPLICA_DISTANCE if (tablet.shouldSkip(hintBuilder)) { continue; } - return tablet.pick(hintBuilder); + return tablet; } return null; } diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/KeyRecipe.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/KeyRecipe.java index eb99d0984e1..1e15b69bf97 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/KeyRecipe.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/KeyRecipe.java @@ -30,6 +30,7 @@ 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; @@ -783,9 +784,23 @@ public TargetRange keySetToTargetRange(KeySet in) { } public TargetRange queryParamsToTargetRange(Struct in) { - final Map lowercaseFields = new HashMap<>(); - for (Map.Entry entry : in.getFieldsMap().entrySet()) { - lowercaseFields.put(entry.getKey().toLowerCase(Locale.ROOT), entry.getValue()); + // 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 fieldNames = new ArrayList<>(in.getFieldsMap().keySet()); + Collections.sort(fieldNames); + final Map lowercaseFields = new HashMap<>(fieldNames.size()); + for (String fieldName : fieldNames) { + lowercaseFields.put(fieldName.toLowerCase(Locale.ROOT), in.getFieldsMap().get(fieldName)); } return encodeKeyInternal( (index, identifier) -> lowercaseFields.get(identifier.toLowerCase(Locale.ROOT)), diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/KeyRecipeTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/KeyRecipeTest.java index 98e5799dc25..3e946f10dc4 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/KeyRecipeTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/KeyRecipeTest.java @@ -130,6 +130,34 @@ public void queryParamsCaseInsensitiveDuplicateUsesLastValue() throws Exception 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();