test(spanner): add location router finder golden tests and centralize textproto test utils - #6482
test(spanner): add location router finder golden tests and centralize textproto test utils#6482olavloite wants to merge 2 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request consolidates and refactors the textproto parsing utilities for Spanner golden tests into a shared helper module (textproto_test_utils.rs), removing duplicate parsing code from key_range_cache/golden_tests.rs and key_recipe/golden_tests.rs. It also introduces a new set of golden conformance tests for the location router (location_router/golden_tests.rs). The review feedback suggests improving null value handling in the newly introduced parsing helpers: specifically, adding support for null_value: NULL_VALUE in parse_constant_value and allowing serde_json::Value::Null in query_params_to_target_range to prevent premature termination of key encoding.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6482 +/- ##
=========================================
Coverage 96.37% 96.37%
=========================================
Files 297 297
Lines 84017 85718 +1701
=========================================
+ Hits 80971 82612 +1641
- Misses 3046 3106 +60 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
… textproto test utils - Add golden conformance test suite for `finder_test.textproto` (36 test cases, 203 request events) validating query key encoding, range cache lookups, server selection, and routing hints. - Centralize all golden test fixtures and parsers (`recipe_test`, `cache_test`, `finder_test`) into `textproto_test_utils.rs`. - Add `unescape_bytes` support for hex `\xHH` sequences and standard C escapes. - Introduce reusable `skip_block` helper and decouple nested list value parsing. - Tighten golden routing hint assertions with strict struct equality, verifying `skipped_tablet_uid` and per-event unhealthy server isolation matching Java and Go reference implementations.
b98507a to
bc7b235
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request consolidates textproto parsing logic from multiple golden test files into a shared utility module, textproto_test_utils.rs, while adding support for hex escape sequences and comprehensive unit tests. It also introduces a new golden conformance test suite for the location router. The review feedback highlights opportunities to optimize performance by avoiding unnecessary vector clones in location_router/golden_tests.rs and suggests enhancing parse_constant_value in textproto_test_utils.rs to support parsing float values.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request consolidates and refactors the textproto parsing logic for Spanner golden tests into a shared utility module (textproto_test_utils.rs), eliminating duplicated parsing code across key_range_cache and key_recipe tests. It also introduces a new suite of golden conformance tests for the location_router finder. Feedback on the new tests highlights several robustness issues: encode_single_value_part should parse string values into their correct types (like Bool or Float64) based on the part's type code to prevent encoding failures, and multiple instances of encode_into have ignored Result values which violates the style guide rule against swallowing errors.
| fn encode_single_value_part( | ||
| ss_key: &mut Vec<u8>, | ||
| part: &Part, | ||
| value: &str, | ||
| is_successor: bool, | ||
| ) -> bool { | ||
| let mut buf = Vec::new(); | ||
| let val = Value::from(value); | ||
| if val.encode_into(&mut buf, part).is_err() { | ||
| return false; | ||
| } | ||
| if is_successor { | ||
| let succ = ssformat::make_prefix_successor(&buf); | ||
| ss_key.extend_from_slice(&succ); | ||
| } else { | ||
| ss_key.extend_from_slice(&buf); | ||
| } | ||
| true | ||
| } |
There was a problem hiding this comment.
In encode_single_value_part, converting all string values to Value using Value::from(value) creates a String kind value. However, Spanner's key encoders for BOOL and FLOAT64 expect Bool and Float64 kinds respectively, and will fail to encode a String kind value. We should parse the string value into the correct type based on the part's type code to ensure robust key encoding for all supported types.
fn encode_single_value_part(
ss_key: &mut Vec<u8>,
part: &Part,
value: &str,
is_successor: bool,
) -> bool {
let mut buf = Vec::new();
let val = if let Some(t) = part.r#type.as_ref() {
match t.code {
crate::model::TypeCode::Bool => value.parse::<bool>().map(Value::from).unwrap_or_else(|_| Value::from(value)),
crate::model::TypeCode::Float64 => value.parse::<f64>().map(Value::from).unwrap_or_else(|_| Value::from(value)),
_ => Value::from(value),
}
} else {
Value::from(value)
};
if val.encode_into(&mut buf, part).is_err() {
return false;
}
if is_successor {
let succ = ssformat::make_prefix_successor(&buf);
ss_key.extend_from_slice(&succ);
} else {
ss_key.extend_from_slice(&buf);
}
true
}| } else if let Some(constant_val) = part.value() { | ||
| let val = json_to_spanner_value(constant_val.as_ref()); | ||
| let _ = val.encode_into(&mut ss_key, part); | ||
| parts_count += 1; |
There was a problem hiding this comment.
In encode_key_internal, the result of val.encode_into(&mut ss_key, part) is ignored with let _ =. If encoding a constant value fails (e.g., due to type mismatch), the function will silently ignore the error, increment parts_count, and continue, which can produce a corrupt key. We should check the result of encode_into and handle failure by setting state_end_of_keys = true and breaking.
| } else if let Some(constant_val) = part.value() { | |
| let val = json_to_spanner_value(constant_val.as_ref()); | |
| let _ = val.encode_into(&mut ss_key, part); | |
| parts_count += 1; | |
| } else if let Some(constant_val) = part.value() { | |
| let val = json_to_spanner_value(constant_val.as_ref()); | |
| if val.encode_into(&mut ss_key, part).is_ok() { | |
| parts_count += 1; | |
| } else { | |
| state_end_of_keys = true; | |
| break; | |
| } |
References
- Never swallow errors or ignore Result types. Fail loudly and explicitly when appropriate. (link)
| } else if let Some(constant_val) = part.value() { | ||
| let val = json_to_spanner_value(constant_val.as_ref()); | ||
| let _ = val.encode_into(&mut ss_key, part); | ||
| parts_count += 1; |
There was a problem hiding this comment.
In query_params_to_target_range, the result of val.encode_into(&mut ss_key, part) is ignored with let _ =. If encoding a constant value fails, the function will silently ignore the error, increment parts_count, and continue. We should check the result of encode_into and handle failure by setting state_end_of_keys = true and breaking.
} else if let Some(constant_val) = part.value() {
let val = json_to_spanner_value(constant_val.as_ref());
if val.encode_into(&mut ss_key, part).is_ok() {
parts_count += 1;
} else {
state_end_of_keys = true;
break;
}References
- Never swallow errors or ignore Result types. Fail loudly and explicitly when appropriate. (link)
| let val = json_to_spanner_value(json_val); | ||
| let _ = val.encode_into(&mut ss_key, part); | ||
| parts_count += 1; |
There was a problem hiding this comment.
In query_params_to_target_range, the result of val.encode_into(&mut ss_key, part) is ignored with let _ =. If encoding a query parameter value fails, the function will silently ignore the error, increment parts_count, and continue. We should check the result of encode_into and handle failure by setting state_end_of_keys = true and breaking.
let val = json_to_spanner_value(json_val);
if val.encode_into(&mut ss_key, part).is_ok() {
parts_count += 1;
} else {
state_end_of_keys = true;
break;
}References
- Never swallow errors or ignore Result types. Fail loudly and explicitly when appropriate. (link)
1 participant
finder_test.textproto(36 test cases, 203 request events) validating query key encoding, range cache lookups, server selection, and routing hints.recipe_test,cache_test,finder_test) intotextproto_test_utils.rs.unescape_bytessupport for hex\xHHsequences and standard C escapes.skip_blockhelper and decouple nested list value parsing.skipped_tablet_uidand per-event unhealthy server isolation matching Java and Go reference implementations.