From a9aad2127d43f096f3d090a045f936cf725c52f7 Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Wed, 18 Sep 2024 08:56:34 +0100 Subject: [PATCH 01/32] Start PoC for named c8ys in tedge config Signed-off-by: James Rhodes --- .../tedge_config_macros/examples/multi.rs | 87 ++++++++++++++ .../tedge_config_macros/impl/src/dto.rs | 15 +++ .../impl/src/input/parse.rs | 4 + .../impl/src/input/validate.rs | 13 ++- .../tedge_config_macros/impl/src/lib.rs | 5 + .../tedge_config_macros/impl/src/query.rs | 4 +- .../tedge_config_macros/impl/src/reader.rs | 106 ++++++++++++++++-- 7 files changed, 222 insertions(+), 12 deletions(-) create mode 100644 crates/common/tedge_config_macros/examples/multi.rs diff --git a/crates/common/tedge_config_macros/examples/multi.rs b/crates/common/tedge_config_macros/examples/multi.rs new file mode 100644 index 00000000000..57188851e36 --- /dev/null +++ b/crates/common/tedge_config_macros/examples/multi.rs @@ -0,0 +1,87 @@ +use camino::Utf8PathBuf; +// use std::net::IpAddr; +// use std::net::Ipv4Addr; +use std::num::NonZeroU16; +// use std::path::PathBuf; +use tedge_config_macros::*; + +static DEFAULT_ROOT_CERT_PATH: &str = "/etc/ssl/certs"; + +#[derive(thiserror::Error, Debug)] +pub enum ReadError { + #[error(transparent)] + ConfigNotSet(#[from] ConfigNotSet), + #[error("Something went wrong: {0}")] + GenericError(String), +} + +pub trait AppendRemoveItem { + type Item; + + fn append(current_value: Option, new_value: Self::Item) -> Option; + + fn remove(current_value: Option, remove_value: Self::Item) -> Option; +} + +impl AppendRemoveItem for T { + type Item = T; + + fn append(_current_value: Option, _new_value: Self::Item) -> Option { + unimplemented!() + } + + fn remove(_current_value: Option, _remove_value: Self::Item) -> Option { + unimplemented!() + } +} + +define_tedge_config! { + #[tedge_config(multi)] + c8y: { + #[tedge_config(reader(private))] + url: String, + }, +} + +fn device_id(_reader: &TEdgeConfigReader) -> Result { + Ok("dummy-device-id".to_owned()) +} + +fn default_device_key(location: &TEdgeConfigLocation) -> Utf8PathBuf { + location + .tedge_config_root_path() + .join("device-certs") + .join("tedge-private-key.pem") +} + +fn default_device_cert(location: &TEdgeConfigLocation) -> Utf8PathBuf { + location + .tedge_config_root_path() + .join("device-certs") + .join("tedge-certificate.pem") +} + +fn default_mqtt_port() -> NonZeroU16 { + NonZeroU16::try_from(1883).unwrap() +} + +fn main() { + // let dto = TEdgeConfigDto::default(); + // dto.mqtt.bind.address = Some(IpAddr::V4(Ipv4Addr::new(1, 2, 3, 4))); + + // let config = TEdgeConfigReader::from_dto(&dto, &TEdgeConfigLocation); + + // Typed reads + // println!( + // "Device id is {}.", + // // We have to pass the config into try_read to avoid TEdgeConfigReader being + // // self-referential + // config.device.id.try_read(&config).as_ref().unwrap() + // ); + // assert_eq!(u16::from(config.mqtt.bind.port), 1883); + // assert_eq!(config.mqtt.external.bind.port.or_none(), None); + // assert_eq!( + // config.read_string(ReadableKey::DeviceId).unwrap(), + // "dummy-device-id" + // ); +} diff --git a/crates/common/tedge_config_macros/impl/src/dto.rs b/crates/common/tedge_config_macros/impl/src/dto.rs index 5377d9e6a36..dd7ab3db3ea 100644 --- a/crates/common/tedge_config_macros/impl/src/dto.rs +++ b/crates/common/tedge_config_macros/impl/src/dto.rs @@ -46,6 +46,21 @@ pub fn generate( }); } } + FieldOrGroup::Multi(group) => { + if !group.dto.skip { + let sub_dto_name = prefixed_type_name(&name, group); + // TODO check if skip serializing if is actually any use, I deleted it because it was complicated to implement + idents.push(&group.ident); + let field_ty = parse_quote_spanned!(group.ident.span()=> ::std::collections::HashMap); + tys.push(field_ty); + sub_dtos.push(Some(generate(sub_dto_name, &group.contents, ""))); + preserved_attrs.push(group.attrs.iter().filter(is_preserved).collect()); + extra_attrs.push(quote! { + #[serde(default)] + #[serde(skip_serializing_if = "::std::collections::HashMap::is_empty")] + }); + } + } } } diff --git a/crates/common/tedge_config_macros/impl/src/input/parse.rs b/crates/common/tedge_config_macros/impl/src/input/parse.rs index 8079e665e9b..1e0a1e8dfa2 100644 --- a/crates/common/tedge_config_macros/impl/src/input/parse.rs +++ b/crates/common/tedge_config_macros/impl/src/input/parse.rs @@ -37,6 +37,8 @@ pub struct ConfigurationAttributes { #[darling(default)] pub dto: GroupDtoSettings, #[darling(default)] + pub multi: bool, + #[darling(default)] pub reader: ReaderSettings, #[darling(default, multiple, rename = "deprecated_name")] pub deprecated_names: Vec>, @@ -49,6 +51,7 @@ pub struct ConfigurationGroup { pub attrs: Vec, pub dto: GroupDtoSettings, pub reader: ReaderSettings, + pub multi: bool, pub deprecated_names: Vec>, pub rename: Option>, pub ident: syn::Ident, @@ -70,6 +73,7 @@ impl Parse for ConfigurationGroup { reader: known_attributes.reader, deprecated_names: known_attributes.deprecated_names, rename: known_attributes.rename, + multi: known_attributes.multi, ident: input.parse()?, colon_token: input.parse()?, brace: syn::braced!(content in input), diff --git a/crates/common/tedge_config_macros/impl/src/input/validate.rs b/crates/common/tedge_config_macros/impl/src/input/validate.rs index 9e2caa09fb6..635af79449b 100644 --- a/crates/common/tedge_config_macros/impl/src/input/validate.rs +++ b/crates/common/tedge_config_macros/impl/src/input/validate.rs @@ -11,6 +11,7 @@ use syn::spanned::Spanned; use crate::error::combine_errors; use crate::optional_error::OptionalError; use crate::optional_error::SynResultExt; +use crate::reader::PathItem; pub use super::parse::FieldDefault; pub use super::parse::FieldDtoSettings; @@ -91,12 +92,14 @@ impl TryFrom for ConfigurationGroup { pub enum FieldOrGroup { Field(ConfigurableField), Group(ConfigurationGroup), + Multi(ConfigurationGroup), } impl FieldOrGroup { pub fn name(&self) -> Cow { let rename = match self { Self::Group(group) => group.rename.as_ref().map(|s| s.as_str()), + Self::Multi(group) => group.rename.as_ref().map(|s| s.as_str()), Self::Field(field) => field.rename(), }; @@ -107,6 +110,7 @@ impl FieldOrGroup { match self { Self::Field(field) => field.ident(), Self::Group(group) => &group.ident, + Self::Multi(group) => &group.ident, } } @@ -116,6 +120,7 @@ impl FieldOrGroup { Self::Field(field) => field.rename().map_or(false, |rename| *target == rename), // Groups don't support renaming at the moment Self::Group(_) => false, + Self::Multi(_) => false, } } @@ -123,6 +128,7 @@ impl FieldOrGroup { match self { Self::Field(field) => Some(field), Self::Group(..) => None, + Self::Multi(..) => None, } } @@ -130,6 +136,7 @@ impl FieldOrGroup { match self { Self::Field(field) => field.reader(), Self::Group(group) => &group.reader, + Self::Multi(group) => &group.reader, } } } @@ -139,6 +146,9 @@ impl TryFrom for FieldOrGroup { fn try_from(value: super::parse::FieldOrGroup) -> Result { match value { super::parse::FieldOrGroup::Field(field) => field.try_into().map(Self::Field), + super::parse::FieldOrGroup::Group(group) if group.multi => { + group.try_into().map(Self::Multi) + } super::parse::FieldOrGroup::Group(group) => group.try_into().map(Self::Group), } } @@ -164,11 +174,12 @@ pub struct ReadOnlyField { } impl ReadOnlyField { - pub fn lazy_reader_name(&self, parents: &[syn::Ident]) -> syn::Ident { + pub fn lazy_reader_name(&self, parents: &[PathItem]) -> syn::Ident { format_ident!( "LazyReader{}{}", parents .iter() + .filter_map(PathItem::as_static) .map(|p| p.to_string().to_upper_camel_case()) .collect::>() .join("."), diff --git a/crates/common/tedge_config_macros/impl/src/lib.rs b/crates/common/tedge_config_macros/impl/src/lib.rs index c994857d78f..cd1b3f851a1 100644 --- a/crates/common/tedge_config_macros/impl/src/lib.rs +++ b/crates/common/tedge_config_macros/impl/src/lib.rs @@ -25,6 +25,7 @@ pub fn generate_configuration(tokens: TokenStream) -> Result unfold_group(Vec::new(), group), + input::FieldOrGroup::Multi(group) => unfold_group(Vec::new(), group), input::FieldOrGroup::Field(field) => { error.combine(syn::Error::new( field.ident().span(), @@ -153,6 +154,10 @@ fn unfold_group( ); output.push((name, field)) } + input::FieldOrGroup::Multi(group) => { + name.push("*".to_owned()); + output.append(&mut unfold_group(name.clone(), group)); + } input::FieldOrGroup::Group(group) => { output.append(&mut unfold_group(name.clone(), group)); } diff --git a/crates/common/tedge_config_macros/impl/src/query.rs b/crates/common/tedge_config_macros/impl/src/query.rs index 77df26e36b4..f927e615ee2 100644 --- a/crates/common/tedge_config_macros/impl/src/query.rs +++ b/crates/common/tedge_config_macros/impl/src/query.rs @@ -237,7 +237,7 @@ fn keys_enum( let as_str_example = (!as_str_example.is_empty()).then(|| { quote! { /// ```compile_fail - /// // This doctest is compile_fail because we have no way import the + /// // This doctest is compile_fail because we have no way to import the /// // current type, but the example is still valuable #( #[doc = #as_str_example] @@ -450,7 +450,7 @@ fn configuration_paths_from(items: &[FieldOrGroup]) -> Vec res.push(VecDeque::from([item])), - FieldOrGroup::Group(group) => { + FieldOrGroup::Group(group) | FieldOrGroup::Multi(group) => { for mut fields in configuration_paths_from(&group.contents) { fields.push_front(item); res.push(fields); diff --git a/crates/common/tedge_config_macros/impl/src/reader.rs b/crates/common/tedge_config_macros/impl/src/reader.rs index 1a6376e3a55..94b378b42d8 100644 --- a/crates/common/tedge_config_macros/impl/src/reader.rs +++ b/crates/common/tedge_config_macros/impl/src/reader.rs @@ -3,10 +3,14 @@ //! When reading the configuration, we want to see default values if nothing has //! been configured use std::iter; +use std::iter::once; +use proc_macro2::Span; use proc_macro2::TokenStream; use quote::quote; use quote::quote_spanned; +use quote::ToTokens; +use quote::TokenStreamExt; use syn::parse_quote; use syn::parse_quote_spanned; use syn::punctuated::Punctuated; @@ -36,7 +40,7 @@ pub fn try_generate( fn generate_structs( name: &proc_macro2::Ident, items: &[FieldOrGroup], - parents: Vec, + parents: Vec, doc_comment: &str, ) -> syn::Result { let mut idents = Vec::new(); @@ -67,12 +71,38 @@ fn generate_structs( false => parse_quote!(pub), }); } + // TODO skipping? + FieldOrGroup::Multi(group) => { + // TODO is this right? + let sub_reader_name = prefixed_type_name(name, group); + idents.push(&group.ident); + tys.push(parse_quote_spanned!(group.ident.span()=> ::std::collections::HashMap)); + let mut parents = parents.clone(); + parents.push(PathItem::Static(group.ident.clone())); + let count_dyn = parents.iter().filter_map(PathItem::as_dynamic).count(); + // TODO rationalise this so it's not hardcoded where we use it + parents.push(PathItem::Dynamic(syn::Ident::new( + &format!("key{count_dyn}"), + Span::call_site(), + ))); + sub_readers.push(Some(generate_structs( + &sub_reader_name, + &group.contents, + parents, + "", + )?)); + attrs.push(group.attrs.to_vec()); + vis.push(match group.reader.private { + true => parse_quote!(), + false => parse_quote!(pub), + }); + } FieldOrGroup::Group(group) if !group.reader.skip => { let sub_reader_name = prefixed_type_name(name, group); idents.push(&group.ident); tys.push(parse_quote_spanned!(group.ident.span()=> #sub_reader_name)); let mut parents = parents.clone(); - parents.push(group.ident.clone()); + parents.push(PathItem::Static(group.ident.clone())); sub_readers.push(Some(generate_structs( &sub_reader_name, &group.contents, @@ -176,7 +206,7 @@ fn find_field<'a>( let is_last_segment = i == key.len() - 1; match target { - FieldOrGroup::Group(group) => fields = &group.contents, + FieldOrGroup::Group(group) | FieldOrGroup::Multi(group) => fields = &group.contents, FieldOrGroup::Field(_) if is_last_segment => (), _ => { let string_path = key.iter().map(<_>::to_string).collect::>(); @@ -195,7 +225,7 @@ fn find_field<'a>( match current_field { // TODO test this appears None => Err(syn::Error::new(key.span(), "key is empty")), - Some(FieldOrGroup::Group(_)) => Err(syn::Error::new( + Some(FieldOrGroup::Group(_) | FieldOrGroup::Multi(_)) => Err(syn::Error::new( key.span(), // TODO test this too "path points to a group of fields, not a single field", @@ -204,9 +234,40 @@ fn find_field<'a>( } } +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum PathItem { + Static(syn::Ident), + Dynamic(syn::Ident), +} + +impl PathItem { + pub fn as_static(&self) -> Option<&syn::Ident> { + match self { + Self::Static(s) => Some(s), + Self::Dynamic(_) => None, + } + } + + pub fn as_dynamic(&self) -> Option<&syn::Ident> { + match self { + Self::Dynamic(d) => Some(d), + Self::Static(_) => None, + } + } +} + +impl ToTokens for PathItem { + fn to_tokens(&self, tokens: &mut TokenStream) { + match self { + Self::Static(s) => s.to_tokens(tokens), + Self::Dynamic(d) => tokens.append_all(quote!(get(#d).unwrap())), + } + } +} + fn reader_value_for_field<'a>( field: &'a ConfigurableField, - parents: &[syn::Ident], + parents: &[PathItem], root_fields: &[FieldOrGroup], mut observed_keys: Vec<&'a Punctuated>, ) -> syn::Result { @@ -215,6 +276,7 @@ fn reader_value_for_field<'a>( ConfigurableField::ReadWrite(field) => { let key = parents .iter() + .filter_map(PathItem::as_static) .map(|p| p.to_string()) .chain(iter::once(name.to_string())) .collect::>() @@ -257,6 +319,7 @@ fn reader_value_for_field<'a>( .iter() .take(default_key.len() - 1) .map(<_>::to_owned) + .map(PathItem::Static) .collect::>(), root_fields, observed_keys, @@ -318,11 +381,16 @@ fn reader_value_for_field<'a>( fn generate_conversions( name: &proc_macro2::Ident, items: &[FieldOrGroup], - parents: Vec, + parents: Vec, root_fields: &[FieldOrGroup], ) -> syn::Result { let mut field_conversions = Vec::new(); let mut rest = Vec::new(); + let extra_args: Vec<_> = parents + .iter() + .filter_map(PathItem::as_dynamic) + .map(|name| quote!(#name: &str)) + .collect(); for item in items { match item { @@ -336,13 +404,33 @@ fn generate_conversions( let name = &group.ident; let mut parents = parents.clone(); - parents.push(group.ident.clone()); + parents.push(PathItem::Static(group.ident.clone())); field_conversions.push(quote!(#name: #sub_reader_name::from_dto(dto, location))); let sub_conversions = generate_conversions(&sub_reader_name, &group.contents, parents, root_fields)?; rest.push(sub_conversions); } - FieldOrGroup::Group(_) => { + FieldOrGroup::Multi(group) if !group.reader.skip => { + let sub_reader_name = prefixed_type_name(name, group); + let name = &group.ident; + + let mut parents = parents.clone(); + parents.push(PathItem::Static(group.ident.clone())); + // TODO this is horrible + let l = extra_args.len(); + let new_arg = syn::Ident::new(&format!("key{l}"), Span::call_site()); + let extra_args: Vec<_> = parents + .iter() + .filter_map(PathItem::as_dynamic) + .chain(once(&new_arg)) + .collect(); + field_conversions.push(quote!(#name: dto.#(#parents).*.keys().map(|#new_arg| (#new_arg.to_owned(), #sub_reader_name::from_dto(dto, location, #(#extra_args),*))).collect())); + parents.push(PathItem::Dynamic(new_arg)); + let sub_conversions = + generate_conversions(&sub_reader_name, &group.contents, parents, root_fields)?; + rest.push(sub_conversions); + } + FieldOrGroup::Group(_) | FieldOrGroup::Multi(_) => { // Skipped } } @@ -353,7 +441,7 @@ fn generate_conversions( #[allow(unused, clippy::clone_on_copy, clippy::useless_conversion)] #[automatically_derived] /// Converts the provided [TEdgeConfigDto] into a reader - pub fn from_dto(dto: &TEdgeConfigDto, location: &TEdgeConfigLocation) -> Self { + pub fn from_dto(dto: &TEdgeConfigDto, location: &TEdgeConfigLocation, #(#extra_args,)*) -> Self { Self { #(#field_conversions),* } From 15176885f5fdd2dbca8567b2c133a49c5429ec1d Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Thu, 19 Sep 2024 15:04:47 +0100 Subject: [PATCH 02/32] Generate enums that mostly work with multi keys Signed-off-by: James Rhodes --- .../tedge_config_macros/impl/src/lib.rs | 11 ++ .../tedge_config_macros/impl/src/query.rs | 170 ++++++++++++++---- 2 files changed, 144 insertions(+), 37 deletions(-) diff --git a/crates/common/tedge_config_macros/impl/src/lib.rs b/crates/common/tedge_config_macros/impl/src/lib.rs index cd1b3f851a1..6004a4b2495 100644 --- a/crates/common/tedge_config_macros/impl/src/lib.rs +++ b/crates/common/tedge_config_macros/impl/src/lib.rs @@ -237,6 +237,17 @@ mod tests { .unwrap(); } + #[test] + fn can_contain_multi_fields() { + generate_configuration(quote! { + #[multi] + c8y: { + url: String + }, + }) + .unwrap(); + } + #[test] fn error_message_suggests_fix_in_case_of_invalid_value() { assert_eq!(generate_configuration(quote! { diff --git a/crates/common/tedge_config_macros/impl/src/query.rs b/crates/common/tedge_config_macros/impl/src/query.rs index f927e615ee2..960e895a402 100644 --- a/crates/common/tedge_config_macros/impl/src/query.rs +++ b/crates/common/tedge_config_macros/impl/src/query.rs @@ -13,11 +13,19 @@ use syn::spanned::Spanned; pub fn generate_writable_keys(items: &[FieldOrGroup]) -> TokenStream { let paths = configuration_paths_from(items); - let (readonly_variant, write_error) = paths + let (readonly_variant, readonly_iter, readonly_destr, write_error): ( + Vec<_>, + Vec<_>, + Vec<_>, + Vec<_>, + ) = paths .iter() .filter_map(|field| { + let (enum_field, iter_field, destr_field) = variant_name(field); Some(( - variant_name(field), + enum_field, + iter_field, + destr_field, field .back()? .field()? @@ -27,7 +35,7 @@ pub fn generate_writable_keys(items: &[FieldOrGroup]) -> TokenStream { .as_str(), )) }) - .unzip::<_, _, Vec<_>, Vec<_>>(); + .multiunzip(); let readable_args = configuration_strings(paths.iter()); let readonly_args = configuration_strings(paths.iter().filter(|path| !is_read_write(path))); let writable_args = configuration_strings(paths.iter().filter(|path| is_read_write(path))); @@ -49,7 +57,7 @@ pub fn generate_writable_keys(items: &[FieldOrGroup]) -> TokenStream { .cloned() .collect::>(), ); - let (static_alias, updated_key) = deprecated_keys(paths.iter()); + let (static_alias, _, iter_updated, _) = deprecated_keys(paths.iter()); quote! { #readable_keys @@ -72,8 +80,10 @@ pub fn generate_writable_keys(items: &[FieldOrGroup]) -> TokenStream { impl ReadOnlyKey { fn write_error(self) -> &'static str { match self { + // TODO these should be underscores #( - Self::#readonly_variant => #write_error, + #[allow(unused)] + Self::#readonly_destr => #write_error, )* } } @@ -100,8 +110,8 @@ pub fn generate_writable_keys(items: &[FieldOrGroup]) -> TokenStream { let Fields::Named { fields } = fields else { panic!("Expected named fields but got {:?}", fields)}; let mut aliases = struct_field_aliases(None, &fields); #( - if let Some(alias) = aliases.insert(Cow::Borrowed(#static_alias), Cow::Borrowed(ReadableKey::#updated_key.as_str())) { - panic!("Duplicate configuration alias for '{}'. It maps to both '{}' and '{}'. Perhaps you provided an incorrect `deprecated_key` for one of these configurations?", #static_alias, alias, ReadableKey::#updated_key.as_str()); + if let Some(alias) = aliases.insert(Cow::Borrowed(#static_alias), Cow::Borrowed(ReadableKey::#iter_updated.as_str())) { + panic!("Duplicate configuration alias for '{}'. It maps to both '{}' and '{}'. Perhaps you provided an incorrect `deprecated_key` for one of these configurations?", #static_alias, alias, ReadableKey::#iter_updated.as_str()); } )* aliases @@ -130,24 +140,37 @@ pub fn generate_writable_keys(items: &[FieldOrGroup]) -> TokenStream { fn configuration_strings<'a>( variants: impl Iterator>, -) -> (Vec, Vec) { +) -> ( + Vec, + Vec, + Vec, + Vec, +) { variants .map(|segments| { + let (x, y, z) = variant_name(segments); ( segments .iter() .map(|variant| variant.name()) .collect::>() .join("."), - variant_name(segments), + x, + y, + z, ) }) - .unzip() + .multiunzip() } fn deprecated_keys<'a>( variants: impl Iterator>, -) -> (Vec<&'a str>, Vec) { +) -> ( + Vec<&'a str>, + Vec, + Vec, + Vec, +) { variants .flat_map(|segments| { segments @@ -156,22 +179,31 @@ fn deprecated_keys<'a>( .field() .unwrap() .deprecated_keys() - .map(|key| (key, variant_name(segments))) + .map(|key| { + let (x, y, z) = variant_name(segments); + (key, x, y, z) + }) }) - .unzip() + .multiunzip() } fn generate_fromstr( type_name: syn::Ident, - (configuration_string, variant_name): &(Vec, Vec), + (configuration_string, enum_variant, iter_variant, match_variant): &( + Vec, + Vec, + Vec, + Vec, + ), error_case: syn::Arm, ) -> TokenStream { let simplified_configuration_string = configuration_string .iter() .map(|s| s.replace('.', "_")) - .zip(variant_name.iter()) + .zip(enum_variant) .map(|(s, v)| quote_spanned!(v.span()=> #s)); + // TODO oh shit make this actually work! quote! { impl ::std::str::FromStr for #type_name { type Err = ParseKeyError; @@ -184,7 +216,7 @@ fn generate_fromstr( if (value != #configuration_string) { warn_about_deprecated_key(value.to_owned(), #configuration_string); } - Ok(Self::#variant_name) + Ok(Self::#iter_variant) }, )* #error_case @@ -196,7 +228,12 @@ fn generate_fromstr( fn generate_fromstr_readable( type_name: syn::Ident, - fields: &(Vec, Vec), + fields: &( + Vec, + Vec, + Vec, + Vec, + ), ) -> TokenStream { generate_fromstr( type_name, @@ -208,7 +245,12 @@ fn generate_fromstr_readable( // TODO test the error messages actually appear fn generate_fromstr_writable( type_name: syn::Ident, - fields: &(Vec, Vec), + fields: &( + Vec, + Vec, + Vec, + Vec, + ), ) -> TokenStream { generate_fromstr( type_name, @@ -225,13 +267,23 @@ fn generate_fromstr_writable( fn keys_enum( type_name: syn::Ident, - (configuration_string, variant_name): &(Vec, Vec), + (configuration_string, enum_variant, iter_variant, match_variant): &( + Vec, + Vec, + Vec, + Vec, + ), doc_fragment: &'static str, ) -> TokenStream { - let as_str_example = variant_name + let as_str_example = iter_variant .iter() .zip(configuration_string.iter()) - .map(|(ident, value)| format!("assert_eq!({type_name}::{ident}.as_str(), \"{value}\");\n")) + .map(|(ident, value)| { + format!( + "assert_eq!({type_name}::{ident}.as_str(), \"{value}\");\n", + ident = quote!(#ident) + ) + }) .take(10) .collect::>(); let as_str_example = (!as_str_example.is_empty()).then(|| { @@ -259,7 +311,7 @@ fn keys_enum( pub enum #type_name { #( #[doc = concat!("`", #configuration_string, "`")] - #variant_name, + #enum_variant, )* } @@ -269,7 +321,7 @@ fn keys_enum( pub fn as_str(self) -> &'static str { match self { #( - Self::#variant_name => #configuration_string, + Self::#match_variant => #configuration_string, )* } } @@ -278,7 +330,7 @@ fn keys_enum( pub fn iter() -> impl Iterator { [ #( - Self::#variant_name, + Self::#iter_variant, )* ].into_iter() } @@ -297,31 +349,33 @@ fn generate_string_readers(paths: &[VecDeque<&FieldOrGroup>]) -> TokenStream { let arms = paths .iter() .zip(variant_names) - .map(|(path, variant_name)| -> syn::Arm { + .map(|(path, (enum_variant, iter_variant, match_variant))| -> syn::Arm { let field = path .back() .expect("Path must have a back as it is nonempty") .field() .expect("Back of path is guaranteed to be a field"); + // TODO get path let segments = path.iter().map(|thing| thing.ident()); let to_string = quote_spanned!(field.ty().span()=> .to_string()); if field.read_only().is_some() { if extract_type_from_result(field.ty()).is_some() { + // TODO test whether the wrong type fails unit tests parse_quote! { - ReadableKey::#variant_name => Ok(self.#(#segments).*.try_read(self)?#to_string), + ReadableKey::#match_variant => Ok(self.#(#segments).*.try_read(self)?#to_string), } } else { parse_quote! { - ReadableKey::#variant_name => Ok(self.#(#segments).*.read(self)#to_string), + ReadableKey::#match_variant => Ok(self.#(#segments).*.read(self)#to_string), } } } else if field.has_guaranteed_default() { parse_quote! { - ReadableKey::#variant_name => Ok(self.#(#segments).*#to_string), + ReadableKey::#match_variant => Ok(self.#(#segments).*#to_string), } } else { parse_quote! { - ReadableKey::#variant_name => Ok(self.#(#segments).*.or_config_not_set()?#to_string), + ReadableKey::#match_variant => Ok(self.#(#segments).*.or_config_not_set()?#to_string), } } }); @@ -346,7 +400,7 @@ fn generate_string_writers(paths: &[VecDeque<&FieldOrGroup>]) -> TokenStream { ) = paths .iter() .zip(variant_names) - .map(|(path, variant_name)| { + .map(|(path, (enum_variant, iter_variant, match_variant))| { let segments = path.iter().map(|thing| thing.ident()).collect::>(); let field = path .iter() @@ -373,16 +427,16 @@ fn generate_string_writers(paths: &[VecDeque<&FieldOrGroup>]) -> TokenStream { ( parse_quote_spanned! {ty.span()=> - WritableKey::#variant_name => self.#(#segments).* = Some(value + WritableKey::#match_variant => self.#(#segments).* = Some(value .#parse .#convert_to_field_ty .map_err(|e| WriteError::ParseValue(Box::new(e)))?), }, parse_quote_spanned! {ty.span()=> - WritableKey::#variant_name => self.#(#segments).* = None, + WritableKey::#match_variant => self.#(#segments).* = None, }, parse_quote_spanned! {ty.span()=> - WritableKey::#variant_name => self.#(#segments).* = <#ty as AppendRemoveItem>::append( + WritableKey::#match_variant => self.#(#segments).* = <#ty as AppendRemoveItem>::append( #current_value, value .#parse @@ -390,7 +444,7 @@ fn generate_string_writers(paths: &[VecDeque<&FieldOrGroup>]) -> TokenStream { .map_err(|e| WriteError::ParseValue(Box::new(e)))?), }, parse_quote_spanned! {ty.span()=> - WritableKey::#variant_name => self.#(#segments).* = <#ty as AppendRemoveItem>::remove( + WritableKey::#match_variant => self.#(#segments).* = <#ty as AppendRemoveItem>::remove( #current_value, value .#parse @@ -433,14 +487,35 @@ fn generate_string_writers(paths: &[VecDeque<&FieldOrGroup>]) -> TokenStream { } } -fn variant_name(segments: &VecDeque<&FieldOrGroup>) -> syn::Ident { - syn::Ident::new( +fn variant_name(segments: &VecDeque<&FieldOrGroup>) -> (syn::Variant, syn::Expr, syn::Pat) { + let ident = syn::Ident::new( &segments .iter() .map(|segment| segment.name().to_upper_camel_case()) .collect::(), segments.iter().last().unwrap().ident().span(), - ) + ); + let count_multi = segments + .iter() + .filter(|fog| matches!(fog, FieldOrGroup::Multi(_))) + .count(); + if count_multi > 0 { + let opt_strs = + std::iter::repeat::(parse_quote!(Option)).take(count_multi); + let enum_field = parse_quote_spanned!(ident.span()=> #ident(#(#opt_strs),*)); + let nones = std::iter::repeat::(parse_quote!(None)).take(count_multi); + let iter_field = parse_quote_spanned!(ident.span()=> #ident(#(#nones),*)); + let var_idents = + (0..count_multi).map(|i| syn::Ident::new(&format!("key{i}"), ident.span())); + let destructure_field = parse_quote_spanned!(ident.span()=> #ident(#(#var_idents),*)); + (enum_field, iter_field, destructure_field) + } else { + ( + parse_quote!(#ident), + parse_quote!(#ident), + parse_quote!(#ident), + ) + } } /// Generates a list of the toml paths for each of the keys in the provided @@ -468,3 +543,24 @@ fn is_read_write(path: &VecDeque<&FieldOrGroup>) -> bool { Some(FieldOrGroup::Field(ConfigurableField::ReadWrite(_))), ) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn output_parses() { + syn::parse2::(generate_writable_keys(&[])).unwrap(); + } + + #[test] + fn output_parses_for_multi() { + let input: crate::input::Configuration = parse_quote! { + #[tedge_config(multi)] + c8y: { + url: String + } + }; + syn::parse2::(generate_writable_keys(&input.groups)).unwrap(); + } +} From 5e50c0eb2dcbd64798f4a6feac1cb172c3d5e235 Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Thu, 19 Sep 2024 15:36:48 +0100 Subject: [PATCH 03/32] Fix the remaining new compiler errors Signed-off-by: James Rhodes --- crates/common/tedge_config_macros/impl/src/query.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/crates/common/tedge_config_macros/impl/src/query.rs b/crates/common/tedge_config_macros/impl/src/query.rs index 960e895a402..a38c7ffa60b 100644 --- a/crates/common/tedge_config_macros/impl/src/query.rs +++ b/crates/common/tedge_config_macros/impl/src/query.rs @@ -78,13 +78,15 @@ pub fn generate_writable_keys(items: &[FieldOrGroup]) -> TokenStream { } impl ReadOnlyKey { - fn write_error(self) -> &'static str { + fn write_error(&self) -> &'static str { match self { // TODO these should be underscores #( #[allow(unused)] Self::#readonly_destr => #write_error, )* + // TODO make this conditional + _ => unimplemented!("Cope with empty enum") } } } @@ -300,7 +302,7 @@ fn keys_enum( let type_name_str = type_name.to_string(); quote! { - #[derive(Copy, Clone, Debug, PartialEq, Eq)] + #[derive(Clone, Debug, PartialEq, Eq)] #[non_exhaustive] #[allow(unused)] #[doc = concat!("A key that can be *", #doc_fragment, "* the configuration\n\n")] @@ -318,11 +320,13 @@ fn keys_enum( impl #type_name { /// Converts this key to the canonical key used by `tedge config` and `tedge.toml` #as_str_example - pub fn as_str(self) -> &'static str { + pub fn as_str(&self) -> &'static str { match self { #( Self::#match_variant => #configuration_string, )* + // TODO make this conditional + _ => unimplemented!("Cope with empty enum") } } From 0efaa46da5bdf63b3775dc32b92f3544310628ad Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Thu, 19 Sep 2024 21:01:36 +0100 Subject: [PATCH 04/32] Replace HashMap with Multi enum, to allow unnamed c8ys to persist Signed-off-by: James Rhodes --- crates/common/tedge_config_macros/Cargo.toml | 1 + .../tedge_config_macros/impl/src/dto.rs | 4 +- .../tedge_config_macros/impl/src/reader.rs | 6 +-- crates/common/tedge_config_macros/src/lib.rs | 2 + .../common/tedge_config_macros/src/multi.rs | 54 +++++++++++++++++++ 5 files changed, 62 insertions(+), 5 deletions(-) create mode 100644 crates/common/tedge_config_macros/src/multi.rs diff --git a/crates/common/tedge_config_macros/Cargo.toml b/crates/common/tedge_config_macros/Cargo.toml index 2f1ac9df8cd..8bf5edab47c 100644 --- a/crates/common/tedge_config_macros/Cargo.toml +++ b/crates/common/tedge_config_macros/Cargo.toml @@ -11,6 +11,7 @@ repository = { workspace = true } [dependencies] camino = { workspace = true, features = ["serde1"] } doku = { workspace = true } +itertools = { workspace = true } once_cell = { workspace = true } serde = { workspace = true } tedge_config_macros-macro = { path = "macro" } diff --git a/crates/common/tedge_config_macros/impl/src/dto.rs b/crates/common/tedge_config_macros/impl/src/dto.rs index dd7ab3db3ea..cebdec0eb60 100644 --- a/crates/common/tedge_config_macros/impl/src/dto.rs +++ b/crates/common/tedge_config_macros/impl/src/dto.rs @@ -51,13 +51,13 @@ pub fn generate( let sub_dto_name = prefixed_type_name(&name, group); // TODO check if skip serializing if is actually any use, I deleted it because it was complicated to implement idents.push(&group.ident); - let field_ty = parse_quote_spanned!(group.ident.span()=> ::std::collections::HashMap); + let field_ty = parse_quote_spanned!(group.ident.span()=> ::tedge_config_macros::Multi<#sub_dto_name>); tys.push(field_ty); sub_dtos.push(Some(generate(sub_dto_name, &group.contents, ""))); preserved_attrs.push(group.attrs.iter().filter(is_preserved).collect()); extra_attrs.push(quote! { #[serde(default)] - #[serde(skip_serializing_if = "::std::collections::HashMap::is_empty")] + #[serde(skip_serializing_if = "::tedge_config_macros::Multi::is_default")] }); } } diff --git a/crates/common/tedge_config_macros/impl/src/reader.rs b/crates/common/tedge_config_macros/impl/src/reader.rs index 94b378b42d8..f7033ca72c7 100644 --- a/crates/common/tedge_config_macros/impl/src/reader.rs +++ b/crates/common/tedge_config_macros/impl/src/reader.rs @@ -76,7 +76,7 @@ fn generate_structs( // TODO is this right? let sub_reader_name = prefixed_type_name(name, group); idents.push(&group.ident); - tys.push(parse_quote_spanned!(group.ident.span()=> ::std::collections::HashMap)); + tys.push(parse_quote_spanned!(group.ident.span()=> ::tedge_config_macros::Multi<#sub_reader_name>)); let mut parents = parents.clone(); parents.push(PathItem::Static(group.ident.clone())); let count_dyn = parents.iter().filter_map(PathItem::as_dynamic).count(); @@ -389,7 +389,7 @@ fn generate_conversions( let extra_args: Vec<_> = parents .iter() .filter_map(PathItem::as_dynamic) - .map(|name| quote!(#name: &str)) + .map(|name| quote!(#name: Option<&str>)) .collect(); for item in items { @@ -424,7 +424,7 @@ fn generate_conversions( .filter_map(PathItem::as_dynamic) .chain(once(&new_arg)) .collect(); - field_conversions.push(quote!(#name: dto.#(#parents).*.keys().map(|#new_arg| (#new_arg.to_owned(), #sub_reader_name::from_dto(dto, location, #(#extra_args),*))).collect())); + field_conversions.push(quote!(#name: dto.#(#parents).*.map(|#new_arg| #sub_reader_name::from_dto(dto, location, #(#extra_args),*)))); parents.push(PathItem::Dynamic(new_arg)); let sub_conversions = generate_conversions(&sub_reader_name, &group.contents, parents, root_fields)?; diff --git a/crates/common/tedge_config_macros/src/lib.rs b/crates/common/tedge_config_macros/src/lib.rs index f6e0515f31a..bfc56c376eb 100644 --- a/crates/common/tedge_config_macros/src/lib.rs +++ b/crates/common/tedge_config_macros/src/lib.rs @@ -8,6 +8,7 @@ pub use connect_url::*; pub use default::*; pub use doku_aliases::*; pub use option::*; +pub use multi::*; mod all_or_nothing; mod connect_url; @@ -16,3 +17,4 @@ mod doku_aliases; #[cfg(doc)] pub mod example; mod option; +mod multi; diff --git a/crates/common/tedge_config_macros/src/multi.rs b/crates/common/tedge_config_macros/src/multi.rs new file mode 100644 index 00000000000..7fb93bc0c7f --- /dev/null +++ b/crates/common/tedge_config_macros/src/multi.rs @@ -0,0 +1,54 @@ +#[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize, doku::Document)] +#[serde(untagged)] +pub enum Multi { + Single(T), + Multi(::std::collections::HashMap) +} + +impl Default for Multi { + fn default() -> Self { + Self::Single(T::default()) + } +} + +impl Multi { + pub fn is_default(&self) -> bool { + *self == Self::default() + } +} + +// TODO possibly expand this with the key name +// TODO use thiserror +#[derive(Debug)] +pub enum MultiError { + SingleNotMulti, + MultiNotSingle, + MultiKeyNotFound, +} + +impl Multi { + // TODO rename this to something more rusty + pub fn get(&self, key: Option<&str>) -> Result<&T, MultiError> { + match (self, key) { + (Self::Single(val), None) => Ok(val), + (Self::Multi(map), Some(key)) => map.get(key).ok_or(MultiError::MultiKeyNotFound), + (Self::Multi(_), None) => Err(MultiError::SingleNotMulti), + (Self::Single(_), Some(_key)) => Err(MultiError::MultiNotSingle), + } + } + + pub fn keys(&self) -> impl Iterator> { + match self { + Self::Single(_) => itertools::Either::Left(std::iter::once(None)), + Self::Multi(map) => itertools::Either::Right(map.keys().map(String::as_str).map(Some)), + } + } + + // TODO clearer name + pub fn map(&self, f: impl Fn(Option<&str>) -> U) -> Multi { + match self { + Self::Single(_) => Multi::Single(f(None)), + Self::Multi(map) => Multi::Multi(map.keys().map(|key| (key.to_owned(), f(Some(key)))).collect()) + } + } +} From d3ea36e39622dc2eff3606f7711153046a19e5d0 Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Fri, 20 Sep 2024 11:13:39 +0100 Subject: [PATCH 05/32] Add another .get call to fix a compiler error message Signed-off-by: James Rhodes --- Cargo.lock | 1 + crates/common/tedge_config_macros/examples/multi.rs | 2 ++ crates/common/tedge_config_macros/impl/src/query.rs | 11 +++++++++-- crates/common/tedge_config_macros/src/multi.rs | 7 +++++-- 4 files changed, 17 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index af07fbabb65..c2c502cf312 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3949,6 +3949,7 @@ version = "1.3.0" dependencies = [ "camino", "doku", + "itertools 0.13.0", "once_cell", "serde", "tedge_config_macros-macro", diff --git a/crates/common/tedge_config_macros/examples/multi.rs b/crates/common/tedge_config_macros/examples/multi.rs index 57188851e36..a26f3b4b7c0 100644 --- a/crates/common/tedge_config_macros/examples/multi.rs +++ b/crates/common/tedge_config_macros/examples/multi.rs @@ -13,6 +13,8 @@ pub enum ReadError { ConfigNotSet(#[from] ConfigNotSet), #[error("Something went wrong: {0}")] GenericError(String), + #[error(transparent)] + Multi(#[from] tedge_config_macros::MultiError) } pub trait AppendRemoveItem { diff --git a/crates/common/tedge_config_macros/impl/src/query.rs b/crates/common/tedge_config_macros/impl/src/query.rs index a38c7ffa60b..4f9fca61f1b 100644 --- a/crates/common/tedge_config_macros/impl/src/query.rs +++ b/crates/common/tedge_config_macros/impl/src/query.rs @@ -1,6 +1,9 @@ use crate::error::extract_type_from_result; +use std::iter::once; +use itertools::Either; use crate::input::ConfigurableField; use crate::input::FieldOrGroup; +use crate::reader::PathItem; use heck::ToUpperCamelCase; use itertools::Itertools; use proc_macro2::TokenStream; @@ -359,8 +362,12 @@ fn generate_string_readers(paths: &[VecDeque<&FieldOrGroup>]) -> TokenStream { .expect("Path must have a back as it is nonempty") .field() .expect("Back of path is guaranteed to be a field"); - // TODO get path - let segments = path.iter().map(|thing| thing.ident()); + let segments = path.iter().map(|&thing| match thing { + FieldOrGroup::Field(f) => { let ident = f.ident(); quote!(#ident) }, + FieldOrGroup::Group(g) => { let ident = &g.ident; quote!(#ident) }, + // TODO don't hardcode key0 that's stupid + FieldOrGroup::Multi(m) => { let ident = &m.ident; quote_spanned!(ident.span()=> #ident.get(key0.as_deref())?) }, + }); let to_string = quote_spanned!(field.ty().span()=> .to_string()); if field.read_only().is_some() { if extract_type_from_result(field.ty()).is_some() { diff --git a/crates/common/tedge_config_macros/src/multi.rs b/crates/common/tedge_config_macros/src/multi.rs index 7fb93bc0c7f..47b28ba6493 100644 --- a/crates/common/tedge_config_macros/src/multi.rs +++ b/crates/common/tedge_config_macros/src/multi.rs @@ -18,11 +18,14 @@ impl Multi { } // TODO possibly expand this with the key name -// TODO use thiserror -#[derive(Debug)] +// TODO better error messages +#[derive(Debug, thiserror::Error)] pub enum MultiError { + #[error("You are trying to access a named field, but the fields are not named")] SingleNotMulti, + #[error("You need a name for this field")] MultiNotSingle, + #[error("You need a name for this field")] MultiKeyNotFound, } From 7eb7515e30967df1f35190bf9c4e63a251eae7af Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Fri, 20 Sep 2024 12:20:15 +0100 Subject: [PATCH 06/32] Make multi example compile Signed-off-by: James Rhodes --- .../tedge_config_macros/examples/multi.rs | 24 ----- .../tedge_config_macros/impl/src/query.rs | 94 ++++++++++++++----- .../common/tedge_config_macros/src/multi.rs | 9 ++ .../common/tedge_config_macros/tests/arc.rs | 4 +- .../tedge/src/cli/config/commands/unset.rs | 2 +- 5 files changed, 85 insertions(+), 48 deletions(-) diff --git a/crates/common/tedge_config_macros/examples/multi.rs b/crates/common/tedge_config_macros/examples/multi.rs index a26f3b4b7c0..a4f7e870091 100644 --- a/crates/common/tedge_config_macros/examples/multi.rs +++ b/crates/common/tedge_config_macros/examples/multi.rs @@ -5,8 +5,6 @@ use std::num::NonZeroU16; // use std::path::PathBuf; use tedge_config_macros::*; -static DEFAULT_ROOT_CERT_PATH: &str = "/etc/ssl/certs"; - #[derive(thiserror::Error, Debug)] pub enum ReadError { #[error(transparent)] @@ -45,28 +43,6 @@ define_tedge_config! { }, } -fn device_id(_reader: &TEdgeConfigReader) -> Result { - Ok("dummy-device-id".to_owned()) -} - -fn default_device_key(location: &TEdgeConfigLocation) -> Utf8PathBuf { - location - .tedge_config_root_path() - .join("device-certs") - .join("tedge-private-key.pem") -} - -fn default_device_cert(location: &TEdgeConfigLocation) -> Utf8PathBuf { - location - .tedge_config_root_path() - .join("device-certs") - .join("tedge-certificate.pem") -} - -fn default_mqtt_port() -> NonZeroU16 { - NonZeroU16::try_from(1883).unwrap() -} - fn main() { // let dto = TEdgeConfigDto::default(); // dto.mqtt.bind.address = Some(IpAddr::V4(Ipv4Addr::new(1, 2, 3, 4))); diff --git a/crates/common/tedge_config_macros/impl/src/query.rs b/crates/common/tedge_config_macros/impl/src/query.rs index 4f9fca61f1b..e48e239d858 100644 --- a/crates/common/tedge_config_macros/impl/src/query.rs +++ b/crates/common/tedge_config_macros/impl/src/query.rs @@ -1,6 +1,7 @@ use crate::error::extract_type_from_result; use std::iter::once; use itertools::Either; +use proc_macro2::Span; use crate::input::ConfigurableField; use crate::input::FieldOrGroup; use crate::reader::PathItem; @@ -78,6 +79,8 @@ pub fn generate_writable_keys(items: &[FieldOrGroup]) -> TokenStream { pub enum WriteError { #[error("Failed to parse input")] ParseValue(#[from] Box), + #[error(transparent)] + Multi(#[from] ::tedge_config_macros::MultiError), } impl ReadOnlyKey { @@ -351,22 +354,46 @@ fn keys_enum( } } +#[derive(Debug, Default)] +struct NumericIdGenerator { + count: u32, +} + +pub trait IdGenerator: Default { + fn next_id(&mut self, span: Span) -> syn::Ident; +} + +impl IdGenerator for NumericIdGenerator { + fn next_id(&mut self, span: Span) -> syn::Ident { + let i = self.count; + self.count += 1; + syn::Ident::new(&format!("key{i}"), span) + } +} + fn generate_string_readers(paths: &[VecDeque<&FieldOrGroup>]) -> TokenStream { let variant_names = paths.iter().map(variant_name); let arms = paths .iter() .zip(variant_names) - .map(|(path, (enum_variant, iter_variant, match_variant))| -> syn::Arm { + .map(|(path, (_enum_variant, _iter_variant, match_variant))| -> syn::Arm { let field = path .back() .expect("Path must have a back as it is nonempty") .field() .expect("Back of path is guaranteed to be a field"); - let segments = path.iter().map(|&thing| match thing { - FieldOrGroup::Field(f) => { let ident = f.ident(); quote!(#ident) }, - FieldOrGroup::Group(g) => { let ident = &g.ident; quote!(#ident) }, - // TODO don't hardcode key0 that's stupid - FieldOrGroup::Multi(m) => { let ident = &m.ident; quote_spanned!(ident.span()=> #ident.get(key0.as_deref())?) }, + let mut id_gen = NumericIdGenerator::default(); + // TODO deduplicate this logic + let segments = path.iter().map(|&thing| { + let ident = thing.ident(); + match thing { + FieldOrGroup::Field(_) => quote!(#ident), + FieldOrGroup::Group(_) => quote!(#ident), + FieldOrGroup::Multi(_) => { + let field = id_gen.next_id(ident.span()); + quote_spanned!(ident.span()=> #ident.get(#field.as_deref())?) + }, + } }); let to_string = quote_spanned!(field.ty().span()=> .to_string()); if field.read_only().is_some() { @@ -411,8 +438,31 @@ fn generate_string_writers(paths: &[VecDeque<&FieldOrGroup>]) -> TokenStream { ) = paths .iter() .zip(variant_names) - .map(|(path, (enum_variant, iter_variant, match_variant))| { - let segments = path.iter().map(|thing| thing.ident()).collect::>(); + .map(|(path, (_enum_variant, _iter_variant, match_variant))| { + let mut id_gen = NumericIdGenerator::default(); + let read_segments = path.iter().map(|&thing| { + let ident = thing.ident(); + match thing { + FieldOrGroup::Field(_) => quote!(#ident), + FieldOrGroup::Group(_) => quote!(#ident), + FieldOrGroup::Multi(_) => { + let field = id_gen.next_id(ident.span()); + quote_spanned!(ident.span()=> #ident.get(#field.as_deref())?) + }, + } + }).collect::>(); + let mut id_gen = NumericIdGenerator::default(); + let write_segments = path.iter().map(|&thing| { + let ident = thing.ident(); + match thing { + FieldOrGroup::Field(_) => quote!(#ident), + FieldOrGroup::Group(_) => quote!(#ident), + FieldOrGroup::Multi(_) => { + let field = id_gen.next_id(ident.span()); + quote_spanned!(ident.span()=> #ident.get_mut(#field.as_deref())?) + }, + } + }).collect::>(); let field = path .iter() .filter_map(|thing| thing.field()) @@ -426,28 +476,28 @@ fn generate_string_writers(paths: &[VecDeque<&FieldOrGroup>]) -> TokenStream { let current_value = if field.read_only().is_some() { if extract_type_from_result(field.ty()).is_some() { - quote_spanned! {ty.span()=> reader.#(#segments).*.try_read(reader).ok()} + quote_spanned! {ty.span()=> reader.#(#read_segments).*.try_read(reader).ok()} } else { - quote_spanned! {ty.span()=> Some(reader.#(#segments).*.read(reader))} + quote_spanned! {ty.span()=> Some(reader.#(#read_segments).*.read(reader))} } } else if field.has_guaranteed_default() { - quote_spanned! {ty.span()=> Some(reader.#(#segments).*.to_owned())} + quote_spanned! {ty.span()=> Some(reader.#(#read_segments).*.to_owned())} } else { - quote_spanned! {ty.span()=> reader.#(#segments).*.or_none().cloned()} + quote_spanned! {ty.span()=> reader.#(#read_segments).*.or_none().cloned()} }; ( parse_quote_spanned! {ty.span()=> - WritableKey::#match_variant => self.#(#segments).* = Some(value + WritableKey::#match_variant => self.#(#write_segments).* = Some(value .#parse .#convert_to_field_ty .map_err(|e| WriteError::ParseValue(Box::new(e)))?), }, parse_quote_spanned! {ty.span()=> - WritableKey::#match_variant => self.#(#segments).* = None, + WritableKey::#match_variant => self.#(#write_segments).* = None, }, parse_quote_spanned! {ty.span()=> - WritableKey::#match_variant => self.#(#segments).* = <#ty as AppendRemoveItem>::append( + WritableKey::#match_variant => self.#(#write_segments).* = <#ty as AppendRemoveItem>::append( #current_value, value .#parse @@ -455,7 +505,7 @@ fn generate_string_writers(paths: &[VecDeque<&FieldOrGroup>]) -> TokenStream { .map_err(|e| WriteError::ParseValue(Box::new(e)))?), }, parse_quote_spanned! {ty.span()=> - WritableKey::#match_variant => self.#(#segments).* = <#ty as AppendRemoveItem>::remove( + WritableKey::#match_variant => self.#(#write_segments).* = <#ty as AppendRemoveItem>::remove( #current_value, value .#parse @@ -466,29 +516,31 @@ fn generate_string_writers(paths: &[VecDeque<&FieldOrGroup>]) -> TokenStream { }) .multiunzip(); + // TODO do we need an _ branch on these? quote! { impl TEdgeConfigDto { - pub fn try_update_str(&mut self, key: WritableKey, value: &str) -> Result<(), WriteError> { + pub fn try_update_str(&mut self, key: &WritableKey, value: &str) -> Result<(), WriteError> { match key { #(#update_arms)* }; Ok(()) } - pub fn unset_key(&mut self, key: WritableKey) { + pub fn try_unset_key(&mut self, key: &WritableKey) -> Result<(), WriteError> { match key { #(#unset_arms)* - } + }; + Ok(()) } - pub fn try_append_str(&mut self, reader: &TEdgeConfigReader, key: WritableKey, value: &str) -> Result<(), WriteError> { + pub fn try_append_str(&mut self, reader: &TEdgeConfigReader, key: &WritableKey, value: &str) -> Result<(), WriteError> { match key { #(#append_arms)* }; Ok(()) } - pub fn try_remove_str(&mut self, reader: &TEdgeConfigReader, key: WritableKey, value: &str) -> Result<(), WriteError> { + pub fn try_remove_str(&mut self, reader: &TEdgeConfigReader, key: &WritableKey, value: &str) -> Result<(), WriteError> { match key { #(#remove_arms)* }; diff --git a/crates/common/tedge_config_macros/src/multi.rs b/crates/common/tedge_config_macros/src/multi.rs index 47b28ba6493..d258f5d4503 100644 --- a/crates/common/tedge_config_macros/src/multi.rs +++ b/crates/common/tedge_config_macros/src/multi.rs @@ -40,6 +40,15 @@ impl Multi { } } + pub fn get_mut(&mut self, key: Option<&str>) -> Result<&mut T, MultiError> { + match (self, key) { + (Self::Single(val), None) => Ok(val), + (Self::Multi(map), Some(key)) => map.get_mut(key).ok_or(MultiError::MultiKeyNotFound), + (Self::Multi(_), None) => Err(MultiError::SingleNotMulti), + (Self::Single(_), Some(_key)) => Err(MultiError::MultiNotSingle), + } + } + pub fn keys(&self) -> impl Iterator> { match self { Self::Single(_) => itertools::Either::Left(std::iter::once(None)), diff --git a/crates/common/tedge_config_macros/tests/arc.rs b/crates/common/tedge_config_macros/tests/arc.rs index d1e8fd0d191..681dd9ac50f 100644 --- a/crates/common/tedge_config_macros/tests/arc.rs +++ b/crates/common/tedge_config_macros/tests/arc.rs @@ -41,10 +41,10 @@ define_tedge_config! { fn arc_str_in_config_can_be_updated() { let mut config = TEdgeConfigDto::default(); config - .try_update_str(WritableKey::DeviceType, "new value") + .try_update_str(&WritableKey::DeviceType, "new value") .unwrap(); config - .try_update_str(WritableKey::DeviceManualFrom, "different value") + .try_update_str(&WritableKey::DeviceManualFrom, "different value") .unwrap(); assert_eq!(config.device.ty, Some(Arc::from("new value"))); diff --git a/crates/core/tedge/src/cli/config/commands/unset.rs b/crates/core/tedge/src/cli/config/commands/unset.rs index 5e4ede0490d..7504b7afa05 100644 --- a/crates/core/tedge/src/cli/config/commands/unset.rs +++ b/crates/core/tedge/src/cli/config/commands/unset.rs @@ -14,7 +14,7 @@ impl Command for UnsetConfigCommand { fn execute(&self) -> anyhow::Result<()> { self.config_location.update_toml(&|dto, _reader| { - dto.unset_key(self.key); + dto.try_unset_key(&self.key); Ok(()) })?; Ok(()) From 5dcdfaff60a1d32d50e49a190b57fe08df85b804 Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Fri, 20 Sep 2024 12:38:13 +0100 Subject: [PATCH 07/32] Update tedge config API usage so the existing tedge config works Signed-off-by: James Rhodes --- crates/common/tedge_config_macros/examples/macro.rs | 2 +- crates/common/tedge_config_macros/examples/multi.rs | 1 + crates/common/tedge_config_macros/impl/src/query.rs | 9 +++------ crates/core/tedge/src/cli/config/commands/add.rs | 2 +- crates/core/tedge/src/cli/config/commands/get.rs | 2 +- crates/core/tedge/src/cli/config/commands/list.rs | 3 ++- crates/core/tedge/src/cli/config/commands/remove.rs | 2 +- crates/core/tedge/src/cli/config/commands/set.rs | 2 +- crates/core/tedge/src/cli/config/commands/unset.rs | 3 +-- 9 files changed, 12 insertions(+), 14 deletions(-) diff --git a/crates/common/tedge_config_macros/examples/macro.rs b/crates/common/tedge_config_macros/examples/macro.rs index 645fc2c2f1b..bc6a02cc46e 100644 --- a/crates/common/tedge_config_macros/examples/macro.rs +++ b/crates/common/tedge_config_macros/examples/macro.rs @@ -227,7 +227,7 @@ fn main() { assert_eq!(u16::from(config.mqtt.bind.port), 1883); assert_eq!(config.mqtt.external.bind.port.or_none(), None); assert_eq!( - config.read_string(ReadableKey::DeviceId).unwrap(), + config.read_string(&ReadableKey::DeviceId).unwrap(), "dummy-device-id" ); } diff --git a/crates/common/tedge_config_macros/examples/multi.rs b/crates/common/tedge_config_macros/examples/multi.rs index a4f7e870091..6324551876f 100644 --- a/crates/common/tedge_config_macros/examples/multi.rs +++ b/crates/common/tedge_config_macros/examples/multi.rs @@ -35,6 +35,7 @@ impl AppendRemoveItem for T { } } +#[allow(dead_code)] define_tedge_config! { #[tedge_config(multi)] c8y: { diff --git a/crates/common/tedge_config_macros/impl/src/query.rs b/crates/common/tedge_config_macros/impl/src/query.rs index e48e239d858..63684b1f667 100644 --- a/crates/common/tedge_config_macros/impl/src/query.rs +++ b/crates/common/tedge_config_macros/impl/src/query.rs @@ -1,10 +1,7 @@ use crate::error::extract_type_from_result; -use std::iter::once; -use itertools::Either; use proc_macro2::Span; use crate::input::ConfigurableField; use crate::input::FieldOrGroup; -use crate::reader::PathItem; use heck::ToUpperCamelCase; use itertools::Itertools; use proc_macro2::TokenStream; @@ -17,7 +14,7 @@ use syn::spanned::Spanned; pub fn generate_writable_keys(items: &[FieldOrGroup]) -> TokenStream { let paths = configuration_paths_from(items); - let (readonly_variant, readonly_iter, readonly_destr, write_error): ( + let (_readonly_variant, _readonly_iter, readonly_destr, write_error): ( Vec<_>, Vec<_>, Vec<_>, @@ -197,7 +194,7 @@ fn deprecated_keys<'a>( fn generate_fromstr( type_name: syn::Ident, - (configuration_string, enum_variant, iter_variant, match_variant): &( + (configuration_string, enum_variant, iter_variant, _match_variant): &( Vec, Vec, Vec, @@ -419,7 +416,7 @@ fn generate_string_readers(paths: &[VecDeque<&FieldOrGroup>]) -> TokenStream { }); quote! { impl TEdgeConfigReader { - pub fn read_string(&self, key: ReadableKey) -> Result { + pub fn read_string(&self, key: &ReadableKey) -> Result { match key { #(#arms)* } diff --git a/crates/core/tedge/src/cli/config/commands/add.rs b/crates/core/tedge/src/cli/config/commands/add.rs index 38d1a804973..8f1fa905544 100644 --- a/crates/core/tedge/src/cli/config/commands/add.rs +++ b/crates/core/tedge/src/cli/config/commands/add.rs @@ -19,7 +19,7 @@ impl Command for AddConfigCommand { fn execute(&self) -> anyhow::Result<()> { self.config_location.update_toml(&|dto, reader| { - dto.try_append_str(reader, self.key, &self.value) + dto.try_append_str(reader, &self.key, &self.value) .map_err(|e| e.into()) })?; Ok(()) diff --git a/crates/core/tedge/src/cli/config/commands/get.rs b/crates/core/tedge/src/cli/config/commands/get.rs index 7fca19aedec..8683de922a8 100644 --- a/crates/core/tedge/src/cli/config/commands/get.rs +++ b/crates/core/tedge/src/cli/config/commands/get.rs @@ -13,7 +13,7 @@ impl Command for GetConfigCommand { } fn execute(&self) -> anyhow::Result<()> { - match self.config.read_string(self.key) { + match self.config.read_string(&self.key) { Ok(value) => { println!("{}", value); } diff --git a/crates/core/tedge/src/cli/config/commands/list.rs b/crates/core/tedge/src/cli/config/commands/list.rs index 2b38e50eeb5..f9f0b5a2542 100644 --- a/crates/core/tedge/src/cli/config/commands/list.rs +++ b/crates/core/tedge/src/cli/config/commands/list.rs @@ -31,8 +31,9 @@ impl Command for ListConfigCommand { fn print_config_list(config: &TEdgeConfig, all: bool) -> Result<(), ConfigError> { let mut keys_without_values = Vec::new(); + // TODO fix this logic, it's just broken atm for config_key in ReadableKey::iter() { - match config.read_string(config_key).ok() { + match config.read_string(&config_key).ok() { Some(value) => { println!("{}={}", config_key, value); } diff --git a/crates/core/tedge/src/cli/config/commands/remove.rs b/crates/core/tedge/src/cli/config/commands/remove.rs index cf0ff19a823..dca3adb09cd 100644 --- a/crates/core/tedge/src/cli/config/commands/remove.rs +++ b/crates/core/tedge/src/cli/config/commands/remove.rs @@ -15,7 +15,7 @@ impl Command for RemoveConfigCommand { fn execute(&self) -> anyhow::Result<()> { self.config_location.update_toml(&|dto, reader| { - dto.try_remove_str(reader, self.key, &self.value) + dto.try_remove_str(reader, &self.key, &self.value) .map_err(|e| e.into()) })?; Ok(()) diff --git a/crates/core/tedge/src/cli/config/commands/set.rs b/crates/core/tedge/src/cli/config/commands/set.rs index d220eda9e19..b109f624000 100644 --- a/crates/core/tedge/src/cli/config/commands/set.rs +++ b/crates/core/tedge/src/cli/config/commands/set.rs @@ -19,7 +19,7 @@ impl Command for SetConfigCommand { fn execute(&self) -> anyhow::Result<()> { self.config_location.update_toml(&|dto, _reader| { - dto.try_update_str(self.key, &self.value) + dto.try_update_str(&self.key, &self.value) .map_err(|e| e.into()) })?; Ok(()) diff --git a/crates/core/tedge/src/cli/config/commands/unset.rs b/crates/core/tedge/src/cli/config/commands/unset.rs index 7504b7afa05..4bcd6ef2238 100644 --- a/crates/core/tedge/src/cli/config/commands/unset.rs +++ b/crates/core/tedge/src/cli/config/commands/unset.rs @@ -14,8 +14,7 @@ impl Command for UnsetConfigCommand { fn execute(&self) -> anyhow::Result<()> { self.config_location.update_toml(&|dto, _reader| { - dto.try_unset_key(&self.key); - Ok(()) + Ok(dto.try_unset_key(&self.key)?) })?; Ok(()) } From a879f501a669223985bcc20bc742bdbca6ed3aa1 Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Fri, 20 Sep 2024 16:05:37 +0100 Subject: [PATCH 08/32] Run formatter and fix some remaining errors Signed-off-by: James Rhodes --- .../tedge_config_macros/examples/multi.rs | 2 +- .../tedge_config_macros/impl/src/query.rs | 24 +++++++++++++++---- crates/common/tedge_config_macros/src/lib.rs | 4 ++-- .../common/tedge_config_macros/src/multi.rs | 10 +++++--- .../tedge/src/cli/config/commands/list.rs | 2 +- .../tedge/src/cli/config/commands/unset.rs | 5 ++-- crates/core/tedge/src/command.rs | 2 +- 7 files changed, 33 insertions(+), 16 deletions(-) diff --git a/crates/common/tedge_config_macros/examples/multi.rs b/crates/common/tedge_config_macros/examples/multi.rs index 6324551876f..5d1da49b133 100644 --- a/crates/common/tedge_config_macros/examples/multi.rs +++ b/crates/common/tedge_config_macros/examples/multi.rs @@ -12,7 +12,7 @@ pub enum ReadError { #[error("Something went wrong: {0}")] GenericError(String), #[error(transparent)] - Multi(#[from] tedge_config_macros::MultiError) + Multi(#[from] tedge_config_macros::MultiError), } pub trait AppendRemoveItem { diff --git a/crates/common/tedge_config_macros/impl/src/query.rs b/crates/common/tedge_config_macros/impl/src/query.rs index 63684b1f667..84148ac84ac 100644 --- a/crates/common/tedge_config_macros/impl/src/query.rs +++ b/crates/common/tedge_config_macros/impl/src/query.rs @@ -1,9 +1,9 @@ use crate::error::extract_type_from_result; -use proc_macro2::Span; use crate::input::ConfigurableField; use crate::input::FieldOrGroup; use heck::ToUpperCamelCase; use itertools::Itertools; +use proc_macro2::Span; use proc_macro2::TokenStream; use quote::quote; use quote::quote_spanned; @@ -60,6 +60,11 @@ pub fn generate_writable_keys(items: &[FieldOrGroup]) -> TokenStream { ); let (static_alias, _, iter_updated, _) = deprecated_keys(paths.iter()); + let fallback_branch: Option = readonly_args + .0 + .is_empty() + .then(|| parse_quote!(_ => unreachable!("ReadOnlyKey is uninhabited"))); + quote! { #readable_keys #readonly_keys @@ -88,8 +93,7 @@ pub fn generate_writable_keys(items: &[FieldOrGroup]) -> TokenStream { #[allow(unused)] Self::#readonly_destr => #write_error, )* - // TODO make this conditional - _ => unimplemented!("Cope with empty enum") + #fallback_branch } } } @@ -379,7 +383,7 @@ fn generate_string_readers(paths: &[VecDeque<&FieldOrGroup>]) -> TokenStream { .expect("Path must have a back as it is nonempty") .field() .expect("Back of path is guaranteed to be a field"); - let mut id_gen = NumericIdGenerator::default(); + let mut id_gen = NumericIdGenerator::default(); // TODO deduplicate this logic let segments = path.iter().map(|&thing| { let ident = thing.ident(); @@ -414,11 +418,15 @@ fn generate_string_readers(paths: &[VecDeque<&FieldOrGroup>]) -> TokenStream { } } }); + let fallback_branch: Option = paths + .is_empty() + .then(|| parse_quote!(_ => unreachable!("ReadableKey is uninhabited"))); quote! { impl TEdgeConfigReader { pub fn read_string(&self, key: &ReadableKey) -> Result { match key { #(#arms)* + #fallback_branch } } } @@ -512,13 +520,16 @@ fn generate_string_writers(paths: &[VecDeque<&FieldOrGroup>]) -> TokenStream { ) }) .multiunzip(); + let fallback_branch: Option = update_arms + .is_empty() + .then(|| parse_quote!(_ => unreachable!("WritableKey is uninhabited"))); - // TODO do we need an _ branch on these? quote! { impl TEdgeConfigDto { pub fn try_update_str(&mut self, key: &WritableKey, value: &str) -> Result<(), WriteError> { match key { #(#update_arms)* + #fallback_branch }; Ok(()) } @@ -526,6 +537,7 @@ fn generate_string_writers(paths: &[VecDeque<&FieldOrGroup>]) -> TokenStream { pub fn try_unset_key(&mut self, key: &WritableKey) -> Result<(), WriteError> { match key { #(#unset_arms)* + #fallback_branch }; Ok(()) } @@ -533,6 +545,7 @@ fn generate_string_writers(paths: &[VecDeque<&FieldOrGroup>]) -> TokenStream { pub fn try_append_str(&mut self, reader: &TEdgeConfigReader, key: &WritableKey, value: &str) -> Result<(), WriteError> { match key { #(#append_arms)* + #fallback_branch }; Ok(()) } @@ -540,6 +553,7 @@ fn generate_string_writers(paths: &[VecDeque<&FieldOrGroup>]) -> TokenStream { pub fn try_remove_str(&mut self, reader: &TEdgeConfigReader, key: &WritableKey, value: &str) -> Result<(), WriteError> { match key { #(#remove_arms)* + #fallback_branch }; Ok(()) } diff --git a/crates/common/tedge_config_macros/src/lib.rs b/crates/common/tedge_config_macros/src/lib.rs index bfc56c376eb..9fff212874e 100644 --- a/crates/common/tedge_config_macros/src/lib.rs +++ b/crates/common/tedge_config_macros/src/lib.rs @@ -7,8 +7,8 @@ pub use all_or_nothing::*; pub use connect_url::*; pub use default::*; pub use doku_aliases::*; -pub use option::*; pub use multi::*; +pub use option::*; mod all_or_nothing; mod connect_url; @@ -16,5 +16,5 @@ mod default; mod doku_aliases; #[cfg(doc)] pub mod example; -mod option; mod multi; +mod option; diff --git a/crates/common/tedge_config_macros/src/multi.rs b/crates/common/tedge_config_macros/src/multi.rs index d258f5d4503..4ffd9230cce 100644 --- a/crates/common/tedge_config_macros/src/multi.rs +++ b/crates/common/tedge_config_macros/src/multi.rs @@ -2,10 +2,10 @@ #[serde(untagged)] pub enum Multi { Single(T), - Multi(::std::collections::HashMap) + Multi(::std::collections::HashMap), } -impl Default for Multi { +impl Default for Multi { fn default() -> Self { Self::Single(T::default()) } @@ -60,7 +60,11 @@ impl Multi { pub fn map(&self, f: impl Fn(Option<&str>) -> U) -> Multi { match self { Self::Single(_) => Multi::Single(f(None)), - Self::Multi(map) => Multi::Multi(map.keys().map(|key| (key.to_owned(), f(Some(key)))).collect()) + Self::Multi(map) => Multi::Multi( + map.keys() + .map(|key| (key.to_owned(), f(Some(key)))) + .collect(), + ), } } } diff --git a/crates/core/tedge/src/cli/config/commands/list.rs b/crates/core/tedge/src/cli/config/commands/list.rs index f9f0b5a2542..ceab4648103 100644 --- a/crates/core/tedge/src/cli/config/commands/list.rs +++ b/crates/core/tedge/src/cli/config/commands/list.rs @@ -31,7 +31,7 @@ impl Command for ListConfigCommand { fn print_config_list(config: &TEdgeConfig, all: bool) -> Result<(), ConfigError> { let mut keys_without_values = Vec::new(); - // TODO fix this logic, it's just broken atm + // TODO fix this logic, it's just broken for multi variants atm for config_key in ReadableKey::iter() { match config.read_string(&config_key).ok() { Some(value) => { diff --git a/crates/core/tedge/src/cli/config/commands/unset.rs b/crates/core/tedge/src/cli/config/commands/unset.rs index 4bcd6ef2238..4262ab568ac 100644 --- a/crates/core/tedge/src/cli/config/commands/unset.rs +++ b/crates/core/tedge/src/cli/config/commands/unset.rs @@ -13,9 +13,8 @@ impl Command for UnsetConfigCommand { } fn execute(&self) -> anyhow::Result<()> { - self.config_location.update_toml(&|dto, _reader| { - Ok(dto.try_unset_key(&self.key)?) - })?; + self.config_location + .update_toml(&|dto, _reader| Ok(dto.try_unset_key(&self.key)?))?; Ok(()) } } diff --git a/crates/core/tedge/src/command.rs b/crates/core/tedge/src/command.rs index 89429a2e82f..8c7c90c8447 100644 --- a/crates/core/tedge/src/command.rs +++ b/crates/core/tedge/src/command.rs @@ -44,7 +44,7 @@ use std::path::Path; /// } /// /// fn execute(&self) -> anyhow::Result<()> { -/// match self.config.read_string(self.key) { +/// match self.config.read_string(&self.key) { /// Ok(value) => println!("{}", value), /// Err(ReadError::ConfigNotSet(_)) => eprintln!("The configuration key `{}` is not set", self.key), /// Err(e) => return Err(e.into()), From 56525cc5b5e825ff594c6412feeaacf908f7304f Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Fri, 20 Sep 2024 16:22:48 +0100 Subject: [PATCH 09/32] Add a proper example for multi c8y config Signed-off-by: James Rhodes --- Cargo.lock | 1 + crates/common/tedge_config_macros/Cargo.toml | 1 + .../tedge_config_macros/examples/multi.rs | 53 +++++++++++-------- .../common/tedge_config_macros/src/multi.rs | 11 ++-- 4 files changed, 39 insertions(+), 27 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c2c502cf312..54338253b25 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3954,6 +3954,7 @@ dependencies = [ "serde", "tedge_config_macros-macro", "thiserror", + "toml 0.7.8", "tracing", "url", ] diff --git a/crates/common/tedge_config_macros/Cargo.toml b/crates/common/tedge_config_macros/Cargo.toml index 8bf5edab47c..b3199712466 100644 --- a/crates/common/tedge_config_macros/Cargo.toml +++ b/crates/common/tedge_config_macros/Cargo.toml @@ -21,6 +21,7 @@ url = { workspace = true } [dev-dependencies] serde = { workspace = true, features = ["rc"] } +toml = { workspace = true } [lints] workspace = true diff --git a/crates/common/tedge_config_macros/examples/multi.rs b/crates/common/tedge_config_macros/examples/multi.rs index 5d1da49b133..91ad1ad065a 100644 --- a/crates/common/tedge_config_macros/examples/multi.rs +++ b/crates/common/tedge_config_macros/examples/multi.rs @@ -1,8 +1,3 @@ -use camino::Utf8PathBuf; -// use std::net::IpAddr; -// use std::net::Ipv4Addr; -use std::num::NonZeroU16; -// use std::path::PathBuf; use tedge_config_macros::*; #[derive(thiserror::Error, Debug)] @@ -35,7 +30,6 @@ impl AppendRemoveItem for T { } } -#[allow(dead_code)] define_tedge_config! { #[tedge_config(multi)] c8y: { @@ -44,23 +38,38 @@ define_tedge_config! { }, } +fn url_for<'a>(reader: &'a TEdgeConfigReader, o: Option<&str>) -> &'a str { + reader.c8y.get(o).unwrap().url.or_config_not_set().unwrap() +} + fn main() { - // let dto = TEdgeConfigDto::default(); - // dto.mqtt.bind.address = Some(IpAddr::V4(Ipv4Addr::new(1, 2, 3, 4))); + let single_c8y_toml = "c8y.url = \"https://example.com\""; + let single_c8y_dto = toml::from_str(&single_c8y_toml).unwrap(); + let single_c8y_reader = TEdgeConfigReader::from_dto(&single_c8y_dto, &TEdgeConfigLocation); + assert_eq!(url_for(&single_c8y_reader, None), "https://example.com"); - // let config = TEdgeConfigReader::from_dto(&dto, &TEdgeConfigLocation); + let multi_c8y_toml = "c8y.cloud.url = \"https://cloud.example.com\"\nc8y.edge.url = \"https://edge.example.com\""; + let multi_c8y_dto = toml::from_str(&multi_c8y_toml).unwrap(); + let multi_c8y_reader = TEdgeConfigReader::from_dto(&multi_c8y_dto, &TEdgeConfigLocation); + assert_eq!( + url_for(&multi_c8y_reader, Some("cloud")), + "https://cloud.example.com" + ); + assert_eq!( + url_for(&multi_c8y_reader, Some("edge")), + "https://edge.example.com" + ); - // Typed reads - // println!( - // "Device id is {}.", - // // We have to pass the config into try_read to avoid TEdgeConfigReader being - // // self-referential - // config.device.id.try_read(&config).as_ref().unwrap() - // ); - // assert_eq!(u16::from(config.mqtt.bind.port), 1883); - // assert_eq!(config.mqtt.external.bind.port.or_none(), None); - // assert_eq!( - // config.read_string(ReadableKey::DeviceId).unwrap(), - // "dummy-device-id" - // ); + assert!(matches!( + single_c8y_reader.c8y.get(Some("cloud")), + Err(MultiError::SingleNotMulti) + )); + assert!(matches!( + multi_c8y_reader.c8y.get(Some("unknown")), + Err(MultiError::MultiKeyNotFound) + )); + assert!(matches!( + multi_c8y_reader.c8y.get(None), + Err(MultiError::MultiNotSingle) + )); } diff --git a/crates/common/tedge_config_macros/src/multi.rs b/crates/common/tedge_config_macros/src/multi.rs index 4ffd9230cce..6e2a4a95818 100644 --- a/crates/common/tedge_config_macros/src/multi.rs +++ b/crates/common/tedge_config_macros/src/multi.rs @@ -1,8 +1,9 @@ #[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize, doku::Document)] #[serde(untagged)] pub enum Multi { - Single(T), + // TODO The ordering of fields is important since we have a default value for T - add a test for it Multi(::std::collections::HashMap), + Single(T), } impl Default for Multi { @@ -35,8 +36,8 @@ impl Multi { match (self, key) { (Self::Single(val), None) => Ok(val), (Self::Multi(map), Some(key)) => map.get(key).ok_or(MultiError::MultiKeyNotFound), - (Self::Multi(_), None) => Err(MultiError::SingleNotMulti), - (Self::Single(_), Some(_key)) => Err(MultiError::MultiNotSingle), + (Self::Multi(_), None) => Err(MultiError::MultiNotSingle), + (Self::Single(_), Some(_key)) => Err(MultiError::SingleNotMulti), } } @@ -44,8 +45,8 @@ impl Multi { match (self, key) { (Self::Single(val), None) => Ok(val), (Self::Multi(map), Some(key)) => map.get_mut(key).ok_or(MultiError::MultiKeyNotFound), - (Self::Multi(_), None) => Err(MultiError::SingleNotMulti), - (Self::Single(_), Some(_key)) => Err(MultiError::MultiNotSingle), + (Self::Multi(_), None) => Err(MultiError::MultiNotSingle), + (Self::Single(_), Some(_key)) => Err(MultiError::SingleNotMulti), } } From 0baeb4cdc8365313f9f1454993dd37b37254e8cd Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Fri, 20 Sep 2024 17:33:02 +0100 Subject: [PATCH 10/32] Refactor some of the ID generation logic Signed-off-by: James Rhodes --- .../tedge_config_macros/examples/multi.rs | 4 +- .../tedge_config_macros/impl/src/dto.rs | 2 + .../tedge_config_macros/impl/src/query.rs | 229 +++++++++--------- 3 files changed, 116 insertions(+), 119 deletions(-) diff --git a/crates/common/tedge_config_macros/examples/multi.rs b/crates/common/tedge_config_macros/examples/multi.rs index 91ad1ad065a..99ad67bd930 100644 --- a/crates/common/tedge_config_macros/examples/multi.rs +++ b/crates/common/tedge_config_macros/examples/multi.rs @@ -44,12 +44,12 @@ fn url_for<'a>(reader: &'a TEdgeConfigReader, o: Option<&str>) -> &'a str { fn main() { let single_c8y_toml = "c8y.url = \"https://example.com\""; - let single_c8y_dto = toml::from_str(&single_c8y_toml).unwrap(); + let single_c8y_dto = toml::from_str(single_c8y_toml).unwrap(); let single_c8y_reader = TEdgeConfigReader::from_dto(&single_c8y_dto, &TEdgeConfigLocation); assert_eq!(url_for(&single_c8y_reader, None), "https://example.com"); let multi_c8y_toml = "c8y.cloud.url = \"https://cloud.example.com\"\nc8y.edge.url = \"https://edge.example.com\""; - let multi_c8y_dto = toml::from_str(&multi_c8y_toml).unwrap(); + let multi_c8y_dto = toml::from_str(multi_c8y_toml).unwrap(); let multi_c8y_reader = TEdgeConfigReader::from_dto(&multi_c8y_dto, &TEdgeConfigLocation); assert_eq!( url_for(&multi_c8y_reader, Some("cloud")), diff --git a/crates/common/tedge_config_macros/impl/src/dto.rs b/crates/common/tedge_config_macros/impl/src/dto.rs index cebdec0eb60..1dbf7c944df 100644 --- a/crates/common/tedge_config_macros/impl/src/dto.rs +++ b/crates/common/tedge_config_macros/impl/src/dto.rs @@ -84,6 +84,8 @@ pub fn generate( } impl #name { + // If #name is a "multi" field, we don't use this method, but it's a pain to conditionally generate it, so just ignore the warning + #[allow(unused)] fn is_default(&self) -> bool { self == &Self::default() } diff --git a/crates/common/tedge_config_macros/impl/src/query.rs b/crates/common/tedge_config_macros/impl/src/query.rs index 84148ac84ac..d53fd3715cc 100644 --- a/crates/common/tedge_config_macros/impl/src/query.rs +++ b/crates/common/tedge_config_macros/impl/src/query.rs @@ -14,19 +14,12 @@ use syn::spanned::Spanned; pub fn generate_writable_keys(items: &[FieldOrGroup]) -> TokenStream { let paths = configuration_paths_from(items); - let (_readonly_variant, _readonly_iter, readonly_destr, write_error): ( - Vec<_>, - Vec<_>, - Vec<_>, - Vec<_>, - ) = paths + let (readonly_destr, write_error): (Vec<_>, Vec<_>) = paths .iter() .filter_map(|field| { - let (enum_field, iter_field, destr_field) = variant_name(field); + let configuration = variant_name(field); Some(( - enum_field, - iter_field, - destr_field, + configuration.match_shape, field .back()? .field()? @@ -58,7 +51,8 @@ pub fn generate_writable_keys(items: &[FieldOrGroup]) -> TokenStream { .cloned() .collect::>(), ); - let (static_alias, _, iter_updated, _) = deprecated_keys(paths.iter()); + let (static_alias, deprecated_keys) = deprecated_keys(paths.iter()); + let iter_updated = deprecated_keys.iter().map(|k| &k.iter_field); let fallback_branch: Option = readonly_args .0 @@ -88,11 +82,7 @@ pub fn generate_writable_keys(items: &[FieldOrGroup]) -> TokenStream { impl ReadOnlyKey { fn write_error(&self) -> &'static str { match self { - // TODO these should be underscores - #( - #[allow(unused)] - Self::#readonly_destr => #write_error, - )* + #(Self::#readonly_destr => #write_error,)* #fallback_branch } } @@ -149,37 +139,25 @@ pub fn generate_writable_keys(items: &[FieldOrGroup]) -> TokenStream { fn configuration_strings<'a>( variants: impl Iterator>, -) -> ( - Vec, - Vec, - Vec, - Vec, -) { +) -> (Vec, Vec) { variants .map(|segments| { - let (x, y, z) = variant_name(segments); + let configuration_key = variant_name(segments); ( segments .iter() .map(|variant| variant.name()) .collect::>() .join("."), - x, - y, - z, + configuration_key, ) }) - .multiunzip() + .unzip() } fn deprecated_keys<'a>( variants: impl Iterator>, -) -> ( - Vec<&'a str>, - Vec, - Vec, - Vec, -) { +) -> (Vec<&'a str>, Vec) { variants .flat_map(|segments| { segments @@ -189,8 +167,8 @@ fn deprecated_keys<'a>( .unwrap() .deprecated_keys() .map(|key| { - let (x, y, z) = variant_name(segments); - (key, x, y, z) + let configuration_key = variant_name(segments); + (key, configuration_key) }) }) .multiunzip() @@ -198,19 +176,15 @@ fn deprecated_keys<'a>( fn generate_fromstr( type_name: syn::Ident, - (configuration_string, enum_variant, iter_variant, _match_variant): &( - Vec, - Vec, - Vec, - Vec, - ), + (configuration_string, configuration_key): &(Vec, Vec), error_case: syn::Arm, ) -> TokenStream { let simplified_configuration_string = configuration_string .iter() .map(|s| s.replace('.', "_")) - .zip(enum_variant) + .zip(configuration_key.iter().map(|k| &k.enum_variant)) .map(|(s, v)| quote_spanned!(v.span()=> #s)); + let iter_variant = configuration_key.iter().map(|k| &k.iter_field); // TODO oh shit make this actually work! quote! { @@ -237,12 +211,7 @@ fn generate_fromstr( fn generate_fromstr_readable( type_name: syn::Ident, - fields: &( - Vec, - Vec, - Vec, - Vec, - ), + fields: &(Vec, Vec), ) -> TokenStream { generate_fromstr( type_name, @@ -254,12 +223,7 @@ fn generate_fromstr_readable( // TODO test the error messages actually appear fn generate_fromstr_writable( type_name: syn::Ident, - fields: &( - Vec, - Vec, - Vec, - Vec, - ), + fields: &(Vec, Vec), ) -> TokenStream { generate_fromstr( type_name, @@ -276,16 +240,12 @@ fn generate_fromstr_writable( fn keys_enum( type_name: syn::Ident, - (configuration_string, enum_variant, iter_variant, match_variant): &( - Vec, - Vec, - Vec, - Vec, - ), + (configuration_string, configuration_key): &(Vec, Vec), doc_fragment: &'static str, ) -> TokenStream { - let as_str_example = iter_variant + let as_str_example = configuration_key .iter() + .map(|k| &k.iter_field) .zip(configuration_string.iter()) .map(|(ident, value)| { format!( @@ -307,6 +267,9 @@ fn keys_enum( } }); let type_name_str = type_name.to_string(); + let enum_variant = configuration_key.iter().map(|k| &k.enum_variant); + let match_shape = configuration_key.iter().map(|k| &k.match_shape); + let iter_field = configuration_key.iter().map(|k| &k.iter_field); quote! { #[derive(Clone, Debug, PartialEq, Eq)] @@ -330,7 +293,7 @@ fn keys_enum( pub fn as_str(&self) -> &'static str { match self { #( - Self::#match_variant => #configuration_string, + Self::#match_shape => #configuration_string, )* // TODO make this conditional _ => unimplemented!("Cope with empty enum") @@ -341,7 +304,7 @@ fn keys_enum( pub fn iter() -> impl Iterator { [ #( - Self::#iter_variant, + Self::#iter_field, )* ].into_iter() } @@ -356,15 +319,18 @@ fn keys_enum( } #[derive(Debug, Default)] -struct NumericIdGenerator { +struct SequentialIdGenerator { count: u32, } +#[derive(Debug, Default)] +struct UnderscoreIdGenerator; + pub trait IdGenerator: Default { fn next_id(&mut self, span: Span) -> syn::Ident; } -impl IdGenerator for NumericIdGenerator { +impl IdGenerator for SequentialIdGenerator { fn next_id(&mut self, span: Span) -> syn::Ident { let i = self.count; self.count += 1; @@ -372,31 +338,59 @@ impl IdGenerator for NumericIdGenerator { } } +impl Iterator for SequentialIdGenerator { + type Item = syn::Ident; + fn next(&mut self) -> Option { + Some(self.next_id(Span::call_site())) + } +} + +impl IdGenerator for UnderscoreIdGenerator { + fn next_id(&mut self, span: Span) -> syn::Ident { + syn::Ident::new("_", span) + } +} + +impl Iterator for UnderscoreIdGenerator { + type Item = syn::Ident; + fn next(&mut self) -> Option { + Some(self.next_id(Span::call_site())) + } +} + +fn generate_field_accessor<'a>( + fields: &'a VecDeque<&FieldOrGroup>, + method: &'a str, +) -> impl Iterator + 'a { + let mut id_gen = SequentialIdGenerator::default(); + let method = syn::Ident::new(method, Span::call_site()); + fields.iter().map(move |field| { + let ident = field.ident(); + match field { + FieldOrGroup::Field(_) => quote!(#ident), + FieldOrGroup::Group(_) => quote!(#ident), + FieldOrGroup::Multi(_) => { + let field = id_gen.next_id(ident.span()); + quote_spanned!(ident.span()=> #ident.#method(#field.as_deref())?) + } + } + }) +} + fn generate_string_readers(paths: &[VecDeque<&FieldOrGroup>]) -> TokenStream { let variant_names = paths.iter().map(variant_name); let arms = paths .iter() .zip(variant_names) - .map(|(path, (_enum_variant, _iter_variant, match_variant))| -> syn::Arm { + .map(|(path, configuration_key)| -> syn::Arm { let field = path .back() .expect("Path must have a back as it is nonempty") .field() .expect("Back of path is guaranteed to be a field"); - let mut id_gen = NumericIdGenerator::default(); - // TODO deduplicate this logic - let segments = path.iter().map(|&thing| { - let ident = thing.ident(); - match thing { - FieldOrGroup::Field(_) => quote!(#ident), - FieldOrGroup::Group(_) => quote!(#ident), - FieldOrGroup::Multi(_) => { - let field = id_gen.next_id(ident.span()); - quote_spanned!(ident.span()=> #ident.get(#field.as_deref())?) - }, - } - }); + let segments = generate_field_accessor(path, "get"); let to_string = quote_spanned!(field.ty().span()=> .to_string()); + let match_variant = configuration_key.match_read_write; if field.read_only().is_some() { if extract_type_from_result(field.ty()).is_some() { // TODO test whether the wrong type fails unit tests @@ -443,36 +437,15 @@ fn generate_string_writers(paths: &[VecDeque<&FieldOrGroup>]) -> TokenStream { ) = paths .iter() .zip(variant_names) - .map(|(path, (_enum_variant, _iter_variant, match_variant))| { - let mut id_gen = NumericIdGenerator::default(); - let read_segments = path.iter().map(|&thing| { - let ident = thing.ident(); - match thing { - FieldOrGroup::Field(_) => quote!(#ident), - FieldOrGroup::Group(_) => quote!(#ident), - FieldOrGroup::Multi(_) => { - let field = id_gen.next_id(ident.span()); - quote_spanned!(ident.span()=> #ident.get(#field.as_deref())?) - }, - } - }).collect::>(); - let mut id_gen = NumericIdGenerator::default(); - let write_segments = path.iter().map(|&thing| { - let ident = thing.ident(); - match thing { - FieldOrGroup::Field(_) => quote!(#ident), - FieldOrGroup::Group(_) => quote!(#ident), - FieldOrGroup::Multi(_) => { - let field = id_gen.next_id(ident.span()); - quote_spanned!(ident.span()=> #ident.get_mut(#field.as_deref())?) - }, - } - }).collect::>(); + .map(|(path, configuration_key)| { + let read_segments = generate_field_accessor(path, "get"); + let write_segments = generate_field_accessor(path, "get_mut").collect::>(); let field = path .iter() .filter_map(|thing| thing.field()) .next() .unwrap(); + let match_variant = configuration_key.match_read_write; let ty = field.ty(); let parse_as = field.from().unwrap_or(field.ty()); @@ -561,14 +534,29 @@ fn generate_string_writers(paths: &[VecDeque<&FieldOrGroup>]) -> TokenStream { } } -fn variant_name(segments: &VecDeque<&FieldOrGroup>) -> (syn::Variant, syn::Expr, syn::Pat) { - let ident = syn::Ident::new( +struct ConfigurationKey { + /// e.g. `C8yUrl(Option)` + enum_variant: syn::Variant, + // TODO kill this when it's not used + iter_field: syn::Expr, + /// e.g. `C8yUrl(key0)` + match_read_write: syn::Pat, + /// e.g. `C8yUrl(_)` + match_shape: syn::Pat, +} + +fn ident_for(segments: &VecDeque<&FieldOrGroup>) -> syn::Ident { + syn::Ident::new( &segments .iter() .map(|segment| segment.name().to_upper_camel_case()) .collect::(), segments.iter().last().unwrap().ident().span(), - ); + ) +} + +fn variant_name(segments: &VecDeque<&FieldOrGroup>) -> ConfigurationKey { + let ident = ident_for(segments); let count_multi = segments .iter() .filter(|fog| matches!(fog, FieldOrGroup::Multi(_))) @@ -576,19 +564,26 @@ fn variant_name(segments: &VecDeque<&FieldOrGroup>) -> (syn::Variant, syn::Expr, if count_multi > 0 { let opt_strs = std::iter::repeat::(parse_quote!(Option)).take(count_multi); - let enum_field = parse_quote_spanned!(ident.span()=> #ident(#(#opt_strs),*)); + let enum_variant = parse_quote_spanned!(ident.span()=> #ident(#(#opt_strs),*)); let nones = std::iter::repeat::(parse_quote!(None)).take(count_multi); let iter_field = parse_quote_spanned!(ident.span()=> #ident(#(#nones),*)); - let var_idents = - (0..count_multi).map(|i| syn::Ident::new(&format!("key{i}"), ident.span())); - let destructure_field = parse_quote_spanned!(ident.span()=> #ident(#(#var_idents),*)); - (enum_field, iter_field, destructure_field) + let var_idents = SequentialIdGenerator::default().take(count_multi); + let match_read_write = parse_quote_spanned!(ident.span()=> #ident(#(#var_idents),*)); + let underscores = UnderscoreIdGenerator.take(count_multi); + let match_shape = parse_quote_spanned!(ident.span()=> #ident(#(#underscores),*)); + ConfigurationKey { + enum_variant, + iter_field, + match_shape, + match_read_write, + } } else { - ( - parse_quote!(#ident), - parse_quote!(#ident), - parse_quote!(#ident), - ) + ConfigurationKey { + enum_variant: parse_quote!(#ident), + iter_field: parse_quote!(#ident), + match_read_write: parse_quote!(#ident), + match_shape: parse_quote!(#ident), + } } } From 10ccd17072c9cc07d624138bc49312cd659f395d Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Fri, 20 Sep 2024 23:59:36 +0100 Subject: [PATCH 11/32] Refactor reader ID generation Signed-off-by: James Rhodes --- .../tedge_config_macros/impl/src/lib.rs | 1 + .../tedge_config_macros/impl/src/namegen.rs | 97 +++++++++++++++++++ .../tedge_config_macros/impl/src/query.rs | 43 +------- .../tedge_config_macros/impl/src/reader.rs | 94 +++++++++--------- 4 files changed, 151 insertions(+), 84 deletions(-) create mode 100644 crates/common/tedge_config_macros/impl/src/namegen.rs diff --git a/crates/common/tedge_config_macros/impl/src/lib.rs b/crates/common/tedge_config_macros/impl/src/lib.rs index 6004a4b2495..6d6ad2fdc1e 100644 --- a/crates/common/tedge_config_macros/impl/src/lib.rs +++ b/crates/common/tedge_config_macros/impl/src/lib.rs @@ -11,6 +11,7 @@ use quote::quote_spanned; mod dto; mod error; mod input; +mod namegen; mod optional_error; mod query; mod reader; diff --git a/crates/common/tedge_config_macros/impl/src/namegen.rs b/crates/common/tedge_config_macros/impl/src/namegen.rs new file mode 100644 index 00000000000..c050719a396 --- /dev/null +++ b/crates/common/tedge_config_macros/impl/src/namegen.rs @@ -0,0 +1,97 @@ +use proc_macro2::Span; + +/// Creates arbitrary [syn::Ident]s for binding variables within a macro +/// +/// For instance, with "multi" fields, we have key enums such as +/// ``` +/// pub enum ReadableKey { +/// C8yUrl(Option), +/// MqttBindPort, +/// // ... +/// } +/// ``` +/// +/// and we wish to generate code, such as +/// +/// ``` +/// #pub enum ReadableKey { +/// # C8yUrl(Option), +/// # MqttBindPort, +/// # // ... +/// #} +/// +/// fn do_something(k: &ReadableKey) { +/// match k { +/// ReadableKey::C8yUrl(key0) => todo!(), +/// ReadableKey::MqttBindPort => todo!(), +/// } +/// } +/// ``` +/// +/// we can use [SequentialIdGenerator] and [IdGenerator::next_id] to generate +/// `key0`, `key1`, etc. +/// +/// If we wanted to discard the data +/// +/// ``` +/// #pub enum ReadableKey { +/// # C8yUrl(Option), +/// # MqttBindPort, +/// # // ... +/// #} +/// +/// fn return_something(k: &ReadableKey) -> &'static str { +/// match k { +/// ReadableKey::C8yUrl(_) => "some independent result", +/// ReadableKey::MqttBindPort => "again, doesn't use the key apart from for matching", +/// } +/// } +/// ``` +/// +/// we can simply replace the usage of [SequentialIdGenerator] with [UnderscoreIdGenerator] +pub trait IdGenerator: Default { + fn next_id(&mut self, span: Span) -> syn::Ident; +} + +#[derive(Debug, Default)] +pub struct SequentialIdGenerator { + count: u32, +} + +impl SequentialIdGenerator { + pub fn replay(&self, span: Span) -> syn::Ident { + let i = self.count - 1; + syn::Ident::new(&format!("key{i}"), span) + } +} + +#[derive(Debug, Default)] +pub struct UnderscoreIdGenerator; + +impl IdGenerator for SequentialIdGenerator { + fn next_id(&mut self, span: Span) -> syn::Ident { + let i = self.count; + self.count += 1; + syn::Ident::new(&format!("key{i}"), span) + } +} + +impl Iterator for SequentialIdGenerator { + type Item = syn::Ident; + fn next(&mut self) -> Option { + Some(self.next_id(Span::call_site())) + } +} + +impl IdGenerator for UnderscoreIdGenerator { + fn next_id(&mut self, span: Span) -> syn::Ident { + syn::Ident::new("_", span) + } +} + +impl Iterator for UnderscoreIdGenerator { + type Item = syn::Ident; + fn next(&mut self) -> Option { + Some(self.next_id(Span::call_site())) + } +} diff --git a/crates/common/tedge_config_macros/impl/src/query.rs b/crates/common/tedge_config_macros/impl/src/query.rs index d53fd3715cc..0c0e8a58f9d 100644 --- a/crates/common/tedge_config_macros/impl/src/query.rs +++ b/crates/common/tedge_config_macros/impl/src/query.rs @@ -1,6 +1,9 @@ use crate::error::extract_type_from_result; use crate::input::ConfigurableField; use crate::input::FieldOrGroup; +use crate::namegen::IdGenerator; +use crate::namegen::SequentialIdGenerator; +use crate::namegen::UnderscoreIdGenerator; use heck::ToUpperCamelCase; use itertools::Itertools; use proc_macro2::Span; @@ -318,46 +321,6 @@ fn keys_enum( } } -#[derive(Debug, Default)] -struct SequentialIdGenerator { - count: u32, -} - -#[derive(Debug, Default)] -struct UnderscoreIdGenerator; - -pub trait IdGenerator: Default { - fn next_id(&mut self, span: Span) -> syn::Ident; -} - -impl IdGenerator for SequentialIdGenerator { - fn next_id(&mut self, span: Span) -> syn::Ident { - let i = self.count; - self.count += 1; - syn::Ident::new(&format!("key{i}"), span) - } -} - -impl Iterator for SequentialIdGenerator { - type Item = syn::Ident; - fn next(&mut self) -> Option { - Some(self.next_id(Span::call_site())) - } -} - -impl IdGenerator for UnderscoreIdGenerator { - fn next_id(&mut self, span: Span) -> syn::Ident { - syn::Ident::new("_", span) - } -} - -impl Iterator for UnderscoreIdGenerator { - type Item = syn::Ident; - fn next(&mut self) -> Option { - Some(self.next_id(Span::call_site())) - } -} - fn generate_field_accessor<'a>( fields: &'a VecDeque<&FieldOrGroup>, method: &'a str, diff --git a/crates/common/tedge_config_macros/impl/src/reader.rs b/crates/common/tedge_config_macros/impl/src/reader.rs index f7033ca72c7..1ec0bcf4432 100644 --- a/crates/common/tedge_config_macros/impl/src/reader.rs +++ b/crates/common/tedge_config_macros/impl/src/reader.rs @@ -9,8 +9,6 @@ use proc_macro2::Span; use proc_macro2::TokenStream; use quote::quote; use quote::quote_spanned; -use quote::ToTokens; -use quote::TokenStreamExt; use syn::parse_quote; use syn::parse_quote_spanned; use syn::punctuated::Punctuated; @@ -21,6 +19,8 @@ use crate::error::extract_type_from_result; use crate::input::ConfigurableField; use crate::input::FieldDefault; use crate::input::FieldOrGroup; +use crate::namegen::IdGenerator; +use crate::namegen::SequentialIdGenerator; use crate::optional_error::OptionalError; use crate::prefixed_type_name; @@ -71,20 +71,13 @@ fn generate_structs( false => parse_quote!(pub), }); } - // TODO skipping? - FieldOrGroup::Multi(group) => { - // TODO is this right? + FieldOrGroup::Multi(group) if !group.reader.skip => { let sub_reader_name = prefixed_type_name(name, group); idents.push(&group.ident); tys.push(parse_quote_spanned!(group.ident.span()=> ::tedge_config_macros::Multi<#sub_reader_name>)); let mut parents = parents.clone(); parents.push(PathItem::Static(group.ident.clone())); - let count_dyn = parents.iter().filter_map(PathItem::as_dynamic).count(); - // TODO rationalise this so it's not hardcoded where we use it - parents.push(PathItem::Dynamic(syn::Ident::new( - &format!("key{count_dyn}"), - Span::call_site(), - ))); + parents.push(PathItem::Dynamic(group.ident.span())); sub_readers.push(Some(generate_structs( &sub_reader_name, &group.contents, @@ -116,7 +109,10 @@ fn generate_structs( }); } FieldOrGroup::Group(_) => { - // Skipped + // Explicitly skipped using `#[tedge_config(reader(skip))]` + } + FieldOrGroup::Multi(_) => { + // Explicitly skipped using `#[tedge_config(reader(skip))]` } } } @@ -234,10 +230,13 @@ fn find_field<'a>( } } -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone)] +/// Part of a path in the DTO pub enum PathItem { + /// A static field e.g. `c8y` or `topic_prefix` Static(syn::Ident), - Dynamic(syn::Ident), + /// A dynamic field that will be replaced by `.get(key0)` when reading the field + Dynamic(Span), } impl PathItem { @@ -247,22 +246,17 @@ impl PathItem { Self::Dynamic(_) => None, } } - - pub fn as_dynamic(&self) -> Option<&syn::Ident> { - match self { - Self::Dynamic(d) => Some(d), - Self::Static(_) => None, - } - } } -impl ToTokens for PathItem { - fn to_tokens(&self, tokens: &mut TokenStream) { - match self { - Self::Static(s) => s.to_tokens(tokens), - Self::Dynamic(d) => tokens.append_all(quote!(get(#d).unwrap())), +fn read_field<'a>(parents: &'a [PathItem]) -> impl Iterator + 'a { + let mut id_gen = SequentialIdGenerator::default(); + parents.into_iter().map(move |parent| match parent { + PathItem::Static(name) => quote!(#name), + PathItem::Dynamic(span) => { + let id = id_gen.next_id(*span); + quote_spanned!(*span=> get(#id).unwrap()) } - } + }) } fn reader_value_for_field<'a>( @@ -274,6 +268,7 @@ fn reader_value_for_field<'a>( let name = field.ident(); Ok(match field { ConfigurableField::ReadWrite(field) => { + // TODO optionalconfig should contain the actual key let key = parents .iter() .filter_map(PathItem::as_static) @@ -281,9 +276,10 @@ fn reader_value_for_field<'a>( .chain(iter::once(name.to_string())) .collect::>() .join("."); + let read_path = read_field(&parents); match &field.default { FieldDefault::None => quote! { - match &dto.#(#parents).*.#name { + match &dto.#(#read_path).*.#name { None => OptionalConfig::Empty(#key), Some(value) => OptionalConfig::Present { value: value.clone(), key: #key }, } @@ -336,32 +332,32 @@ fn reader_value_for_field<'a>( }; quote_spanned! {name.span()=> - match &dto.#(#parents).*.#name { + match &dto.#(#read_path).*.#name { Some(value) => #value, None => #default, } } } FieldDefault::Function(function) => quote_spanned! {function.span()=> - match &dto.#(#parents).*.#name { + match &dto.#(#read_path).*.#name { None => TEdgeConfigDefault::::call(#function, dto, location), Some(value) => value.clone(), } }, FieldDefault::Value(default) => quote_spanned! {name.span()=> - match &dto.#(#parents).*.#name { + match &dto.#(#read_path).*.#name { None => #default.into(), Some(value) => value.clone(), } }, FieldDefault::Variable(default) => quote_spanned! {name.span()=> - match &dto.#(#parents).*.#name { + match &dto.#(#read_path).*.#name { None => #default.into(), Some(value) => value.clone(), } }, FieldDefault::FromStr(default) => quote_spanned! {name.span()=> - match &dto.#(#parents).*.#name { + match &dto.#(#read_path).*.#name { None => #default.parse().unwrap(), Some(value) => value.clone(), } @@ -386,10 +382,16 @@ fn generate_conversions( ) -> syn::Result { let mut field_conversions = Vec::new(); let mut rest = Vec::new(); + let mut id_gen = SequentialIdGenerator::default(); let extra_args: Vec<_> = parents .iter() - .filter_map(PathItem::as_dynamic) - .map(|name| quote!(#name: Option<&str>)) + .filter_map(|path_item| match path_item { + PathItem::Static(_) => None, + PathItem::Dynamic(span) => { + let id = id_gen.next_id(*span); + Some(quote_spanned!(*span=> #id: Option<&str>)) + } + }) .collect(); for item in items { @@ -414,18 +416,22 @@ fn generate_conversions( let sub_reader_name = prefixed_type_name(name, group); let name = &group.ident; - let mut parents = parents.clone(); - parents.push(PathItem::Static(group.ident.clone())); - // TODO this is horrible - let l = extra_args.len(); - let new_arg = syn::Ident::new(&format!("key{l}"), Span::call_site()); - let extra_args: Vec<_> = parents + let new_arg = PathItem::Dynamic(group.ident.span()); + let mut id_gen = SequentialIdGenerator::default(); + let extra_call_args: Vec<_> = parents .iter() - .filter_map(PathItem::as_dynamic) .chain(once(&new_arg)) + .filter_map(|path_item| match path_item { + PathItem::Static(_) => None, + PathItem::Dynamic(span) => Some(id_gen.next_id(*span)), + }) .collect(); - field_conversions.push(quote!(#name: dto.#(#parents).*.map(|#new_arg| #sub_reader_name::from_dto(dto, location, #(#extra_args),*)))); - parents.push(PathItem::Dynamic(new_arg)); + let mut parents = parents.clone(); + parents.push(PathItem::Static(group.ident.clone())); + let read_path = read_field(&parents); + let new_arg2 = id_gen.replay(group.ident.span()); + field_conversions.push(quote!(#name: dto.#(#read_path).*.map(|#new_arg2| #sub_reader_name::from_dto(dto, location, #(#extra_call_args),*)))); + parents.push(new_arg); let sub_conversions = generate_conversions(&sub_reader_name, &group.contents, parents, root_fields)?; rest.push(sub_conversions); From 7d4745e203c86a41b41d16a88342c6e77c8cdcef Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Sat, 21 Sep 2024 00:14:24 +0100 Subject: [PATCH 12/32] Rename `get` to `try_get` Signed-off-by: James Rhodes --- crates/common/tedge_config_macros/examples/multi.rs | 8 ++++---- crates/common/tedge_config_macros/impl/src/query.rs | 6 +++--- crates/common/tedge_config_macros/impl/src/reader.rs | 4 ++-- crates/common/tedge_config_macros/src/multi.rs | 4 ++-- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/crates/common/tedge_config_macros/examples/multi.rs b/crates/common/tedge_config_macros/examples/multi.rs index 99ad67bd930..20c34be17bf 100644 --- a/crates/common/tedge_config_macros/examples/multi.rs +++ b/crates/common/tedge_config_macros/examples/multi.rs @@ -39,7 +39,7 @@ define_tedge_config! { } fn url_for<'a>(reader: &'a TEdgeConfigReader, o: Option<&str>) -> &'a str { - reader.c8y.get(o).unwrap().url.or_config_not_set().unwrap() + reader.c8y.try_get(o).unwrap().url.or_config_not_set().unwrap() } fn main() { @@ -61,15 +61,15 @@ fn main() { ); assert!(matches!( - single_c8y_reader.c8y.get(Some("cloud")), + single_c8y_reader.c8y.try_get(Some("cloud")), Err(MultiError::SingleNotMulti) )); assert!(matches!( - multi_c8y_reader.c8y.get(Some("unknown")), + multi_c8y_reader.c8y.try_get(Some("unknown")), Err(MultiError::MultiKeyNotFound) )); assert!(matches!( - multi_c8y_reader.c8y.get(None), + multi_c8y_reader.c8y.try_get(None), Err(MultiError::MultiNotSingle) )); } diff --git a/crates/common/tedge_config_macros/impl/src/query.rs b/crates/common/tedge_config_macros/impl/src/query.rs index 0c0e8a58f9d..e956f0ddb18 100644 --- a/crates/common/tedge_config_macros/impl/src/query.rs +++ b/crates/common/tedge_config_macros/impl/src/query.rs @@ -351,7 +351,7 @@ fn generate_string_readers(paths: &[VecDeque<&FieldOrGroup>]) -> TokenStream { .expect("Path must have a back as it is nonempty") .field() .expect("Back of path is guaranteed to be a field"); - let segments = generate_field_accessor(path, "get"); + let segments = generate_field_accessor(path, "try_get"); let to_string = quote_spanned!(field.ty().span()=> .to_string()); let match_variant = configuration_key.match_read_write; if field.read_only().is_some() { @@ -401,8 +401,8 @@ fn generate_string_writers(paths: &[VecDeque<&FieldOrGroup>]) -> TokenStream { .iter() .zip(variant_names) .map(|(path, configuration_key)| { - let read_segments = generate_field_accessor(path, "get"); - let write_segments = generate_field_accessor(path, "get_mut").collect::>(); + let read_segments = generate_field_accessor(path, "try_get"); + let write_segments = generate_field_accessor(path, "try_get_mut").collect::>(); let field = path .iter() .filter_map(|thing| thing.field()) diff --git a/crates/common/tedge_config_macros/impl/src/reader.rs b/crates/common/tedge_config_macros/impl/src/reader.rs index 1ec0bcf4432..591ce8f67d3 100644 --- a/crates/common/tedge_config_macros/impl/src/reader.rs +++ b/crates/common/tedge_config_macros/impl/src/reader.rs @@ -235,7 +235,7 @@ fn find_field<'a>( pub enum PathItem { /// A static field e.g. `c8y` or `topic_prefix` Static(syn::Ident), - /// A dynamic field that will be replaced by `.get(key0)` when reading the field + /// A dynamic field that will be replaced by `.try_get(key0)` when reading the field Dynamic(Span), } @@ -254,7 +254,7 @@ fn read_field<'a>(parents: &'a [PathItem]) -> impl Iterator PathItem::Static(name) => quote!(#name), PathItem::Dynamic(span) => { let id = id_gen.next_id(*span); - quote_spanned!(*span=> get(#id).unwrap()) + quote_spanned!(*span=> try_get(#id).unwrap()) } }) } diff --git a/crates/common/tedge_config_macros/src/multi.rs b/crates/common/tedge_config_macros/src/multi.rs index 6e2a4a95818..ba0750c0325 100644 --- a/crates/common/tedge_config_macros/src/multi.rs +++ b/crates/common/tedge_config_macros/src/multi.rs @@ -32,7 +32,7 @@ pub enum MultiError { impl Multi { // TODO rename this to something more rusty - pub fn get(&self, key: Option<&str>) -> Result<&T, MultiError> { + pub fn try_get(&self, key: Option<&str>) -> Result<&T, MultiError> { match (self, key) { (Self::Single(val), None) => Ok(val), (Self::Multi(map), Some(key)) => map.get(key).ok_or(MultiError::MultiKeyNotFound), @@ -41,7 +41,7 @@ impl Multi { } } - pub fn get_mut(&mut self, key: Option<&str>) -> Result<&mut T, MultiError> { + pub fn try_get_mut(&mut self, key: Option<&str>) -> Result<&mut T, MultiError> { match (self, key) { (Self::Single(val), None) => Ok(val), (Self::Multi(map), Some(key)) => map.get_mut(key).ok_or(MultiError::MultiKeyNotFound), From b962d72aea015533a24cd07bac9885a96f2bdc47 Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Sat, 21 Sep 2024 00:18:29 +0100 Subject: [PATCH 13/32] Fix clippy and test errors Signed-off-by: James Rhodes --- .../tedge_config_macros/examples/multi.rs | 8 +++++++- .../tedge_config_macros/impl/src/namegen.rs | 18 +++++++++--------- .../tedge_config_macros/impl/src/reader.rs | 6 +++--- 3 files changed, 19 insertions(+), 13 deletions(-) diff --git a/crates/common/tedge_config_macros/examples/multi.rs b/crates/common/tedge_config_macros/examples/multi.rs index 20c34be17bf..4c79adbc9bc 100644 --- a/crates/common/tedge_config_macros/examples/multi.rs +++ b/crates/common/tedge_config_macros/examples/multi.rs @@ -39,7 +39,13 @@ define_tedge_config! { } fn url_for<'a>(reader: &'a TEdgeConfigReader, o: Option<&str>) -> &'a str { - reader.c8y.try_get(o).unwrap().url.or_config_not_set().unwrap() + reader + .c8y + .try_get(o) + .unwrap() + .url + .or_config_not_set() + .unwrap() } fn main() { diff --git a/crates/common/tedge_config_macros/impl/src/namegen.rs b/crates/common/tedge_config_macros/impl/src/namegen.rs index c050719a396..ac572fb14a1 100644 --- a/crates/common/tedge_config_macros/impl/src/namegen.rs +++ b/crates/common/tedge_config_macros/impl/src/namegen.rs @@ -14,12 +14,12 @@ use proc_macro2::Span; /// and we wish to generate code, such as /// /// ``` -/// #pub enum ReadableKey { +/// # pub enum ReadableKey { /// # C8yUrl(Option), /// # MqttBindPort, /// # // ... -/// #} -/// +/// # } +/// # /// fn do_something(k: &ReadableKey) { /// match k { /// ReadableKey::C8yUrl(key0) => todo!(), @@ -34,12 +34,12 @@ use proc_macro2::Span; /// If we wanted to discard the data /// /// ``` -/// #pub enum ReadableKey { -/// # C8yUrl(Option), -/// # MqttBindPort, -/// # // ... -/// #} -/// +/// # pub enum ReadableKey { +/// # C8yUrl(Option), +/// # MqttBindPort, +/// # // ... +/// # } +/// # /// fn return_something(k: &ReadableKey) -> &'static str { /// match k { /// ReadableKey::C8yUrl(_) => "some independent result", diff --git a/crates/common/tedge_config_macros/impl/src/reader.rs b/crates/common/tedge_config_macros/impl/src/reader.rs index 591ce8f67d3..0f35d52f2c7 100644 --- a/crates/common/tedge_config_macros/impl/src/reader.rs +++ b/crates/common/tedge_config_macros/impl/src/reader.rs @@ -248,9 +248,9 @@ impl PathItem { } } -fn read_field<'a>(parents: &'a [PathItem]) -> impl Iterator + 'a { +fn read_field(parents: &[PathItem]) -> impl Iterator + '_ { let mut id_gen = SequentialIdGenerator::default(); - parents.into_iter().map(move |parent| match parent { + parents.iter().map(move |parent| match parent { PathItem::Static(name) => quote!(#name), PathItem::Dynamic(span) => { let id = id_gen.next_id(*span); @@ -276,7 +276,7 @@ fn reader_value_for_field<'a>( .chain(iter::once(name.to_string())) .collect::>() .join("."); - let read_path = read_field(&parents); + let read_path = read_field(parents); match &field.default { FieldDefault::None => quote! { match &dto.#(#read_path).*.#name { From 66ea8c715a00a44b3ea924ec9bf1c4a0140fb71d Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Mon, 23 Sep 2024 14:04:51 +0100 Subject: [PATCH 14/32] Update `FromStr` implementation to cope with multi-value fields --- Cargo.lock | 91 +++++++---- Cargo.toml | 2 + crates/common/tedge_config_macros/Cargo.toml | 1 + .../tedge_config_macros/examples/multi.rs | 34 ++++ .../tedge_config_macros/impl/Cargo.toml | 3 + .../tedge_config_macros/impl/src/dto.rs | 1 - .../tedge_config_macros/impl/src/query.rs | 148 +++++++++++++++++- 7 files changed, 242 insertions(+), 38 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 54338253b25..7b0f88272e4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -192,7 +192,7 @@ checksum = "c980ee35e870bd1a4d2c8294d4c04d0499e67bca1e4b5cefcc693c2fa00caea9" dependencies = [ "proc-macro2", "quote", - "syn 2.0.48", + "syn 2.0.77", ] [[package]] @@ -392,7 +392,7 @@ dependencies = [ "heck", "proc-macro2", "quote", - "syn 2.0.48", + "syn 2.0.77", ] [[package]] @@ -890,7 +890,7 @@ dependencies = [ "heck", "proc-macro2", "quote", - "syn 2.0.48", + "syn 2.0.77", ] [[package]] @@ -1121,7 +1121,7 @@ dependencies = [ "proc-macro2", "quote", "strsim", - "syn 2.0.48", + "syn 2.0.77", ] [[package]] @@ -1143,7 +1143,7 @@ checksum = "836a9bbc7ad63342d6d6e7b815ccab164bc77a2d95d84bc3117a8c0d5c98e2d5" dependencies = [ "darling_core 0.20.3", "quote", - "syn 2.0.48", + "syn 2.0.77", ] [[package]] @@ -1176,6 +1176,12 @@ dependencies = [ "serde", ] +[[package]] +name = "diff" +version = "0.1.13" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "56254986775e3233ffa9c4d7d3faaf6d36a2c09d30b20687e9f88bc8bafc16c8" + [[package]] name = "difflib" version = "0.4.0" @@ -1200,7 +1206,7 @@ checksum = "487585f4d0c6655fe74905e2504d8ad6908e4db67f744eb140876906c2f3175d" dependencies = [ "proc-macro2", "quote", - "syn 2.0.48", + "syn 2.0.77", ] [[package]] @@ -1505,7 +1511,7 @@ checksum = "87750cf4b7a4c0625b1529e4c543c2182106e4dedc60a2a6455e00d212c489ac" dependencies = [ "proc-macro2", "quote", - "syn 2.0.48", + "syn 2.0.77", ] [[package]] @@ -2137,7 +2143,7 @@ checksum = "49e7bc1560b95a3c4a25d03de42fe76ca718ab92d1a22a55b9b4cf67b3ae635c" dependencies = [ "proc-macro2", "quote", - "syn 2.0.48", + "syn 2.0.77", ] [[package]] @@ -2508,7 +2514,7 @@ checksum = "4ccca0f6c17acc81df8e242ed473ec144cbf5c98037e69aa6d144780aad103c8" dependencies = [ "inlinable_string", "pear_codegen", - "yansi 1.0.0-rc.1", + "yansi 1.0.1", ] [[package]] @@ -2520,7 +2526,7 @@ dependencies = [ "proc-macro2", "proc-macro2-diagnostics", "quote", - "syn 2.0.48", + "syn 2.0.77", ] [[package]] @@ -2579,7 +2585,7 @@ dependencies = [ "pest_meta", "proc-macro2", "quote", - "syn 2.0.48", + "syn 2.0.77", ] [[package]] @@ -2620,7 +2626,7 @@ checksum = "266c042b60c9c76b8d53061e52b2e0d1116abc57cefc8c5cd671619a56ac3690" dependencies = [ "proc-macro2", "quote", - "syn 2.0.48", + "syn 2.0.77", ] [[package]] @@ -2721,6 +2727,26 @@ dependencies = [ "termtree", ] +[[package]] +name = "pretty_assertions" +version = "1.4.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3ae130e2f271fbc2ac3a40fb1d07180839cdbbe443c7a27e1e3c13c5cac0116d" +dependencies = [ + "diff", + "yansi 1.0.1", +] + +[[package]] +name = "prettyplease" +version = "0.2.22" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "479cf940fbbb3426c32c5d5176f62ad57549a0bb84773423ba8be9d089f5faba" +dependencies = [ + "proc-macro2", + "syn 2.0.77", +] + [[package]] name = "proc-macro-error" version = "1.0.4" @@ -2747,9 +2773,9 @@ dependencies = [ [[package]] name = "proc-macro2" -version = "1.0.78" +version = "1.0.86" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e2422ad645d89c99f8f3e6b88a9fdeca7fabeac836b1002371c4367c8f984aae" +checksum = "5e719e8df665df0d1c8fbfd238015744736151d4445ec0836b8e628aae103b77" dependencies = [ "unicode-ident", ] @@ -2762,9 +2788,9 @@ checksum = "af066a9c399a26e020ada66a034357a868728e72cd426f3adcd35f80d88d88c8" dependencies = [ "proc-macro2", "quote", - "syn 2.0.48", + "syn 2.0.77", "version_check", - "yansi 1.0.0-rc.1", + "yansi 1.0.1", ] [[package]] @@ -3322,7 +3348,7 @@ checksum = "33c85360c95e7d137454dc81d9a4ed2b8efd8fbe19cee57357b32b9771fccb67" dependencies = [ "proc-macro2", "quote", - "syn 2.0.48", + "syn 2.0.77", ] [[package]] @@ -3621,9 +3647,9 @@ dependencies = [ [[package]] name = "syn" -version = "2.0.48" +version = "2.0.77" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0f3531638e407dfc0814761abb7c00a5b54992b849452a0646b7f65c9f770f3f" +checksum = "9f35bcdf61fd8e7be6caf75f429fdca8beb3ed76584befb503b1569faee373ed" dependencies = [ "proc-macro2", "quote", @@ -3951,6 +3977,7 @@ dependencies = [ "doku", "itertools 0.13.0", "once_cell", + "regex", "serde", "tedge_config_macros-macro", "thiserror", @@ -3966,9 +3993,11 @@ dependencies = [ "darling 0.20.3", "heck", "itertools 0.13.0", + "pretty_assertions", + "prettyplease", "proc-macro2", "quote", - "syn 2.0.48", + "syn 2.0.77", ] [[package]] @@ -3977,7 +4006,7 @@ version = "1.3.0" dependencies = [ "proc-macro2", "quote", - "syn 2.0.48", + "syn 2.0.77", "tedge_config_macros-impl", ] @@ -4285,7 +4314,7 @@ dependencies = [ "cfg-if", "proc-macro2", "quote", - "syn 2.0.48", + "syn 2.0.77", ] [[package]] @@ -4296,7 +4325,7 @@ checksum = "5c89e72a01ed4c579669add59014b9a524d609c0c88c6a585ce37485879f6ffb" dependencies = [ "proc-macro2", "quote", - "syn 2.0.48", + "syn 2.0.77", "test-case-core", ] @@ -4328,7 +4357,7 @@ checksum = "46c3384250002a6d5af4d114f2845d37b57521033f30d5c3f46c4d70e1197533" dependencies = [ "proc-macro2", "quote", - "syn 2.0.48", + "syn 2.0.77", ] [[package]] @@ -4425,7 +4454,7 @@ checksum = "5b8a1e28f2deaa14e508979454cb3a223b10b938b45af148bc0986de36f1923b" dependencies = [ "proc-macro2", "quote", - "syn 2.0.48", + "syn 2.0.77", ] [[package]] @@ -4585,7 +4614,7 @@ checksum = "34704c8d6ebcbc939824180af020566b01a7c01f80641264eba0999f6c2b6be7" dependencies = [ "proc-macro2", "quote", - "syn 2.0.48", + "syn 2.0.77", ] [[package]] @@ -4905,7 +4934,7 @@ dependencies = [ "once_cell", "proc-macro2", "quote", - "syn 2.0.48", + "syn 2.0.77", "wasm-bindgen-shared", ] @@ -4939,7 +4968,7 @@ checksum = "bae1abb6806dc1ad9e560ed242107c0f6c84335f1749dd4e8ddb012ebd5e25a7" dependencies = [ "proc-macro2", "quote", - "syn 2.0.48", + "syn 2.0.77", "wasm-bindgen-backend", "wasm-bindgen-shared", ] @@ -5266,9 +5295,9 @@ checksum = "09041cd90cf85f7f8b2df60c646f853b7f535ce68f85244eb6731cf89fa498ec" [[package]] name = "yansi" -version = "1.0.0-rc.1" +version = "1.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1367295b8f788d371ce2dbc842c7b709c73ee1364d30351dd300ec2203b12377" +checksum = "cfe53a6657fd280eaa890a3bc59152892ffa3e30101319d168b781ed6529b049" [[package]] name = "yasna" @@ -5296,7 +5325,7 @@ checksum = "9ce1b18ccd8e73a9321186f97e46f9f04b778851177567b1975109d26a08d2a6" dependencies = [ "proc-macro2", "quote", - "syn 2.0.48", + "syn 2.0.77", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index f1bcaf64d22..3307a427c0e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -107,6 +107,8 @@ pem = "1.0" pin-project = { version = "1.1.3", features = [] } plugin_sm = { path = "crates/core/plugin_sm" } predicates = "2.1" +pretty_assertions = "1.4.1" +prettyplease = "0.2.22" proc-macro2 = "1" proptest = "1.0" quote = "1" diff --git a/crates/common/tedge_config_macros/Cargo.toml b/crates/common/tedge_config_macros/Cargo.toml index b3199712466..69cd92c2e62 100644 --- a/crates/common/tedge_config_macros/Cargo.toml +++ b/crates/common/tedge_config_macros/Cargo.toml @@ -13,6 +13,7 @@ camino = { workspace = true, features = ["serde1"] } doku = { workspace = true } itertools = { workspace = true } once_cell = { workspace = true } +regex = { workspace = true } serde = { workspace = true } tedge_config_macros-macro = { path = "macro" } thiserror = { workspace = true } diff --git a/crates/common/tedge_config_macros/examples/multi.rs b/crates/common/tedge_config_macros/examples/multi.rs index 4c79adbc9bc..ac8daada42e 100644 --- a/crates/common/tedge_config_macros/examples/multi.rs +++ b/crates/common/tedge_config_macros/examples/multi.rs @@ -35,6 +35,10 @@ define_tedge_config! { c8y: { #[tedge_config(reader(private))] url: String, + #[tedge_config(multi)] + something: { + test: String, + } }, } @@ -78,4 +82,34 @@ fn main() { multi_c8y_reader.c8y.try_get(None), Err(MultiError::MultiNotSingle) )); + + assert_eq!( + "c8y.url".parse::().unwrap(), + ReadableKey::C8yUrl(None) + ); + assert_eq!( + "c8y.cloud.url".parse::().unwrap(), + ReadableKey::C8yUrl(Some("cloud".to_owned())) + ); + assert_eq!( + "c8y.cloud.something.test".parse::().unwrap(), + ReadableKey::C8ySomethingTest(Some("cloud".to_owned()), None) + ); + assert_eq!( + "c8y.cloud.something.thing.test" + .parse::() + .unwrap(), + ReadableKey::C8ySomethingTest(Some("cloud".to_owned()), Some("thing".to_owned())) + ); + assert_eq!( + "c8y.something.thing.test".parse::().unwrap(), + ReadableKey::C8ySomethingTest(None, Some("thing".to_owned())) + ); + assert_eq!( + "c8y.cloud.not_a_real_key" + .parse::() + .unwrap_err() + .to_string(), + "Unknown key: 'c8y.cloud.not_a_real_key'" + ); } diff --git a/crates/common/tedge_config_macros/impl/Cargo.toml b/crates/common/tedge_config_macros/impl/Cargo.toml index 6c9d026ca56..36d37b8971a 100644 --- a/crates/common/tedge_config_macros/impl/Cargo.toml +++ b/crates/common/tedge_config_macros/impl/Cargo.toml @@ -16,6 +16,9 @@ proc-macro2 = { workspace = true } quote = { workspace = true } syn = { workspace = true } +[dev-dependencies] +pretty_assertions = { workspace = true } +prettyplease = { workspace = true } [lints] workspace = true diff --git a/crates/common/tedge_config_macros/impl/src/dto.rs b/crates/common/tedge_config_macros/impl/src/dto.rs index 1dbf7c944df..28a67231fbb 100644 --- a/crates/common/tedge_config_macros/impl/src/dto.rs +++ b/crates/common/tedge_config_macros/impl/src/dto.rs @@ -49,7 +49,6 @@ pub fn generate( FieldOrGroup::Multi(group) => { if !group.dto.skip { let sub_dto_name = prefixed_type_name(&name, group); - // TODO check if skip serializing if is actually any use, I deleted it because it was complicated to implement idents.push(&group.ident); let field_ty = parse_quote_spanned!(group.ident.span()=> ::tedge_config_macros::Multi<#sub_dto_name>); tys.push(field_ty); diff --git a/crates/common/tedge_config_macros/impl/src/query.rs b/crates/common/tedge_config_macros/impl/src/query.rs index e956f0ddb18..75b2b5d5346 100644 --- a/crates/common/tedge_config_macros/impl/src/query.rs +++ b/crates/common/tedge_config_macros/impl/src/query.rs @@ -188,6 +188,27 @@ fn generate_fromstr( .zip(configuration_key.iter().map(|k| &k.enum_variant)) .map(|(s, v)| quote_spanned!(v.span()=> #s)); let iter_variant = configuration_key.iter().map(|k| &k.iter_field); + let regex_patterns = configuration_key + .iter() + .filter_map(|c| Some((c.regex_parser.clone()?, c))) + .map(|(mut r, c)| { + let match_read_write = &c.match_read_write; + let own_branches = c + .field_names + .iter() + .enumerate() + .map::(|(n, id)| { + let n = n + 1; + parse_quote! { + let #id = captures.get(#n).map(|m| m.as_str().to_owned()); + } + }); + r.then_branch = parse_quote!({ + #(#own_branches)* + return Ok(Self::#match_read_write); + }); + r + }); // TODO oh shit make this actually work! quote! { @@ -196,17 +217,19 @@ fn generate_fromstr( fn from_str(value: &str) -> Result { // If we get an unreachable pattern, it means we have the same key twice #[deny(unreachable_patterns)] - match replace_aliases(value.to_owned()).replace(".", "_").as_str() { + let res = match replace_aliases(value.to_owned()).replace(".", "_").as_str() { #( #simplified_configuration_string => { - if (value != #configuration_string) { + if value != #configuration_string { warn_about_deprecated_key(value.to_owned(), #configuration_string); } - Ok(Self::#iter_variant) + return Ok(Self::#iter_variant) }, )* #error_case - } + }; + #(#regex_patterns;)* + res } } } @@ -506,6 +529,20 @@ struct ConfigurationKey { match_read_write: syn::Pat, /// e.g. `C8yUrl(_)` match_shape: syn::Pat, + /// An if statement for extracting the multi field names out of value using a Regex + /// + /// This takes the string being matched using the identifier `value`, and binds `captures` if + /// to [Regex::captures] if the string matches the key in question. The captures can be read + /// using `captures.get(n)` where n is 1 for the first multi field, 2 for the second, etc. + /// If the user is using a single configuration inside the multi field, the relevant capture will + /// be `None`. + /// + /// If the field is not a "multi" field, `regex_parser` will be set to `None` + regex_parser: Option, + /// The number of multi fields + count_multi: usize, + /// The variable names assigned to the multi fields within this configuration + field_names: Vec, } fn ident_for(segments: &VecDeque<&FieldOrGroup>) -> syn::Ident { @@ -530,15 +567,30 @@ fn variant_name(segments: &VecDeque<&FieldOrGroup>) -> ConfigurationKey { let enum_variant = parse_quote_spanned!(ident.span()=> #ident(#(#opt_strs),*)); let nones = std::iter::repeat::(parse_quote!(None)).take(count_multi); let iter_field = parse_quote_spanned!(ident.span()=> #ident(#(#nones),*)); - let var_idents = SequentialIdGenerator::default().take(count_multi); - let match_read_write = parse_quote_spanned!(ident.span()=> #ident(#(#var_idents),*)); + let field_names = SequentialIdGenerator::default() + .take(count_multi) + .collect::>(); + let match_read_write = parse_quote_spanned!(ident.span()=> #ident(#(#field_names),*)); let underscores = UnderscoreIdGenerator.take(count_multi); let match_shape = parse_quote_spanned!(ident.span()=> #ident(#(#underscores),*)); + let re = segments + .iter() + .map(|fog| match fog { + FieldOrGroup::Multi(m) => format!("{}(?:\\.([A-z_]+))?", m.ident), + FieldOrGroup::Field(f) => f.ident().to_string(), + FieldOrGroup::Group(g) => g.ident.to_string(), + }) + .collect::>() + .join("\\."); + let regex_parser = parse_quote_spanned!(ident.span()=> if let Some(captures) = ::regex::Regex::new(#re).unwrap().captures(value) {}); ConfigurationKey { enum_variant, iter_field, match_shape, match_read_write, + regex_parser: Some(regex_parser), + count_multi, + field_names, } } else { ConfigurationKey { @@ -546,6 +598,9 @@ fn variant_name(segments: &VecDeque<&FieldOrGroup>) -> ConfigurationKey { iter_field: parse_quote!(#ident), match_read_write: parse_quote!(#ident), match_shape: parse_quote!(#ident), + regex_parser: None, + count_multi, + field_names: vec![], } } } @@ -595,4 +650,85 @@ mod tests { }; syn::parse2::(generate_writable_keys(&input.groups)).unwrap(); } + + #[test] + fn from_str_does_not_generate_regex_matches_for_normal_fields() { + let input: crate::input::Configuration = parse_quote!( + c8y: { + url: String, + } + ); + let paths = configuration_paths_from(&input.groups); + let c = configuration_strings(paths.iter()); + let generated = generate_fromstr( + syn::Ident::new("ReadableKey", Span::call_site()), + &c, + parse_quote!(_ => unimplemented!("just a test")), + ); + let expected = parse_quote!( + impl ::std::str::FromStr for ReadableKey { + type Err = ParseKeyError; + fn from_str(value: &str) -> Result { + #[deny(unreachable_patterns)] + let res = match replace_aliases(value.to_owned()).replace(".", "_").as_str() { + "c8y_url" => { + if value != "c8y.url" { + warn_about_deprecated_key(value.to_owned(), "c8y.url"); + } + return Ok(Self::C8yUrl); + }, + _ => unimplemented!("just a test"), + }; + res + } + } + ); + pretty_assertions::assert_eq!( + prettyplease::unparse(&syn::parse2(generated).unwrap()), + prettyplease::unparse(&expected) + ); + } + + #[test] + fn from_str_generates_regex_matches_for_multi_fields() { + let input: crate::input::Configuration = parse_quote!( + #[tedge_config(multi)] + c8y: { + url: String, + } + ); + let paths = configuration_paths_from(&input.groups); + let c = configuration_strings(paths.iter()); + let generated = generate_fromstr( + syn::Ident::new("ReadableKey", Span::call_site()), + &c, + parse_quote!(_ => unimplemented!("just a test")), + ); + let expected = parse_quote!( + impl ::std::str::FromStr for ReadableKey { + type Err = ParseKeyError; + fn from_str(value: &str) -> Result { + #[deny(unreachable_patterns)] + let res = match replace_aliases(value.to_owned()).replace(".", "_").as_str() { + "c8y_url" => { + if value != "c8y.url" { + warn_about_deprecated_key(value.to_owned(), "c8y.url"); + } + return Ok(Self::C8yUrl(None)); + }, + _ => unimplemented!("just a test"), + }; + if let Some(captures) = ::regex::Regex::new("c8y(?:\\.([A-z_]+))?\\.url").unwrap().captures(value) { + let key0 = captures.get(1usize).map(|m| m.as_str().to_owned()); + return Ok(Self::C8yUrl(key0)); + }; + res + } + } + ); + pretty_assertions::assert_eq!( + prettyplease::unparse(&syn::parse2(generated).unwrap()), + prettyplease::unparse(&expected) + ); + } } From e3ee82a49583a007bffbe91189d4078ba4f1914c Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Tue, 24 Sep 2024 12:40:00 +0100 Subject: [PATCH 15/32] Add support for iterating over readable keys Signed-off-by: James Rhodes --- .../tedge_config_macros/impl/src/query.rs | 340 +++++++++++++++++- 1 file changed, 333 insertions(+), 7 deletions(-) diff --git a/crates/common/tedge_config_macros/impl/src/query.rs b/crates/common/tedge_config_macros/impl/src/query.rs index 75b2b5d5346..d214d93660a 100644 --- a/crates/common/tedge_config_macros/impl/src/query.rs +++ b/crates/common/tedge_config_macros/impl/src/query.rs @@ -4,6 +4,7 @@ use crate::input::FieldOrGroup; use crate::namegen::IdGenerator; use crate::namegen::SequentialIdGenerator; use crate::namegen::UnderscoreIdGenerator; +use heck::ToSnekCase; use heck::ToUpperCamelCase; use itertools::Itertools; use proc_macro2::Span; @@ -16,7 +17,7 @@ use syn::parse_quote_spanned; use syn::spanned::Spanned; pub fn generate_writable_keys(items: &[FieldOrGroup]) -> TokenStream { - let paths = configuration_paths_from(items); + let mut paths = configuration_paths_from(items); let (readonly_destr, write_error): (Vec<_>, Vec<_>) = paths .iter() .filter_map(|field| { @@ -54,6 +55,33 @@ pub fn generate_writable_keys(items: &[FieldOrGroup]) -> TokenStream { .cloned() .collect::>(), ); + + let paths_vec = paths + .iter_mut() + .map(|vd| &*vd.make_contiguous()) + .collect::>(); + let readable_keys_iter = key_iterators( + parse_quote!(TEdgeConfigReader), + parse_quote!(ReadableKey), + &paths_vec, + "", + &[], + ); + let readonly_keys_iter = key_iterators( + parse_quote!(TEdgeConfigReader), + parse_quote!(ReadOnlyKey), + &paths_vec.iter().copied().filter(|r| r.last().unwrap().field().unwrap().read_only().is_some()).collect::>(), + "", + &[], + ); + let writable_keys_iter = key_iterators( + parse_quote!(TEdgeConfigReader), + parse_quote!(WritableKey), + &paths_vec.iter().copied().filter(|r| r.last().unwrap().field().unwrap().read_only().is_none()).collect::>(), + "", + &[], + ); + let (static_alias, deprecated_keys) = deprecated_keys(paths.iter()); let iter_updated = deprecated_keys.iter().map(|k| &k.iter_field); @@ -71,6 +99,9 @@ pub fn generate_writable_keys(items: &[FieldOrGroup]) -> TokenStream { #fromstr_writable #read_string #write_string + #readable_keys_iter + #readonly_keys_iter + #writable_keys_iter #[derive(::thiserror::Error, Debug)] /// An error encountered when writing to a configuration value from a @@ -264,6 +295,151 @@ fn generate_fromstr_writable( ) } +fn key_iterators<'a>( + reader_ty: syn::Ident, + type_name: syn::Ident, + fields: &[&[&FieldOrGroup]], + prefix: &str, + args: &[syn::Ident], +) -> TokenStream { + let mut function_name = type_name.to_string().to_snek_case(); + // Pluralise the name + function_name += "s"; + let function_name = syn::Ident::new(&function_name, type_name.span()); + + let mut stmts: Vec = Vec::new(); + let mut exprs: VecDeque = VecDeque::new(); + let mut complete_fields: Vec = Vec::new(); + let mut global: Vec = Vec::new(); + let chunks = fields + .iter() + .chunk_by(|fog| *fog.first().unwrap() as *const FieldOrGroup); + for (_, fields) in chunks.into_iter() { + let fields = fields.collect::>(); + let field = fields.first().unwrap(); + match field.split_first() { + Some((FieldOrGroup::Multi(m), rest)) => { + let ident = &m.ident; + let upper_ident = m.ident.to_string().to_upper_camel_case(); + let sub_type_name = + syn::Ident::new(&format!("{reader_ty}{upper_ident}"), m.ident.span()); + let keys_ident = syn::Ident::new(&format!("{}_keys", ident), ident.span()); + stmts.push(parse_quote!(let #keys_ident = if let ::tedge_config_macros::Multi::Multi(map) = &self.#ident { + map.keys().map(|k| Some(k.to_owned())).collect() + } else { + vec![None] + };)); + let prefix = format!("{prefix}{upper_ident}"); + let remaining_fields = fields.iter().map(|fs| &fs[1..]).collect::>(); + let arg_clone_stmts = args + .iter() + .map::(|arg| parse_quote!(let #arg = #arg.clone();)); + let cloned_args = args + .iter() + .map::(|arg| parse_quote!(#arg.clone())); + let body: syn::Expr = if args.is_empty() { + parse_quote! { + |#ident| self.#ident.try_get(#ident.as_deref()).unwrap().#function_name(#ident) + } + } else { + parse_quote! { + { + #(#arg_clone_stmts)* + move |#ident| { + self.#ident.try_get(#ident.as_deref()).unwrap().#function_name(#(#cloned_args,)* #ident) + } + } + } + }; + stmts.push(parse_quote! { + let #keys_ident = #keys_ident + .into_iter() + .flat_map(#body); + }); + exprs.push_back(parse_quote!(#keys_ident)); + + let mut args = args.to_owned(); + args.push(m.ident.clone()); + global.push(key_iterators( + sub_type_name, + type_name.clone(), + &remaining_fields, + &prefix, + &args, + )); + } + Some((FieldOrGroup::Group(g), rest)) => { + let upper_ident = g.ident.to_string().to_upper_camel_case(); + let sub_type_name = + syn::Ident::new(&format!("{reader_ty}{upper_ident}"), g.ident.span()); + let prefix = format!("{prefix}{upper_ident}"); + let remaining_fields = fields.iter().map(|fs| &fs[1..]).collect::>(); + global.push(key_iterators( + sub_type_name, + type_name.clone(), + &remaining_fields, + &prefix, + args, + )); + let ident = &g.ident; + // TODO test the args are correct + exprs.push_back(parse_quote! { + self.#ident.#function_name(#(#args),*) + }); + } + Some((FieldOrGroup::Field(f), _nothing)) => { + let ident = f.ident(); + let field_name = syn::Ident::new( + &format!( + "{}{}", + prefix, + f.rename() + .map(<_>::to_upper_camel_case) + .unwrap_or_else(|| ident.to_string().to_upper_camel_case()) + ), + ident.span(), + ); + let args = match args.len() { + 0 => TokenStream::new(), + _ => { + quote!((#(#args.clone()),*)) + } + }; + complete_fields.push(parse_quote!(#type_name::#field_name #args)) + } + None => panic!("Expected FieldOrGroup list te be nonempty"), + }; + } + + if !complete_fields.is_empty() { + // Iterate through fields before groups + exprs.push_front(parse_quote!([#(#complete_fields),*].into_iter())); + } + + if exprs.is_empty() { + // If the enum is empty, we need something to iterate over, so generate an empty iterator + exprs.push_back(parse_quote!(std::iter::empty())); + } + let exprs = exprs.into_iter().enumerate().map(|(i, expr)| { + if i > 0 { + parse_quote!(chain(#expr)) + } else { + expr + } + }); + + quote! { + impl #reader_ty { + pub fn #function_name(&self #(, #args: Option)*) -> impl Iterator + '_ { + #(#stmts)* + #(#exprs).* + } + } + + #(#global)* + } +} + fn keys_enum( type_name: syn::Ident, (configuration_string, configuration_key): &(Vec, Vec), @@ -296,6 +472,9 @@ fn keys_enum( let enum_variant = configuration_key.iter().map(|k| &k.enum_variant); let match_shape = configuration_key.iter().map(|k| &k.match_shape); let iter_field = configuration_key.iter().map(|k| &k.iter_field); + let uninhabited_catch_all = configuration_key + .is_empty() + .then_some::(parse_quote!(_ => unimplemented!("Cope with empty enum"))); quote! { #[derive(Clone, Debug, PartialEq, Eq)] @@ -321,12 +500,12 @@ fn keys_enum( #( Self::#match_shape => #configuration_string, )* - // TODO make this conditional - _ => unimplemented!("Cope with empty enum") + #uninhabited_catch_all } } /// Iterates through all the variants of this enum + #[deprecated] pub fn iter() -> impl Iterator { [ #( @@ -539,8 +718,6 @@ struct ConfigurationKey { /// /// If the field is not a "multi" field, `regex_parser` will be set to `None` regex_parser: Option, - /// The number of multi fields - count_multi: usize, /// The variable names assigned to the multi fields within this configuration field_names: Vec, } @@ -589,7 +766,6 @@ fn variant_name(segments: &VecDeque<&FieldOrGroup>) -> ConfigurationKey { match_shape, match_read_write, regex_parser: Some(regex_parser), - count_multi, field_names, } } else { @@ -599,7 +775,6 @@ fn variant_name(segments: &VecDeque<&FieldOrGroup>) -> ConfigurationKey { match_read_write: parse_quote!(#ident), match_shape: parse_quote!(#ident), regex_parser: None, - count_multi, field_names: vec![], } } @@ -731,4 +906,155 @@ mod tests { prettyplease::unparse(&expected) ); } + + #[test] + fn iteration_of_multi_fields() { + let input: crate::input::Configuration = parse_quote!( + #[tedge_config(multi)] + c8y: { + url: String, + #[tedge_config(multi)] + something: { + test: u16, + } + } + ); + let mut paths = configuration_paths_from(&input.groups); + let paths = paths.iter_mut().map(|vd| &*vd.make_contiguous()); + let generated = key_iterators( + parse_quote!(TEdgeConfigReader), + parse_quote!(ReadableKey), + &paths.collect::>(), + "", + &[], + ); + let expected = parse_quote! { + impl TEdgeConfigReader { + fn readable_keys(&self) -> impl Iterator + '_ { + let c8y_keys = if let ::tedge_config_macros::Multi::Multi(map) = &self.c8y { + map.keys().map(|k| Some(k.to_owned())).collect() + } else { + vec![None] + }; + + let c8y_keys = c8y_keys + .into_iter() + .flat_map(|c8y| self.c8y.try_get(c8y.as_deref()).unwrap().readable_keys(c8y)); + + c8y_keys + } + } + + impl TEdgeConfigReaderC8y { + fn readable_keys(&self, c8y: Option) -> impl Iterator + '_ { + let something_keys = if let ::tedge_config_macros::Multi::Multi(map) = &self.something { + map.keys().map(|k| Some(k.to_owned())).collect() + } else { + vec![None] + }; + let something_keys = something_keys.into_iter().flat_map({ + let c8y = c8y.clone(); + move |something| { + self.something + .try_get(something.as_deref()) + .unwrap() + .readable_keys(c8y.clone(), something) + } + }); + + [ReadableKey::C8yUrl(c8y.clone())].into_iter().chain(something_keys) + } + } + + impl TEdgeConfigReaderC8ySomething { + fn readable_keys( + &self, + c8y: Option, + something: Option, + ) -> impl Iterator + '_ { + [ReadableKey::C8ySomethingTest( + c8y.clone(), + something.clone(), + )] + .into_iter() + } + } + }; + + pretty_assertions::assert_eq!( + prettyplease::unparse(&syn::parse2(generated).unwrap()), + prettyplease::unparse(&expected) + ); + } + + #[test] + fn iteration_of_non_multi_fields() { + let input: crate::input::Configuration = parse_quote!( + c8y: { + url: String, + } + ); + let mut paths = configuration_paths_from(&input.groups); + let paths = paths.iter_mut().map(|vd| &*vd.make_contiguous()); + let generated = key_iterators( + parse_quote!(TEdgeConfigReader), + parse_quote!(ReadableKey), + &paths.collect::>(), + "", + &[], + ); + let expected = parse_quote! { + impl TEdgeConfigReader { + fn readable_keys(&self) -> impl Iterator + '_ { + self.c8y.readable_keys() + } + } + + impl TEdgeConfigReaderC8y { + fn readable_keys(&self) -> impl Iterator + '_ { + [ReadableKey::C8yUrl].into_iter() + } + } + }; + + pretty_assertions::assert_eq!( + prettyplease::unparse(&syn::parse2(generated).unwrap()), + prettyplease::unparse(&expected) + ); + } + + #[test] + fn iteration_of_empty_field_enum() { + // let input: crate::input::Configuration = parse_quote!( + // #[tedge_config(multi)] + // c8y: { + // url: String, + // #[tedge_config(multi)] + // something: { + // test: u16, + // } + // } + // ); + // let mut paths = configuration_paths_from(&input.groups); + // let paths = paths.iter_mut().map(|vd| &*vd.make_contiguous()); + let generated = key_iterators( + parse_quote!(TEdgeConfigReader), + parse_quote!(ReadableKey), + &[], + "", + &[], + ); + let expected = parse_quote! { + impl TEdgeConfigReader { + fn readable_keys(&self) -> impl Iterator + '_ { + std::iter::empty() + } + } + }; + + pretty_assertions::assert_eq!( + prettyplease::unparse(&syn::parse2(generated).unwrap()), + prettyplease::unparse(&expected) + ); + } } From 866658285f0232ac7e4f9148e9ea8ab0a213cbca Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Wed, 25 Sep 2024 10:19:23 +0100 Subject: [PATCH 16/32] Add support for stringifying multi-value fields correctly Signed-off-by: James Rhodes --- .../src/tedge_config_cli/figment.rs | 2 +- .../src/tedge_config_cli/tedge_config.rs | 14 +- .../tedge_config_macros/examples/multi.rs | 45 +++ .../examples/renaming_keys.rs | 4 +- .../tedge_config_macros/impl/src/query.rs | 326 ++++++++++++++---- .../core/tedge/src/cli/config/commands/add.rs | 2 +- .../tedge/src/cli/config/commands/list.rs | 14 +- .../core/tedge/src/cli/config/commands/set.rs | 2 +- 8 files changed, 327 insertions(+), 82 deletions(-) diff --git a/crates/common/tedge_config/src/tedge_config_cli/figment.rs b/crates/common/tedge_config/src/tedge_config_cli/figment.rs index a0db53e1f93..9e5affcddc1 100644 --- a/crates/common/tedge_config/src/tedge_config_cli/figment.rs +++ b/crates/common/tedge_config/src/tedge_config_cli/figment.rs @@ -216,7 +216,7 @@ impl TEdgeEnv { tracing::subscriber::NoSubscriber::default(), || lowercase_name.parse::(), ) - .map(|key| key.as_str().to_owned()) + .map(|key| key.to_string()) .map_err(|err| { let is_read_only_key = matches!(err, crate::ParseKeyError::ReadOnly(_)); if is_read_only_key && !WARNINGS.lock().unwrap().insert(lowercase_name.clone()) { diff --git a/crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs b/crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs index a301be34089..5ce4bff5323 100644 --- a/crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs +++ b/crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs @@ -173,7 +173,7 @@ pub enum TomlMigrationStep { MoveKey { original: &'static str, - target: &'static str, + target: Cow<'static, str>, }, RemoveTableIfEmpty { @@ -284,7 +284,7 @@ impl TEdgeTomlVersion { use WritableKey::*; let mv = |original, target: WritableKey| TomlMigrationStep::MoveKey { original, - target: target.as_str(), + target: target.to_cow_str(), }; let update_version_field = || TomlMigrationStep::UpdateFieldValue { key: "config.version", @@ -1135,14 +1135,14 @@ fn default_http_bind_address(dto: &TEdgeConfigDto) -> IpAddr { fn device_id(reader: &TEdgeConfigReader) -> Result { let pem = PemCertificate::from_pem_file(&reader.device.cert_path) - .map_err(|err| cert_error_into_config_error(ReadOnlyKey::DeviceId.as_str(), err))?; + .map_err(|err| cert_error_into_config_error(ReadOnlyKey::DeviceId.to_cow_str(), err))?; let device_id = pem .subject_common_name() - .map_err(|err| cert_error_into_config_error(ReadOnlyKey::DeviceId.as_str(), err))?; + .map_err(|err| cert_error_into_config_error(ReadOnlyKey::DeviceId.to_cow_str(), err))?; Ok(device_id) } -fn cert_error_into_config_error(key: &'static str, err: CertificateError) -> ReadError { +fn cert_error_into_config_error(key: Cow<'static, str>, err: CertificateError) -> ReadError { match &err { CertificateError::IoError(io_err) => match io_err.kind() { std::io::ErrorKind::NotFound => ReadError::ReadOnlyNotFound { @@ -1196,12 +1196,12 @@ pub enum ReadError { #[error("Config value {key}, cannot be read: {message} ")] ReadOnlyNotFound { - key: &'static str, + key: Cow<'static, str>, message: &'static str, }, #[error("Derivation for `{key}` failed: {cause}")] - DerivationFailed { key: &'static str, cause: String }, + DerivationFailed { key: Cow<'static, str>, cause: String }, } /// An abstraction over the possible default functions for tedge config values diff --git a/crates/common/tedge_config_macros/examples/multi.rs b/crates/common/tedge_config_macros/examples/multi.rs index ac8daada42e..f789f1633ed 100644 --- a/crates/common/tedge_config_macros/examples/multi.rs +++ b/crates/common/tedge_config_macros/examples/multi.rs @@ -52,6 +52,44 @@ fn url_for<'a>(reader: &'a TEdgeConfigReader, o: Option<&str>) -> &'a str { .unwrap() } +// fn readable_keys(config: &TEdgeConfigReader) -> Vec { +// let c8y_keys = if let Multi::Multi(map) = &config.c8y { +// map.keys().map(|k| Some(k.to_owned())).collect() +// } else { +// vec![None] +// }; + +// c8y_keys.into_iter().flat_map(|c8y| readable_keys_c8y(config.c8y.try_get(c8y.as_deref()).unwrap(), c8y)).collect() +// } + +// fn readable_keys_c8y( +// config: &TEdgeConfigReaderC8y, +// c8y: Option, +// ) -> impl Iterator + '_ { +// let something_keys = if let Multi::Multi(map) = &config.something { +// map.keys().map(|k| Some(k.to_owned())).collect() +// } else { +// vec![None] +// }; +// let something_keys = something_keys.into_iter().flat_map({ +// let c8y = c8y.clone(); +// move |something| readable_keys_c8y_something(config.something.try_get(something.as_deref()).unwrap(), c8y.clone(), something) +// }); + +// std::iter::once(ReadableKey::C8yUrl(c8y)).chain(something_keys) +// } + +// fn readable_keys_c8y_something( +// _config: &TEdgeConfigReaderC8ySomething, +// c8y: Option, +// something: Option, +// ) -> impl Iterator + '_ { +// [ReadableKey::C8ySomethingTest( +// c8y.clone(), +// something.clone(), +// )].into_iter() +// } + fn main() { let single_c8y_toml = "c8y.url = \"https://example.com\""; let single_c8y_dto = toml::from_str(single_c8y_toml).unwrap(); @@ -112,4 +150,11 @@ fn main() { .to_string(), "Unknown key: 'c8y.cloud.not_a_real_key'" ); + + assert_eq!(multi_c8y_reader.readable_keys().map(|r| r.to_string()).collect::>(), [ + "c8y.cloud.url", + "c8y.cloud.something.test", + "c8y.edge.url", + "c8y.edge.something.test", + ]); } diff --git a/crates/common/tedge_config_macros/examples/renaming_keys.rs b/crates/common/tedge_config_macros/examples/renaming_keys.rs index 175fb0aa274..3ac21d92a90 100644 --- a/crates/common/tedge_config_macros/examples/renaming_keys.rs +++ b/crates/common/tedge_config_macros/examples/renaming_keys.rs @@ -67,10 +67,10 @@ fn main() { let parsed_deprecated_key = "mqtt.port".parse::().unwrap(); let parsed_current_key = "mqtt.bind.port".parse::().unwrap(); assert_eq!(parsed_deprecated_key, parsed_current_key); - assert_eq!(parsed_deprecated_key.as_str(), "mqtt.bind.port"); + assert_eq!(parsed_deprecated_key.to_cow_str(), "mqtt.bind.port"); let parsed_deprecated_key = "mqtt.client.auth.cafile".parse::().unwrap(); let parsed_current_key = "mqtt.client.auth.ca_file".parse::().unwrap(); assert_eq!(parsed_deprecated_key, parsed_current_key); - assert_eq!(parsed_deprecated_key.as_str(), "mqtt.client.auth.ca_file") + assert_eq!(parsed_deprecated_key.to_cow_str(), "mqtt.client.auth.ca_file") } diff --git a/crates/common/tedge_config_macros/impl/src/query.rs b/crates/common/tedge_config_macros/impl/src/query.rs index d214d93660a..652657e75f6 100644 --- a/crates/common/tedge_config_macros/impl/src/query.rs +++ b/crates/common/tedge_config_macros/impl/src/query.rs @@ -11,6 +11,7 @@ use proc_macro2::Span; use proc_macro2::TokenStream; use quote::quote; use quote::quote_spanned; +use std::borrow::Cow; use std::collections::VecDeque; use syn::parse_quote; use syn::parse_quote_spanned; @@ -70,14 +71,22 @@ pub fn generate_writable_keys(items: &[FieldOrGroup]) -> TokenStream { let readonly_keys_iter = key_iterators( parse_quote!(TEdgeConfigReader), parse_quote!(ReadOnlyKey), - &paths_vec.iter().copied().filter(|r| r.last().unwrap().field().unwrap().read_only().is_some()).collect::>(), + &paths_vec + .iter() + .copied() + .filter(|r| r.last().unwrap().field().unwrap().read_only().is_some()) + .collect::>(), "", &[], ); let writable_keys_iter = key_iterators( parse_quote!(TEdgeConfigReader), parse_quote!(WritableKey), - &paths_vec.iter().copied().filter(|r| r.last().unwrap().field().unwrap().read_only().is_none()).collect::>(), + &paths_vec + .iter() + .copied() + .filter(|r| r.last().unwrap().field().unwrap().read_only().is_none()) + .collect::>(), "", &[], ); @@ -143,8 +152,8 @@ pub fn generate_writable_keys(items: &[FieldOrGroup]) -> TokenStream { let Fields::Named { fields } = fields else { panic!("Expected named fields but got {:?}", fields)}; let mut aliases = struct_field_aliases(None, &fields); #( - if let Some(alias) = aliases.insert(Cow::Borrowed(#static_alias), Cow::Borrowed(ReadableKey::#iter_updated.as_str())) { - panic!("Duplicate configuration alias for '{}'. It maps to both '{}' and '{}'. Perhaps you provided an incorrect `deprecated_key` for one of these configurations?", #static_alias, alias, ReadableKey::#iter_updated.as_str()); + if let Some(alias) = aliases.insert(Cow::Borrowed(#static_alias), ReadableKey::#iter_updated.to_cow_str()) { + panic!("Duplicate configuration alias for '{}'. It maps to both '{}' and '{}'. Perhaps you provided an incorrect `deprecated_key` for one of these configurations?", #static_alias, alias, ReadableKey::#iter_updated.to_cow_str()); } )* aliases @@ -219,27 +228,26 @@ fn generate_fromstr( .zip(configuration_key.iter().map(|k| &k.enum_variant)) .map(|(s, v)| quote_spanned!(v.span()=> #s)); let iter_variant = configuration_key.iter().map(|k| &k.iter_field); - let regex_patterns = configuration_key - .iter() - .filter_map(|c| Some((c.regex_parser.clone()?, c))) - .map(|(mut r, c)| { - let match_read_write = &c.match_read_write; - let own_branches = c - .field_names - .iter() - .enumerate() - .map::(|(n, id)| { - let n = n + 1; - parse_quote! { - let #id = captures.get(#n).map(|m| m.as_str().to_owned()); - } + let regex_patterns = + configuration_key + .iter() + .filter_map(|c| Some((c.regex_parser.clone()?, c))) + .map(|(mut r, c)| { + let match_read_write = &c.match_read_write; + let own_branches = c.field_names.iter().enumerate().map::( + |(n, id)| { + let n = n + 1; + parse_quote! { + let #id = captures.get(#n).map(|re_match| re_match.as_str().to_owned()); + } + }, + ); + r.then_branch = parse_quote!({ + #(#own_branches)* + return Ok(Self::#match_read_write); }); - r.then_branch = parse_quote!({ - #(#own_branches)* - return Ok(Self::#match_read_write); + r }); - r - }); // TODO oh shit make this actually work! quote! { @@ -317,8 +325,8 @@ fn key_iterators<'a>( for (_, fields) in chunks.into_iter() { let fields = fields.collect::>(); let field = fields.first().unwrap(); - match field.split_first() { - Some((FieldOrGroup::Multi(m), rest)) => { + match field.first() { + Some(FieldOrGroup::Multi(m)) => { let ident = &m.ident; let upper_ident = m.ident.to_string().to_upper_camel_case(); let sub_type_name = @@ -368,7 +376,7 @@ fn key_iterators<'a>( &args, )); } - Some((FieldOrGroup::Group(g), rest)) => { + Some(FieldOrGroup::Group(g)) => { let upper_ident = g.ident.to_string().to_upper_camel_case(); let sub_type_name = syn::Ident::new(&format!("{reader_ty}{upper_ident}"), g.ident.span()); @@ -387,7 +395,7 @@ fn key_iterators<'a>( self.#ident.#function_name(#(#args),*) }); } - Some((FieldOrGroup::Field(f), _nothing)) => { + Some(FieldOrGroup::Field(f)) => { let ident = f.ident(); let field_name = syn::Ident::new( &format!( @@ -470,8 +478,10 @@ fn keys_enum( }); let type_name_str = type_name.to_string(); let enum_variant = configuration_key.iter().map(|k| &k.enum_variant); - let match_shape = configuration_key.iter().map(|k| &k.match_shape); - let iter_field = configuration_key.iter().map(|k| &k.iter_field); + let (fmt_match, fmt_ret): (Vec<_>, Vec<_>) = configuration_key + .iter() + .flat_map(|k| k.formatters.clone()) + .unzip(); let uninhabited_catch_all = configuration_key .is_empty() .then_some::(parse_quote!(_ => unimplemented!("Cope with empty enum"))); @@ -495,29 +505,19 @@ fn keys_enum( impl #type_name { /// Converts this key to the canonical key used by `tedge config` and `tedge.toml` #as_str_example - pub fn as_str(&self) -> &'static str { + pub fn to_cow_str(&self) -> ::std::borrow::Cow<'static, str> { match self { #( - Self::#match_shape => #configuration_string, + Self::#fmt_match => #fmt_ret, )* #uninhabited_catch_all } } - - /// Iterates through all the variants of this enum - #[deprecated] - pub fn iter() -> impl Iterator { - [ - #( - Self::#iter_field, - )* - ].into_iter() - } } impl ::std::fmt::Display for #type_name { fn fmt(&self, f: &mut ::std::fmt::Formatter<'_>) -> Result<(), ::std::fmt::Error> { - self.as_str().fmt(f) + self.to_cow_str().fmt(f) } } } @@ -720,6 +720,7 @@ struct ConfigurationKey { regex_parser: Option, /// The variable names assigned to the multi fields within this configuration field_names: Vec, + formatters: Vec<(syn::Pat, syn::Expr)>, } fn ident_for(segments: &VecDeque<&FieldOrGroup>) -> syn::Ident { @@ -738,6 +739,11 @@ fn variant_name(segments: &VecDeque<&FieldOrGroup>) -> ConfigurationKey { .iter() .filter(|fog| matches!(fog, FieldOrGroup::Multi(_))) .count(); + let key_str = segments + .iter() + .map(|segment| segment.name()) + .interleave(std::iter::repeat(Cow::Borrowed(".")).take(segments.len() - 1)) + .collect::(); if count_multi > 0 { let opt_strs = std::iter::repeat::(parse_quote!(Option)).take(count_multi); @@ -760,6 +766,41 @@ fn variant_name(segments: &VecDeque<&FieldOrGroup>) -> ConfigurationKey { .collect::>() .join("\\."); let regex_parser = parse_quote_spanned!(ident.span()=> if let Some(captures) = ::regex::Regex::new(#re).unwrap().captures(value) {}); + let formatters = field_names + .iter() + .map(|name| [parse_quote!(None), parse_quote!(Some(#name))]) + .multi_cartesian_product() + .enumerate() + .map(|(i, options): (_, Vec)| { + if i == 0 { + ( + parse_quote!(#ident(#(#options),*)), + parse_quote!(::std::borrow::Cow::Borrowed(#key_str)), + ) + } else { + let none: syn::Pat = parse_quote!(None); + let mut idents = field_names + .iter().zip(options.iter()); + let format_str = segments + .iter() + .map(|segment| match segment { + FieldOrGroup::Multi(m) => { + let (binding, opt) = idents.next().unwrap(); + if *opt == none { + m.ident.to_string() + } else { + format!("{}.{{{}}}", m.ident, binding) + } + } + FieldOrGroup::Group(g) => g.ident.to_string(), + FieldOrGroup::Field(f) => f.ident().to_string(), + }) + .interleave(std::iter::repeat(".".to_owned()).take(segments.len() - 1)) + .collect::(); + (parse_quote!(#ident(#(#options),*)), parse_quote!(::std::borrow::Cow::Owned(format!(#format_str)))) + } + }) + .collect(); ConfigurationKey { enum_variant, iter_field, @@ -767,6 +808,7 @@ fn variant_name(segments: &VecDeque<&FieldOrGroup>) -> ConfigurationKey { match_read_write, regex_parser: Some(regex_parser), field_names, + formatters, } } else { ConfigurationKey { @@ -776,6 +818,10 @@ fn variant_name(segments: &VecDeque<&FieldOrGroup>) -> ConfigurationKey { match_shape: parse_quote!(#ident), regex_parser: None, field_names: vec![], + formatters: vec![( + parse_quote!(#ident), + parse_quote!(::std::borrow::Cow::Borrowed(#key_str)), + )], } } } @@ -894,7 +940,7 @@ mod tests { _ => unimplemented!("just a test"), }; if let Some(captures) = ::regex::Regex::new("c8y(?:\\.([A-z_]+))?\\.url").unwrap().captures(value) { - let key0 = captures.get(1usize).map(|m| m.as_str().to_owned()); + let key0 = captures.get(1usize).map(|re_match| re_match.as_str().to_owned()); return Ok(Self::C8yUrl(key0)); }; res @@ -930,7 +976,7 @@ mod tests { ); let expected = parse_quote! { impl TEdgeConfigReader { - fn readable_keys(&self) -> impl Iterator + '_ { + pub fn readable_keys(&self) -> impl Iterator + '_ { let c8y_keys = if let ::tedge_config_macros::Multi::Multi(map) = &self.c8y { map.keys().map(|k| Some(k.to_owned())).collect() } else { @@ -946,7 +992,7 @@ mod tests { } impl TEdgeConfigReaderC8y { - fn readable_keys(&self, c8y: Option) -> impl Iterator + '_ { + pub fn readable_keys(&self, c8y: Option) -> impl Iterator + '_ { let something_keys = if let ::tedge_config_macros::Multi::Multi(map) = &self.something { map.keys().map(|k| Some(k.to_owned())).collect() } else { @@ -967,7 +1013,7 @@ mod tests { } impl TEdgeConfigReaderC8ySomething { - fn readable_keys( + pub fn readable_keys( &self, c8y: Option, something: Option, @@ -1005,13 +1051,13 @@ mod tests { ); let expected = parse_quote! { impl TEdgeConfigReader { - fn readable_keys(&self) -> impl Iterator + '_ { + pub fn readable_keys(&self) -> impl Iterator + '_ { self.c8y.readable_keys() } } impl TEdgeConfigReaderC8y { - fn readable_keys(&self) -> impl Iterator + '_ { + pub fn readable_keys(&self) -> impl Iterator + '_ { [ReadableKey::C8yUrl].into_iter() } } @@ -1024,19 +1070,7 @@ mod tests { } #[test] - fn iteration_of_empty_field_enum() { - // let input: crate::input::Configuration = parse_quote!( - // #[tedge_config(multi)] - // c8y: { - // url: String, - // #[tedge_config(multi)] - // something: { - // test: u16, - // } - // } - // ); - // let mut paths = configuration_paths_from(&input.groups); - // let paths = paths.iter_mut().map(|vd| &*vd.make_contiguous()); + fn iteration_of_empty_field_enum_is_an_empty_iterator() { let generated = key_iterators( parse_quote!(TEdgeConfigReader), parse_quote!(ReadableKey), @@ -1046,7 +1080,7 @@ mod tests { ); let expected = parse_quote! { impl TEdgeConfigReader { - fn readable_keys(&self) -> impl Iterator + '_ { + pub fn readable_keys(&self) -> impl Iterator + '_ { std::iter::empty() } } @@ -1057,4 +1091,172 @@ mod tests { prettyplease::unparse(&expected) ); } + + #[test] + fn impl_for_simple_group() { + let input: crate::input::Configuration = parse_quote!( + c8y: { + url: String, + } + ); + let paths = configuration_paths_from(&input.groups); + let config_keys = configuration_strings(paths.iter()); + let generated = keys_enum(parse_quote!(ReadableKey), &config_keys, "DOC FRAGMENT"); + let generated_file: syn::File = syn::parse2(generated).unwrap(); + let mut impl_block = generated_file + .items + .into_iter() + .find_map(|item| { + if let syn::Item::Impl(r#impl @ syn::ItemImpl { trait_: None, .. }) = item { + Some(r#impl) + } else { + None + } + }) + .expect("Should generate an impl block for ReadableKey"); + for item in &mut impl_block.items { + match item { + syn::ImplItem::Fn(f) => f + .attrs + .retain(|f| f.path().get_ident().unwrap().to_string() != "doc"), + _ => (), + } + } + let expected = parse_quote! { + impl ReadableKey { + #[deprecated] + pub fn as_str(&self) -> &'static str { + match self { + Self::C8yUrl => "c8y.url", + } + } + + pub fn to_cow_str(&self) -> ::std::borrow::Cow<'static, str> { + match self { + Self::C8yUrl => ::std::borrow::Cow::Borrowed("c8y.url"), + } + } + } + }; + + pretty_assertions::assert_eq!( + prettyplease::unparse(&parse_quote!(#impl_block)), + prettyplease::unparse(&expected) + ); + } + + #[test] + fn impl_for_multi() { + let input: crate::input::Configuration = parse_quote!( + #[tedge_config(multi)] + c8y: { + url: String, + } + ); + let paths = configuration_paths_from(&input.groups); + let config_keys = configuration_strings(paths.iter()); + let generated = keys_enum(parse_quote!(ReadableKey), &config_keys, "DOC FRAGMENT"); + let generated_file: syn::File = syn::parse2(generated).unwrap(); + let mut impl_block = generated_file + .items + .into_iter() + .find_map(|item| { + if let syn::Item::Impl(r#impl @ syn::ItemImpl { trait_: None, .. }) = item { + Some(r#impl) + } else { + None + } + }) + .expect("Should generate an impl block for ReadableKey"); + for item in &mut impl_block.items { + match item { + syn::ImplItem::Fn(f) => f + .attrs + .retain(|f| f.path().get_ident().unwrap().to_string() != "doc"), + _ => (), + } + } + let expected = parse_quote! { + impl ReadableKey { + #[deprecated] + pub fn as_str(&self) -> &'static str { + match self { + Self::C8yUrl(_) => "c8y.url", + } + } + + pub fn to_cow_str(&self) -> ::std::borrow::Cow<'static, str> { + match self { + Self::C8yUrl(None) => ::std::borrow::Cow::Borrowed("c8y.url"), + Self::C8yUrl(Some(key0)) => ::std::borrow::Cow::Owned(format!("c8y.{key0}.url")), + } + } + } + }; + + pretty_assertions::assert_eq!( + prettyplease::unparse(&parse_quote!(#impl_block)), + prettyplease::unparse(&expected) + ); + } + + #[test] + fn impl_for_nested_multi() { + let input: crate::input::Configuration = parse_quote!( + #[tedge_config(multi)] + top: { + #[tedge_config(multi)] + nested: { + field: String, + } + } + ); + let paths = configuration_paths_from(&input.groups); + let config_keys = configuration_strings(paths.iter()); + let generated = keys_enum(parse_quote!(ReadableKey), &config_keys, "DOC FRAGMENT"); + let generated_file: syn::File = syn::parse2(generated).unwrap(); + let mut impl_block = generated_file + .items + .into_iter() + .find_map(|item| { + if let syn::Item::Impl(r#impl @ syn::ItemImpl { trait_: None, .. }) = item { + Some(r#impl) + } else { + None + } + }) + .expect("Should generate an impl block for ReadableKey"); + for item in &mut impl_block.items { + match item { + syn::ImplItem::Fn(f) => f + .attrs + .retain(|f| f.path().get_ident().unwrap().to_string() != "doc"), + _ => (), + } + } + let expected = parse_quote! { + impl ReadableKey { + #[deprecated] + pub fn as_str(&self) -> &'static str { + match self { + Self::TopNestedField(_, _) => "top.nested.field", + } + } + + pub fn to_cow_str(&self) -> ::std::borrow::Cow<'static, str> { + match self { + Self::TopNestedField(None, None) => ::std::borrow::Cow::Borrowed("top.nested.field"), + Self::TopNestedField(None, Some(key1)) => ::std::borrow::Cow::Owned(format!("top.nested.{key1}.field")), + Self::TopNestedField(Some(key0), None) => ::std::borrow::Cow::Owned(format!("top.{key0}.nested.field")), + Self::TopNestedField(Some(key0), Some(key1)) => ::std::borrow::Cow::Owned(format!("top.{key0}.nested.{key1}.field")), + } + } + } + }; + + pretty_assertions::assert_eq!( + prettyplease::unparse(&parse_quote!(#impl_block)), + prettyplease::unparse(&expected) + ); + } } diff --git a/crates/core/tedge/src/cli/config/commands/add.rs b/crates/core/tedge/src/cli/config/commands/add.rs index 8f1fa905544..594a19fa5bf 100644 --- a/crates/core/tedge/src/cli/config/commands/add.rs +++ b/crates/core/tedge/src/cli/config/commands/add.rs @@ -12,7 +12,7 @@ impl Command for AddConfigCommand { fn description(&self) -> String { format!( "set the configuration key: '{}' with value: {}.", - self.key.as_str(), + self.key.to_cow_str(), self.value ) } diff --git a/crates/core/tedge/src/cli/config/commands/list.rs b/crates/core/tedge/src/cli/config/commands/list.rs index ceab4648103..6033846f407 100644 --- a/crates/core/tedge/src/cli/config/commands/list.rs +++ b/crates/core/tedge/src/cli/config/commands/list.rs @@ -1,9 +1,8 @@ use crate::command::Command; use crate::ConfigError; -use pad::PadStr; use std::io::stdout; use std::io::IsTerminal; -use tedge_config::ReadableKey; +use pad::PadStr; use tedge_config::TEdgeConfig; use tedge_config::READABLE_KEYS; @@ -20,7 +19,7 @@ impl Command for ListConfigCommand { fn execute(&self) -> anyhow::Result<()> { if self.is_doc { - print_config_doc(); + print_config_doc(&self.config); } else { print_config_list(&self.config, self.is_all)?; } @@ -31,8 +30,7 @@ impl Command for ListConfigCommand { fn print_config_list(config: &TEdgeConfig, all: bool) -> Result<(), ConfigError> { let mut keys_without_values = Vec::new(); - // TODO fix this logic, it's just broken for multi variants atm - for config_key in ReadableKey::iter() { + for config_key in config.readable_keys() { match config.read_string(&config_key).ok() { Some(value) => { println!("{}={}", config_key, value); @@ -51,13 +49,13 @@ fn print_config_list(config: &TEdgeConfig, all: bool) -> Result<(), ConfigError> Ok(()) } -fn print_config_doc() { +fn print_config_doc(config: &TEdgeConfig) { if !stdout().is_terminal() { yansi::Paint::disable(); } - let max_length = ReadableKey::iter() - .map(|c| c.as_str().len()) + let max_length = config.readable_keys() + .map(|c| c.to_cow_str().len()) .max() .unwrap_or_default(); diff --git a/crates/core/tedge/src/cli/config/commands/set.rs b/crates/core/tedge/src/cli/config/commands/set.rs index b109f624000..7b6dbd8bd96 100644 --- a/crates/core/tedge/src/cli/config/commands/set.rs +++ b/crates/core/tedge/src/cli/config/commands/set.rs @@ -12,7 +12,7 @@ impl Command for SetConfigCommand { fn description(&self) -> String { format!( "set the configuration key: '{}' with value: {}.", - self.key.as_str(), + self.key.to_cow_str(), self.value ) } From 9c816dac3fd54cf07181611c05415f905538b2c6 Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Wed, 25 Sep 2024 10:21:57 +0100 Subject: [PATCH 17/32] Tidy up code following the new multi-value logic Signed-off-by: James Rhodes --- .../src/tedge_config_cli/tedge_config.rs | 5 +- .../tedge_config_macros/examples/multi.rs | 18 ++- .../examples/renaming_keys.rs | 5 +- .../tedge_config_macros/impl/src/query.rs | 143 ++++++------------ .../tedge/src/cli/config/commands/list.rs | 5 +- 5 files changed, 71 insertions(+), 105 deletions(-) diff --git a/crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs b/crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs index 5ce4bff5323..b0d26666609 100644 --- a/crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs +++ b/crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs @@ -1201,7 +1201,10 @@ pub enum ReadError { }, #[error("Derivation for `{key}` failed: {cause}")] - DerivationFailed { key: Cow<'static, str>, cause: String }, + DerivationFailed { + key: Cow<'static, str>, + cause: String, + }, } /// An abstraction over the possible default functions for tedge config values diff --git a/crates/common/tedge_config_macros/examples/multi.rs b/crates/common/tedge_config_macros/examples/multi.rs index f789f1633ed..3604cef19b2 100644 --- a/crates/common/tedge_config_macros/examples/multi.rs +++ b/crates/common/tedge_config_macros/examples/multi.rs @@ -151,10 +151,16 @@ fn main() { "Unknown key: 'c8y.cloud.not_a_real_key'" ); - assert_eq!(multi_c8y_reader.readable_keys().map(|r| r.to_string()).collect::>(), [ - "c8y.cloud.url", - "c8y.cloud.something.test", - "c8y.edge.url", - "c8y.edge.something.test", - ]); + assert_eq!( + multi_c8y_reader + .readable_keys() + .map(|r| r.to_string()) + .collect::>(), + [ + "c8y.cloud.url", + "c8y.cloud.something.test", + "c8y.edge.url", + "c8y.edge.something.test", + ] + ); } diff --git a/crates/common/tedge_config_macros/examples/renaming_keys.rs b/crates/common/tedge_config_macros/examples/renaming_keys.rs index 3ac21d92a90..7efa1296c21 100644 --- a/crates/common/tedge_config_macros/examples/renaming_keys.rs +++ b/crates/common/tedge_config_macros/examples/renaming_keys.rs @@ -72,5 +72,8 @@ fn main() { let parsed_deprecated_key = "mqtt.client.auth.cafile".parse::().unwrap(); let parsed_current_key = "mqtt.client.auth.ca_file".parse::().unwrap(); assert_eq!(parsed_deprecated_key, parsed_current_key); - assert_eq!(parsed_deprecated_key.to_cow_str(), "mqtt.client.auth.ca_file") + assert_eq!( + parsed_deprecated_key.to_cow_str(), + "mqtt.client.auth.ca_file" + ) } diff --git a/crates/common/tedge_config_macros/impl/src/query.rs b/crates/common/tedge_config_macros/impl/src/query.rs index 652657e75f6..eca547c2f71 100644 --- a/crates/common/tedge_config_macros/impl/src/query.rs +++ b/crates/common/tedge_config_macros/impl/src/query.rs @@ -22,7 +22,7 @@ pub fn generate_writable_keys(items: &[FieldOrGroup]) -> TokenStream { let (readonly_destr, write_error): (Vec<_>, Vec<_>) = paths .iter() .filter_map(|field| { - let configuration = variant_name(field); + let configuration = enum_variant(field); Some(( configuration.match_shape, field @@ -185,7 +185,7 @@ fn configuration_strings<'a>( ) -> (Vec, Vec) { variants .map(|segments| { - let configuration_key = variant_name(segments); + let configuration_key = enum_variant(segments); ( segments .iter() @@ -210,7 +210,7 @@ fn deprecated_keys<'a>( .unwrap() .deprecated_keys() .map(|key| { - let configuration_key = variant_name(segments); + let configuration_key = enum_variant(segments); (key, configuration_key) }) }) @@ -543,10 +543,10 @@ fn generate_field_accessor<'a>( } fn generate_string_readers(paths: &[VecDeque<&FieldOrGroup>]) -> TokenStream { - let variant_names = paths.iter().map(variant_name); + let enum_variants = paths.iter().map(enum_variant); let arms = paths .iter() - .zip(variant_names) + .zip(enum_variants) .map(|(path, configuration_key)| -> syn::Arm { let field = path .back() @@ -593,7 +593,7 @@ fn generate_string_readers(paths: &[VecDeque<&FieldOrGroup>]) -> TokenStream { } fn generate_string_writers(paths: &[VecDeque<&FieldOrGroup>]) -> TokenStream { - let variant_names = paths.iter().map(variant_name); + let enum_variants = paths.iter().map(enum_variant); let (update_arms, unset_arms, append_arms, remove_arms): ( Vec, Vec, @@ -601,7 +601,7 @@ fn generate_string_writers(paths: &[VecDeque<&FieldOrGroup>]) -> TokenStream { Vec, ) = paths .iter() - .zip(variant_names) + .zip(enum_variants) .map(|(path, configuration_key)| { let read_segments = generate_field_accessor(path, "try_get"); let write_segments = generate_field_accessor(path, "try_get_mut").collect::>(); @@ -733,7 +733,7 @@ fn ident_for(segments: &VecDeque<&FieldOrGroup>) -> syn::Ident { ) } -fn variant_name(segments: &VecDeque<&FieldOrGroup>) -> ConfigurationKey { +fn enum_variant(segments: &VecDeque<&FieldOrGroup>) -> ConfigurationKey { let ident = ident_for(segments); let count_multi = segments .iter() @@ -779,8 +779,7 @@ fn variant_name(segments: &VecDeque<&FieldOrGroup>) -> ConfigurationKey { ) } else { let none: syn::Pat = parse_quote!(None); - let mut idents = field_names - .iter().zip(options.iter()); + let mut idents = field_names.iter().zip(options.iter()); let format_str = segments .iter() .map(|segment| match segment { @@ -797,7 +796,10 @@ fn variant_name(segments: &VecDeque<&FieldOrGroup>) -> ConfigurationKey { }) .interleave(std::iter::repeat(".".to_owned()).take(segments.len() - 1)) .collect::(); - (parse_quote!(#ident(#(#options),*)), parse_quote!(::std::borrow::Cow::Owned(format!(#format_str)))) + ( + parse_quote!(#ident(#(#options),*)), + parse_quote!(::std::borrow::Cow::Owned(format!(#format_str))), + ) } }) .collect(); @@ -855,6 +857,7 @@ fn is_read_write(path: &VecDeque<&FieldOrGroup>) -> bool { #[cfg(test)] mod tests { use super::*; + use syn::ItemImpl; #[test] fn output_parses() { @@ -884,7 +887,7 @@ mod tests { let generated = generate_fromstr( syn::Ident::new("ReadableKey", Span::call_site()), &c, - parse_quote!(_ => unimplemented!("just a test")), + parse_quote!(_ => unimplemented!("just a test, no error handling")), ); let expected = parse_quote!( impl ::std::str::FromStr for ReadableKey { @@ -898,7 +901,7 @@ mod tests { } return Ok(Self::C8yUrl); }, - _ => unimplemented!("just a test"), + _ => unimplemented!("just a test, no error handling"), }; res } @@ -923,7 +926,7 @@ mod tests { let generated = generate_fromstr( syn::Ident::new("ReadableKey", Span::call_site()), &c, - parse_quote!(_ => unimplemented!("just a test")), + parse_quote!(_ => unimplemented!("just a test, no error handling")), ); let expected = parse_quote!( impl ::std::str::FromStr for ReadableKey { @@ -937,7 +940,7 @@ mod tests { } return Ok(Self::C8yUrl(None)); }, - _ => unimplemented!("just a test"), + _ => unimplemented!("just a test, no error handling"), }; if let Some(captures) = ::regex::Regex::new("c8y(?:\\.([A-z_]+))?\\.url").unwrap().captures(value) { let key0 = captures.get(1usize).map(|re_match| re_match.as_str().to_owned()); @@ -1101,36 +1104,10 @@ mod tests { ); let paths = configuration_paths_from(&input.groups); let config_keys = configuration_strings(paths.iter()); - let generated = keys_enum(parse_quote!(ReadableKey), &config_keys, "DOC FRAGMENT"); - let generated_file: syn::File = syn::parse2(generated).unwrap(); - let mut impl_block = generated_file - .items - .into_iter() - .find_map(|item| { - if let syn::Item::Impl(r#impl @ syn::ItemImpl { trait_: None, .. }) = item { - Some(r#impl) - } else { - None - } - }) - .expect("Should generate an impl block for ReadableKey"); - for item in &mut impl_block.items { - match item { - syn::ImplItem::Fn(f) => f - .attrs - .retain(|f| f.path().get_ident().unwrap().to_string() != "doc"), - _ => (), - } - } + let impl_block = keys_enum_impl_block(&config_keys); + let expected = parse_quote! { impl ReadableKey { - #[deprecated] - pub fn as_str(&self) -> &'static str { - match self { - Self::C8yUrl => "c8y.url", - } - } - pub fn to_cow_str(&self) -> ::std::borrow::Cow<'static, str> { match self { Self::C8yUrl => ::std::borrow::Cow::Borrowed("c8y.url"), @@ -1155,36 +1132,10 @@ mod tests { ); let paths = configuration_paths_from(&input.groups); let config_keys = configuration_strings(paths.iter()); - let generated = keys_enum(parse_quote!(ReadableKey), &config_keys, "DOC FRAGMENT"); - let generated_file: syn::File = syn::parse2(generated).unwrap(); - let mut impl_block = generated_file - .items - .into_iter() - .find_map(|item| { - if let syn::Item::Impl(r#impl @ syn::ItemImpl { trait_: None, .. }) = item { - Some(r#impl) - } else { - None - } - }) - .expect("Should generate an impl block for ReadableKey"); - for item in &mut impl_block.items { - match item { - syn::ImplItem::Fn(f) => f - .attrs - .retain(|f| f.path().get_ident().unwrap().to_string() != "doc"), - _ => (), - } - } + let impl_block = keys_enum_impl_block(&config_keys); + let expected = parse_quote! { impl ReadableKey { - #[deprecated] - pub fn as_str(&self) -> &'static str { - match self { - Self::C8yUrl(_) => "c8y.url", - } - } - pub fn to_cow_str(&self) -> ::std::borrow::Cow<'static, str> { match self { Self::C8yUrl(None) => ::std::borrow::Cow::Borrowed("c8y.url"), @@ -1213,19 +1164,43 @@ mod tests { ); let paths = configuration_paths_from(&input.groups); let config_keys = configuration_strings(paths.iter()); + let impl_block = keys_enum_impl_block(&config_keys); + + let expected = parse_quote! { + impl ReadableKey { + pub fn to_cow_str(&self) -> ::std::borrow::Cow<'static, str> { + match self { + Self::TopNestedField(None, None) => ::std::borrow::Cow::Borrowed("top.nested.field"), + Self::TopNestedField(None, Some(key1)) => ::std::borrow::Cow::Owned(format!("top.nested.{key1}.field")), + Self::TopNestedField(Some(key0), None) => ::std::borrow::Cow::Owned(format!("top.{key0}.nested.field")), + Self::TopNestedField(Some(key0), Some(key1)) => ::std::borrow::Cow::Owned(format!("top.{key0}.nested.{key1}.field")), + } + } + } + }; + + pretty_assertions::assert_eq!( + prettyplease::unparse(&parse_quote!(#impl_block)), + prettyplease::unparse(&expected) + ); + } + + fn keys_enum_impl_block(config_keys: &(Vec, Vec)) -> ItemImpl { let generated = keys_enum(parse_quote!(ReadableKey), &config_keys, "DOC FRAGMENT"); let generated_file: syn::File = syn::parse2(generated).unwrap(); let mut impl_block = generated_file .items .into_iter() .find_map(|item| { - if let syn::Item::Impl(r#impl @ syn::ItemImpl { trait_: None, .. }) = item { + if let syn::Item::Impl(r#impl @ ItemImpl { trait_: None, .. }) = item { Some(r#impl) } else { None } }) .expect("Should generate an impl block for ReadableKey"); + + // Remove doc comments from items for item in &mut impl_block.items { match item { syn::ImplItem::Fn(f) => f @@ -1234,29 +1209,7 @@ mod tests { _ => (), } } - let expected = parse_quote! { - impl ReadableKey { - #[deprecated] - pub fn as_str(&self) -> &'static str { - match self { - Self::TopNestedField(_, _) => "top.nested.field", - } - } - pub fn to_cow_str(&self) -> ::std::borrow::Cow<'static, str> { - match self { - Self::TopNestedField(None, None) => ::std::borrow::Cow::Borrowed("top.nested.field"), - Self::TopNestedField(None, Some(key1)) => ::std::borrow::Cow::Owned(format!("top.nested.{key1}.field")), - Self::TopNestedField(Some(key0), None) => ::std::borrow::Cow::Owned(format!("top.{key0}.nested.field")), - Self::TopNestedField(Some(key0), Some(key1)) => ::std::borrow::Cow::Owned(format!("top.{key0}.nested.{key1}.field")), - } - } - } - }; - - pretty_assertions::assert_eq!( - prettyplease::unparse(&parse_quote!(#impl_block)), - prettyplease::unparse(&expected) - ); + impl_block } } diff --git a/crates/core/tedge/src/cli/config/commands/list.rs b/crates/core/tedge/src/cli/config/commands/list.rs index 6033846f407..0e9e0e1b024 100644 --- a/crates/core/tedge/src/cli/config/commands/list.rs +++ b/crates/core/tedge/src/cli/config/commands/list.rs @@ -1,8 +1,8 @@ use crate::command::Command; use crate::ConfigError; +use pad::PadStr; use std::io::stdout; use std::io::IsTerminal; -use pad::PadStr; use tedge_config::TEdgeConfig; use tedge_config::READABLE_KEYS; @@ -54,7 +54,8 @@ fn print_config_doc(config: &TEdgeConfig) { yansi::Paint::disable(); } - let max_length = config.readable_keys() + let max_length = config + .readable_keys() .map(|c| c.to_cow_str().len()) .max() .unwrap_or_default(); From 615d0f82de8e218bdbd0a75b2f1a312e8e6318e8 Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Wed, 25 Sep 2024 10:23:37 +0100 Subject: [PATCH 18/32] Remove dead code Signed-off-by: James Rhodes --- .../tedge_config_macros/examples/multi.rs | 38 ------------------- 1 file changed, 38 deletions(-) diff --git a/crates/common/tedge_config_macros/examples/multi.rs b/crates/common/tedge_config_macros/examples/multi.rs index 3604cef19b2..50833d31bfa 100644 --- a/crates/common/tedge_config_macros/examples/multi.rs +++ b/crates/common/tedge_config_macros/examples/multi.rs @@ -52,44 +52,6 @@ fn url_for<'a>(reader: &'a TEdgeConfigReader, o: Option<&str>) -> &'a str { .unwrap() } -// fn readable_keys(config: &TEdgeConfigReader) -> Vec { -// let c8y_keys = if let Multi::Multi(map) = &config.c8y { -// map.keys().map(|k| Some(k.to_owned())).collect() -// } else { -// vec![None] -// }; - -// c8y_keys.into_iter().flat_map(|c8y| readable_keys_c8y(config.c8y.try_get(c8y.as_deref()).unwrap(), c8y)).collect() -// } - -// fn readable_keys_c8y( -// config: &TEdgeConfigReaderC8y, -// c8y: Option, -// ) -> impl Iterator + '_ { -// let something_keys = if let Multi::Multi(map) = &config.something { -// map.keys().map(|k| Some(k.to_owned())).collect() -// } else { -// vec![None] -// }; -// let something_keys = something_keys.into_iter().flat_map({ -// let c8y = c8y.clone(); -// move |something| readable_keys_c8y_something(config.something.try_get(something.as_deref()).unwrap(), c8y.clone(), something) -// }); - -// std::iter::once(ReadableKey::C8yUrl(c8y)).chain(something_keys) -// } - -// fn readable_keys_c8y_something( -// _config: &TEdgeConfigReaderC8ySomething, -// c8y: Option, -// something: Option, -// ) -> impl Iterator + '_ { -// [ReadableKey::C8ySomethingTest( -// c8y.clone(), -// something.clone(), -// )].into_iter() -// } - fn main() { let single_c8y_toml = "c8y.url = \"https://example.com\""; let single_c8y_dto = toml::from_str(single_c8y_toml).unwrap(); From d923fc3bf73a25d0379b72ed1e31d80fd046a711 Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Wed, 25 Sep 2024 16:37:24 +0100 Subject: [PATCH 19/32] Tidy up most of the remaining TODOs Signed-off-by: James Rhodes --- Cargo.lock | 1 + crates/common/axum_tls/src/config.rs | 6 +- crates/common/tedge_config_macros/Cargo.toml | 1 + .../tedge_config_macros/impl/src/namegen.rs | 2 +- .../tedge_config_macros/impl/src/query.rs | 89 +++++++++++++++--- .../tedge_config_macros/impl/src/reader.rs | 37 ++++++-- .../tedge_config_macros/src/all_or_nothing.rs | 22 ++--- .../common/tedge_config_macros/src/multi.rs | 90 +++++++++++++++++-- .../common/tedge_config_macros/src/option.rs | 43 +++++---- .../tedge_config_macros/tests/optional.rs | 2 +- .../src/file_transfer_server/actor.rs | 2 +- 11 files changed, 236 insertions(+), 59 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7b0f88272e4..69fded42a52 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3979,6 +3979,7 @@ dependencies = [ "once_cell", "regex", "serde", + "serde_json", "tedge_config_macros-macro", "thiserror", "toml 0.7.8", diff --git a/crates/common/axum_tls/src/config.rs b/crates/common/axum_tls/src/config.rs index 8891cb8e3f6..53768374eb1 100644 --- a/crates/common/axum_tls/src/config.rs +++ b/crates/common/axum_tls/src/config.rs @@ -77,7 +77,7 @@ pub fn load_ssl_config( let cert_key = cert_path.key(); let key_key = key_path.key(); let ca_key = ca_path.key(); - if let Some((cert, key)) = load_certificate_and_key(cert_path, key_path)? { + if let Some((cert, key)) = load_certificate_and_key(&cert_path, &key_path)? { let trust_store = match ca_path.or_none() { Some(path) => path .load_trust_store() @@ -103,8 +103,8 @@ pub fn load_ssl_config( type CertKeyPair = (Vec>, Vec); fn load_certificate_and_key( - cert_path: OptionalConfig, - key_path: OptionalConfig, + cert_path: &OptionalConfig, + key_path: &OptionalConfig, ) -> anyhow::Result> { let paths = tedge_config::all_or_nothing((cert_path.as_ref(), key_path.as_ref())) .map_err(|e| anyhow!("{e}"))?; diff --git a/crates/common/tedge_config_macros/Cargo.toml b/crates/common/tedge_config_macros/Cargo.toml index 69cd92c2e62..9fee90ef222 100644 --- a/crates/common/tedge_config_macros/Cargo.toml +++ b/crates/common/tedge_config_macros/Cargo.toml @@ -22,6 +22,7 @@ url = { workspace = true } [dev-dependencies] serde = { workspace = true, features = ["rc"] } +serde_json = { workspace = true } toml = { workspace = true } [lints] diff --git a/crates/common/tedge_config_macros/impl/src/namegen.rs b/crates/common/tedge_config_macros/impl/src/namegen.rs index ac572fb14a1..d2212e51de1 100644 --- a/crates/common/tedge_config_macros/impl/src/namegen.rs +++ b/crates/common/tedge_config_macros/impl/src/namegen.rs @@ -55,7 +55,7 @@ pub trait IdGenerator: Default { #[derive(Debug, Default)] pub struct SequentialIdGenerator { - count: u32, + pub count: u32, } impl SequentialIdGenerator { diff --git a/crates/common/tedge_config_macros/impl/src/query.rs b/crates/common/tedge_config_macros/impl/src/query.rs index eca547c2f71..95b9613e33e 100644 --- a/crates/common/tedge_config_macros/impl/src/query.rs +++ b/crates/common/tedge_config_macros/impl/src/query.rs @@ -249,7 +249,6 @@ fn generate_fromstr( r }); - // TODO oh shit make this actually work! quote! { impl ::std::str::FromStr for #type_name { type Err = ParseKeyError; @@ -303,7 +302,7 @@ fn generate_fromstr_writable( ) } -fn key_iterators<'a>( +fn key_iterators( reader_ty: syn::Ident, type_name: syn::Ident, fields: &[&[&FieldOrGroup]], @@ -390,7 +389,6 @@ fn key_iterators<'a>( args, )); let ident = &g.ident; - // TODO test the args are correct exprs.push_back(parse_quote! { self.#ident.#function_name(#(#args),*) }); @@ -558,7 +556,6 @@ fn generate_string_readers(paths: &[VecDeque<&FieldOrGroup>]) -> TokenStream { let match_variant = configuration_key.match_read_write; if field.read_only().is_some() { if extract_type_from_result(field.ty()).is_some() { - // TODO test whether the wrong type fails unit tests parse_quote! { ReadableKey::#match_variant => Ok(self.#(#segments).*.try_read(self)?#to_string), } @@ -699,10 +696,13 @@ fn generate_string_writers(paths: &[VecDeque<&FieldOrGroup>]) -> TokenStream { } } +/// A configuration key that is stored in an enum variant +/// +/// The macro generates e.g. `ReadableKey` to list the variants struct ConfigurationKey { /// e.g. `C8yUrl(Option)` enum_variant: syn::Variant, - // TODO kill this when it's not used + /// An example of each field, with any multi-value keys set to `None` iter_field: syn::Expr, /// e.g. `C8yUrl(key0)` match_read_write: syn::Pat, @@ -720,6 +720,15 @@ struct ConfigurationKey { regex_parser: Option, /// The variable names assigned to the multi fields within this configuration field_names: Vec, + /// Format strings for each field e.g. + /// ```compile_fail + /// vec![ + /// (C8yUrl(None), Cow::Borrowed("c8y.url")), + /// (C8yTopicPrefix(None), Cow::Borrowed("c8y.topic_prefix")), + /// (C8yUrl(Some(c8y_name)), Cow::Owned(format!("c8y.{c8y_name}.url"))), + /// (C8yTopicPrefix(Some(c8y_name)), Cow::Owned(format!("c8y.{c8y_name}.topic_prefix"))), + /// ] + /// ``` formatters: Vec<(syn::Pat, syn::Expr)>, } @@ -1095,6 +1104,66 @@ mod tests { ); } + #[test] + fn iteration_of_non_multi_groups() { + let input: crate::input::Configuration = parse_quote!( + #[tedge_config(multi)] + c8y: { + nested: { + field: String, + } + } + ); + let mut paths = configuration_paths_from(&input.groups); + let paths = paths.iter_mut().map(|vd| &*vd.make_contiguous()); + let generated = key_iterators( + parse_quote!(TEdgeConfigReader), + parse_quote!(ReadableKey), + &paths.collect::>(), + "", + &[], + ); + let expected = parse_quote! { + impl TEdgeConfigReader { + pub fn readable_keys(&self) -> impl Iterator + '_ { + let c8y_keys = if let ::tedge_config_macros::Multi::Multi(map) = &self.c8y { + map.keys().map(|k| Some(k.to_owned())).collect() + } else { + vec![None] + }; + + let c8y_keys = c8y_keys + .into_iter() + .flat_map(|c8y| self.c8y.try_get(c8y.as_deref()).unwrap().readable_keys(c8y)); + + c8y_keys + } + } + + impl TEdgeConfigReaderC8y { + pub fn readable_keys(&self, c8y: Option) -> impl Iterator + '_ { + self.nested.readable_keys(c8y) + } + } + + impl TEdgeConfigReaderC8yNested { + pub fn readable_keys( + &self, + c8y: Option, + ) -> impl Iterator + '_ { + [ReadableKey::C8yNestedField( + c8y.clone(), + )] + .into_iter() + } + } + }; + + pretty_assertions::assert_eq!( + prettyplease::unparse(&syn::parse2(generated).unwrap()), + prettyplease::unparse(&expected) + ); + } #[test] fn impl_for_simple_group() { let input: crate::input::Configuration = parse_quote!( @@ -1186,7 +1255,7 @@ mod tests { } fn keys_enum_impl_block(config_keys: &(Vec, Vec)) -> ItemImpl { - let generated = keys_enum(parse_quote!(ReadableKey), &config_keys, "DOC FRAGMENT"); + let generated = keys_enum(parse_quote!(ReadableKey), config_keys, "DOC FRAGMENT"); let generated_file: syn::File = syn::parse2(generated).unwrap(); let mut impl_block = generated_file .items @@ -1202,11 +1271,9 @@ mod tests { // Remove doc comments from items for item in &mut impl_block.items { - match item { - syn::ImplItem::Fn(f) => f - .attrs - .retain(|f| f.path().get_ident().unwrap().to_string() != "doc"), - _ => (), + if let syn::ImplItem::Fn(f) = item { + f.attrs + .retain(|f| *f.path().get_ident().unwrap() != "doc"); } } diff --git a/crates/common/tedge_config_macros/impl/src/reader.rs b/crates/common/tedge_config_macros/impl/src/reader.rs index 0f35d52f2c7..028a9f3b02f 100644 --- a/crates/common/tedge_config_macros/impl/src/reader.rs +++ b/crates/common/tedge_config_macros/impl/src/reader.rs @@ -5,6 +5,7 @@ use std::iter; use std::iter::once; +use itertools::Itertools; use proc_macro2::Span; use proc_macro2::TokenStream; use quote::quote; @@ -268,14 +269,34 @@ fn reader_value_for_field<'a>( let name = field.ident(); Ok(match field { ConfigurableField::ReadWrite(field) => { - // TODO optionalconfig should contain the actual key - let key = parents - .iter() - .filter_map(PathItem::as_static) - .map(|p| p.to_string()) - .chain(iter::once(name.to_string())) - .collect::>() - .join("."); + let key: syn::Expr = if parents.iter().all(|p| p.as_static().is_some()) { + #[allow(unstable_name_collisions)] + let key_str = parents + .iter() + .map(|p| match p { + PathItem::Static(p) => p.to_string(), + PathItem::Dynamic(_) => { + unreachable!("all pathitems are static in this if branch") + } + }) + .chain(iter::once(name.to_string())) + .intersperse(".".to_owned()) + .collect::(); + parse_quote!(#key_str.into()) + } else { + let mut id_gen = SequentialIdGenerator::default(); + let elems = parents.iter().map::(|p| match p { + PathItem::Static(p) => { + let p_str = p.to_string(); + parse_quote!(Some(#p_str)) + } + PathItem::Dynamic(span) => { + let ident = id_gen.next_id(*span); + parse_quote!(#ident) + } + }); + parse_quote!([#(#elems),*].into_iter().filter_map(|id| id).collect::>().join(".").into()) + }; let read_path = read_field(parents); match &field.default { FieldDefault::None => quote! { diff --git a/crates/common/tedge_config_macros/src/all_or_nothing.rs b/crates/common/tedge_config_macros/src/all_or_nothing.rs index ce46523cd93..54dddecc4b4 100644 --- a/crates/common/tedge_config_macros/src/all_or_nothing.rs +++ b/crates/common/tedge_config_macros/src/all_or_nothing.rs @@ -1,3 +1,5 @@ +use std::borrow::Cow; + use crate::OptionalConfig; /// An abstraction over "all or nothing" configurations @@ -12,8 +14,8 @@ pub trait MultiOption { /// The keys which were and weren't provided as part of an all or nothing group pub struct PartialConfiguration { - present: Vec<&'static str>, - missing: Vec<&'static str>, + present: Vec>, + missing: Vec>, } impl PartialConfiguration { @@ -139,11 +141,11 @@ mod tests { all_or_nothing(( OptionalConfig::Present { value: "first", - key: "test.key" + key: "test.key".into() }, OptionalConfig::Present { value: "second", - key: "test.key2" + key: "test.key2".into() } )), Ok(Some(("first", "second"))) @@ -154,8 +156,8 @@ mod tests { fn all_or_nothing_returns_none_when_both_values_when_neither_value_is_configured() { assert_eq!( all_or_nothing(( - OptionalConfig::::Empty("first.key"), - OptionalConfig::::Empty("second.key") + OptionalConfig::::Empty("first.key".into()), + OptionalConfig::::Empty("second.key".into()) )), Ok(None) ) @@ -166,9 +168,9 @@ mod tests { assert!(all_or_nothing(( OptionalConfig::Present { value: "test", - key: "first.key" + key: "first.key".into() }, - OptionalConfig::::Empty("second.key") + OptionalConfig::::Empty("second.key".into()) )) .is_err()) } @@ -176,10 +178,10 @@ mod tests { #[test] fn all_or_nothing_returns_an_error_if_only_the_second_value_is_configured() { assert!(all_or_nothing(( - OptionalConfig::::Empty("first.key"), + OptionalConfig::::Empty("first.key".into()), OptionalConfig::Present { value: "test", - key: "second.key" + key: "second.key".into() }, )) .is_err()) diff --git a/crates/common/tedge_config_macros/src/multi.rs b/crates/common/tedge_config_macros/src/multi.rs index ba0750c0325..2968c1029bd 100644 --- a/crates/common/tedge_config_macros/src/multi.rs +++ b/crates/common/tedge_config_macros/src/multi.rs @@ -1,7 +1,6 @@ -#[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize, doku::Document)] +#[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] #[serde(untagged)] pub enum Multi { - // TODO The ordering of fields is important since we have a default value for T - add a test for it Multi(::std::collections::HashMap), Single(T), } @@ -11,6 +10,11 @@ impl Default for Multi { Self::Single(T::default()) } } +impl doku::Document for Multi { + fn ty() -> doku::Type { + T::ty() + } +} impl Multi { pub fn is_default(&self) -> bool { @@ -18,20 +22,17 @@ impl Multi { } } -// TODO possibly expand this with the key name -// TODO better error messages #[derive(Debug, thiserror::Error)] pub enum MultiError { #[error("You are trying to access a named field, but the fields are not named")] SingleNotMulti, #[error("You need a name for this field")] MultiNotSingle, - #[error("You need a name for this field")] + #[error("Key not found in multi-value group")] MultiKeyNotFound, } impl Multi { - // TODO rename this to something more rusty pub fn try_get(&self, key: Option<&str>) -> Result<&T, MultiError> { match (self, key) { (Self::Single(val), None) => Ok(val), @@ -69,3 +70,80 @@ impl Multi { } } } + +#[cfg(test)] +mod tests { + use super::*; + use serde::Deserialize; + use serde_json::json; + + #[derive(Deserialize, Debug, PartialEq, Eq)] + struct TEdgeConfigDto { + c8y: Multi, + } + + #[derive(Deserialize, Debug, PartialEq, Eq)] + struct C8y { + url: String, + } + + #[test] + fn multi_can_deser_unnamed_group() { + let val: TEdgeConfigDto = serde_json::from_value(json!({ + "c8y": { "url": "https://example.com" } + })) + .unwrap(); + + assert_eq!( + val.c8y, + Multi::Single(C8y { + url: "https://example.com".into() + }) + ); + } + + #[test] + fn multi_can_deser_named_group() { + let val: TEdgeConfigDto = serde_json::from_value(json!({ + "c8y": { "cloud": { "url": "https://example.com" } } + })) + .unwrap(); + + assert_eq!( + val.c8y, + Multi::Multi( + [( + "cloud".to_owned(), + C8y { + url: "https://example.com".into() + } + )] + .into(), + ) + ); + } + + #[test] + fn multi_can_retrieve_field_from_single() { + let val = Multi::Single("value"); + + assert_eq!(*val.try_get(None).unwrap(), "value"); + } + + #[test] + fn multi_can_retrieve_field_from_multi() { + let val = Multi::Multi([("key".to_owned(), "value")].into()); + + assert_eq!(*val.try_get(Some("key")).unwrap(), "value"); + } + + #[test] + fn multi_gives_appropriate_error_retrieving_keyed_field_from_single() { + let val = Multi::Single("value"); + + assert_eq!( + val.try_get(Some("unknown")).unwrap_err().to_string(), + "You are trying to access a named field, but the fields are not named" + ); + } +} diff --git a/crates/common/tedge_config_macros/src/option.rs b/crates/common/tedge_config_macros/src/option.rs index 597ccb0be63..bbe2fc5d2fc 100644 --- a/crates/common/tedge_config_macros/src/option.rs +++ b/crates/common/tedge_config_macros/src/option.rs @@ -4,9 +4,10 @@ //! values, but with the addition of metadata (such as the relevant //! configuration key) to aid in producing informative error messages. +use std::borrow::Cow; use std::ops::Deref; -#[derive(serde::Serialize, Clone, Copy, PartialEq, Eq, Debug)] +#[derive(serde::Serialize, Clone, PartialEq, Eq, Debug)] #[serde(into = "Option", bound = "T: Clone + serde::Serialize")] /// The value for an optional configuration (i.e. one without a default value) /// @@ -20,19 +21,22 @@ use std::ops::Deref; /// ``` pub enum OptionalConfig { /// Equivalent to `Some(T)` - Present { value: T, key: &'static str }, + Present { value: T, key: Cow<'static, str> }, /// Equivalent to `None`, but stores the configuration key to create a /// better error message - Empty(&'static str), + Empty(Cow<'static, str>), } impl OptionalConfig { - pub fn present(value: T, key: &'static str) -> Self { - Self::Present { value, key } + pub fn present(value: T, key: impl Into>) -> Self { + Self::Present { + value, + key: key.into(), + } } - pub fn empty(key: &'static str) -> Self { - Self::Empty(key) + pub fn empty(key: impl Into>) -> Self { + Self::Empty(key.into()) } } @@ -56,7 +60,7 @@ impl From> for Option { /// [OptionalConfig::or_config_not_set], and this will convert to a descriptive /// error message telling the user which key to set. pub struct ConfigNotSet { - key: &'static str, + key: Cow<'static, str>, } impl OptionalConfig { @@ -84,7 +88,7 @@ impl OptionalConfig { pub fn or_config_not_set(&self) -> Result<&T, ConfigNotSet> { match self { Self::Present { value, .. } => Ok(value), - Self::Empty(key) => Err(ConfigNotSet { key }), + Self::Empty(key) => Err(ConfigNotSet { key: key.clone() }), } } @@ -95,37 +99,40 @@ impl OptionalConfig { match self { Self::Present { ref value, key } => OptionalConfig::Present { value: value.deref(), - key, + key: key.clone(), }, - Self::Empty(key) => OptionalConfig::Empty(key), + Self::Empty(key) => OptionalConfig::Empty(key.clone()), } } - pub fn key(&self) -> &'static str { + pub fn key(&self) -> &Cow<'static, str> { match self { Self::Present { key, .. } => key, Self::Empty(key) => key, } } - pub fn key_if_present(&self) -> Option<&'static str> { + pub fn key_if_present(&self) -> Option> { match self { - Self::Present { key, .. } => Some(key), + Self::Present { key, .. } => Some(key.clone()), Self::Empty(..) => None, } } - pub fn key_if_empty(&self) -> Option<&'static str> { + pub fn key_if_empty(&self) -> Option> { match self { - Self::Empty(key) => Some(key), + Self::Empty(key) => Some(key.clone()), Self::Present { .. } => None, } } pub fn as_ref(&self) -> OptionalConfig<&T> { match self { - Self::Present { value, key } => OptionalConfig::Present { value, key }, - Self::Empty(key) => OptionalConfig::Empty(key), + Self::Present { value, key } => OptionalConfig::Present { + value, + key: key.clone(), + }, + Self::Empty(key) => OptionalConfig::Empty(key.clone()), } } diff --git a/crates/common/tedge_config_macros/tests/optional.rs b/crates/common/tedge_config_macros/tests/optional.rs index 090d66dc334..9f7fa997c8c 100644 --- a/crates/common/tedge_config_macros/tests/optional.rs +++ b/crates/common/tedge_config_macros/tests/optional.rs @@ -43,6 +43,6 @@ fn vacant_optional_configurations_contain_the_relevant_key() { assert_eq!( config.mqtt.external.bind.port, - OptionalConfig::Empty("mqtt.external.bind.port") + OptionalConfig::Empty("mqtt.external.bind.port".into()) ); } diff --git a/crates/core/tedge_agent/src/file_transfer_server/actor.rs b/crates/core/tedge_agent/src/file_transfer_server/actor.rs index 51a2ec3c400..9e8fe660c05 100644 --- a/crates/core/tedge_agent/src/file_transfer_server/actor.rs +++ b/crates/core/tedge_agent/src/file_transfer_server/actor.rs @@ -388,7 +388,7 @@ mod tests { key_path: OptionalConfig::present(InjectedValue(key), "http.key_path"), ca_path: root_certs .map(|c| OptionalConfig::present(InjectedValue(c), "http.ca_path")) - .unwrap_or_else(|| OptionalConfig::Empty("http.ca_path")), + .unwrap_or_else(|| OptionalConfig::empty("http.ca_path")), bind_addr: ([127, 0, 0, 1], 0).into(), }) } From c786ae2874cbfc347403d0c17fa06b186b507bdc Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Wed, 25 Sep 2024 16:38:57 +0100 Subject: [PATCH 20/32] Run formatter --- crates/common/tedge_config_macros/impl/src/query.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/common/tedge_config_macros/impl/src/query.rs b/crates/common/tedge_config_macros/impl/src/query.rs index 95b9613e33e..a25d6894ac5 100644 --- a/crates/common/tedge_config_macros/impl/src/query.rs +++ b/crates/common/tedge_config_macros/impl/src/query.rs @@ -1272,8 +1272,7 @@ mod tests { // Remove doc comments from items for item in &mut impl_block.items { if let syn::ImplItem::Fn(f) = item { - f.attrs - .retain(|f| *f.path().get_ident().unwrap() != "doc"); + f.attrs.retain(|f| *f.path().get_ident().unwrap() != "doc"); } } From 386029293e54f8a545ae17b995f55737d1aef8fc Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Thu, 26 Sep 2024 10:10:41 +0100 Subject: [PATCH 21/32] Fix doc test Signed-off-by: James Rhodes --- crates/common/tedge_config_macros/src/option.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/common/tedge_config_macros/src/option.rs b/crates/common/tedge_config_macros/src/option.rs index bbe2fc5d2fc..7c518579ab0 100644 --- a/crates/common/tedge_config_macros/src/option.rs +++ b/crates/common/tedge_config_macros/src/option.rs @@ -15,7 +15,7 @@ use std::ops::Deref; /// use tedge_config_macros::*; /// /// assert_eq!( -/// OptionalConfig::Present { value: "test", key: "device.type" }.or_none(), +/// OptionalConfig::present("test", "device.type").or_none(), /// Some(&"test"), /// ); /// ``` @@ -70,11 +70,11 @@ impl OptionalConfig { /// use tedge_config_macros::*; /// /// assert_eq!( - /// OptionalConfig::Present { value: "test", key: "device.type" }.or_none(), + /// OptionalConfig::present("test", "device.type").or_none(), /// Some(&"test"), /// ); /// - /// assert_eq!(OptionalConfig::Empty::<&str>("device.type").or_none(), None); + /// assert_eq!(OptionalConfig::Empty::<&str>("device.type".into()).or_none(), None); /// ``` pub fn or_none(&self) -> Option<&T> { match self { From fdc4751bf82995410b790746f5b3595d2860cba1 Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Thu, 26 Sep 2024 10:43:54 +0100 Subject: [PATCH 22/32] Fix imports in docs --- .../tedge_config/src/tedge_config_cli/tedge_config.rs | 5 +---- crates/common/tedge_config_macros/impl/src/dto.rs | 4 ++-- crates/common/tedge_config_macros/impl/src/query.rs | 10 +++++----- crates/common/tedge_config_macros/impl/src/reader.rs | 2 +- 4 files changed, 9 insertions(+), 12 deletions(-) diff --git a/crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs b/crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs index b0d26666609..76c013a0bec 100644 --- a/crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs +++ b/crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs @@ -37,10 +37,7 @@ use std::path::PathBuf; use std::str::FromStr; use std::sync::Arc; use std::sync::OnceLock; -use tedge_config_macros::all_or_nothing; -use tedge_config_macros::define_tedge_config; -use tedge_config_macros::struct_field_aliases; -use tedge_config_macros::struct_field_paths; +use tedge_config_macros::*; pub use tedge_config_macros::ConfigNotSet; use tedge_config_macros::OptionalConfig; use toml::Table; diff --git a/crates/common/tedge_config_macros/impl/src/dto.rs b/crates/common/tedge_config_macros/impl/src/dto.rs index 28a67231fbb..ca88ea30da0 100644 --- a/crates/common/tedge_config_macros/impl/src/dto.rs +++ b/crates/common/tedge_config_macros/impl/src/dto.rs @@ -50,13 +50,13 @@ pub fn generate( if !group.dto.skip { let sub_dto_name = prefixed_type_name(&name, group); idents.push(&group.ident); - let field_ty = parse_quote_spanned!(group.ident.span()=> ::tedge_config_macros::Multi<#sub_dto_name>); + let field_ty = parse_quote_spanned!(group.ident.span()=> Multi<#sub_dto_name>); tys.push(field_ty); sub_dtos.push(Some(generate(sub_dto_name, &group.contents, ""))); preserved_attrs.push(group.attrs.iter().filter(is_preserved).collect()); extra_attrs.push(quote! { #[serde(default)] - #[serde(skip_serializing_if = "::tedge_config_macros::Multi::is_default")] + #[serde(skip_serializing_if = "Multi::is_default")] }); } } diff --git a/crates/common/tedge_config_macros/impl/src/query.rs b/crates/common/tedge_config_macros/impl/src/query.rs index a25d6894ac5..b404bb7fee7 100644 --- a/crates/common/tedge_config_macros/impl/src/query.rs +++ b/crates/common/tedge_config_macros/impl/src/query.rs @@ -119,7 +119,7 @@ pub fn generate_writable_keys(items: &[FieldOrGroup]) -> TokenStream { #[error("Failed to parse input")] ParseValue(#[from] Box), #[error(transparent)] - Multi(#[from] ::tedge_config_macros::MultiError), + Multi(#[from] MultiError), } impl ReadOnlyKey { @@ -331,7 +331,7 @@ fn key_iterators( let sub_type_name = syn::Ident::new(&format!("{reader_ty}{upper_ident}"), m.ident.span()); let keys_ident = syn::Ident::new(&format!("{}_keys", ident), ident.span()); - stmts.push(parse_quote!(let #keys_ident = if let ::tedge_config_macros::Multi::Multi(map) = &self.#ident { + stmts.push(parse_quote!(let #keys_ident = if let Multi::Multi(map) = &self.#ident { map.keys().map(|k| Some(k.to_owned())).collect() } else { vec![None] @@ -989,7 +989,7 @@ mod tests { let expected = parse_quote! { impl TEdgeConfigReader { pub fn readable_keys(&self) -> impl Iterator + '_ { - let c8y_keys = if let ::tedge_config_macros::Multi::Multi(map) = &self.c8y { + let c8y_keys = if let Multi::Multi(map) = &self.c8y { map.keys().map(|k| Some(k.to_owned())).collect() } else { vec![None] @@ -1005,7 +1005,7 @@ mod tests { impl TEdgeConfigReaderC8y { pub fn readable_keys(&self, c8y: Option) -> impl Iterator + '_ { - let something_keys = if let ::tedge_config_macros::Multi::Multi(map) = &self.something { + let something_keys = if let Multi::Multi(map) = &self.something { map.keys().map(|k| Some(k.to_owned())).collect() } else { vec![None] @@ -1126,7 +1126,7 @@ mod tests { let expected = parse_quote! { impl TEdgeConfigReader { pub fn readable_keys(&self) -> impl Iterator + '_ { - let c8y_keys = if let ::tedge_config_macros::Multi::Multi(map) = &self.c8y { + let c8y_keys = if let Multi::Multi(map) = &self.c8y { map.keys().map(|k| Some(k.to_owned())).collect() } else { vec![None] diff --git a/crates/common/tedge_config_macros/impl/src/reader.rs b/crates/common/tedge_config_macros/impl/src/reader.rs index 028a9f3b02f..918bc96a25d 100644 --- a/crates/common/tedge_config_macros/impl/src/reader.rs +++ b/crates/common/tedge_config_macros/impl/src/reader.rs @@ -75,7 +75,7 @@ fn generate_structs( FieldOrGroup::Multi(group) if !group.reader.skip => { let sub_reader_name = prefixed_type_name(name, group); idents.push(&group.ident); - tys.push(parse_quote_spanned!(group.ident.span()=> ::tedge_config_macros::Multi<#sub_reader_name>)); + tys.push(parse_quote_spanned!(group.ident.span()=> Multi<#sub_reader_name>)); let mut parents = parents.clone(); parents.push(PathItem::Static(group.ident.clone())); parents.push(PathItem::Dynamic(group.ident.span())); From d508c9079f4c5396f269a4e8ae8de94de588a73c Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Thu, 26 Sep 2024 11:47:34 +0100 Subject: [PATCH 23/32] Run formatter Signed-off-by: James Rhodes --- .../tedge_config/src/tedge_config_cli/tedge_config.rs | 2 +- crates/common/tedge_config_macros/impl/src/query.rs | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs b/crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs index 76c013a0bec..9b435eae773 100644 --- a/crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs +++ b/crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs @@ -37,9 +37,9 @@ use std::path::PathBuf; use std::str::FromStr; use std::sync::Arc; use std::sync::OnceLock; -use tedge_config_macros::*; pub use tedge_config_macros::ConfigNotSet; use tedge_config_macros::OptionalConfig; +use tedge_config_macros::*; use toml::Table; use tracing::error; diff --git a/crates/common/tedge_config_macros/impl/src/query.rs b/crates/common/tedge_config_macros/impl/src/query.rs index b404bb7fee7..2f4981a9e30 100644 --- a/crates/common/tedge_config_macros/impl/src/query.rs +++ b/crates/common/tedge_config_macros/impl/src/query.rs @@ -331,11 +331,13 @@ fn key_iterators( let sub_type_name = syn::Ident::new(&format!("{reader_ty}{upper_ident}"), m.ident.span()); let keys_ident = syn::Ident::new(&format!("{}_keys", ident), ident.span()); - stmts.push(parse_quote!(let #keys_ident = if let Multi::Multi(map) = &self.#ident { + stmts.push( + parse_quote!(let #keys_ident = if let Multi::Multi(map) = &self.#ident { map.keys().map(|k| Some(k.to_owned())).collect() } else { vec![None] - };)); + };), + ); let prefix = format!("{prefix}{upper_ident}"); let remaining_fields = fields.iter().map(|fs| &fs[1..]).collect::>(); let arg_clone_stmts = args From edc2d6154b6768f3ad26a1d810745b37c5abe41b Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Mon, 30 Sep 2024 13:19:03 +0100 Subject: [PATCH 24/32] Rename map -> map_keys --- crates/common/tedge_config_macros/impl/src/reader.rs | 4 ++-- crates/common/tedge_config_macros/src/multi.rs | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/crates/common/tedge_config_macros/impl/src/reader.rs b/crates/common/tedge_config_macros/impl/src/reader.rs index 918bc96a25d..21c09800af4 100644 --- a/crates/common/tedge_config_macros/impl/src/reader.rs +++ b/crates/common/tedge_config_macros/impl/src/reader.rs @@ -450,8 +450,8 @@ fn generate_conversions( let mut parents = parents.clone(); parents.push(PathItem::Static(group.ident.clone())); let read_path = read_field(&parents); - let new_arg2 = id_gen.replay(group.ident.span()); - field_conversions.push(quote!(#name: dto.#(#read_path).*.map(|#new_arg2| #sub_reader_name::from_dto(dto, location, #(#extra_call_args),*)))); + let new_arg2 = extra_call_args.last().unwrap().clone(); + field_conversions.push(quote!(#name: dto.#(#read_path).*.map_keys(|#new_arg2| #sub_reader_name::from_dto(dto, location, #(#extra_call_args),*)))); parents.push(new_arg); let sub_conversions = generate_conversions(&sub_reader_name, &group.contents, parents, root_fields)?; diff --git a/crates/common/tedge_config_macros/src/multi.rs b/crates/common/tedge_config_macros/src/multi.rs index 2968c1029bd..fb4ca4d4b59 100644 --- a/crates/common/tedge_config_macros/src/multi.rs +++ b/crates/common/tedge_config_macros/src/multi.rs @@ -58,8 +58,7 @@ impl Multi { } } - // TODO clearer name - pub fn map(&self, f: impl Fn(Option<&str>) -> U) -> Multi { + pub fn map_keys(&self, f: impl Fn(Option<&str>) -> U) -> Multi { match self { Self::Single(_) => Multi::Single(f(None)), Self::Multi(map) => Multi::Multi( From acdd944faaba64ad7eaaff12bad7c80309db82ee Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Tue, 1 Oct 2024 16:40:04 +0100 Subject: [PATCH 25/32] Improve error messages for multi fields --- .../tedge_config_macros/examples/multi.rs | 46 ++++--- .../tedge_config_macros/impl/src/dto.rs | 5 +- .../tedge_config_macros/impl/src/namegen.rs | 7 - .../tedge_config_macros/impl/src/query.rs | 32 +++-- .../tedge_config_macros/impl/src/reader.rs | 21 ++- .../common/tedge_config_macros/src/multi.rs | 120 +++++++++++++----- 6 files changed, 161 insertions(+), 70 deletions(-) diff --git a/crates/common/tedge_config_macros/examples/multi.rs b/crates/common/tedge_config_macros/examples/multi.rs index 50833d31bfa..464284ad323 100644 --- a/crates/common/tedge_config_macros/examples/multi.rs +++ b/crates/common/tedge_config_macros/examples/multi.rs @@ -70,18 +70,18 @@ fn main() { "https://edge.example.com" ); - assert!(matches!( - single_c8y_reader.c8y.try_get(Some("cloud")), - Err(MultiError::SingleNotMulti) - )); - assert!(matches!( - multi_c8y_reader.c8y.try_get(Some("unknown")), - Err(MultiError::MultiKeyNotFound) - )); - assert!(matches!( - multi_c8y_reader.c8y.try_get(None), - Err(MultiError::MultiNotSingle) - )); + assert_eq!( + single_c8y_reader.c8y.try_get(Some("cloud")).unwrap_err().to_string(), + "You are trying to access a named field (cloud) of c8y, but the fields are not named" + ); + assert_eq!( + multi_c8y_reader.c8y.try_get(Some("unknown")).unwrap_err().to_string(), + "Key c8y.unknown not found in multi-value group" + ); + assert_eq!( + multi_c8y_reader.c8y.try_get(None).unwrap_err().to_string(), + "You need a name for the field c8y" + ); assert_eq!( "c8y.url".parse::().unwrap(), @@ -112,17 +112,27 @@ fn main() { .to_string(), "Unknown key: 'c8y.cloud.not_a_real_key'" ); + assert_eq!( + "c8y.urll" + .parse::() + .unwrap_err() + .to_string(), + "Unknown key: 'c8y.urll'" + ); + let mut keys = multi_c8y_reader + .readable_keys() + .map(|r| r.to_string()) + .collect::>(); + // We need to sort the keys as the map iteration doesn't produce a consistent ordering + keys.sort(); assert_eq!( - multi_c8y_reader - .readable_keys() - .map(|r| r.to_string()) - .collect::>(), + keys, [ - "c8y.cloud.url", "c8y.cloud.something.test", - "c8y.edge.url", + "c8y.cloud.url", "c8y.edge.something.test", + "c8y.edge.url", ] ); } diff --git a/crates/common/tedge_config_macros/impl/src/dto.rs b/crates/common/tedge_config_macros/impl/src/dto.rs index ca88ea30da0..4fbe7e661f3 100644 --- a/crates/common/tedge_config_macros/impl/src/dto.rs +++ b/crates/common/tedge_config_macros/impl/src/dto.rs @@ -50,13 +50,14 @@ pub fn generate( if !group.dto.skip { let sub_dto_name = prefixed_type_name(&name, group); idents.push(&group.ident); - let field_ty = parse_quote_spanned!(group.ident.span()=> Multi<#sub_dto_name>); + let field_ty = + parse_quote_spanned!(group.ident.span()=> MultiDto<#sub_dto_name>); tys.push(field_ty); sub_dtos.push(Some(generate(sub_dto_name, &group.contents, ""))); preserved_attrs.push(group.attrs.iter().filter(is_preserved).collect()); extra_attrs.push(quote! { #[serde(default)] - #[serde(skip_serializing_if = "Multi::is_default")] + #[serde(skip_serializing_if = "MultiDto::is_default")] }); } } diff --git a/crates/common/tedge_config_macros/impl/src/namegen.rs b/crates/common/tedge_config_macros/impl/src/namegen.rs index d2212e51de1..8b8c763638d 100644 --- a/crates/common/tedge_config_macros/impl/src/namegen.rs +++ b/crates/common/tedge_config_macros/impl/src/namegen.rs @@ -58,13 +58,6 @@ pub struct SequentialIdGenerator { pub count: u32, } -impl SequentialIdGenerator { - pub fn replay(&self, span: Span) -> syn::Ident { - let i = self.count - 1; - syn::Ident::new(&format!("key{i}"), span) - } -} - #[derive(Debug, Default)] pub struct UnderscoreIdGenerator; diff --git a/crates/common/tedge_config_macros/impl/src/query.rs b/crates/common/tedge_config_macros/impl/src/query.rs index 2f4981a9e30..5355a2285e5 100644 --- a/crates/common/tedge_config_macros/impl/src/query.rs +++ b/crates/common/tedge_config_macros/impl/src/query.rs @@ -332,7 +332,7 @@ fn key_iterators( syn::Ident::new(&format!("{reader_ty}{upper_ident}"), m.ident.span()); let keys_ident = syn::Ident::new(&format!("{}_keys", ident), ident.span()); stmts.push( - parse_quote!(let #keys_ident = if let Multi::Multi(map) = &self.#ident { + parse_quote!(let #keys_ident = if let MultiReader::Multi(map, _) = &self.#ident { map.keys().map(|k| Some(k.to_owned())).collect() } else { vec![None] @@ -526,18 +526,32 @@ fn keys_enum( fn generate_field_accessor<'a>( fields: &'a VecDeque<&FieldOrGroup>, method: &'a str, + // TODO use an enum or something + exclude_parents: bool, ) -> impl Iterator + 'a { let mut id_gen = SequentialIdGenerator::default(); let method = syn::Ident::new(method, Span::call_site()); + let mut fields_so_far = Vec::new(); fields.iter().map(move |field| { let ident = field.ident(); + fields_so_far.push(ident); match field { FieldOrGroup::Field(_) => quote!(#ident), FieldOrGroup::Group(_) => quote!(#ident), - FieldOrGroup::Multi(_) => { + FieldOrGroup::Multi(_) if exclude_parents => { let field = id_gen.next_id(ident.span()); quote_spanned!(ident.span()=> #ident.#method(#field.as_deref())?) } + FieldOrGroup::Multi(_) => { + let field = id_gen.next_id(ident.span()); + #[allow(unstable_name_collisions)] + let parents = fields_so_far + .iter() + .map(|id| id.to_string()) + .intersperse(".".to_owned()) + .collect::(); + quote_spanned!(ident.span()=> #ident.#method(#field.as_deref(), #parents)?) + } } }) } @@ -553,7 +567,7 @@ fn generate_string_readers(paths: &[VecDeque<&FieldOrGroup>]) -> TokenStream { .expect("Path must have a back as it is nonempty") .field() .expect("Back of path is guaranteed to be a field"); - let segments = generate_field_accessor(path, "try_get"); + let segments = generate_field_accessor(path, "try_get", true); let to_string = quote_spanned!(field.ty().span()=> .to_string()); let match_variant = configuration_key.match_read_write; if field.read_only().is_some() { @@ -602,8 +616,8 @@ fn generate_string_writers(paths: &[VecDeque<&FieldOrGroup>]) -> TokenStream { .iter() .zip(enum_variants) .map(|(path, configuration_key)| { - let read_segments = generate_field_accessor(path, "try_get"); - let write_segments = generate_field_accessor(path, "try_get_mut").collect::>(); + let read_segments = generate_field_accessor(path, "try_get", true); + let write_segments = generate_field_accessor(path, "try_get_mut", false).collect::>(); let field = path .iter() .filter_map(|thing| thing.field()) @@ -776,6 +790,7 @@ fn enum_variant(segments: &VecDeque<&FieldOrGroup>) -> ConfigurationKey { }) .collect::>() .join("\\."); + let re = format!("^{re}$"); let regex_parser = parse_quote_spanned!(ident.span()=> if let Some(captures) = ::regex::Regex::new(#re).unwrap().captures(value) {}); let formatters = field_names .iter() @@ -991,7 +1006,7 @@ mod tests { let expected = parse_quote! { impl TEdgeConfigReader { pub fn readable_keys(&self) -> impl Iterator + '_ { - let c8y_keys = if let Multi::Multi(map) = &self.c8y { + let c8y_keys = if let MultiReader::Multi(map, _) = &self.c8y { map.keys().map(|k| Some(k.to_owned())).collect() } else { vec![None] @@ -1007,7 +1022,7 @@ mod tests { impl TEdgeConfigReaderC8y { pub fn readable_keys(&self, c8y: Option) -> impl Iterator + '_ { - let something_keys = if let Multi::Multi(map) = &self.something { + let something_keys = if let MultiReader::Multi(map, _) = &self.something { map.keys().map(|k| Some(k.to_owned())).collect() } else { vec![None] @@ -1128,7 +1143,7 @@ mod tests { let expected = parse_quote! { impl TEdgeConfigReader { pub fn readable_keys(&self) -> impl Iterator + '_ { - let c8y_keys = if let Multi::Multi(map) = &self.c8y { + let c8y_keys = if let MultiReader::Multi(map, _) = &self.c8y { map.keys().map(|k| Some(k.to_owned())).collect() } else { vec![None] @@ -1166,6 +1181,7 @@ mod tests { prettyplease::unparse(&expected) ); } + #[test] fn impl_for_simple_group() { let input: crate::input::Configuration = parse_quote!( diff --git a/crates/common/tedge_config_macros/impl/src/reader.rs b/crates/common/tedge_config_macros/impl/src/reader.rs index 21c09800af4..76c2dc6b24f 100644 --- a/crates/common/tedge_config_macros/impl/src/reader.rs +++ b/crates/common/tedge_config_macros/impl/src/reader.rs @@ -14,6 +14,7 @@ use syn::parse_quote; use syn::parse_quote_spanned; use syn::punctuated::Punctuated; use syn::spanned::Spanned; +use syn::Path; use syn::Token; use crate::error::extract_type_from_result; @@ -75,7 +76,7 @@ fn generate_structs( FieldOrGroup::Multi(group) if !group.reader.skip => { let sub_reader_name = prefixed_type_name(name, group); idents.push(&group.ident); - tys.push(parse_quote_spanned!(group.ident.span()=> Multi<#sub_reader_name>)); + tys.push(parse_quote_spanned!(group.ident.span()=> MultiReader<#sub_reader_name>)); let mut parents = parents.clone(); parents.push(PathItem::Static(group.ident.clone())); parents.push(PathItem::Dynamic(group.ident.span())); @@ -251,11 +252,15 @@ impl PathItem { fn read_field(parents: &[PathItem]) -> impl Iterator + '_ { let mut id_gen = SequentialIdGenerator::default(); + let mut parent_key = String::new(); parents.iter().map(move |parent| match parent { - PathItem::Static(name) => quote!(#name), + PathItem::Static(name) => { + parent_key += &name.to_string(); + quote!(#name) + } PathItem::Dynamic(span) => { let id = id_gen.next_id(*span); - quote_spanned!(*span=> try_get(#id).unwrap()) + quote_spanned!(*span=> try_get(#id, #parent_key).unwrap()) } }) } @@ -450,8 +455,16 @@ fn generate_conversions( let mut parents = parents.clone(); parents.push(PathItem::Static(group.ident.clone())); let read_path = read_field(&parents); + let parent_key = parents + .iter() + .filter_map(|p| match p { + PathItem::Static(s) => Some(s.to_string()), + _ => None, + }) + .intersperse(".".to_owned()) + .collect::(); let new_arg2 = extra_call_args.last().unwrap().clone(); - field_conversions.push(quote!(#name: dto.#(#read_path).*.map_keys(|#new_arg2| #sub_reader_name::from_dto(dto, location, #(#extra_call_args),*)))); + field_conversions.push(quote!(#name: dto.#(#read_path).*.map_keys(|#new_arg2| #sub_reader_name::from_dto(dto, location, #(#extra_call_args),*), #parent_key))); parents.push(new_arg); let sub_conversions = generate_conversions(&sub_reader_name, &group.contents, parents, root_fields)?; diff --git a/crates/common/tedge_config_macros/src/multi.rs b/crates/common/tedge_config_macros/src/multi.rs index fb4ca4d4b59..e93b0eb3bb2 100644 --- a/crates/common/tedge_config_macros/src/multi.rs +++ b/crates/common/tedge_config_macros/src/multi.rs @@ -1,22 +1,36 @@ #[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] #[serde(untagged)] -pub enum Multi { +pub enum MultiDto { Multi(::std::collections::HashMap), Single(T), } -impl Default for Multi { +#[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] +#[serde(untagged)] +pub enum MultiReader { + Multi(::std::collections::HashMap, &'static str), + Single(T, &'static str), +} + +impl Default for MultiDto { fn default() -> Self { Self::Single(T::default()) } } -impl doku::Document for Multi { + +impl doku::Document for MultiDto { fn ty() -> doku::Type { T::ty() } } -impl Multi { +impl doku::Document for MultiReader { + fn ty() -> doku::Type { + T::ty() + } +} + +impl MultiDto { pub fn is_default(&self) -> bool { *self == Self::default() } @@ -24,30 +38,34 @@ impl Multi { #[derive(Debug, thiserror::Error)] pub enum MultiError { - #[error("You are trying to access a named field, but the fields are not named")] - SingleNotMulti, - #[error("You need a name for this field")] - MultiNotSingle, - #[error("Key not found in multi-value group")] - MultiKeyNotFound, + #[error("You are trying to access a named field ({1}) of {0}, but the fields are not named")] + SingleNotMulti(String, String), + #[error("You need a name for the field {0}")] + MultiNotSingle(String), + #[error("Key {0}.{1} not found in multi-value group")] + MultiKeyNotFound(String, String), } -impl Multi { - pub fn try_get(&self, key: Option<&str>) -> Result<&T, MultiError> { +impl MultiDto { + pub fn try_get(&self, key: Option<&str>, parent: &str) -> Result<&T, MultiError> { match (self, key) { (Self::Single(val), None) => Ok(val), - (Self::Multi(map), Some(key)) => map.get(key).ok_or(MultiError::MultiKeyNotFound), - (Self::Multi(_), None) => Err(MultiError::MultiNotSingle), - (Self::Single(_), Some(_key)) => Err(MultiError::SingleNotMulti), + (Self::Multi(map), Some(key)) => map + .get(key) + .ok_or_else(|| MultiError::MultiKeyNotFound(parent.to_owned(), key.to_owned())), + (Self::Multi(_), None) => Err(MultiError::MultiNotSingle(parent.to_owned())), + (Self::Single(_), Some(key)) => Err(MultiError::SingleNotMulti(parent.into(), key.into())), } } - pub fn try_get_mut(&mut self, key: Option<&str>) -> Result<&mut T, MultiError> { + pub fn try_get_mut(&mut self, key: Option<&str>, parent: &str) -> Result<&mut T, MultiError> { match (self, key) { (Self::Single(val), None) => Ok(val), - (Self::Multi(map), Some(key)) => map.get_mut(key).ok_or(MultiError::MultiKeyNotFound), - (Self::Multi(_), None) => Err(MultiError::MultiNotSingle), - (Self::Single(_), Some(_key)) => Err(MultiError::SingleNotMulti), + (Self::Multi(map), Some(key)) => map + .get_mut(key) + .ok_or_else(|| MultiError::MultiKeyNotFound(parent.to_owned(), key.to_owned())), + (Self::Multi(_), None) => Err(MultiError::MultiNotSingle(parent.to_owned())), + (Self::Single(_), Some(key)) => Err(MultiError::SingleNotMulti(parent.into(), key.into())), } } @@ -57,14 +75,54 @@ impl Multi { Self::Multi(map) => itertools::Either::Right(map.keys().map(String::as_str).map(Some)), } } +} + +impl MultiReader { + pub fn try_get(&self, key: Option<&str>) -> Result<&T, MultiError> { + match (self, key) { + (Self::Single(val, _parent), None) => Ok(val), + (Self::Multi(map, parent), Some(key)) => { + map.get(key).ok_or_else(|| MultiError::MultiKeyNotFound((*parent).into(), key.into())) + } + (Self::Multi(_, parent), None) => Err(MultiError::MultiNotSingle((*parent).into())), + (Self::Single(_, parent), Some(key)) => Err(MultiError::SingleNotMulti((*parent).into(), key.into())), + } + } + + pub fn try_get_mut(&mut self, key: Option<&str>) -> Result<&mut T, MultiError> { + match (self, key) { + (Self::Single(val, _parent), None) => Ok(val), + (Self::Multi(map, parent), Some(key)) => { + map.get_mut(key).ok_or_else(|| MultiError::MultiKeyNotFound((*parent).into(), key.into())) + } + (Self::Multi(_, parent), None) => Err(MultiError::MultiNotSingle((*parent).into())), + (Self::Single(_, parent), Some(key)) => Err(MultiError::SingleNotMulti((*parent).into(), key.into())), + } + } + + pub fn keys(&self) -> impl Iterator> { + match self { + Self::Single(_, _) => itertools::Either::Left(std::iter::once(None)), + Self::Multi(map, _) => { + itertools::Either::Right(map.keys().map(String::as_str).map(Some)) + } + } + } +} - pub fn map_keys(&self, f: impl Fn(Option<&str>) -> U) -> Multi { +impl MultiDto { + pub fn map_keys( + &self, + f: impl Fn(Option<&str>) -> U, + parent: &'static str, + ) -> MultiReader { match self { - Self::Single(_) => Multi::Single(f(None)), - Self::Multi(map) => Multi::Multi( + Self::Single(_) => MultiReader::Single(f(None), parent), + Self::Multi(map) => MultiReader::Multi( map.keys() .map(|key| (key.to_owned(), f(Some(key)))) .collect(), + parent, ), } } @@ -78,7 +136,7 @@ mod tests { #[derive(Deserialize, Debug, PartialEq, Eq)] struct TEdgeConfigDto { - c8y: Multi, + c8y: MultiDto, } #[derive(Deserialize, Debug, PartialEq, Eq)] @@ -95,7 +153,7 @@ mod tests { assert_eq!( val.c8y, - Multi::Single(C8y { + MultiDto::Single(C8y { url: "https://example.com".into() }) ); @@ -110,7 +168,7 @@ mod tests { assert_eq!( val.c8y, - Multi::Multi( + MultiDto::Multi( [( "cloud".to_owned(), C8y { @@ -124,24 +182,24 @@ mod tests { #[test] fn multi_can_retrieve_field_from_single() { - let val = Multi::Single("value"); + let val = MultiDto::Single("value"); - assert_eq!(*val.try_get(None).unwrap(), "value"); + assert_eq!(*val.try_get(None, "c8y").unwrap(), "value"); } #[test] fn multi_can_retrieve_field_from_multi() { - let val = Multi::Multi([("key".to_owned(), "value")].into()); + let val = MultiDto::Multi([("key".to_owned(), "value")].into()); - assert_eq!(*val.try_get(Some("key")).unwrap(), "value"); + assert_eq!(*val.try_get(Some("key"), "c8y").unwrap(), "value"); } #[test] fn multi_gives_appropriate_error_retrieving_keyed_field_from_single() { - let val = Multi::Single("value"); + let val = MultiDto::Single("value"); assert_eq!( - val.try_get(Some("unknown")).unwrap_err().to_string(), + val.try_get(Some("unknown"), "c8y").unwrap_err().to_string(), "You are trying to access a named field, but the fields are not named" ); } From 582f1f65c5fbdb956ac08d1d8e7730e66978e006 Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Tue, 1 Oct 2024 17:03:45 +0100 Subject: [PATCH 26/32] Use named fields inside the multi --- .../tedge_config_macros/examples/multi.rs | 17 ++++-- .../tedge_config_macros/impl/src/query.rs | 10 ++-- .../tedge_config_macros/impl/src/reader.rs | 2 +- .../common/tedge_config_macros/src/multi.rs | 58 ++++++++++++------- 4 files changed, 55 insertions(+), 32 deletions(-) diff --git a/crates/common/tedge_config_macros/examples/multi.rs b/crates/common/tedge_config_macros/examples/multi.rs index 464284ad323..2b65bc47141 100644 --- a/crates/common/tedge_config_macros/examples/multi.rs +++ b/crates/common/tedge_config_macros/examples/multi.rs @@ -71,11 +71,19 @@ fn main() { ); assert_eq!( - single_c8y_reader.c8y.try_get(Some("cloud")).unwrap_err().to_string(), + single_c8y_reader + .c8y + .try_get(Some("cloud")) + .unwrap_err() + .to_string(), "You are trying to access a named field (cloud) of c8y, but the fields are not named" ); assert_eq!( - multi_c8y_reader.c8y.try_get(Some("unknown")).unwrap_err().to_string(), + multi_c8y_reader + .c8y + .try_get(Some("unknown")) + .unwrap_err() + .to_string(), "Key c8y.unknown not found in multi-value group" ); assert_eq!( @@ -113,10 +121,7 @@ fn main() { "Unknown key: 'c8y.cloud.not_a_real_key'" ); assert_eq!( - "c8y.urll" - .parse::() - .unwrap_err() - .to_string(), + "c8y.urll".parse::().unwrap_err().to_string(), "Unknown key: 'c8y.urll'" ); diff --git a/crates/common/tedge_config_macros/impl/src/query.rs b/crates/common/tedge_config_macros/impl/src/query.rs index 5355a2285e5..a5bfb1e0238 100644 --- a/crates/common/tedge_config_macros/impl/src/query.rs +++ b/crates/common/tedge_config_macros/impl/src/query.rs @@ -332,7 +332,7 @@ fn key_iterators( syn::Ident::new(&format!("{reader_ty}{upper_ident}"), m.ident.span()); let keys_ident = syn::Ident::new(&format!("{}_keys", ident), ident.span()); stmts.push( - parse_quote!(let #keys_ident = if let MultiReader::Multi(map, _) = &self.#ident { + parse_quote!(let #keys_ident = if let MultiReader::Multi { map, .. } = &self.#ident { map.keys().map(|k| Some(k.to_owned())).collect() } else { vec![None] @@ -968,7 +968,7 @@ mod tests { }, _ => unimplemented!("just a test, no error handling"), }; - if let Some(captures) = ::regex::Regex::new("c8y(?:\\.([A-z_]+))?\\.url").unwrap().captures(value) { + if let Some(captures) = ::regex::Regex::new("^c8y(?:\\.([A-z_]+))?\\.url$").unwrap().captures(value) { let key0 = captures.get(1usize).map(|re_match| re_match.as_str().to_owned()); return Ok(Self::C8yUrl(key0)); }; @@ -1006,7 +1006,7 @@ mod tests { let expected = parse_quote! { impl TEdgeConfigReader { pub fn readable_keys(&self) -> impl Iterator + '_ { - let c8y_keys = if let MultiReader::Multi(map, _) = &self.c8y { + let c8y_keys = if let MultiReader::Multi { map, .. } = &self.c8y { map.keys().map(|k| Some(k.to_owned())).collect() } else { vec![None] @@ -1022,7 +1022,7 @@ mod tests { impl TEdgeConfigReaderC8y { pub fn readable_keys(&self, c8y: Option) -> impl Iterator + '_ { - let something_keys = if let MultiReader::Multi(map, _) = &self.something { + let something_keys = if let MultiReader::Multi { map, .. } = &self.something { map.keys().map(|k| Some(k.to_owned())).collect() } else { vec![None] @@ -1143,7 +1143,7 @@ mod tests { let expected = parse_quote! { impl TEdgeConfigReader { pub fn readable_keys(&self) -> impl Iterator + '_ { - let c8y_keys = if let MultiReader::Multi(map, _) = &self.c8y { + let c8y_keys = if let MultiReader::Multi { map, .. } = &self.c8y { map.keys().map(|k| Some(k.to_owned())).collect() } else { vec![None] diff --git a/crates/common/tedge_config_macros/impl/src/reader.rs b/crates/common/tedge_config_macros/impl/src/reader.rs index 76c2dc6b24f..7ad3a01883c 100644 --- a/crates/common/tedge_config_macros/impl/src/reader.rs +++ b/crates/common/tedge_config_macros/impl/src/reader.rs @@ -14,7 +14,6 @@ use syn::parse_quote; use syn::parse_quote_spanned; use syn::punctuated::Punctuated; use syn::spanned::Spanned; -use syn::Path; use syn::Token; use crate::error::extract_type_from_result; @@ -455,6 +454,7 @@ fn generate_conversions( let mut parents = parents.clone(); parents.push(PathItem::Static(group.ident.clone())); let read_path = read_field(&parents); + #[allow(unstable_name_collisions)] let parent_key = parents .iter() .filter_map(|p| match p { diff --git a/crates/common/tedge_config_macros/src/multi.rs b/crates/common/tedge_config_macros/src/multi.rs index e93b0eb3bb2..5111fe57497 100644 --- a/crates/common/tedge_config_macros/src/multi.rs +++ b/crates/common/tedge_config_macros/src/multi.rs @@ -8,8 +8,14 @@ pub enum MultiDto { #[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] #[serde(untagged)] pub enum MultiReader { - Multi(::std::collections::HashMap, &'static str), - Single(T, &'static str), + Multi { + map: ::std::collections::HashMap, + parent: &'static str, + }, + Single { + value: T, + parent: &'static str, + }, } impl Default for MultiDto { @@ -54,7 +60,9 @@ impl MultiDto { .get(key) .ok_or_else(|| MultiError::MultiKeyNotFound(parent.to_owned(), key.to_owned())), (Self::Multi(_), None) => Err(MultiError::MultiNotSingle(parent.to_owned())), - (Self::Single(_), Some(key)) => Err(MultiError::SingleNotMulti(parent.into(), key.into())), + (Self::Single(_), Some(key)) => { + Err(MultiError::SingleNotMulti(parent.into(), key.into())) + } } } @@ -65,7 +73,9 @@ impl MultiDto { .get_mut(key) .ok_or_else(|| MultiError::MultiKeyNotFound(parent.to_owned(), key.to_owned())), (Self::Multi(_), None) => Err(MultiError::MultiNotSingle(parent.to_owned())), - (Self::Single(_), Some(key)) => Err(MultiError::SingleNotMulti(parent.into(), key.into())), + (Self::Single(_), Some(key)) => { + Err(MultiError::SingleNotMulti(parent.into(), key.into())) + } } } @@ -80,30 +90,34 @@ impl MultiDto { impl MultiReader { pub fn try_get(&self, key: Option<&str>) -> Result<&T, MultiError> { match (self, key) { - (Self::Single(val, _parent), None) => Ok(val), - (Self::Multi(map, parent), Some(key)) => { - map.get(key).ok_or_else(|| MultiError::MultiKeyNotFound((*parent).into(), key.into())) + (Self::Single { value, .. }, None) => Ok(value), + (Self::Multi { map, parent }, Some(key)) => map + .get(key) + .ok_or_else(|| MultiError::MultiKeyNotFound((*parent).into(), key.into())), + (Self::Multi { parent, .. }, None) => Err(MultiError::MultiNotSingle((*parent).into())), + (Self::Single { parent, .. }, Some(key)) => { + Err(MultiError::SingleNotMulti((*parent).into(), key.into())) } - (Self::Multi(_, parent), None) => Err(MultiError::MultiNotSingle((*parent).into())), - (Self::Single(_, parent), Some(key)) => Err(MultiError::SingleNotMulti((*parent).into(), key.into())), } } pub fn try_get_mut(&mut self, key: Option<&str>) -> Result<&mut T, MultiError> { match (self, key) { - (Self::Single(val, _parent), None) => Ok(val), - (Self::Multi(map, parent), Some(key)) => { - map.get_mut(key).ok_or_else(|| MultiError::MultiKeyNotFound((*parent).into(), key.into())) + (Self::Single { value, .. }, None) => Ok(value), + (Self::Multi { map, parent }, Some(key)) => map + .get_mut(key) + .ok_or_else(|| MultiError::MultiKeyNotFound((*parent).into(), key.into())), + (Self::Multi { parent, .. }, None) => Err(MultiError::MultiNotSingle((*parent).into())), + (Self::Single { parent, .. }, Some(key)) => { + Err(MultiError::SingleNotMulti((*parent).into(), key.into())) } - (Self::Multi(_, parent), None) => Err(MultiError::MultiNotSingle((*parent).into())), - (Self::Single(_, parent), Some(key)) => Err(MultiError::SingleNotMulti((*parent).into(), key.into())), } } pub fn keys(&self) -> impl Iterator> { match self { - Self::Single(_, _) => itertools::Either::Left(std::iter::once(None)), - Self::Multi(map, _) => { + Self::Single { .. } => itertools::Either::Left(std::iter::once(None)), + Self::Multi { map, .. } => { itertools::Either::Right(map.keys().map(String::as_str).map(Some)) } } @@ -117,13 +131,17 @@ impl MultiDto { parent: &'static str, ) -> MultiReader { match self { - Self::Single(_) => MultiReader::Single(f(None), parent), - Self::Multi(map) => MultiReader::Multi( - map.keys() + Self::Single(_) => MultiReader::Single { + value: f(None), + parent, + }, + Self::Multi(map) => MultiReader::Multi { + map: map + .keys() .map(|key| (key.to_owned(), f(Some(key)))) .collect(), parent, - ), + }, } } } From 63fee73e2e4d3cdc532960af21153d7430b73633 Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Tue, 1 Oct 2024 17:16:56 +0100 Subject: [PATCH 27/32] Allow writing to novel tedge config multi keys --- .../common/tedge_config_macros/src/multi.rs | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/crates/common/tedge_config_macros/src/multi.rs b/crates/common/tedge_config_macros/src/multi.rs index 5111fe57497..b15aa662cec 100644 --- a/crates/common/tedge_config_macros/src/multi.rs +++ b/crates/common/tedge_config_macros/src/multi.rs @@ -52,7 +52,7 @@ pub enum MultiError { MultiKeyNotFound(String, String), } -impl MultiDto { +impl MultiDto { pub fn try_get(&self, key: Option<&str>, parent: &str) -> Result<&T, MultiError> { match (self, key) { (Self::Single(val), None) => Ok(val), @@ -69,9 +69,7 @@ impl MultiDto { pub fn try_get_mut(&mut self, key: Option<&str>, parent: &str) -> Result<&mut T, MultiError> { match (self, key) { (Self::Single(val), None) => Ok(val), - (Self::Multi(map), Some(key)) => map - .get_mut(key) - .ok_or_else(|| MultiError::MultiKeyNotFound(parent.to_owned(), key.to_owned())), + (Self::Multi(map), Some(key)) => Ok(map.entry((*key).to_owned()).or_default()), (Self::Multi(_), None) => Err(MultiError::MultiNotSingle(parent.to_owned())), (Self::Single(_), Some(key)) => { Err(MultiError::SingleNotMulti(parent.into(), key.into())) @@ -101,19 +99,6 @@ impl MultiReader { } } - pub fn try_get_mut(&mut self, key: Option<&str>) -> Result<&mut T, MultiError> { - match (self, key) { - (Self::Single { value, .. }, None) => Ok(value), - (Self::Multi { map, parent }, Some(key)) => map - .get_mut(key) - .ok_or_else(|| MultiError::MultiKeyNotFound((*parent).into(), key.into())), - (Self::Multi { parent, .. }, None) => Err(MultiError::MultiNotSingle((*parent).into())), - (Self::Single { parent, .. }, Some(key)) => { - Err(MultiError::SingleNotMulti((*parent).into(), key.into())) - } - } - } - pub fn keys(&self) -> impl Iterator> { match self { Self::Single { .. } => itertools::Either::Left(std::iter::once(None)), From 8bbf9180711216821bc96c87c8e03c1e51a63e72 Mon Sep 17 00:00:00 2001 From: James Rhodes <30299230+jarhodes314@users.noreply.github.com> Date: Wed, 2 Oct 2024 09:55:48 +0100 Subject: [PATCH 28/32] Update crates/common/tedge_config_macros/src/multi.rs Co-authored-by: Didier Wenzek --- crates/common/tedge_config_macros/src/multi.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/common/tedge_config_macros/src/multi.rs b/crates/common/tedge_config_macros/src/multi.rs index b15aa662cec..ba5c5181620 100644 --- a/crates/common/tedge_config_macros/src/multi.rs +++ b/crates/common/tedge_config_macros/src/multi.rs @@ -44,11 +44,11 @@ impl MultiDto { #[derive(Debug, thiserror::Error)] pub enum MultiError { - #[error("You are trying to access a named field ({1}) of {0}, but the fields are not named")] + #[error("Unexpected profile {1} for the non multi-profile property {0}")] SingleNotMulti(String, String), - #[error("You need a name for the field {0}")] + #[error("A profile is required for the multi-profile property {0}")] MultiNotSingle(String), - #[error("Key {0}.{1} not found in multi-value group")] + #[error("Unknown profile {1} for the multi-profile property {0}")] MultiKeyNotFound(String, String), } From 64ddb228c4b9d960f00b34810a4c60101d66fb26 Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Wed, 2 Oct 2024 10:19:45 +0100 Subject: [PATCH 29/32] Fix tests and add more tests around multi Signed-off-by: James Rhodes --- .../common/tedge_config_macros/src/multi.rs | 98 ++++++++++++++++++- 1 file changed, 95 insertions(+), 3 deletions(-) diff --git a/crates/common/tedge_config_macros/src/multi.rs b/crates/common/tedge_config_macros/src/multi.rs index ba5c5181620..d2713560626 100644 --- a/crates/common/tedge_config_macros/src/multi.rs +++ b/crates/common/tedge_config_macros/src/multi.rs @@ -44,7 +44,9 @@ impl MultiDto { #[derive(Debug, thiserror::Error)] pub enum MultiError { - #[error("Unexpected profile {1} for the non multi-profile property {0}")] + #[error( + "You are trying to access a profile `{1}` of {0}, but profiles are not enabled for {0}" + )] SingleNotMulti(String, String), #[error("A profile is required for the multi-profile property {0}")] MultiNotSingle(String), @@ -190,6 +192,26 @@ mod tests { assert_eq!(*val.try_get(None, "c8y").unwrap(), "value"); } + #[test] + fn multi_reader_can_retrieve_field_from_single() { + let val = MultiReader::Single { + value: "value", + parent: "c8y", + }; + + assert_eq!(*val.try_get(None).unwrap(), "value"); + } + + #[test] + fn multi_reader_can_retrieve_field_from_multi() { + let val = MultiReader::Multi { + map: [("key".to_owned(), "value")].into(), + parent: "c8y", + }; + + assert_eq!(*val.try_get(Some("key")).unwrap(), "value"); + } + #[test] fn multi_can_retrieve_field_from_multi() { let val = MultiDto::Multi([("key".to_owned(), "value")].into()); @@ -198,12 +220,82 @@ mod tests { } #[test] - fn multi_gives_appropriate_error_retrieving_keyed_field_from_single() { + fn multi_dto_gives_appropriate_error_retrieving_keyed_field_from_single() { let val = MultiDto::Single("value"); assert_eq!( val.try_get(Some("unknown"), "c8y").unwrap_err().to_string(), - "You are trying to access a named field, but the fields are not named" + "You are trying to access a profile `unknown` of c8y, but profiles are not enabled for c8y" ); } + + #[test] + fn multi_reader_gives_appropriate_error_retrieving_keyed_field_from_single() { + let val = MultiReader::Single { + value: "value", + parent: "c8y", + }; + + assert_eq!( + val.try_get(Some("unknown")).unwrap_err().to_string(), + "You are trying to access a profile `unknown` of c8y, but profiles are not enabled for c8y" + ); + } + + #[test] + fn multi_dto_gives_appropriate_error_retrieving_no_profile_from_multi() { + let val = MultiDto::Multi([("key".to_owned(), "value")].into()); + + assert_eq!( + val.try_get(None, "c8y").unwrap_err().to_string(), + "A profile is required for the multi-profile property c8y" + ); + } + + #[test] + fn multi_reader_gives_appropriate_error_retrieving_no_profile_from_multi() { + let val = MultiReader::Multi { + map: [("key".to_owned(), "value")].into(), + parent: "c8y", + }; + + assert_eq!( + val.try_get(None).unwrap_err().to_string(), + "A profile is required for the multi-profile property c8y" + ); + } + + #[test] + fn multi_dto_gives_appropriate_error_retrieving_unknown_profile_from_multi() { + let val = MultiDto::Multi([("key".to_owned(), "value")].into()); + + assert_eq!( + val.try_get(Some("unknown"), "c8y").unwrap_err().to_string(), + "Unknown profile unknown for the multi-profile property c8y" + ); + } + + #[test] + fn multi_reader_gives_appropriate_error_retrieving_unknown_profile_from_multi() { + let val = MultiReader::Multi { + map: [("key".to_owned(), "value")].into(), + parent: "c8y", + }; + + assert_eq!( + val.try_get(Some("unknown")).unwrap_err().to_string(), + "Unknown profile unknown for the multi-profile property c8y" + ); + } + + #[test] + fn multi_dto_inserts_into_map_retrieving_unknown_mutable_profile() { + let mut val = MultiDto::Multi([("key".to_owned(), "value")].into()); + + assert_eq!(*val.try_get_mut(Some("new_key"), "c8y").unwrap(), ""); + let MultiDto::Multi(map) = val else { + unreachable!() + }; + assert_eq!(map.len(), 2); + } } From 85b30f82e42672999fdaa6389f2bccbd882a8bba Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Wed, 2 Oct 2024 15:46:50 +0100 Subject: [PATCH 30/32] Fix issues with normal groups inside multi-value groups Signed-off-by: James Rhodes --- .../tedge_config_macros/examples/multi.rs | 23 +- .../tedge_config_macros/impl/src/query.rs | 4 +- .../tedge_config_macros/impl/src/reader.rs | 238 ++++++++++++++++-- .../common/tedge_config_macros/src/multi.rs | 24 +- 4 files changed, 263 insertions(+), 26 deletions(-) diff --git a/crates/common/tedge_config_macros/examples/multi.rs b/crates/common/tedge_config_macros/examples/multi.rs index 2b65bc47141..2e6e6295cf4 100644 --- a/crates/common/tedge_config_macros/examples/multi.rs +++ b/crates/common/tedge_config_macros/examples/multi.rs @@ -33,8 +33,19 @@ impl AppendRemoveItem for T { define_tedge_config! { #[tedge_config(multi)] c8y: { + #[tedge_config(example = "your-tenant.cumulocity.com")] #[tedge_config(reader(private))] url: String, + + #[tedge_config(default(from_optional_key = "c8y.url"))] + http: String, + + smartrest: { + /// Switch using 501-503 (without operation ID) or 504-506 (with operation ID) SmartREST messages for operation status update + #[tedge_config(example = "true", default(value = true))] + use_operation_id: bool, + }, + #[tedge_config(multi)] something: { test: String, @@ -76,7 +87,7 @@ fn main() { .try_get(Some("cloud")) .unwrap_err() .to_string(), - "You are trying to access a named field (cloud) of c8y, but the fields are not named" + "You are trying to access a profile `cloud` of c8y, but profiles are not enabled for c8y" ); assert_eq!( multi_c8y_reader @@ -84,11 +95,11 @@ fn main() { .try_get(Some("unknown")) .unwrap_err() .to_string(), - "Key c8y.unknown not found in multi-value group" + "Unknown profile `unknown` for the multi-profile property c8y" ); assert_eq!( multi_c8y_reader.c8y.try_get(None).unwrap_err().to_string(), - "You need a name for the field c8y" + "A profile is required for the multi-profile property c8y" ); assert_eq!( @@ -134,10 +145,14 @@ fn main() { assert_eq!( keys, [ + "c8y.cloud.http", + "c8y.cloud.smartrest.use_operation_id", "c8y.cloud.something.test", "c8y.cloud.url", + "c8y.edge.http", + "c8y.edge.smartrest.use_operation_id", "c8y.edge.something.test", - "c8y.edge.url", + "c8y.edge.url" ] ); } diff --git a/crates/common/tedge_config_macros/impl/src/query.rs b/crates/common/tedge_config_macros/impl/src/query.rs index a5bfb1e0238..d6e611b4d84 100644 --- a/crates/common/tedge_config_macros/impl/src/query.rs +++ b/crates/common/tedge_config_macros/impl/src/query.rs @@ -392,7 +392,7 @@ fn key_iterators( )); let ident = &g.ident; exprs.push_back(parse_quote! { - self.#ident.#function_name(#(#args),*) + self.#ident.#function_name(#(#args.clone()),*) }); } Some(FieldOrGroup::Field(f)) => { @@ -1159,7 +1159,7 @@ mod tests { impl TEdgeConfigReaderC8y { pub fn readable_keys(&self, c8y: Option) -> impl Iterator + '_ { - self.nested.readable_keys(c8y) + self.nested.readable_keys(c8y.clone()) } } diff --git a/crates/common/tedge_config_macros/impl/src/reader.rs b/crates/common/tedge_config_macros/impl/src/reader.rs index 7ad3a01883c..4b935bf59d7 100644 --- a/crates/common/tedge_config_macros/impl/src/reader.rs +++ b/crates/common/tedge_config_macros/impl/src/reader.rs @@ -289,16 +289,22 @@ fn reader_value_for_field<'a>( parse_quote!(#key_str.into()) } else { let mut id_gen = SequentialIdGenerator::default(); - let elems = parents.iter().map::(|p| match p { - PathItem::Static(p) => { - let p_str = p.to_string(); - parse_quote!(Some(#p_str)) - } - PathItem::Dynamic(span) => { - let ident = id_gen.next_id(*span); - parse_quote!(#ident) - } - }); + let elems = parents + .iter() + .map::(|p| match p { + PathItem::Static(p) => { + let p_str = p.to_string(); + parse_quote!(Some(#p_str)) + } + PathItem::Dynamic(span) => { + let ident = id_gen.next_id(*span); + parse_quote!(#ident) + } + }) + .chain({ + let name = name.to_string(); + iter::once(parse_quote!(Some(#name))) + }); parse_quote!([#(#elems),*].into_iter().filter_map(|id| id).collect::>().join(".").into()) }; let read_path = read_field(parents); @@ -334,14 +340,50 @@ fn reader_value_for_field<'a>( } FieldDefault::FromKey(default_key) | FieldDefault::FromOptionalKey(default_key) => { observed_keys.push(default_key); + // TODO error if the field touches an unrelated multi? + // Trace through the key to make sure we pick up dynamic paths as we expect + let mut parents = parents.iter().peekable(); + let mut fields = root_fields; + let mut new_parents = vec![]; + for (i, field) in default_key.iter().take(default_key.len() - 1).enumerate() { + new_parents.push(PathItem::Static(field.to_owned())); + if let Some(PathItem::Static(s)) = parents.next() { + if field == s { + if let Some(PathItem::Dynamic(span)) = parents.peek() { + parents.next(); + new_parents.push(PathItem::Dynamic(*span)); + } + } else { + while parents.next().is_some() {} + } + } + let root_field = fields.iter().find(|fog| fog.ident() == field).unwrap(); + match root_field { + FieldOrGroup::Multi(m) => { + if matches!(new_parents.last().unwrap(), PathItem::Dynamic(_)) { + fields = &m.contents; + } else { + let multi_key = default_key + .iter() + .take(i + 1) + .map(|i| i.to_string()) + .collect::>() + .join("."); + return Err(syn::Error::new( + default_key.span(), + format!("{multi_key} is a multi-value field"), + )); + } + } + FieldOrGroup::Group(g) => { + fields = &g.contents; + } + FieldOrGroup::Field(_) => {} + } + } let default = reader_value_for_field( find_field(root_fields, default_key)?, - &default_key - .iter() - .take(default_key.len() - 1) - .map(<_>::to_owned) - .map(PathItem::Static) - .collect::>(), + &new_parents, root_fields, observed_keys, )?; @@ -432,7 +474,17 @@ fn generate_conversions( let mut parents = parents.clone(); parents.push(PathItem::Static(group.ident.clone())); - field_conversions.push(quote!(#name: #sub_reader_name::from_dto(dto, location))); + let mut id_gen = SequentialIdGenerator::default(); + let extra_call_args: Vec<_> = parents + .iter() + .filter_map(|path_item| match path_item { + PathItem::Static(_) => None, + PathItem::Dynamic(span) => Some(id_gen.next_id(*span)), + }) + .collect(); + field_conversions.push( + quote!(#name: #sub_reader_name::from_dto(dto, location, #(#extra_call_args),*)), + ); let sub_conversions = generate_conversions(&sub_reader_name, &group.contents, parents, root_fields)?; rest.push(sub_conversions); @@ -491,3 +543,155 @@ fn generate_conversions( #(#rest)* }) } + +#[cfg(test)] +mod tests { + use super::*; + use syn::parse_quote; + + #[test] + fn from_optional_key_reuses_multi_fields() { + let input: crate::input::Configuration = parse_quote!( + #[tedge_config(multi)] + c8y: { + url: String, + + #[tedge_config(default(from_optional_key = "c8y.url"))] + http: String, + } + ); + let FieldOrGroup::Multi(m) = &input.groups[0] else { + unreachable!() + }; + let http = m.contents[1].field().unwrap(); + let actual = reader_value_for_field( + http, + &[ + PathItem::Static(parse_quote!(c8y)), + PathItem::Dynamic(Span::call_site()), + ], + &input.groups, + vec![], + ) + .unwrap(); + let actual: syn::File = parse_quote!(fn dummy() { #actual }); + let c8y_http_key = quote! { + [Some("c8y"), key0, Some("http")] + .into_iter() + .filter_map(|id| id) + .collect::>() + .join(".") + .into() + }; + let c8y_url_key = quote! { + [Some("c8y"), key0, Some("url")] + .into_iter() + .filter_map(|id| id) + .collect::>() + .join(".") + .into() + }; + let expected: syn::Expr = parse_quote!(match &dto.c8y.try_get(key0, "c8y").unwrap().http { + Some(value) => { + OptionalConfig::Present { + value: value.clone(), + key: #c8y_http_key, + } + } + None => { + match &dto.c8y.try_get(key0, "c8y").unwrap().url { + None => OptionalConfig::Empty(#c8y_url_key), + Some(value) => OptionalConfig::Present { + value: value.clone(), + key: #c8y_url_key, + }, + } + .map(|v| v.into()) + } + }); + let expected = parse_quote!(fn dummy() { #expected }); + pretty_assertions::assert_eq!( + prettyplease::unparse(&actual), + prettyplease::unparse(&expected), + ) + } + + #[test] + fn from_optional_key_returns_error_with_invalid_multi() { + let input: crate::input::Configuration = parse_quote!( + #[tedge_config(multi)] + c8y: { + url: String, + }, + az: { + // We can't derive this from c8y.url, as we don't have a profile to select from c8y + #[tedge_config(default(from_optional_key = "c8y.url"))] + url: String, + } + ); + + let FieldOrGroup::Group(g) = &input.groups[1] else { + unreachable!() + }; + let az_url = g.contents[0].field().unwrap(); + let error = reader_value_for_field( + az_url, + &[PathItem::Static(parse_quote!(az))], + &input.groups, + vec![], + ) + .unwrap_err(); + + assert_eq!(error.to_string(), "c8y is a multi-value field"); + } + + #[test] + fn generate_conversions_() { + let input: crate::input::Configuration = parse_quote!( + #[tedge_config(multi)] + c8y: { + smartrest: { + templates: TemplatesSet, + } + }, + ); + let actual = generate_conversions( + &parse_quote!(TEdgeConfigReader), + &input.groups, + Vec::new(), + &input.groups, + ) + .unwrap(); + let file: syn::File = syn::parse2(actual).unwrap(); + let r#impl = file + .items + .into_iter() + .find_map(|item| match item { + syn::Item::Impl(i) if i.self_ty == parse_quote!(TEdgeConfigReaderC8y) => Some(i), + _ => None, + }) + .unwrap(); + + let expected = parse_quote! { + impl TEdgeConfigReaderC8y { + #[allow(unused, clippy::clone_on_copy, clippy::useless_conversion)] + #[automatically_derived] + /// Converts the provided [TEdgeConfigDto] into a reader + pub fn from_dto( + dto: &TEdgeConfigDto, + location: &TEdgeConfigLocation, + key0: Option<&str>, + ) -> Self { + Self { + smartrest: TEdgeConfigReaderC8ySmartrest::from_dto(dto, location, key0) + } + } + } + }; + + pretty_assertions::assert_eq!( + prettyplease::unparse(&parse_quote!(#r#impl)), + prettyplease::unparse(&expected) + ) + } +} diff --git a/crates/common/tedge_config_macros/src/multi.rs b/crates/common/tedge_config_macros/src/multi.rs index d2713560626..8ac8a32c970 100644 --- a/crates/common/tedge_config_macros/src/multi.rs +++ b/crates/common/tedge_config_macros/src/multi.rs @@ -1,3 +1,5 @@ +use itertools::Either; + #[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] #[serde(untagged)] pub enum MultiDto { @@ -50,7 +52,7 @@ pub enum MultiError { SingleNotMulti(String, String), #[error("A profile is required for the multi-profile property {0}")] MultiNotSingle(String), - #[error("Unknown profile {1} for the multi-profile property {0}")] + #[error("Unknown profile `{1}` for the multi-profile property {0}")] MultiKeyNotFound(String, String), } @@ -109,6 +111,22 @@ impl MultiReader { } } } + + pub fn entries(&self) -> impl Iterator, &T)> { + match self { + Self::Single { value, .. } => Either::Left(std::iter::once((None, value))), + Self::Multi { map, .. } => { + Either::Right(map.iter().map(|(k, v)| (Some(k.as_str()), v))) + } + } + } + + pub fn values(&self) -> impl Iterator { + match self { + Self::Single { value, .. } => Either::Left(std::iter::once(value)), + Self::Multi { map, .. } => Either::Right(map.iter().map(|(_, v)| v)), + } + } } impl MultiDto { @@ -271,7 +289,7 @@ mod tests { assert_eq!( val.try_get(Some("unknown"), "c8y").unwrap_err().to_string(), - "Unknown profile unknown for the multi-profile property c8y" + "Unknown profile `unknown` for the multi-profile property c8y" ); } @@ -284,7 +302,7 @@ mod tests { assert_eq!( val.try_get(Some("unknown")).unwrap_err().to_string(), - "Unknown profile unknown for the multi-profile property c8y" + "Unknown profile `unknown` for the multi-profile property c8y" ); } From 87da9c3fc57f13e87fb8b13495566cca4b4d1d82 Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Wed, 2 Oct 2024 17:02:25 +0100 Subject: [PATCH 31/32] Refactoring Signed-off-by: James Rhodes --- .../tedge_config_macros/impl/src/reader.rs | 120 +++++++++++------- 1 file changed, 77 insertions(+), 43 deletions(-) diff --git a/crates/common/tedge_config_macros/impl/src/reader.rs b/crates/common/tedge_config_macros/impl/src/reader.rs index 4b935bf59d7..9260f2af2fa 100644 --- a/crates/common/tedge_config_macros/impl/src/reader.rs +++ b/crates/common/tedge_config_macros/impl/src/reader.rs @@ -340,50 +340,9 @@ fn reader_value_for_field<'a>( } FieldDefault::FromKey(default_key) | FieldDefault::FromOptionalKey(default_key) => { observed_keys.push(default_key); - // TODO error if the field touches an unrelated multi? - // Trace through the key to make sure we pick up dynamic paths as we expect - let mut parents = parents.iter().peekable(); - let mut fields = root_fields; - let mut new_parents = vec![]; - for (i, field) in default_key.iter().take(default_key.len() - 1).enumerate() { - new_parents.push(PathItem::Static(field.to_owned())); - if let Some(PathItem::Static(s)) = parents.next() { - if field == s { - if let Some(PathItem::Dynamic(span)) = parents.peek() { - parents.next(); - new_parents.push(PathItem::Dynamic(*span)); - } - } else { - while parents.next().is_some() {} - } - } - let root_field = fields.iter().find(|fog| fog.ident() == field).unwrap(); - match root_field { - FieldOrGroup::Multi(m) => { - if matches!(new_parents.last().unwrap(), PathItem::Dynamic(_)) { - fields = &m.contents; - } else { - let multi_key = default_key - .iter() - .take(i + 1) - .map(|i| i.to_string()) - .collect::>() - .join("."); - return Err(syn::Error::new( - default_key.span(), - format!("{multi_key} is a multi-value field"), - )); - } - } - FieldOrGroup::Group(g) => { - fields = &g.contents; - } - FieldOrGroup::Field(_) => {} - } - } let default = reader_value_for_field( find_field(root_fields, default_key)?, - &new_parents, + &parents_for(default_key, parents, root_fields)?, root_fields, observed_keys, )?; @@ -440,6 +399,81 @@ fn reader_value_for_field<'a>( }) } +/// Generates the list of parent keys for the given tedge config key +/// +/// This cross-correlates the provided key with the current key's parents, +/// so keys with profiles will work. +/// +/// For example, in the case +/// +/// ```no_compile +/// define_tedge_config! { +/// #[tedge_config(multi)] +/// c8y: { +/// url: String, +/// +/// #[tedge_config(default(from_optional_key = "c8y.url"))] +/// http: String, +/// } +/// } +/// ``` +/// +/// The parents used in the default value for `c8y.*.http` will be equivalent `c8y.*.url`. +/// This means the c8y.url value used for c8y.http will use the same profile as the relevant +/// c8y.http. +fn parents_for( + key: &Punctuated, + parents: &[PathItem], + root_fields: &[FieldOrGroup], +) -> syn::Result> { + // Trace through the key to make sure we pick up dynamic paths as we expect + // This allows e.g. c8y.http to have a default value of c8y.url, and we will pass in key0, key1 as expected + // If we hit multi fields that aren't also parents of the current key, this is an error, + // as there isn't a sensible resolution to this + let mut parents = parents.iter().peekable(); + let mut fields = root_fields; + let mut new_parents = vec![]; + for (i, field) in key.iter().take(key.len() - 1).enumerate() { + new_parents.push(PathItem::Static(field.to_owned())); + if let Some(PathItem::Static(s)) = parents.next() { + if field == s { + if let Some(PathItem::Dynamic(span)) = parents.peek() { + parents.next(); + new_parents.push(PathItem::Dynamic(*span)); + } + } else { + // This key has diverged from the currrent key's parents, so empty the list + // This will prevent unwanted matches that aren't genuine + while parents.next().is_some() {} + } + } + let root_field = fields.iter().find(|fog| fog.ident() == field).unwrap(); + match root_field { + FieldOrGroup::Multi(m) => { + if matches!(new_parents.last().unwrap(), PathItem::Dynamic(_)) { + fields = &m.contents; + } else { + let multi_key = key + .iter() + .take(i + 1) + .map(|i| i.to_string()) + .collect::>() + .join("."); + return Err(syn::Error::new( + key.span(), + format!("{multi_key} is a multi-value field"), + )); + } + } + FieldOrGroup::Group(g) => { + fields = &g.contents; + } + FieldOrGroup::Field(_) => {} + } + } + Ok(new_parents) +} + /// Generate the conversion methods from DTOs to Readers fn generate_conversions( name: &proc_macro2::Ident, @@ -646,7 +680,7 @@ mod tests { } #[test] - fn generate_conversions_() { + fn generate_conversions_passes_profile_keys_to_conversions_for_groups() { let input: crate::input::Configuration = parse_quote!( #[tedge_config(multi)] c8y: { From 3936b7d5bfd96f6947946a30a7f1d1e651c624d7 Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Thu, 3 Oct 2024 12:20:28 +0100 Subject: [PATCH 32/32] Support converting between single and multi fields Signed-off-by: James Rhodes --- .../common/tedge_config_macros/src/multi.rs | 53 +++++++++++++++++-- 1 file changed, 50 insertions(+), 3 deletions(-) diff --git a/crates/common/tedge_config_macros/src/multi.rs b/crates/common/tedge_config_macros/src/multi.rs index 8ac8a32c970..7678a214420 100644 --- a/crates/common/tedge_config_macros/src/multi.rs +++ b/crates/common/tedge_config_macros/src/multi.rs @@ -1,4 +1,5 @@ use itertools::Either; +use std::collections::HashMap; #[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] #[serde(untagged)] @@ -56,7 +57,7 @@ pub enum MultiError { MultiKeyNotFound(String, String), } -impl MultiDto { +impl MultiDto { pub fn try_get(&self, key: Option<&str>, parent: &str) -> Result<&T, MultiError> { match (self, key) { (Self::Single(val), None) => Ok(val), @@ -74,10 +75,26 @@ impl MultiDto { match (self, key) { (Self::Single(val), None) => Ok(val), (Self::Multi(map), Some(key)) => Ok(map.entry((*key).to_owned()).or_default()), - (Self::Multi(_), None) => Err(MultiError::MultiNotSingle(parent.to_owned())), - (Self::Single(_), Some(key)) => { + (Self::Multi(map), None) if map.values().any(|v| *v != T::default()) => { + Err(MultiError::MultiNotSingle(parent.to_owned())) + } + (multi @ Self::Multi(_), None) => { + *multi = Self::Single(T::default()); + let Self::Single(value) = multi else { + unreachable!() + }; + Ok(value) + } + (Self::Single(t), Some(key)) if *t != T::default() => { Err(MultiError::SingleNotMulti(parent.into(), key.into())) } + (multi @ Self::Single(_), Some(key)) => { + *multi = Self::Multi(HashMap::new()); + let Self::Multi(map) = multi else { + unreachable!() + }; + Ok(map.entry((*key).to_owned()).or_default()) + } } } @@ -316,4 +333,34 @@ mod tests { }; assert_eq!(map.len(), 2); } + + #[test] + fn multi_dto_can_convert_default_single_config_to_multi() { + let mut val = MultiDto::Single(""); + + assert_eq!(*val.try_get_mut(Some("new_key"), "c8y").unwrap(), ""); + let MultiDto::Multi(map) = val else { + unreachable!() + }; + assert_eq!(map.len(), 1); + } + + #[test] + fn multi_dto_can_convert_default_multi_config_to_single() { + let mut val = MultiDto::Multi([("key".to_owned(), ""), ("key2".to_owned(), "")].into()); + + assert_eq!(*val.try_get_mut(None, "c8y").unwrap(), ""); + assert_eq!(val, MultiDto::Single("")); + } + + #[test] + fn multi_dto_refuses_to_convert_non_default_multi_config_to_single() { + let mut val = + MultiDto::Multi([("key".to_owned(), "non default"), ("key2".to_owned(), "")].into()); + + assert_eq!( + val.try_get_mut(None, "c8y").unwrap_err().to_string(), + "A profile is required for the multi-profile property c8y" + ); + } }