martin-g commented on code in PR #548:
URL: https://github.com/apache/avro-rs/pull/548#discussion_r3332485313
##########
avro/src/schema/parser.rs:
##########
@@ -795,29 +799,36 @@ impl Parser {
&self,
aliases: Option<Vec<String>>,
namespace: NamespaceRef,
- ) -> Aliases {
- aliases.map(|aliases| {
- aliases
- .iter()
- .map(|alias| {
- if alias.find('.').is_none() {
- match namespace {
- Some(ns) => format!("{ns}.{alias}"),
- None => alias.clone(),
- }
- } else {
- alias.clone()
- }
- })
- .map(|alias| Alias::new(alias.as_str()).unwrap())
- .collect()
- })
+ ) -> AvroResult<Aliases> {
+ aliases
+ .map(|aliases| {
+ aliases
+ .iter()
+ .map(|alias| {
+ let qualified = if alias.find('.').is_none() {
+ match namespace {
+ Some(ns) => format!("{ns}.{alias}"),
+ None => alias.clone(),
+ }
+ } else {
+ alias.clone()
+ };
+ // An alias that is not a valid Avro name is a schema
+ // error — propagate it instead of unwrapping (which
+ // would panic on attacker-controlled schema JSON).
+ Alias::new(qualified.as_str())
+ })
+ .collect::<AvroResult<Vec<_>>>()
+ })
+ .transpose()
}
fn get_schema_type_name(&self, name: Name, value: &Value) -> Name {
match value.get("type") {
Some(Value::Object(complex_type)) => match complex_type.name() {
- Some(name) => Name::new(name).unwrap(),
+ // Fall back to the enclosing name if the nested `type` name is
+ // not a valid Avro name, rather than panicking on `unwrap()`.
+ Some(type_name) => Name::new(type_name).unwrap_or(name),
Review Comment:
I am not sure about this one.
Maybe we should change the signature to `Result<Name>` ?!
Silently ignoring the nested type name feels wrong.
##########
avro/src/schema/parser.rs:
##########
@@ -795,29 +799,36 @@ impl Parser {
&self,
aliases: Option<Vec<String>>,
namespace: NamespaceRef,
- ) -> Aliases {
- aliases.map(|aliases| {
- aliases
- .iter()
- .map(|alias| {
- if alias.find('.').is_none() {
- match namespace {
- Some(ns) => format!("{ns}.{alias}"),
- None => alias.clone(),
- }
- } else {
- alias.clone()
- }
- })
- .map(|alias| Alias::new(alias.as_str()).unwrap())
- .collect()
- })
+ ) -> AvroResult<Aliases> {
+ aliases
+ .map(|aliases| {
+ aliases
+ .iter()
+ .map(|alias| {
+ let qualified = if alias.find('.').is_none() {
+ match namespace {
+ Some(ns) => format!("{ns}.{alias}"),
+ None => alias.clone(),
+ }
+ } else {
+ alias.clone()
+ };
+ // An alias that is not a valid Avro name is a schema
+ // error — propagate it instead of unwrapping (which
+ // would panic on attacker-controlled schema JSON).
+ Alias::new(qualified.as_str())
Review Comment:
Let's improve this a bit further:
1) use `contains()`
2) pass &str to avoid some clones
```suggestion
// An alias that is not a valid Avro name is a schema
// error — propagate it instead of unwrapping (which
// would panic on attacker-controlled schema JSON).
if alias.contains('.') {
Alias::new(alias.as_str())
} else {
match namespace {
Some(ns) =>
Alias::new(&format!("{ns}.{alias}")),
None => Alias::new(alias.as_str()),
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
