Schema generates conflicting names for enum variants/across crates #92
Comments
|
Good catch! Thanks for reporting it, but I believe borsh schema issues won't be addressed at the moment as it is not used in production and was just an experiment without a clear specification |
|
what is possible outline of fix for this? I have clash for same named struct in different parts of my big account struct. |
|
I see next:
So there are some users, may be a lot if search on GH. cc @frol |
|
Did quick tests on TLDRFor same named items schemars generates second declaration/definition under +1 suffixed name. Long:See
mod a {
use borsh::{de, BorshDeserialize, BorshSchema, BorshSerialize};
#[derive(Default, Debug, BorshSerialize, BorshSchema, arbitrary::Arbitrary, schemars::JsonSchema, serde::Serialize)]
pub struct A {
a: u8,
}
#[derive(Default, Debug, BorshSerialize, BorshSchema, arbitrary::Arbitrary, schemars::JsonSchema, serde::Serialize)]
pub struct B {
a: A,
}
}
mod b {
use borsh::{de, BorshDeserialize, BorshSchema, BorshSerialize};
#[derive(Default, Debug, BorshSerialize, BorshSchema, arbitrary::Arbitrary, schemars::JsonSchema, serde::Serialize)]
pub struct A {
a: u8,
}
#[derive(Default, Debug, BorshSerialize, BorshSchema, arbitrary::Arbitrary, schemars::JsonSchema, serde::Serialize)]
pub struct C {
a: A,
}
}
use a::B;
use b::C;
#[derive(Default, Debug, BorshSerialize, BorshSchema, arbitrary::Arbitrary, schemars::JsonSchema, serde::Serialize)]
struct D {
b: B,
c: C,
}
#[test]
fn serde_name_conflict() {
let schema = schemars::gen::SchemaGenerator::default()
.into_root_schema_for::<D>();
panic!("{:?}", schema);
} |
|
@frol @itegulov @miraclx @austinabell would PR which does exact same bump of number suffix in declaration accepted as fix as in schemars? solutionSo for type possible improvement
alternativePrefix definition with In case Alternative seems complicated. Not sure if one can get full Rust compile time info about modules in proc macro. |
|
I don’t see a universal solution to avoid naming collisions when names are autogenerated. The most flexible solution is to introduce macro attribute to control/override the naming. @dzmitry-lahoda Would you like to contribute the solution to allow naming customization? |
|
@dzmitry-lahoda there's some solution for simple cases without deep nesting of conflicting types diff --git a/borsh-derive/src/internals/attributes/field/schema.rs b/borsh-derive/src/internals/attributes/field/schema.rs
index 01353bf1..af632f93 100644
--- a/borsh-derive/src/internals/attributes/field/schema.rs
+++ b/borsh-derive/src/internals/attributes/field/schema.rs
@@ -24,31 +24,31 @@ pub static SCHEMA_FIELD_PARSE_MAP: Lazy<BTreeMap<Symbol, Box<ParseFn>>> = Lazy::
// assigning closure `let f = |args| {...};` and boxing closure `let f: Box<ParseFn> = Box::new(f);`
// on 2 separate lines doesn't work
let f_params: Box<ParseFn> = Box::new(|attr_name, meta_item_name, meta| {
parse_lit_into_vec::<ParameterOverride>(attr_name, meta_item_name, meta)
.map(Variants::Params)
});
let f_with_funcs: Box<ParseFn> = Box::new(|_attr_name, _meta_item_name, meta| {
let map_result = meta_get_by_symbol_keys(WITH_FUNCS, meta, &WITH_FUNCS_FIELD_PARSE_MAP)?;
let with_funcs: WithFuncs = map_result.into();
- if (with_funcs.declaration.is_some() && with_funcs.definitions.is_none())
- || (with_funcs.declaration.is_none() && with_funcs.definitions.is_some())
- {
- return Err(syn::Error::new_spanned(
- &meta.path,
- format!(
- "both `{}` and `{}` have to be specified at the same time",
- DECLARATION.1, DEFINITIONS.1,
- ),
- ));
- }
+ // if (with_funcs.declaration.is_some() && with_funcs.definitions.is_none())
+ // || (with_funcs.declaration.is_none() && with_funcs.definitions.is_some())
+ // {
+ // return Err(syn::Error::new_spanned(
+ // &meta.path,
+ // format!(
+ // "both `{}` and `{}` have to be specified at the same time",
+ // DECLARATION.1, DEFINITIONS.1,
+ // ),
+ // ));
+ // }
Ok(Variants::WithFuncs(with_funcs))
});
m.insert(PARAMS, f_params);
m.insert(WITH_FUNCS, f_with_funcs);
m
});
/**
Struct describes an entry like `order_param => override_type`, e.g. `K => <K as TraitName>::Associated`
*/
diff --git a/borsh/Cargo.toml b/borsh/Cargo.toml
index e0a9465f..b208a7a6 100644
--- a/borsh/Cargo.toml
+++ b/borsh/Cargo.toml
@@ -38,19 +38,19 @@ bytes = { version = "1", optional = true }
bson = { version = "2", optional = true }
[dev-dependencies]
insta = "1.29.0"
[package.metadata.docs.rs]
features = ["derive", "unstable__schema", "rc"]
targets = ["x86_64-unknown-linux-gnu"]
[features]
-default = ["std"]
+default = ["std", "unstable__schema"]
derive = ["borsh-derive"]
unstable__schema = ["derive", "borsh-derive/schema"]
std = []
# Opt into impls for Rc<T> and Arc<T>. Serializing and deserializing these types
# does not preserve identity and may result in multiple copies of the same data.
# Be sure that this is what you want before enabling this feature.
rc = []
de_strict_order = []
diff --git a/borsh/tests/schema/test_schema_with_third_party.rs b/borsh/tests/schema/test_schema_with_third_party.rs
index 30562f89..ea068e85 100644
--- a/borsh/tests/schema/test_schema_with_third_party.rs
+++ b/borsh/tests/schema/test_schema_with_third_party.rs
@@ -1,24 +1,25 @@
use crate::common_macro::schema_imports::*;
// use alloc::collections::BTreeMap;
#[allow(unused)]
+#[derive(BorshSchema)]
struct ThirdParty<K, V>(BTreeMap<K, V>);
#[allow(unused)]
mod third_party_impl {
use crate::common_macro::schema_imports::*;
pub(super) fn declaration<K: borsh::BorshSchema, V: borsh::BorshSchema>(
) -> borsh::schema::Declaration {
let params = vec![<K>::declaration(), <V>::declaration()];
- format!(r#"{}<{}>"#, "ThirdParty", params.join(", "))
+ format!(r#"{}<{}>"#, "ThirdPartyOneHasNoControlOfRenamed", params.join(", "))
}
pub(super) fn add_definitions_recursively<K: borsh::BorshSchema, V: borsh::BorshSchema>(
definitions: &mut BTreeMap<borsh::schema::Declaration, borsh::schema::Definition>,
) {
let fields = borsh::schema::Fields::UnnamedFields(vec![
<BTreeMap<K, V> as borsh::BorshSchema>::declaration(),
]);
let definition = borsh::schema::Definition::Struct { fields };
let no_recursion_flag = definitions.get(&declaration::<K, V>()).is_none();
@@ -58,24 +59,24 @@ enum C<K, V> {
pub fn struct_overriden() {
assert_eq!(
"A<u64, String>".to_string(),
<A<u64, String>>::declaration()
);
let mut defs = Default::default();
<A<u64, String>>::add_definitions_recursively(&mut defs);
assert_eq!(
schema_map! {
"A<u64, String>" => Definition::Struct { fields: Fields::NamedFields(vec![
- ("x".to_string(), "ThirdParty<u64, String>".to_string()),
+ ("x".to_string(), "ThirdPartyOneHasNoControlOfRenamed<u64, String>".to_string()),
("y".to_string(), "u64".to_string())]
)},
- "ThirdParty<u64, String>" => Definition::Struct { fields: Fields::UnnamedFields(vec![
+ "ThirdPartyOneHasNoControlOfRenamed<u64, String>" => Definition::Struct { fields: Fields::UnnamedFields(vec![
"BTreeMap<u64, String>".to_string(),
]) },
"BTreeMap<u64, String>"=> Definition::Sequence {
length_width: Definition::DEFAULT_LENGTH_WIDTH,
length_range: Definition::DEFAULT_LENGTH_RANGE,
elements: "(u64, String)".to_string(),
},
"(u64, String)" => Definition::Tuple { elements: vec!["u64".to_string(), "String".to_string()]},
"u64" => Definition::Primitive(8),
"String" => Definition::Sequence {
@@ -101,23 +102,23 @@ pub fn enum_overriden() {
schema_map! {
"C<u64, String>" => Definition::Enum {
tag_width: 1,
variants: vec![
(0, "C3".to_string(), "CC3".to_string()),
(1, "C4".to_string(), "CC4<u64, String>".to_string())
]
},
"CC3" => Definition::Struct { fields: Fields::UnnamedFields(vec!["u64".to_string(), "u64".to_string()]) },
"CC4<u64, String>" => Definition::Struct { fields: Fields::UnnamedFields(vec![
- "u64".to_string(), "ThirdParty<u64, String>".to_string()
+ "u64".to_string(), "ThirdPartyOneHasNoControlOfRenamed<u64, String>".to_string()
]) },
- "ThirdParty<u64, String>" => Definition::Struct { fields: Fields::UnnamedFields(vec![
+ "ThirdPartyOneHasNoControlOfRenamed<u64, String>" => Definition::Struct { fields: Fields::UnnamedFields(vec![
"BTreeMap<u64, String>".to_string(),
]) },
"BTreeMap<u64, String>"=> Definition::Sequence {
length_width: Definition::DEFAULT_LENGTH_WIDTH,
length_range: Definition::DEFAULT_LENGTH_RANGE,
elements: "(u64, String)".to_string(),
},
"(u64, String)" => Definition::Tuple { elements: vec!["u64".to_string(), "String".to_string()]},
"u64" => Definition::Primitive(8),
"String" => Definition::Sequence {this diff is vs https://github.com/near/borsh-rs/tree/f16cd07e3c982539352aa43f65abf3607461a7bc cd borsh && cargo test --features unstable__schema 'schema::test_schema_with_third_party'it appears that doing a robust solution for arbitrary levels of nesting will require considerable breakage |
dj8yfo
changed the title
|
after some discussion and above comments, it seems that if i am mistaken,
For me good is |
@dzmitry-lahoda yeah, well, more-or-less robust automatic renames instead of conflict detection should be possible, if one introduces impl schemars::JsonSchema for A {
fn schema_name() -> std::string::String {
"A".to_owned()
}
fn schema_id() -> std::borrow::Cow<'static, str> {
std::borrow::Cow::Borrowed("example_crate::a::A")
}Someone willing to replace somebody's else's type schema intentionally will still be able to do so, a more robust solution would require compiler support for type_id of arbitrary types |
Original issue (conflicting names of enum variants with outer structs)
The following code fails to compile because the schema name generation for enum variants can produce name clashes.
I believe the error is a result of using a simple concatenation to generate the name.
borsh-rs/borsh-schema-derive-internal/src/enum_schema.rs
Line 28 in 7325a0a
Widened issue (conflicting names for types globally across multiple crates)
As per research by @dzmitry-lahoda,
BorshSchemais problematic to use when multiple third-party crates are involved,which can introduce conflicting names and only be panicing with conflicts when tried to be used elsewhere.
The text was updated successfully, but these errors were encountered: