From 894637b20f51647dc6731a391f286281e4e2d286 Mon Sep 17 00:00:00 2001 From: Renjie Liu Date: Thu, 6 Aug 2026 14:24:12 +0800 Subject: [PATCH 1/5] Add derive properties macro --- Cargo.lock | 11 + Cargo.toml | 1 + crates/property-macro/Cargo.toml | 47 ++ crates/property-macro/README.md | 137 ++++ crates/property-macro/src/lib.rs | 48 ++ crates/property-macro/src/properties.rs | 789 ++++++++++++++++++++++ crates/property-macro/tests/properties.rs | 304 +++++++++ 7 files changed, 1337 insertions(+) create mode 100644 crates/property-macro/Cargo.toml create mode 100644 crates/property-macro/README.md create mode 100644 crates/property-macro/src/lib.rs create mode 100644 crates/property-macro/src/properties.rs create mode 100644 crates/property-macro/tests/properties.rs diff --git a/Cargo.lock b/Cargo.lock index 0f264cc1f5..d48f9beb79 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4007,6 +4007,17 @@ dependencies = [ "tracing-subscriber", ] +[[package]] +name = "iceberg-property-macro" +version = "0.10.0" +dependencies = [ + "proc-macro2", + "quote", + "serde", + "serde_json", + "syn", +] + [[package]] name = "iceberg-sqllogictest" version = "0.10.0" diff --git a/Cargo.toml b/Cargo.toml index a789ef1967..bcdd080347 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,6 +21,7 @@ members = [ "crates/catalog/*", "crates/examples", "crates/iceberg", + "crates/property-macro", "crates/integration_tests", "crates/integrations/*", "crates/sqllogictest", diff --git a/crates/property-macro/Cargo.toml b/crates/property-macro/Cargo.toml new file mode 100644 index 0000000000..075cf73f63 --- /dev/null +++ b/crates/property-macro/Cargo.toml @@ -0,0 +1,47 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +[package] +edition = { workspace = true } +homepage = { workspace = true } +name = "iceberg-property-macro" +publish = true +readme = "README.md" +rust-version = { workspace = true } +version = { workspace = true } + +license = { workspace = true } +repository = { workspace = true } + +categories = ["database"] +description = "Property derive macro for Apache Iceberg Rust" +keywords = ["iceberg"] + +[lib] +proc-macro = true + +[dependencies] +proc-macro2 = "1" +quote = "1" +syn = { version = "2", features = ["full"] } + +[dev-dependencies] +serde = { workspace = true, features = ["derive"] } +serde_json = { workspace = true } + +[lints] +workspace = true diff --git a/crates/property-macro/README.md b/crates/property-macro/README.md new file mode 100644 index 0000000000..aab8a9c9c5 --- /dev/null +++ b/crates/property-macro/README.md @@ -0,0 +1,137 @@ + + +# Iceberg property derive macro + +`Properties` generates inherent methods for reading and writing a typed struct +from a flat `HashMap`. It deliberately does not implement +`Default`, `Serialize`, `Deserialize`, or any other trait. + +Leaf fields declare a property key and the default used when that key is absent. +Public accessors are opt-in: + +```rust +use iceberg_property_macro::Properties; + +#[derive(Default, Properties)] +struct WriteProperties { + #[property( + key = "commit.retry.num-retries", + default = 0, + pub(getter), + pub(setter) + )] + retries: u64, +} + +let mut properties = WriteProperties::default(); +properties.set_retries(4); +assert_eq!(*properties.retries(), 4); +``` + +The annotated property default is independent of the value produced by a +derived `Default` implementation. When both are used, keep them aligned. + +## Using a property map with Serde + +Serde's standard derives serialize a struct's fields and cannot infer the +property-map representation from `Properties` attributes. A transparent adapter +keeps that conversion explicit while allowing `Default`, `Serialize`, and +`Deserialize` to remain ordinary derives: + +```rust +use std::collections::HashMap; + +use iceberg_property_macro::Properties; +use serde::{Deserialize, Deserializer, Serialize, Serializer}; + +#[derive(Debug, Default, PartialEq, Properties)] +struct WriteProperties { + #[property( + key = "commit.retry.num-retries", + default = 0, + pub(getter), + pub(setter) + )] + retries: u64, + + #[property(key = "owner", default = None)] + owner: Option, +} + +mod property_map { + use super::*; + + pub fn serialize(value: &WriteProperties, serializer: S) -> Result + where + S: Serializer, + { + let mut properties = HashMap::new(); + value.write_properties(&mut properties); + properties.serialize(serializer) + } + + pub fn deserialize<'de, D>(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + let properties = HashMap::::deserialize(deserializer)?; + WriteProperties::from_properties(&properties).map_err(serde::de::Error::custom) + } +} + +#[derive(Debug, Default, Serialize, Deserialize)] +#[serde(transparent)] +struct PropertyDocument(#[serde(with = "property_map")] WriteProperties); + +fn main() -> Result<(), Box> { + let mut document = PropertyDocument::default(); + document.0.set_retries(4); + + let json = serde_json::to_string(&document)?; + assert_eq!(json, r#"{"commit.retry.num-retries":"4"}"#); + + let decoded: PropertyDocument = serde_json::from_str(&json)?; + assert_eq!(*decoded.0.retries(), 4); + Ok(()) +} +``` + +Property options may be grouped under `#[property(...)]`, which avoids a +collision between the standalone `#[default(...)]` helper and Rust's `Default` +derive. The standalone annotations from the original framework remain +supported. + +`#[prefix(...)]` captures a family of properties in a `HashMap`, +keyed by the suffix after the prefix. `#[nested]` embeds another `Properties` +struct while keeping the property map flat. `#[parse_with(...)]` and +`#[serialize_with(...)]` customize conversion for one exact-key field. The +latter name refers to conversion into a property string and does not require +Serde. + +`#[parse_properties_with(...)]` and `#[write_properties_with(...)]` receive the +complete property map for fields represented by more than one key. +`#[additional_key(...)]` supplies a second key to those hooks. Custom write +hooks receive the field default and are responsible for removing or omitting +default-valued properties. + +Boolean property values are parsed case-insensitively. Other values require +`FromStr` and `ToString` unless custom conversion hooks are supplied. Leaf +fields require `PartialEq` so default values can be omitted. String-literal and +path defaults are converted into their field type with `Into`. diff --git a/crates/property-macro/src/lib.rs b/crates/property-macro/src/lib.rs new file mode 100644 index 0000000000..018a7c1f04 --- /dev/null +++ b/crates/property-macro/src/lib.rs @@ -0,0 +1,48 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#![doc = include_str!("../README.md")] + +use proc_macro::TokenStream; +use syn::{DeriveInput, parse_macro_input}; + +mod properties; + +/// Derives property-map parsing, writing, and opt-in accessors for a struct. +#[proc_macro_derive( + Properties, + attributes( + key, + additional_key, + prefix, + nested, + default, + parse_with, + serialize_with, + parse_properties_with, + write_properties_with, + property + ) +)] +pub fn derive_properties(input: TokenStream) -> TokenStream { + let input = parse_macro_input!(input as DeriveInput); + + match properties::expand_properties(input) { + Ok(tokens) => tokens.into(), + Err(error) => error.into_compile_error().into(), + } +} diff --git a/crates/property-macro/src/properties.rs b/crates/property-macro/src/properties.rs new file mode 100644 index 0000000000..564a52bf49 --- /dev/null +++ b/crates/property-macro/src/properties.rs @@ -0,0 +1,789 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use proc_macro2::TokenStream as TokenStream2; +use quote::{format_ident, quote}; +use syn::parse::{Parse, ParseStream}; +use syn::punctuated::Punctuated; +use syn::{ + Attribute, Data, DeriveInput, Error, Expr, ExprLit, ExprPath, Field, Fields, GenericArgument, + Ident, Lit, Meta, Path, PathArguments, Token, Type, parenthesized, +}; + +struct PropertyField { + ident: Ident, + ty: Type, + key: Option, + additional_key: Option, + prefix: Option, + nested: bool, + default: Option, + parse_with: Option, + serialize_with: Option, + parse_properties_with: Option, + write_properties_with: Option, + option_inner_type: Option, + map_value_type: Option, + public_getter: bool, + public_setter: bool, + doc_attributes: Vec, +} + +enum PublicAccessor { + Getter, + Setter, +} + +enum PropertyOption { + Key(Expr), + AdditionalKey(Expr), + Prefix(Expr), + Nested, + Default(Expr), + ParseWith(Path), + SerializeWith(Path), + ParsePropertiesWith(Path), + WritePropertiesWith(Path), + Accessor(PublicAccessor), +} + +#[derive(Default)] +struct PropertyOptions { + key: Option, + additional_key: Option, + prefix: Option, + nested: bool, + default: Option, + parse_with: Option, + serialize_with: Option, + parse_properties_with: Option, + write_properties_with: Option, + public_getter: bool, + public_setter: bool, +} + +impl Parse for PublicAccessor { + fn parse(input: ParseStream<'_>) -> syn::Result { + input.parse::()?; + let content; + parenthesized!(content in input); + let accessor = content.parse::()?; + if !content.is_empty() { + return Err(content.error("expected getter or setter")); + } + + match accessor.to_string().as_str() { + "getter" => Ok(Self::Getter), + "setter" => Ok(Self::Setter), + _ => Err(Error::new_spanned(accessor, "expected getter or setter")), + } + } +} + +impl Parse for PropertyOption { + fn parse(input: ParseStream<'_>) -> syn::Result { + if input.peek(Token![pub]) { + return input.parse().map(Self::Accessor); + } + + let name = input.parse::()?; + let option_name = name.to_string(); + if option_name == "nested" { + return Ok(Self::Nested); + } + + input.parse::()?; + let expression = input.parse::()?; + match option_name.as_str() { + "key" => Ok(Self::Key(expression)), + "additional_key" => Ok(Self::AdditionalKey(expression)), + "prefix" => Ok(Self::Prefix(expression)), + "default" => Ok(Self::Default(expression)), + "parse_with" => expression_path(expression, "parse_with").map(Self::ParseWith), + "serialize_with" => { + expression_path(expression, "serialize_with").map(Self::SerializeWith) + } + "parse_properties_with" => { + expression_path(expression, "parse_properties_with").map(Self::ParsePropertiesWith) + } + "write_properties_with" => { + expression_path(expression, "write_properties_with").map(Self::WritePropertiesWith) + } + _ => Err(Error::new_spanned(name, "unknown property option")), + } + } +} + +pub(crate) fn expand_properties(input: DeriveInput) -> syn::Result { + let struct_name = input.ident; + let generics = input.generics; + let fields = match input.data { + Data::Struct(data) => match data.fields { + Fields::Named(fields) => fields.named, + _ => { + return Err(Error::new_spanned( + struct_name, + "Properties can only be derived for structs with named fields", + )); + } + }, + _ => { + return Err(Error::new_spanned( + struct_name, + "Properties can only be derived for structs", + )); + } + }; + + let fields = fields + .iter() + .map(parse_property_field) + .collect::>>()?; + let parses = fields.iter().map(parse_field); + let property_writes = fields.iter().map(write_field); + let accessors = fields.iter().map(field_accessors); + let (impl_generics, type_generics, where_clause) = generics.split_for_impl(); + + Ok(quote! { + impl #impl_generics #struct_name #type_generics #where_clause { + #(#accessors)* + + pub(crate) fn from_properties( + properties: &::std::collections::HashMap< + ::std::string::String, + ::std::string::String, + >, + ) -> ::std::result::Result { + Ok(Self { + #(#parses,)* + }) + } + + pub(crate) fn write_properties( + &self, + properties: &mut ::std::collections::HashMap< + ::std::string::String, + ::std::string::String, + >, + ) { + #(#property_writes)* + } + } + }) +} + +fn parse_property_field(field: &Field) -> syn::Result { + let ident = field + .ident + .clone() + .ok_or_else(|| Error::new_spanned(field, "Properties fields must be named"))?; + let property_options = property_options(&field.attrs)?; + let key = merge_attribute_option( + attribute_expression_value(&field.attrs, "key")?, + property_options.key, + field, + "key", + )?; + let additional_key = merge_attribute_option( + attribute_expression_value(&field.attrs, "additional_key")?, + property_options.additional_key, + field, + "additional_key", + )?; + let prefix = merge_attribute_option( + attribute_expression_value(&field.attrs, "prefix")?, + property_options.prefix, + field, + "prefix", + )?; + let standalone_nested = marker_attribute(&field.attrs, "nested")?; + if standalone_nested && property_options.nested { + return Err(Error::new_spanned( + field, + "duplicate nested property option", + )); + } + let nested = standalone_nested || property_options.nested; + + if usize::from(key.is_some()) + usize::from(prefix.is_some()) + usize::from(nested) != 1 { + return Err(Error::new_spanned( + field, + "Properties fields must declare exactly one of #[key(...)], #[prefix(...)], or #[nested]", + )); + } + + let default = merge_attribute_option( + attribute_expression_value(&field.attrs, "default")?, + property_options.default, + field, + "default", + )?; + if nested && default.is_some() { + return Err(Error::new_spanned( + field, + "#[nested] fields obtain defaults from their own property annotations and cannot declare #[default(...)]", + )); + } + if !nested && default.is_none() { + return Err(Error::new_spanned( + field, + "Properties leaf fields must declare #[default(...)]", + )); + } + + let map_value_type = hash_map_value_type(&field.ty); + if prefix.is_some() && map_value_type.is_none() { + return Err(Error::new_spanned( + &field.ty, + "#[prefix(...)] fields must have type HashMap", + )); + } + + let parse_with = merge_attribute_option( + attribute_path_value(&field.attrs, "parse_with")?, + property_options.parse_with, + field, + "parse_with", + )?; + let serialize_with = merge_attribute_option( + attribute_path_value(&field.attrs, "serialize_with")?, + property_options.serialize_with, + field, + "serialize_with", + )?; + let parse_properties_with = merge_attribute_option( + attribute_path_value(&field.attrs, "parse_properties_with")?, + property_options.parse_properties_with, + field, + "parse_properties_with", + )?; + let write_properties_with = merge_attribute_option( + attribute_path_value(&field.attrs, "write_properties_with")?, + property_options.write_properties_with, + field, + "write_properties_with", + )?; + + if additional_key.is_some() + && parse_properties_with.is_none() + && write_properties_with.is_none() + { + return Err(Error::new_spanned( + field, + "#[additional_key(...)] requires parse_properties_with or write_properties_with", + )); + } + if (prefix.is_some() || nested) + && (additional_key.is_some() + || parse_with.is_some() + || serialize_with.is_some() + || parse_properties_with.is_some() + || write_properties_with.is_some()) + { + return Err(Error::new_spanned( + field, + "#[prefix(...)] and #[nested] fields do not support custom parse or write functions", + )); + } + if parse_with.is_some() && parse_properties_with.is_some() { + return Err(Error::new_spanned( + field, + "fields cannot declare both parse_with and parse_properties_with", + )); + } + if serialize_with.is_some() && write_properties_with.is_some() { + return Err(Error::new_spanned( + field, + "fields cannot declare both serialize_with and write_properties_with", + )); + } + + Ok(PropertyField { + ident, + ty: field.ty.clone(), + key, + additional_key, + prefix, + nested, + default, + parse_with, + serialize_with, + parse_properties_with, + write_properties_with, + option_inner_type: option_inner_type(&field.ty), + map_value_type, + public_getter: property_options.public_getter, + public_setter: property_options.public_setter, + doc_attributes: field + .attrs + .iter() + .filter(|attribute| attribute.path().is_ident("doc")) + .cloned() + .collect(), + }) +} + +fn property_options(attributes: &[Attribute]) -> syn::Result { + let Some(attribute) = find_attribute(attributes, "property")? else { + return Ok(PropertyOptions::default()); + }; + + let parsed = + attribute.parse_args_with(Punctuated::::parse_terminated)?; + if parsed.is_empty() { + return Err(Error::new_spanned( + attribute, + "property must declare at least one option", + )); + } + + let mut options = PropertyOptions::default(); + for option in parsed { + match option { + PropertyOption::Key(value) => { + set_property_option(&mut options.key, value, attribute, "key")? + } + PropertyOption::AdditionalKey(value) => set_property_option( + &mut options.additional_key, + value, + attribute, + "additional_key", + )?, + PropertyOption::Prefix(value) => { + set_property_option(&mut options.prefix, value, attribute, "prefix")? + } + PropertyOption::Nested => { + if options.nested { + return Err(Error::new_spanned( + attribute, + "duplicate nested property option", + )); + } + options.nested = true; + } + PropertyOption::Default(value) => { + set_property_option(&mut options.default, value, attribute, "default")? + } + PropertyOption::ParseWith(value) => { + set_property_option(&mut options.parse_with, value, attribute, "parse_with")? + } + PropertyOption::SerializeWith(value) => set_property_option( + &mut options.serialize_with, + value, + attribute, + "serialize_with", + )?, + PropertyOption::ParsePropertiesWith(value) => set_property_option( + &mut options.parse_properties_with, + value, + attribute, + "parse_properties_with", + )?, + PropertyOption::WritePropertiesWith(value) => set_property_option( + &mut options.write_properties_with, + value, + attribute, + "write_properties_with", + )?, + PropertyOption::Accessor(accessor) => { + let selected = match accessor { + PublicAccessor::Getter => &mut options.public_getter, + PublicAccessor::Setter => &mut options.public_setter, + }; + if *selected { + return Err(Error::new_spanned(attribute, "duplicate property accessor")); + } + *selected = true; + } + } + } + + Ok(options) +} + +fn set_property_option( + target: &mut Option, + value: T, + attribute: &Attribute, + name: &str, +) -> syn::Result<()> { + if target.is_some() { + return Err(Error::new_spanned( + attribute, + format!("duplicate {name} property option"), + )); + } + *target = Some(value); + Ok(()) +} + +fn merge_attribute_option( + standalone: Option, + grouped: Option, + field: &Field, + name: &str, +) -> syn::Result> { + match (standalone, grouped) { + (Some(_), Some(_)) => Err(Error::new_spanned( + field, + format!("duplicate {name} property option"), + )), + (Some(value), None) | (None, Some(value)) => Ok(Some(value)), + (None, None) => Ok(None), + } +} + +fn field_accessors(field: &PropertyField) -> TokenStream2 { + let ident = &field.ident; + let ty = &field.ty; + let docs = &field.doc_attributes; + let getter = field.public_getter.then(|| { + quote! { + #(#docs)* + pub fn #ident(&self) -> &#ty { + &self.#ident + } + } + }); + let setter = field.public_setter.then(|| { + let setter_ident = format_ident!("set_{}", ident); + let setter_doc = format!("Sets `{ident}`."); + quote! { + #[doc = #setter_doc] + pub fn #setter_ident(&mut self, value: #ty) { + self.#ident = value; + } + } + }); + + quote! { + #getter + #setter + } +} + +fn marker_attribute(attributes: &[Attribute], name: &str) -> syn::Result { + let Some(attribute) = find_attribute(attributes, name)? else { + return Ok(false); + }; + + match &attribute.meta { + Meta::Path(_) => Ok(true), + _ => Err(Error::new_spanned( + attribute, + format!("{name} must use the form #[{name}]"), + )), + } +} + +fn attribute_expression_value(attributes: &[Attribute], name: &str) -> syn::Result> { + let Some(attribute) = find_attribute(attributes, name)? else { + return Ok(None); + }; + + match &attribute.meta { + Meta::NameValue(name_value) => Ok(Some(name_value.value.clone())), + Meta::List(_) => attribute.parse_args::().map(Some), + _ => Err(Error::new_spanned( + attribute, + format!("{name} must use the form #[{name}(...)]"), + )), + } +} + +fn attribute_path_value(attributes: &[Attribute], name: &str) -> syn::Result> { + let Some(expression) = attribute_expression_value(attributes, name)? else { + return Ok(None); + }; + + expression_path(expression, name).map(Some) +} + +fn expression_path(expression: Expr, name: &str) -> syn::Result { + match expression { + Expr::Path(ExprPath { path, .. }) => Ok(path), + _ => Err(Error::new_spanned( + expression, + format!("{name} must be a path"), + )), + } +} + +fn find_attribute<'a>( + attributes: &'a [Attribute], + name: &str, +) -> syn::Result> { + let mut matching = attributes + .iter() + .filter(|attribute| attribute.path().is_ident(name)); + let first = matching.next(); + if let Some(duplicate) = matching.next() { + return Err(Error::new_spanned( + duplicate, + format!("duplicate #[{name}] attribute"), + )); + } + Ok(first) +} + +fn parse_field(field: &PropertyField) -> TokenStream2 { + let ident = &field.ident; + if field.nested { + let ty = &field.ty; + return quote!(#ident: <#ty>::from_properties(properties)?); + } + + let ty = &field.ty; + let default = typed_default(field); + + if let Some(parse_properties_with) = &field.parse_properties_with { + let key = field.key.as_ref().expect("exact-key fields have a key"); + let parse = match &field.additional_key { + Some(additional_key) => { + quote!(#parse_properties_with(properties, #key, #additional_key, #default)) + } + None => quote!(#parse_properties_with(properties, #key, #default)), + }; + return quote! { + #ident: #parse.map_err(|error| { + format!("Invalid value for {}: {error}", #key) + })? + }; + } + + if let Some(prefix) = &field.prefix { + let value_type = field + .map_value_type + .as_ref() + .expect("prefix fields are validated as maps"); + let parse = if is_bool(value_type) { + quote!(value.to_ascii_lowercase().parse::<#value_type>()) + } else { + quote!(value.parse::<#value_type>()) + }; + return quote! { + #ident: { + let parsed = properties + .iter() + .filter_map(|(key, value)| { + key.strip_prefix(#prefix).map(|suffix| { + #parse + .map(|parsed| (suffix.to_string(), parsed)) + .map_err(|error| format!("Invalid value for {key}: {error}")) + }) + }) + .collect::<::std::result::Result< + ::std::collections::HashMap<_, _>, + ::std::string::String, + >>()?; + if parsed.is_empty() { + #default + } else { + parsed + } + } + }; + } + + let key = field.key.as_ref().expect("exact-key fields have a key"); + let parse = match (&field.parse_with, &field.option_inner_type) { + (Some(parse_with), _) => quote! { + #parse_with(value).map_err(|error| { + format!("Invalid value for {}: {error}", #key) + })? + }, + (None, Some(inner_type)) if is_bool(inner_type) => quote! { + Some(value.to_ascii_lowercase().parse::<#inner_type>().map_err(|error| { + format!("Invalid value for {}: {error}", #key) + })?) + }, + (None, Some(inner_type)) => quote! { + Some(value.parse::<#inner_type>().map_err(|error| { + format!("Invalid value for {}: {error}", #key) + })?) + }, + (None, None) if is_bool(ty) => quote! { + value.to_ascii_lowercase().parse::<#ty>().map_err(|error| { + format!("Invalid value for {}: {error}", #key) + })? + }, + (None, None) => quote! { + value.parse::<#ty>().map_err(|error| { + format!("Invalid value for {}: {error}", #key) + })? + }, + }; + + quote! { + #ident: match properties.get(#key) { + Some(value) => #parse, + None => #default, + } + } +} + +fn typed_default(field: &PropertyField) -> TokenStream2 { + let ty = &field.ty; + let default = default_value( + field.default.as_ref().expect("leaf fields have defaults"), + ty, + ); + quote!({ + let value: #ty = #default; + value + }) +} + +fn default_value(default: &Expr, ty: &Type) -> TokenStream2 { + if matches!( + default, + Expr::Lit(ExprLit { + lit: Lit::Str(_), + .. + }) | Expr::Path(_) + ) { + quote!(::std::convert::Into::<#ty>::into(#default)) + } else { + quote!(#default) + } +} + +fn option_inner_type(ty: &Type) -> Option { + let Type::Path(type_path) = ty else { + return None; + }; + + let segment = type_path.path.segments.last()?; + if segment.ident != "Option" { + return None; + } + + let PathArguments::AngleBracketed(arguments) = &segment.arguments else { + return None; + }; + let Some(GenericArgument::Type(inner_type)) = arguments.args.first() else { + return None; + }; + + Some(inner_type.clone()) +} + +fn hash_map_value_type(ty: &Type) -> Option { + let Type::Path(type_path) = ty else { + return None; + }; + + let segment = type_path.path.segments.last()?; + if segment.ident != "HashMap" { + return None; + } + + let PathArguments::AngleBracketed(arguments) = &segment.arguments else { + return None; + }; + let mut arguments = arguments.args.iter(); + let Some(GenericArgument::Type(key_type)) = arguments.next() else { + return None; + }; + let Some(GenericArgument::Type(value_type)) = arguments.next() else { + return None; + }; + if !is_named_type(key_type, "String") { + return None; + } + + Some(value_type.clone()) +} + +fn is_bool(ty: &Type) -> bool { + is_named_type(ty, "bool") +} + +fn is_named_type(ty: &Type, name: &str) -> bool { + let Type::Path(type_path) = ty else { + return false; + }; + + type_path + .path + .segments + .last() + .is_some_and(|segment| segment.ident == name) +} + +fn write_field(field: &PropertyField) -> TokenStream2 { + let ident = &field.ident; + if field.nested { + return quote! { + self.#ident.write_properties(properties); + }; + } + + let default = typed_default(field); + + if let Some(write_properties_with) = &field.write_properties_with { + let key = field.key.as_ref().expect("exact-key fields have a key"); + let write = match &field.additional_key { + Some(additional_key) => { + quote!(#write_properties_with(&self.#ident, properties, #key, #additional_key, &#default)) + } + None => quote!(#write_properties_with(&self.#ident, properties, #key, &#default)), + }; + return quote! { + #write; + }; + } + + if let Some(prefix) = &field.prefix { + return quote! { + properties.retain(|key, _| !key.starts_with(#prefix)); + if self.#ident != #default { + for (suffix, value) in &self.#ident { + let key = format!("{}{}", #prefix, suffix); + properties.insert(key, ::std::string::ToString::to_string(value)); + } + } + }; + } + + let key = field.key.as_ref().expect("exact-key fields have a key"); + let value = match (&field.serialize_with, &field.option_inner_type) { + (Some(serialize_with), _) => quote!(#serialize_with(&self.#ident)), + (None, Some(_)) => quote!(::std::string::ToString::to_string( + self.#ident.as_ref().expect("checked is_some above") + )), + (None, None) => quote!(::std::string::ToString::to_string(&self.#ident)), + }; + let insert = if field.option_inner_type.is_some() { + quote! { + if self.#ident != #default && self.#ident.is_some() { + properties.insert((#key).to_string(), #value); + } + } + } else { + quote! { + if self.#ident != #default { + properties.insert((#key).to_string(), #value); + } + } + }; + + quote! { + properties.remove(#key); + #insert + } +} diff --git a/crates/property-macro/tests/properties.rs b/crates/property-macro/tests/properties.rs new file mode 100644 index 0000000000..ae33198a74 --- /dev/null +++ b/crates/property-macro/tests/properties.rs @@ -0,0 +1,304 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use std::collections::HashMap; + +use iceberg_property_macro::Properties; + +const RETRIES: &str = "commit.retry.num-retries"; +const OWNER: &str = "owner"; +const FORMAT: &str = "write.format.default"; +const FANOUT_ENABLED: &str = "write.fanout.enabled"; +const COLUMN_FPP_PREFIX: &str = "write.parquet.bloom-filter-fpp.column."; +const WIDTH: &str = "dimensions.width"; +const HEIGHT: &str = "dimensions.height"; + +fn parse_dimensions( + properties: &HashMap, + width_key: &str, + height_key: &str, + default: (u64, u64), +) -> Result<(u64, u64), String> { + let parse = |property_key: &str, default| { + properties + .get(property_key) + .map(|value| value.parse::().map_err(|error| error.to_string())) + .transpose() + .map(|value| value.unwrap_or(default)) + }; + + Ok((parse(width_key, default.0)?, parse(height_key, default.1)?)) +} + +fn write_dimensions( + dimensions: &(u64, u64), + properties: &mut HashMap, + width_key: &str, + height_key: &str, + default: &(u64, u64), +) { + properties.remove(width_key); + properties.remove(height_key); + if dimensions != default { + properties.insert(width_key.to_string(), dimensions.0.to_string()); + properties.insert(height_key.to_string(), dimensions.1.to_string()); + } +} + +#[derive(Debug, Properties)] +struct TestProperties { + #[key(RETRIES)] + #[default(4)] + pub retries: u64, + + #[key(OWNER)] + #[default(None)] + pub owner: Option, + + #[key(FORMAT)] + #[default("parquet")] + pub format: String, + + #[key(FANOUT_ENABLED)] + #[default(true)] + pub fanout_enabled: bool, + + #[prefix(COLUMN_FPP_PREFIX)] + #[default(HashMap::new())] + pub column_fpp: HashMap, + + #[key(WIDTH)] + #[additional_key(HEIGHT)] + #[default((640, 480))] + #[parse_properties_with(parse_dimensions)] + #[write_properties_with(write_dimensions)] + pub dimensions: (u64, u64), +} + +#[test] +fn reads_defaults_and_overrides() { + let defaults = TestProperties::from_properties(&HashMap::new()).unwrap(); + assert_eq!(defaults.retries, 4); + assert_eq!(defaults.owner, None); + assert_eq!(defaults.format, "parquet"); + assert!(defaults.fanout_enabled); + assert!(defaults.column_fpp.is_empty()); + assert_eq!(defaults.dimensions, (640, 480)); + + let properties = HashMap::from([ + (RETRIES.to_string(), "8".to_string()), + (OWNER.to_string(), "iceberg".to_string()), + (FORMAT.to_string(), "orc".to_string()), + (FANOUT_ENABLED.to_string(), "FALSE".to_string()), + (format!("{COLUMN_FPP_PREFIX}id"), "0.01".to_string()), + (WIDTH.to_string(), "1920".to_string()), + (HEIGHT.to_string(), "1080".to_string()), + ]); + let parsed = TestProperties::from_properties(&properties).unwrap(); + + assert_eq!(parsed.retries, 8); + assert_eq!(parsed.owner.as_deref(), Some("iceberg")); + assert_eq!(parsed.format, "orc"); + assert!(!parsed.fanout_enabled); + assert_eq!(parsed.column_fpp["id"], 0.01); + assert_eq!(parsed.dimensions, (1920, 1080)); +} + +#[test] +fn writes_overrides_and_preserves_unrelated_properties() { + let parsed = TestProperties::from_properties(&HashMap::from([ + (RETRIES.to_string(), "8".to_string()), + (OWNER.to_string(), "iceberg".to_string()), + (FORMAT.to_string(), "orc".to_string()), + (FANOUT_ENABLED.to_string(), "false".to_string()), + (format!("{COLUMN_FPP_PREFIX}id"), "0.01".to_string()), + (WIDTH.to_string(), "1920".to_string()), + (HEIGHT.to_string(), "1080".to_string()), + ])) + .unwrap(); + let mut properties = HashMap::from([("unrelated".to_string(), "value".to_string())]); + + parsed.write_properties(&mut properties); + + assert_eq!(properties[RETRIES], "8"); + assert_eq!(properties[OWNER], "iceberg"); + assert_eq!(properties[FORMAT], "orc"); + assert_eq!(properties[FANOUT_ENABLED], "false"); + assert_eq!(properties[&format!("{COLUMN_FPP_PREFIX}id")], "0.01"); + assert_eq!(properties[WIDTH], "1920"); + assert_eq!(properties[HEIGHT], "1080"); + assert_eq!(properties["unrelated"], "value"); +} + +#[test] +fn writing_defaults_removes_modeled_properties() { + let defaults = TestProperties::from_properties(&HashMap::new()).unwrap(); + let mut properties = HashMap::from([ + (RETRIES.to_string(), "8".to_string()), + (OWNER.to_string(), "iceberg".to_string()), + (FORMAT.to_string(), "orc".to_string()), + (FANOUT_ENABLED.to_string(), "false".to_string()), + (format!("{COLUMN_FPP_PREFIX}id"), "0.01".to_string()), + (WIDTH.to_string(), "1920".to_string()), + (HEIGHT.to_string(), "1080".to_string()), + ("unrelated".to_string(), "value".to_string()), + ]); + + defaults.write_properties(&mut properties); + + assert_eq!( + properties, + HashMap::from([("unrelated".to_string(), "value".to_string())]) + ); +} + +#[test] +fn reports_the_property_with_an_invalid_value() { + let numeric_error = TestProperties::from_properties(&HashMap::from([( + RETRIES.to_string(), + "many".to_string(), + )])) + .unwrap_err(); + assert!(numeric_error.contains(RETRIES)); + + let boolean_error = TestProperties::from_properties(&HashMap::from([( + FANOUT_ENABLED.to_string(), + "sometimes".to_string(), + )])) + .unwrap_err(); + assert!(boolean_error.contains(FANOUT_ENABLED)); + + let prefixed_key = format!("{COLUMN_FPP_PREFIX}id"); + let prefix_error = TestProperties::from_properties(&HashMap::from([( + prefixed_key.clone(), + "low".to_string(), + )])) + .unwrap_err(); + assert!(prefix_error.contains(&prefixed_key)); +} + +#[derive(Clone, Debug, Properties)] +struct CommitProperties { + #[key = "commit.retry.num-retries"] + #[default = 4] + pub num_retries: u64, +} + +#[derive(Debug, Properties)] +struct NestedProperties { + #[nested] + pub commit: CommitProperties, +} + +#[test] +fn nested_properties_use_a_flat_property_map() { + let mut properties = NestedProperties::from_properties(&HashMap::new()).unwrap(); + assert_eq!(properties.commit.num_retries, 4); + + properties.commit.num_retries = 9; + let mut written = HashMap::new(); + properties.write_properties(&mut written); + assert_eq!( + written, + HashMap::from([("commit.retry.num-retries".to_string(), "9".to_string())]) + ); + + let decoded = NestedProperties::from_properties(&written).unwrap(); + assert_eq!(decoded.commit.num_retries, 9); +} + +fn parse_non_empty(value: &str) -> Result { + let value = value.trim(); + if value.is_empty() { + Err("value must not be empty") + } else { + Ok(value.to_string()) + } +} + +fn serialize_trimmed(value: &str) -> String { + value.trim().to_string() +} + +#[derive(Debug, Properties)] +struct ValidatedProperties { + #[key = "location"] + #[default = "default"] + #[parse_with(parse_non_empty)] + #[serialize_with(serialize_trimmed)] + location: String, +} + +#[test] +fn custom_single_value_hooks_can_validate_and_normalize() { + let parsed = ValidatedProperties::from_properties(&HashMap::from([( + "location".to_string(), + " path ".to_string(), + )])) + .unwrap(); + assert_eq!(parsed.location, "path"); + + let error = ValidatedProperties::from_properties(&HashMap::from([( + "location".to_string(), + " ".to_string(), + )])) + .unwrap_err(); + assert_eq!(error, "Invalid value for location: value must not be empty"); + + let properties = ValidatedProperties { + location: " normalized ".to_string(), + }; + let mut written = HashMap::new(); + properties.write_properties(&mut written); + assert_eq!(written["location"], "normalized"); +} + +mod accessor_fixture { + use iceberg_property_macro::Properties; + + #[derive(Debug, Default, Properties)] + pub struct AccessorProperties { + #[doc = "A property with public read and write access."] + #[property(key = "public.both", default = 0, pub(getter), pub(setter))] + both: u64, + + #[property(key = "public.getter", default = "", pub(getter))] + getter_only: String, + + #[property(key = "public.setter", default = false, pub(setter))] + setter_only: bool, + } + + impl AccessorProperties { + pub fn setter_only_for_test(&self) -> bool { + self.setter_only + } + } +} + +#[test] +fn coexists_with_derived_default_and_generates_opt_in_accessors() { + let mut properties = accessor_fixture::AccessorProperties::default(); + + assert_eq!(*properties.both(), 0); + properties.set_both(2); + assert_eq!(*properties.both(), 2); + assert_eq!(properties.getter_only(), ""); + + properties.set_setter_only(true); + assert!(properties.setter_only_for_test()); +} From a1fd056965d0c9fd41ad6e481668a6df94db060c Mon Sep 17 00:00:00 2001 From: Renjie Liu Date: Thu, 6 Aug 2026 15:38:15 +0800 Subject: [PATCH 2/5] Ready --- crates/property-macro/README.md | 235 ++++++++++++++++++++-- crates/property-macro/src/lib.rs | 4 +- crates/property-macro/src/properties.rs | 147 +++++++++----- crates/property-macro/tests/properties.rs | 96 ++++++--- 4 files changed, 395 insertions(+), 87 deletions(-) diff --git a/crates/property-macro/README.md b/crates/property-macro/README.md index aab8a9c9c5..4ab30c126f 100644 --- a/crates/property-macro/README.md +++ b/crates/property-macro/README.md @@ -23,26 +23,229 @@ from a flat `HashMap`. It deliberately does not implement `Default`, `Serialize`, `Deserialize`, or any other trait. -Leaf fields declare a property key and the default used when that key is absent. -Public accessors are opt-in: +## Generated methods + +For every annotated struct, `#[derive(Properties)]` generates these inherent +methods: + +```text +impl MyProperties { + pub fn from_properties( + properties: &HashMap, + ) -> Result; + + pub fn write_properties( + &self, + properties: &mut HashMap, + ) -> Result<(), String>; +} +``` + +`from_properties` parses every modeled property, uses its annotated default +when absent, and returns an error containing the primary property key when a +value is invalid. Unknown keys are ignored. + +`write_properties` updates an existing map. It removes modeled keys whose +values equal their annotated defaults, writes non-default values as strings, +and preserves unknown keys. It returns an error when a custom +`serialize_with` or `serialize_properties_with` hook fails. + +## Complete example + +This example exercises the complete generated API: exact keys and defaults, +optional values, case-insensitive booleans, prefixed maps, nested property +groups, custom single-value conversion, custom multi-key conversion, public +accessors, contextual errors, and writing into an existing property map. ```rust +use std::collections::HashMap; + use iceberg_property_macro::Properties; -#[derive(Default, Properties)] -struct WriteProperties { +const RETRIES: &str = "commit.retry.num-retries"; +const OWNER: &str = "owner"; +const FANOUT: &str = "write.fanout.enabled"; +const COLUMN_FPP_PREFIX: &str = "write.parquet.bloom-filter-fpp.column."; +const LOCATION: &str = "write.data.path"; +const WIDTH: &str = "dimensions.width"; +const HEIGHT: &str = "dimensions.height"; +const DEPTH: &str = "dimensions.depth"; + +fn parse_location(value: &str) -> Result { + let location = value.trim().trim_end_matches('/'); + if location.is_empty() { + Err("location must not be empty") + } else { + Ok(location.to_string()) + } +} + +fn serialize_location(value: &str) -> Result { + let location = value.trim().trim_end_matches('/'); + if location.is_empty() { + Err("location must not be empty") + } else { + Ok(location.to_string()) + } +} + +fn parse_dimensions( + properties: &HashMap, + width_key: &str, + additional_keys: &[&str], + default: (u64, u64, u64), +) -> Result<(u64, u64, u64), String> { + if additional_keys.len() != 2 { + return Err("dimensions require height and depth keys".to_string()); + } + let parse = |key: &str, default| { + properties + .get(key) + .map(|value| value.parse::().map_err(|error| error.to_string())) + .transpose() + .map(|value| value.unwrap_or(default)) + }; + + Ok(( + parse(width_key, default.0)?, + parse(additional_keys[0], default.1)?, + parse(additional_keys[1], default.2)?, + )) +} + +fn serialize_dimensions( + dimensions: &(u64, u64, u64), + properties: &mut HashMap, + width_key: &str, + additional_keys: &[&str], + default: &(u64, u64, u64), +) -> Result<(), String> { + if additional_keys.len() != 2 { + return Err("dimensions require height and depth keys".to_string()); + } + if dimensions.0 == 0 || dimensions.1 == 0 || dimensions.2 == 0 { + return Err("dimensions must be positive".to_string()); + } + properties.remove(width_key); + properties.remove(additional_keys[0]); + properties.remove(additional_keys[1]); + if dimensions != default { + properties.insert(width_key.to_string(), dimensions.0.to_string()); + properties.insert(additional_keys[0].to_string(), dimensions.1.to_string()); + properties.insert(additional_keys[1].to_string(), dimensions.2.to_string()); + } + Ok(()) +} + +#[derive(Debug, Properties)] +struct CommitProperties { #[property( - key = "commit.retry.num-retries", - default = 0, + key = RETRIES, + default = 4, pub(getter), pub(setter) )] - retries: u64, + retries: usize, } -let mut properties = WriteProperties::default(); -properties.set_retries(4); -assert_eq!(*properties.retries(), 4); +#[derive(Debug, Properties)] +struct TableLikeProperties { + // Nested groups still read and write the same flat property map. + #[property(nested)] + commit: CommitProperties, + + // Option distinguishes an absent property from a present value. + #[property(key = OWNER, default = None, pub(getter), pub(setter))] + owner: Option, + + // Boolean values are parsed case-insensitively. + #[property(key = FANOUT, default = true, pub(getter))] + fanout_enabled: bool, + + // A prefix captures suffix/value pairs into a typed map. + #[property(prefix = COLUMN_FPP_PREFIX, default = HashMap::new(), pub(getter))] + column_fpp: HashMap, + + // Single-key hooks provide validation and custom string conversion. + #[property( + key = LOCATION, + default = "warehouse", + parse_with = parse_location, + serialize_with = serialize_location, + pub(getter), + pub(setter) + )] + location: String, + + // Full-map hooks can model one field with multiple property keys. + #[property( + key = WIDTH, + additional_keys = [HEIGHT, DEPTH], + default = (640, 480, 320), + parse_properties_with = parse_dimensions, + serialize_properties_with = serialize_dimensions, + pub(getter) + )] + dimensions: (u64, u64, u64), +} + +fn main() -> Result<(), String> { + // An empty map uses every annotated property default. + let defaults = TableLikeProperties::from_properties(&HashMap::new())?; + assert_eq!(*defaults.commit.retries(), 4); + assert_eq!(defaults.owner(), &None); + assert!(*defaults.fanout_enabled()); + assert!(defaults.column_fpp().is_empty()); + assert_eq!(defaults.location(), "warehouse"); + assert_eq!(defaults.dimensions(), &(640, 480, 320)); + + let mut raw = HashMap::from([ + (RETRIES.to_string(), "8".to_string()), + (OWNER.to_string(), "iceberg".to_string()), + (FANOUT.to_string(), "FALSE".to_string()), + (format!("{COLUMN_FPP_PREFIX}id"), "0.01".to_string()), + (LOCATION.to_string(), " s3://bucket/table/ ".to_string()), + (WIDTH.to_string(), "1920".to_string()), + (HEIGHT.to_string(), "1080".to_string()), + (DEPTH.to_string(), "720".to_string()), + ("unmodeled".to_string(), "preserved".to_string()), + ]); + + let mut properties = TableLikeProperties::from_properties(&raw)?; + assert_eq!(*properties.commit.retries(), 8); + assert_eq!(properties.owner().as_deref(), Some("iceberg")); + assert!(!properties.fanout_enabled()); + assert_eq!(properties.column_fpp()["id"], 0.01); + assert_eq!(properties.location(), "s3://bucket/table"); + assert_eq!(properties.dimensions(), &(1920, 1080, 720)); + + // Generated setters modify private fields. Writing removes modeled values + // reset to their defaults and preserves properties the struct does not own. + properties.commit.set_retries(10); + properties.set_owner(None); + properties.set_location("s3://bucket/new-table/".to_string()); + properties.write_properties(&mut raw)?; + + assert_eq!(raw[RETRIES], "10"); + assert!(!raw.contains_key(OWNER)); + assert_eq!(raw[FANOUT], "false"); + assert_eq!(raw[&format!("{COLUMN_FPP_PREFIX}id")], "0.01"); + assert_eq!(raw[LOCATION], "s3://bucket/new-table"); + assert_eq!(raw[WIDTH], "1920"); + assert_eq!(raw[HEIGHT], "1080"); + assert_eq!(raw[DEPTH], "720"); + assert_eq!(raw["unmodeled"], "preserved"); + + // Parsing errors identify the primary property key. + let error = TableLikeProperties::from_properties(&HashMap::from([( + LOCATION.to_string(), + "/".to_string(), + )])) + .unwrap_err(); + assert!(error.contains(LOCATION)); + + Ok(()) +} ``` The annotated property default is independent of the value produced by a @@ -83,7 +286,9 @@ mod property_map { S: Serializer, { let mut properties = HashMap::new(); - value.write_properties(&mut properties); + value + .write_properties(&mut properties) + .map_err(serde::ser::Error::custom)?; properties.serialize(serializer) } @@ -125,11 +330,11 @@ struct while keeping the property map flat. `#[parse_with(...)]` and latter name refers to conversion into a property string and does not require Serde. -`#[parse_properties_with(...)]` and `#[write_properties_with(...)]` receive the +`#[parse_properties_with(...)]` and `#[serialize_properties_with(...)]` receive the complete property map for fields represented by more than one key. -`#[additional_key(...)]` supplies a second key to those hooks. Custom write -hooks receive the field default and are responsible for removing or omitting -default-valued properties. +`#[additional_keys(...)]` supplies a list of secondary keys to those hooks. +Custom serialization hooks return `Result`, receive the field default, and are +responsible for removing or omitting default-valued properties. Boolean property values are parsed case-insensitively. Other values require `FromStr` and `ToString` unless custom conversion hooks are supplied. Leaf diff --git a/crates/property-macro/src/lib.rs b/crates/property-macro/src/lib.rs index 018a7c1f04..11b1746d76 100644 --- a/crates/property-macro/src/lib.rs +++ b/crates/property-macro/src/lib.rs @@ -27,14 +27,14 @@ mod properties; Properties, attributes( key, - additional_key, + additional_keys, prefix, nested, default, parse_with, serialize_with, parse_properties_with, - write_properties_with, + serialize_properties_with, property ) )] diff --git a/crates/property-macro/src/properties.rs b/crates/property-macro/src/properties.rs index 564a52bf49..f808195c39 100644 --- a/crates/property-macro/src/properties.rs +++ b/crates/property-macro/src/properties.rs @@ -28,14 +28,14 @@ struct PropertyField { ident: Ident, ty: Type, key: Option, - additional_key: Option, + additional_keys: Option>, prefix: Option, nested: bool, default: Option, parse_with: Option, serialize_with: Option, parse_properties_with: Option, - write_properties_with: Option, + serialize_properties_with: Option, option_inner_type: Option, map_value_type: Option, public_getter: bool, @@ -50,28 +50,28 @@ enum PublicAccessor { enum PropertyOption { Key(Expr), - AdditionalKey(Expr), + AdditionalKeys(Vec), Prefix(Expr), Nested, Default(Expr), ParseWith(Path), SerializeWith(Path), ParsePropertiesWith(Path), - WritePropertiesWith(Path), + SerializePropertiesWith(Path), Accessor(PublicAccessor), } #[derive(Default)] struct PropertyOptions { key: Option, - additional_key: Option, + additional_keys: Option>, prefix: Option, nested: bool, default: Option, parse_with: Option, serialize_with: Option, parse_properties_with: Option, - write_properties_with: Option, + serialize_properties_with: Option, public_getter: bool, public_setter: bool, } @@ -110,7 +110,9 @@ impl Parse for PropertyOption { let expression = input.parse::()?; match option_name.as_str() { "key" => Ok(Self::Key(expression)), - "additional_key" => Ok(Self::AdditionalKey(expression)), + "additional_keys" => { + expression_list(expression, "additional_keys").map(Self::AdditionalKeys) + } "prefix" => Ok(Self::Prefix(expression)), "default" => Ok(Self::Default(expression)), "parse_with" => expression_path(expression, "parse_with").map(Self::ParseWith), @@ -120,9 +122,8 @@ impl Parse for PropertyOption { "parse_properties_with" => { expression_path(expression, "parse_properties_with").map(Self::ParsePropertiesWith) } - "write_properties_with" => { - expression_path(expression, "write_properties_with").map(Self::WritePropertiesWith) - } + "serialize_properties_with" => expression_path(expression, "serialize_properties_with") + .map(Self::SerializePropertiesWith), _ => Err(Error::new_spanned(name, "unknown property option")), } } @@ -162,7 +163,7 @@ pub(crate) fn expand_properties(input: DeriveInput) -> syn::Result impl #impl_generics #struct_name #type_generics #where_clause { #(#accessors)* - pub(crate) fn from_properties( + pub fn from_properties( properties: &::std::collections::HashMap< ::std::string::String, ::std::string::String, @@ -173,14 +174,15 @@ pub(crate) fn expand_properties(input: DeriveInput) -> syn::Result }) } - pub(crate) fn write_properties( + pub fn write_properties( &self, properties: &mut ::std::collections::HashMap< ::std::string::String, ::std::string::String, >, - ) { + ) -> ::std::result::Result<(), ::std::string::String> { #(#property_writes)* + Ok(()) } } }) @@ -198,11 +200,11 @@ fn parse_property_field(field: &Field) -> syn::Result { field, "key", )?; - let additional_key = merge_attribute_option( - attribute_expression_value(&field.attrs, "additional_key")?, - property_options.additional_key, + let additional_keys = merge_attribute_option( + attribute_expression_list(&field.attrs, "additional_keys")?, + property_options.additional_keys, field, - "additional_key", + "additional_keys", )?; let prefix = merge_attribute_option( attribute_expression_value(&field.attrs, "prefix")?, @@ -271,28 +273,28 @@ fn parse_property_field(field: &Field) -> syn::Result { field, "parse_properties_with", )?; - let write_properties_with = merge_attribute_option( - attribute_path_value(&field.attrs, "write_properties_with")?, - property_options.write_properties_with, + let serialize_properties_with = merge_attribute_option( + attribute_path_value(&field.attrs, "serialize_properties_with")?, + property_options.serialize_properties_with, field, - "write_properties_with", + "serialize_properties_with", )?; - if additional_key.is_some() + if additional_keys.is_some() && parse_properties_with.is_none() - && write_properties_with.is_none() + && serialize_properties_with.is_none() { return Err(Error::new_spanned( field, - "#[additional_key(...)] requires parse_properties_with or write_properties_with", + "#[additional_keys(...)] requires parse_properties_with or serialize_properties_with", )); } if (prefix.is_some() || nested) - && (additional_key.is_some() + && (additional_keys.is_some() || parse_with.is_some() || serialize_with.is_some() || parse_properties_with.is_some() - || write_properties_with.is_some()) + || serialize_properties_with.is_some()) { return Err(Error::new_spanned( field, @@ -305,10 +307,10 @@ fn parse_property_field(field: &Field) -> syn::Result { "fields cannot declare both parse_with and parse_properties_with", )); } - if serialize_with.is_some() && write_properties_with.is_some() { + if serialize_with.is_some() && serialize_properties_with.is_some() { return Err(Error::new_spanned( field, - "fields cannot declare both serialize_with and write_properties_with", + "fields cannot declare both serialize_with and serialize_properties_with", )); } @@ -316,14 +318,14 @@ fn parse_property_field(field: &Field) -> syn::Result { ident, ty: field.ty.clone(), key, - additional_key, + additional_keys, prefix, nested, default, parse_with, serialize_with, parse_properties_with, - write_properties_with, + serialize_properties_with, option_inner_type: option_inner_type(&field.ty), map_value_type, public_getter: property_options.public_getter, @@ -357,11 +359,11 @@ fn property_options(attributes: &[Attribute]) -> syn::Result { PropertyOption::Key(value) => { set_property_option(&mut options.key, value, attribute, "key")? } - PropertyOption::AdditionalKey(value) => set_property_option( - &mut options.additional_key, + PropertyOption::AdditionalKeys(value) => set_property_option( + &mut options.additional_keys, value, attribute, - "additional_key", + "additional_keys", )?, PropertyOption::Prefix(value) => { set_property_option(&mut options.prefix, value, attribute, "prefix")? @@ -393,11 +395,11 @@ fn property_options(attributes: &[Attribute]) -> syn::Result { attribute, "parse_properties_with", )?, - PropertyOption::WritePropertiesWith(value) => set_property_option( - &mut options.write_properties_with, + PropertyOption::SerializePropertiesWith(value) => set_property_option( + &mut options.serialize_properties_with, value, attribute, - "write_properties_with", + "serialize_properties_with", )?, PropertyOption::Accessor(accessor) => { let selected = match accessor { @@ -505,6 +507,37 @@ fn attribute_expression_value(attributes: &[Attribute], name: &str) -> syn::Resu } } +fn attribute_expression_list( + attributes: &[Attribute], + name: &str, +) -> syn::Result>> { + let Some(attribute) = find_attribute(attributes, name)? else { + return Ok(None); + }; + + let expressions = match &attribute.meta { + Meta::NameValue(name_value) => expression_list(name_value.value.clone(), name)?, + Meta::List(_) => attribute + .parse_args_with(Punctuated::::parse_terminated)? + .into_iter() + .collect(), + _ => { + return Err(Error::new_spanned( + attribute, + format!("{name} must contain a non-empty list of keys"), + )); + } + }; + + if expressions.is_empty() { + return Err(Error::new_spanned( + attribute, + format!("{name} must contain at least one key"), + )); + } + Ok(Some(expressions)) +} + fn attribute_path_value(attributes: &[Attribute], name: &str) -> syn::Result> { let Some(expression) = attribute_expression_value(attributes, name)? else { return Ok(None); @@ -523,6 +556,22 @@ fn expression_path(expression: Expr, name: &str) -> syn::Result { } } +fn expression_list(expression: Expr, name: &str) -> syn::Result> { + let Expr::Array(array) = expression else { + return Err(Error::new_spanned( + expression, + format!("{name} must be an array of keys"), + )); + }; + if array.elems.is_empty() { + return Err(Error::new_spanned( + array, + format!("{name} must contain at least one key"), + )); + } + Ok(array.elems.into_iter().collect()) +} + fn find_attribute<'a>( attributes: &'a [Attribute], name: &str, @@ -552,9 +601,9 @@ fn parse_field(field: &PropertyField) -> TokenStream2 { if let Some(parse_properties_with) = &field.parse_properties_with { let key = field.key.as_ref().expect("exact-key fields have a key"); - let parse = match &field.additional_key { - Some(additional_key) => { - quote!(#parse_properties_with(properties, #key, #additional_key, #default)) + let parse = match &field.additional_keys { + Some(additional_keys) => { + quote!(#parse_properties_with(properties, #key, &[#(#additional_keys),*], #default)) } None => quote!(#parse_properties_with(properties, #key, #default)), }; @@ -729,22 +778,24 @@ fn write_field(field: &PropertyField) -> TokenStream2 { let ident = &field.ident; if field.nested { return quote! { - self.#ident.write_properties(properties); + self.#ident.write_properties(properties)?; }; } let default = typed_default(field); - if let Some(write_properties_with) = &field.write_properties_with { + if let Some(serialize_properties_with) = &field.serialize_properties_with { let key = field.key.as_ref().expect("exact-key fields have a key"); - let write = match &field.additional_key { - Some(additional_key) => { - quote!(#write_properties_with(&self.#ident, properties, #key, #additional_key, &#default)) + let serialize = match &field.additional_keys { + Some(additional_keys) => { + quote!(#serialize_properties_with(&self.#ident, properties, #key, &[#(#additional_keys),*], &#default)) } - None => quote!(#write_properties_with(&self.#ident, properties, #key, &#default)), + None => quote!(#serialize_properties_with(&self.#ident, properties, #key, &#default)), }; return quote! { - #write; + #serialize.map_err(|error| { + format!("Failed to serialize {}: {error}", #key) + })?; }; } @@ -762,7 +813,9 @@ fn write_field(field: &PropertyField) -> TokenStream2 { let key = field.key.as_ref().expect("exact-key fields have a key"); let value = match (&field.serialize_with, &field.option_inner_type) { - (Some(serialize_with), _) => quote!(#serialize_with(&self.#ident)), + (Some(serialize_with), _) => quote!(#serialize_with(&self.#ident).map_err(|error| { + format!("Failed to serialize {}: {error}", #key) + })?), (None, Some(_)) => quote!(::std::string::ToString::to_string( self.#ident.as_ref().expect("checked is_some above") )), diff --git a/crates/property-macro/tests/properties.rs b/crates/property-macro/tests/properties.rs index ae33198a74..8c75f2d635 100644 --- a/crates/property-macro/tests/properties.rs +++ b/crates/property-macro/tests/properties.rs @@ -26,13 +26,17 @@ const FANOUT_ENABLED: &str = "write.fanout.enabled"; const COLUMN_FPP_PREFIX: &str = "write.parquet.bloom-filter-fpp.column."; const WIDTH: &str = "dimensions.width"; const HEIGHT: &str = "dimensions.height"; +const DEPTH: &str = "dimensions.depth"; fn parse_dimensions( properties: &HashMap, width_key: &str, - height_key: &str, - default: (u64, u64), -) -> Result<(u64, u64), String> { + additional_keys: &[&str], + default: (u64, u64, u64), +) -> Result<(u64, u64, u64), String> { + if additional_keys.len() != 2 { + return Err("dimensions require height and depth keys".to_string()); + } let parse = |property_key: &str, default| { properties .get(property_key) @@ -41,22 +45,35 @@ fn parse_dimensions( .map(|value| value.unwrap_or(default)) }; - Ok((parse(width_key, default.0)?, parse(height_key, default.1)?)) + Ok(( + parse(width_key, default.0)?, + parse(additional_keys[0], default.1)?, + parse(additional_keys[1], default.2)?, + )) } -fn write_dimensions( - dimensions: &(u64, u64), +fn serialize_dimensions( + dimensions: &(u64, u64, u64), properties: &mut HashMap, width_key: &str, - height_key: &str, - default: &(u64, u64), -) { + additional_keys: &[&str], + default: &(u64, u64, u64), +) -> Result<(), String> { + if additional_keys.len() != 2 { + return Err("dimensions require height and depth keys".to_string()); + } + if dimensions.0 == 0 || dimensions.1 == 0 || dimensions.2 == 0 { + return Err("dimensions must be positive".to_string()); + } properties.remove(width_key); - properties.remove(height_key); + properties.remove(additional_keys[0]); + properties.remove(additional_keys[1]); if dimensions != default { properties.insert(width_key.to_string(), dimensions.0.to_string()); - properties.insert(height_key.to_string(), dimensions.1.to_string()); + properties.insert(additional_keys[0].to_string(), dimensions.1.to_string()); + properties.insert(additional_keys[1].to_string(), dimensions.2.to_string()); } + Ok(()) } #[derive(Debug, Properties)] @@ -82,11 +99,11 @@ struct TestProperties { pub column_fpp: HashMap, #[key(WIDTH)] - #[additional_key(HEIGHT)] - #[default((640, 480))] + #[additional_keys(HEIGHT, DEPTH)] + #[default((640, 480, 320))] #[parse_properties_with(parse_dimensions)] - #[write_properties_with(write_dimensions)] - pub dimensions: (u64, u64), + #[serialize_properties_with(serialize_dimensions)] + pub dimensions: (u64, u64, u64), } #[test] @@ -97,7 +114,7 @@ fn reads_defaults_and_overrides() { assert_eq!(defaults.format, "parquet"); assert!(defaults.fanout_enabled); assert!(defaults.column_fpp.is_empty()); - assert_eq!(defaults.dimensions, (640, 480)); + assert_eq!(defaults.dimensions, (640, 480, 320)); let properties = HashMap::from([ (RETRIES.to_string(), "8".to_string()), @@ -107,6 +124,7 @@ fn reads_defaults_and_overrides() { (format!("{COLUMN_FPP_PREFIX}id"), "0.01".to_string()), (WIDTH.to_string(), "1920".to_string()), (HEIGHT.to_string(), "1080".to_string()), + (DEPTH.to_string(), "720".to_string()), ]); let parsed = TestProperties::from_properties(&properties).unwrap(); @@ -115,7 +133,7 @@ fn reads_defaults_and_overrides() { assert_eq!(parsed.format, "orc"); assert!(!parsed.fanout_enabled); assert_eq!(parsed.column_fpp["id"], 0.01); - assert_eq!(parsed.dimensions, (1920, 1080)); + assert_eq!(parsed.dimensions, (1920, 1080, 720)); } #[test] @@ -128,11 +146,12 @@ fn writes_overrides_and_preserves_unrelated_properties() { (format!("{COLUMN_FPP_PREFIX}id"), "0.01".to_string()), (WIDTH.to_string(), "1920".to_string()), (HEIGHT.to_string(), "1080".to_string()), + (DEPTH.to_string(), "720".to_string()), ])) .unwrap(); let mut properties = HashMap::from([("unrelated".to_string(), "value".to_string())]); - parsed.write_properties(&mut properties); + parsed.write_properties(&mut properties).unwrap(); assert_eq!(properties[RETRIES], "8"); assert_eq!(properties[OWNER], "iceberg"); @@ -141,6 +160,7 @@ fn writes_overrides_and_preserves_unrelated_properties() { assert_eq!(properties[&format!("{COLUMN_FPP_PREFIX}id")], "0.01"); assert_eq!(properties[WIDTH], "1920"); assert_eq!(properties[HEIGHT], "1080"); + assert_eq!(properties[DEPTH], "720"); assert_eq!(properties["unrelated"], "value"); } @@ -155,10 +175,11 @@ fn writing_defaults_removes_modeled_properties() { (format!("{COLUMN_FPP_PREFIX}id"), "0.01".to_string()), (WIDTH.to_string(), "1920".to_string()), (HEIGHT.to_string(), "1080".to_string()), + (DEPTH.to_string(), "720".to_string()), ("unrelated".to_string(), "value".to_string()), ]); - defaults.write_properties(&mut properties); + defaults.write_properties(&mut properties).unwrap(); assert_eq!( properties, @@ -211,7 +232,7 @@ fn nested_properties_use_a_flat_property_map() { properties.commit.num_retries = 9; let mut written = HashMap::new(); - properties.write_properties(&mut written); + properties.write_properties(&mut written).unwrap(); assert_eq!( written, HashMap::from([("commit.retry.num-retries".to_string(), "9".to_string())]) @@ -230,8 +251,13 @@ fn parse_non_empty(value: &str) -> Result { } } -fn serialize_trimmed(value: &str) -> String { - value.trim().to_string() +fn serialize_trimmed(value: &str) -> Result { + let value = value.trim(); + if value.is_empty() { + Err("value must not be empty") + } else { + Ok(value.to_string()) + } } #[derive(Debug, Properties)] @@ -263,10 +289,34 @@ fn custom_single_value_hooks_can_validate_and_normalize() { location: " normalized ".to_string(), }; let mut written = HashMap::new(); - properties.write_properties(&mut written); + properties.write_properties(&mut written).unwrap(); assert_eq!(written["location"], "normalized"); } +#[test] +fn reports_custom_serialization_errors() { + let invalid_location = ValidatedProperties { + location: " ".to_string(), + }; + let error = invalid_location + .write_properties(&mut HashMap::new()) + .unwrap_err(); + assert_eq!( + error, + "Failed to serialize location: value must not be empty" + ); + + let mut invalid_dimensions = TestProperties::from_properties(&HashMap::new()).unwrap(); + invalid_dimensions.dimensions = (0, 480, 320); + let error = invalid_dimensions + .write_properties(&mut HashMap::new()) + .unwrap_err(); + assert_eq!( + error, + "Failed to serialize dimensions.width: dimensions must be positive" + ); +} + mod accessor_fixture { use iceberg_property_macro::Properties; From 0068cce9872d1d269158f078b6b821d9dab68456 Mon Sep 17 00:00:00 2001 From: Renjie Liu Date: Thu, 6 Aug 2026 16:03:34 +0800 Subject: [PATCH 3/5] Fix property macro CI checks --- .typos.toml | 1 + crates/property-macro/public-api.txt | 2 ++ 2 files changed, 3 insertions(+) create mode 100644 crates/property-macro/public-api.txt diff --git a/.typos.toml b/.typos.toml index e9fa0028f5..cde21f7091 100644 --- a/.typos.toml +++ b/.typos.toml @@ -21,6 +21,7 @@ extend-ignore-identifiers-re = ["^bimap$"] [default.extend-words] AGS = "AGS" ags = "ags" +ser = "ser" [files] extend-exclude = ["**/testdata", "CHANGELOG.md", "**/public-api.txt"] diff --git a/crates/property-macro/public-api.txt b/crates/property-macro/public-api.txt new file mode 100644 index 0000000000..8edb5d0952 --- /dev/null +++ b/crates/property-macro/public-api.txt @@ -0,0 +1,2 @@ +pub mod iceberg_property_macro +pub proc macro iceberg_property_macro::#[derive(Properties)] From 4e35b2c825e1b5b903c407454f46895b707ca13b Mon Sep 17 00:00:00 2001 From: Renjie Liu Date: Fri, 7 Aug 2026 16:57:19 +0800 Subject: [PATCH 4/5] Remove write support --- crates/property-macro/README.md | 245 ++++++------------- crates/property-macro/src/lib.rs | 4 +- crates/property-macro/src/properties.rs | 260 +++++++------------- crates/property-macro/tests/properties.rs | 275 ++++++---------------- 4 files changed, 230 insertions(+), 554 deletions(-) diff --git a/crates/property-macro/README.md b/crates/property-macro/README.md index 4ab30c126f..48445bdc1e 100644 --- a/crates/property-macro/README.md +++ b/crates/property-macro/README.md @@ -19,43 +19,40 @@ # Iceberg property derive macro -`Properties` generates inherent methods for reading and writing a typed struct -from a flat `HashMap`. It deliberately does not implement -`Default`, `Serialize`, `Deserialize`, or any other trait. +`Properties` parses a typed struct from a flat `HashMap` and +can generate opt-in read-only getters. It deliberately does not generate +property-map serialization or implement `Default`, `Serialize`, `Deserialize`, +or any other trait. -## Generated methods +## Generated API -For every annotated struct, `#[derive(Properties)]` generates these inherent -methods: +For every annotated struct, `#[derive(Properties)]` generates this inherent +constructor: ```text impl MyProperties { pub fn from_properties( properties: &HashMap, ) -> Result; - - pub fn write_properties( - &self, - properties: &mut HashMap, - ) -> Result<(), String>; } ``` -`from_properties` parses every modeled property, uses its annotated default -when absent, and returns an error containing the primary property key when a -value is invalid. Unknown keys are ignored. +`from_properties` borrows the source map, parses every modeled property, and +uses its annotated default when a property is absent. Unknown keys are ignored. +An invalid value returns an error containing its primary property key. -`write_properties` updates an existing map. It removes modeled keys whose -values equal their annotated defaults, writes non-default values as strings, -and preserves unknown keys. It returns an error when a custom -`serialize_with` or `serialize_properties_with` hook fails. +Adding `pub(getter)` to a field generates an immutable accessor with the field +name. Structurally known `Copy` types return `T`; other types return `&T`. +Documentation attributes on the field are copied to the generated getter. The +macro generates no setters, backing fields, or conversion back to a property +map. ## Complete example -This example exercises the complete generated API: exact keys and defaults, -optional values, case-insensitive booleans, prefixed maps, nested property -groups, custom single-value conversion, custom multi-key conversion, public -accessors, contextual errors, and writing into an existing property map. +This example covers exact keys and defaults, optional values, case-insensitive +booleans, prefixed maps, nested groups, custom single-value parsing, custom +multi-key parsing, lists of additional keys, read-only getters, ignored unknown +keys, and contextual errors. ```rust use std::collections::HashMap; @@ -80,15 +77,6 @@ fn parse_location(value: &str) -> Result { } } -fn serialize_location(value: &str) -> Result { - let location = value.trim().trim_end_matches('/'); - if location.is_empty() { - Err("location must not be empty") - } else { - Ok(location.to_string()) - } -} - fn parse_dimensions( properties: &HashMap, width_key: &str, @@ -113,93 +101,61 @@ fn parse_dimensions( )) } -fn serialize_dimensions( - dimensions: &(u64, u64, u64), - properties: &mut HashMap, - width_key: &str, - additional_keys: &[&str], - default: &(u64, u64, u64), -) -> Result<(), String> { - if additional_keys.len() != 2 { - return Err("dimensions require height and depth keys".to_string()); - } - if dimensions.0 == 0 || dimensions.1 == 0 || dimensions.2 == 0 { - return Err("dimensions must be positive".to_string()); - } - properties.remove(width_key); - properties.remove(additional_keys[0]); - properties.remove(additional_keys[1]); - if dimensions != default { - properties.insert(width_key.to_string(), dimensions.0.to_string()); - properties.insert(additional_keys[0].to_string(), dimensions.1.to_string()); - properties.insert(additional_keys[1].to_string(), dimensions.2.to_string()); - } - Ok(()) -} - #[derive(Debug, Properties)] struct CommitProperties { - #[property( - key = RETRIES, - default = 4, - pub(getter), - pub(setter) - )] + /// Maximum number of times to retry a commit. + #[property(key = RETRIES, default = 4, pub(getter))] retries: usize, } #[derive(Debug, Properties)] struct TableLikeProperties { - // Nested groups still read and write the same flat property map. - #[property(nested)] + /// Nested groups parse from the same flat property map. + #[property(nested, pub(getter))] commit: CommitProperties, - // Option distinguishes an absent property from a present value. - #[property(key = OWNER, default = None, pub(getter), pub(setter))] + /// Option distinguishes an absent property from a present value. + #[property(key = OWNER, default = None, pub(getter))] owner: Option, - // Boolean values are parsed case-insensitively. + /// Boolean values are parsed case-insensitively. #[property(key = FANOUT, default = true, pub(getter))] fanout_enabled: bool, - // A prefix captures suffix/value pairs into a typed map. + /// A prefix captures suffix/value pairs into a typed map. #[property(prefix = COLUMN_FPP_PREFIX, default = HashMap::new(), pub(getter))] column_fpp: HashMap, - // Single-key hooks provide validation and custom string conversion. + /// A single-key parser can validate and normalize a property value. #[property( key = LOCATION, default = "warehouse", parse_with = parse_location, - serialize_with = serialize_location, - pub(getter), - pub(setter) + pub(getter) )] location: String, - // Full-map hooks can model one field with multiple property keys. + /// A full-map parser can model one field with multiple property keys. #[property( key = WIDTH, additional_keys = [HEIGHT, DEPTH], default = (640, 480, 320), parse_properties_with = parse_dimensions, - serialize_properties_with = serialize_dimensions, pub(getter) )] dimensions: (u64, u64, u64), } fn main() -> Result<(), String> { - // An empty map uses every annotated property default. let defaults = TableLikeProperties::from_properties(&HashMap::new())?; - assert_eq!(*defaults.commit.retries(), 4); + assert_eq!(defaults.commit().retries(), 4); assert_eq!(defaults.owner(), &None); - assert!(*defaults.fanout_enabled()); + assert!(defaults.fanout_enabled()); assert!(defaults.column_fpp().is_empty()); assert_eq!(defaults.location(), "warehouse"); - assert_eq!(defaults.dimensions(), &(640, 480, 320)); + assert_eq!(defaults.dimensions(), (640, 480, 320)); - let mut raw = HashMap::from([ + let raw = HashMap::from([ (RETRIES.to_string(), "8".to_string()), (OWNER.to_string(), "iceberg".to_string()), (FANOUT.to_string(), "FALSE".to_string()), @@ -208,35 +164,17 @@ fn main() -> Result<(), String> { (WIDTH.to_string(), "1920".to_string()), (HEIGHT.to_string(), "1080".to_string()), (DEPTH.to_string(), "720".to_string()), - ("unmodeled".to_string(), "preserved".to_string()), + ("unmodeled".to_string(), "ignored".to_string()), ]); - let mut properties = TableLikeProperties::from_properties(&raw)?; - assert_eq!(*properties.commit.retries(), 8); + let properties = TableLikeProperties::from_properties(&raw)?; + assert_eq!(properties.commit().retries(), 8); assert_eq!(properties.owner().as_deref(), Some("iceberg")); assert!(!properties.fanout_enabled()); assert_eq!(properties.column_fpp()["id"], 0.01); assert_eq!(properties.location(), "s3://bucket/table"); - assert_eq!(properties.dimensions(), &(1920, 1080, 720)); - - // Generated setters modify private fields. Writing removes modeled values - // reset to their defaults and preserves properties the struct does not own. - properties.commit.set_retries(10); - properties.set_owner(None); - properties.set_location("s3://bucket/new-table/".to_string()); - properties.write_properties(&mut raw)?; - - assert_eq!(raw[RETRIES], "10"); - assert!(!raw.contains_key(OWNER)); - assert_eq!(raw[FANOUT], "false"); - assert_eq!(raw[&format!("{COLUMN_FPP_PREFIX}id")], "0.01"); - assert_eq!(raw[LOCATION], "s3://bucket/new-table"); - assert_eq!(raw[WIDTH], "1920"); - assert_eq!(raw[HEIGHT], "1080"); - assert_eq!(raw[DEPTH], "720"); - assert_eq!(raw["unmodeled"], "preserved"); - - // Parsing errors identify the primary property key. + assert_eq!(properties.dimensions(), (1920, 1080, 720)); + let error = TableLikeProperties::from_properties(&HashMap::from([( LOCATION.to_string(), "/".to_string(), @@ -248,95 +186,56 @@ fn main() -> Result<(), String> { } ``` -The annotated property default is independent of the value produced by a -derived `Default` implementation. When both are used, keep them aligned. - -## Using a property map with Serde +## Using ordinary derives together -Serde's standard derives serialize a struct's fields and cannot infer the -property-map representation from `Properties` attributes. A transparent adapter -keeps that conversion explicit while allowing `Default`, `Serialize`, and -`Deserialize` to remain ordinary derives: +`Properties` does not implicitly derive other traits, so `Default`, +`Serialize`, and `Deserialize` can be selected independently and behave like +ordinary Rust derives: ```rust use std::collections::HashMap; use iceberg_property_macro::Properties; -use serde::{Deserialize, Deserializer, Serialize, Serializer}; +use serde::{Deserialize, Serialize}; -#[derive(Debug, Default, PartialEq, Properties)] -struct WriteProperties { +#[derive(Debug, Default, Serialize, Deserialize, Properties)] +struct ReadProperties { #[property( key = "commit.retry.num-retries", - default = 0, - pub(getter), - pub(setter) + default = 4, + pub(getter) )] retries: u64, - - #[property(key = "owner", default = None)] - owner: Option, } -mod property_map { - use super::*; - - pub fn serialize(value: &WriteProperties, serializer: S) -> Result - where - S: Serializer, - { - let mut properties = HashMap::new(); - value - .write_properties(&mut properties) - .map_err(serde::ser::Error::custom)?; - properties.serialize(serializer) - } - - pub fn deserialize<'de, D>(deserializer: D) -> Result - where - D: Deserializer<'de>, - { - let properties = HashMap::::deserialize(deserializer)?; - WriteProperties::from_properties(&properties).map_err(serde::de::Error::custom) - } -} - -#[derive(Debug, Default, Serialize, Deserialize)] -#[serde(transparent)] -struct PropertyDocument(#[serde(with = "property_map")] WriteProperties); - fn main() -> Result<(), Box> { - let mut document = PropertyDocument::default(); - document.0.set_retries(4); - - let json = serde_json::to_string(&document)?; - assert_eq!(json, r#"{"commit.retry.num-retries":"4"}"#); - - let decoded: PropertyDocument = serde_json::from_str(&json)?; - assert_eq!(*decoded.0.retries(), 4); + // The property annotation supplies the default used by from_properties. + let properties = ReadProperties::from_properties(&HashMap::new())?; + assert_eq!(properties.retries(), 4); + + // The ordinary Default derive uses the field's Rust default instead. + let defaults = ReadProperties::default(); + assert_eq!(defaults.retries(), 0); + + // Ordinary Serde derives use Rust field names, not property keys. + let json = serde_json::to_string(&properties)?; + assert_eq!(json, r#"{"retries":4}"#); + let decoded: ReadProperties = serde_json::from_str(r#"{"retries":7}"#)?; + assert_eq!(decoded.retries(), 7); Ok(()) } ``` Property options may be grouped under `#[property(...)]`, which avoids a collision between the standalone `#[default(...)]` helper and Rust's `Default` -derive. The standalone annotations from the original framework remain -supported. - -`#[prefix(...)]` captures a family of properties in a `HashMap`, -keyed by the suffix after the prefix. `#[nested]` embeds another `Properties` -struct while keeping the property map flat. `#[parse_with(...)]` and -`#[serialize_with(...)]` customize conversion for one exact-key field. The -latter name refers to conversion into a property string and does not require -Serde. - -`#[parse_properties_with(...)]` and `#[serialize_properties_with(...)]` receive the -complete property map for fields represented by more than one key. -`#[additional_keys(...)]` supplies a list of secondary keys to those hooks. -Custom serialization hooks return `Result`, receive the field default, and are -responsible for removing or omitting default-valued properties. - -Boolean property values are parsed case-insensitively. Other values require -`FromStr` and `ToString` unless custom conversion hooks are supplied. Leaf -fields require `PartialEq` so default values can be omitted. String-literal and -path defaults are converted into their field type with `Into`. +derive. The original standalone annotations remain supported. + +`#[prefix(...)]` requires `HashMap`. `#[nested]` embeds another +`Properties` struct while reading the same flat map. `#[parse_with(...)]` +customizes parsing for one exact-key field. `#[parse_properties_with(...)]` +receives the complete property map, and `#[additional_keys(...)]` supplies its +list of secondary keys. + +Boolean values are parsed case-insensitively. Other values require `FromStr` +unless a custom parser is supplied. String-literal and path defaults are +converted into their field type with `Into`. diff --git a/crates/property-macro/src/lib.rs b/crates/property-macro/src/lib.rs index 11b1746d76..1e7e9da4f9 100644 --- a/crates/property-macro/src/lib.rs +++ b/crates/property-macro/src/lib.rs @@ -22,7 +22,7 @@ use syn::{DeriveInput, parse_macro_input}; mod properties; -/// Derives property-map parsing, writing, and opt-in accessors for a struct. +/// Derives property-map parsing and opt-in read-only accessors for a struct. #[proc_macro_derive( Properties, attributes( @@ -32,9 +32,7 @@ mod properties; nested, default, parse_with, - serialize_with, parse_properties_with, - serialize_properties_with, property ) )] diff --git a/crates/property-macro/src/properties.rs b/crates/property-macro/src/properties.rs index f808195c39..67446a7aeb 100644 --- a/crates/property-macro/src/properties.rs +++ b/crates/property-macro/src/properties.rs @@ -16,7 +16,7 @@ // under the License. use proc_macro2::TokenStream as TokenStream2; -use quote::{format_ident, quote}; +use quote::quote; use syn::parse::{Parse, ParseStream}; use syn::punctuated::Punctuated; use syn::{ @@ -33,20 +33,14 @@ struct PropertyField { nested: bool, default: Option, parse_with: Option, - serialize_with: Option, parse_properties_with: Option, - serialize_properties_with: Option, option_inner_type: Option, map_value_type: Option, public_getter: bool, - public_setter: bool, doc_attributes: Vec, } -enum PublicAccessor { - Getter, - Setter, -} +struct PublicGetter; enum PropertyOption { Key(Expr), @@ -55,10 +49,8 @@ enum PropertyOption { Nested, Default(Expr), ParseWith(Path), - SerializeWith(Path), ParsePropertiesWith(Path), - SerializePropertiesWith(Path), - Accessor(PublicAccessor), + Getter(PublicGetter), } #[derive(Default)] @@ -69,27 +61,24 @@ struct PropertyOptions { nested: bool, default: Option, parse_with: Option, - serialize_with: Option, parse_properties_with: Option, - serialize_properties_with: Option, public_getter: bool, - public_setter: bool, } -impl Parse for PublicAccessor { +impl Parse for PublicGetter { fn parse(input: ParseStream<'_>) -> syn::Result { input.parse::()?; let content; parenthesized!(content in input); let accessor = content.parse::()?; if !content.is_empty() { - return Err(content.error("expected getter or setter")); + return Err(content.error("expected getter")); } - match accessor.to_string().as_str() { - "getter" => Ok(Self::Getter), - "setter" => Ok(Self::Setter), - _ => Err(Error::new_spanned(accessor, "expected getter or setter")), + if accessor == "getter" { + Ok(Self) + } else { + Err(Error::new_spanned(accessor, "expected getter")) } } } @@ -97,7 +86,7 @@ impl Parse for PublicAccessor { impl Parse for PropertyOption { fn parse(input: ParseStream<'_>) -> syn::Result { if input.peek(Token![pub]) { - return input.parse().map(Self::Accessor); + return input.parse().map(Self::Getter); } let name = input.parse::()?; @@ -116,14 +105,9 @@ impl Parse for PropertyOption { "prefix" => Ok(Self::Prefix(expression)), "default" => Ok(Self::Default(expression)), "parse_with" => expression_path(expression, "parse_with").map(Self::ParseWith), - "serialize_with" => { - expression_path(expression, "serialize_with").map(Self::SerializeWith) - } "parse_properties_with" => { expression_path(expression, "parse_properties_with").map(Self::ParsePropertiesWith) } - "serialize_properties_with" => expression_path(expression, "serialize_properties_with") - .map(Self::SerializePropertiesWith), _ => Err(Error::new_spanned(name, "unknown property option")), } } @@ -152,11 +136,10 @@ pub(crate) fn expand_properties(input: DeriveInput) -> syn::Result let fields = fields .iter() - .map(parse_property_field) + .map(|field| parse_property_field(field, property_options(&field.attrs)?)) .collect::>>()?; let parses = fields.iter().map(parse_field); - let property_writes = fields.iter().map(write_field); - let accessors = fields.iter().map(field_accessors); + let accessors = fields.iter().map(field_getter); let (impl_generics, type_generics, where_clause) = generics.split_for_impl(); Ok(quote! { @@ -173,27 +156,18 @@ pub(crate) fn expand_properties(input: DeriveInput) -> syn::Result #(#parses,)* }) } - - pub fn write_properties( - &self, - properties: &mut ::std::collections::HashMap< - ::std::string::String, - ::std::string::String, - >, - ) -> ::std::result::Result<(), ::std::string::String> { - #(#property_writes)* - Ok(()) - } } }) } -fn parse_property_field(field: &Field) -> syn::Result { +fn parse_property_field( + field: &Field, + property_options: PropertyOptions, +) -> syn::Result { let ident = field .ident .clone() .ok_or_else(|| Error::new_spanned(field, "Properties fields must be named"))?; - let property_options = property_options(&field.attrs)?; let key = merge_attribute_option( attribute_expression_value(&field.attrs, "key")?, property_options.key, @@ -261,44 +235,25 @@ fn parse_property_field(field: &Field) -> syn::Result { field, "parse_with", )?; - let serialize_with = merge_attribute_option( - attribute_path_value(&field.attrs, "serialize_with")?, - property_options.serialize_with, - field, - "serialize_with", - )?; let parse_properties_with = merge_attribute_option( attribute_path_value(&field.attrs, "parse_properties_with")?, property_options.parse_properties_with, field, "parse_properties_with", )?; - let serialize_properties_with = merge_attribute_option( - attribute_path_value(&field.attrs, "serialize_properties_with")?, - property_options.serialize_properties_with, - field, - "serialize_properties_with", - )?; - if additional_keys.is_some() - && parse_properties_with.is_none() - && serialize_properties_with.is_none() - { + if additional_keys.is_some() && parse_properties_with.is_none() { return Err(Error::new_spanned( field, - "#[additional_keys(...)] requires parse_properties_with or serialize_properties_with", + "#[additional_keys(...)] requires parse_properties_with", )); } if (prefix.is_some() || nested) - && (additional_keys.is_some() - || parse_with.is_some() - || serialize_with.is_some() - || parse_properties_with.is_some() - || serialize_properties_with.is_some()) + && (additional_keys.is_some() || parse_with.is_some() || parse_properties_with.is_some()) { return Err(Error::new_spanned( field, - "#[prefix(...)] and #[nested] fields do not support custom parse or write functions", + "#[prefix(...)] and #[nested] fields do not support custom parse functions", )); } if parse_with.is_some() && parse_properties_with.is_some() { @@ -307,13 +262,6 @@ fn parse_property_field(field: &Field) -> syn::Result { "fields cannot declare both parse_with and parse_properties_with", )); } - if serialize_with.is_some() && serialize_properties_with.is_some() { - return Err(Error::new_spanned( - field, - "fields cannot declare both serialize_with and serialize_properties_with", - )); - } - Ok(PropertyField { ident, ty: field.ty.clone(), @@ -323,13 +271,10 @@ fn parse_property_field(field: &Field) -> syn::Result { nested, default, parse_with, - serialize_with, parse_properties_with, - serialize_properties_with, option_inner_type: option_inner_type(&field.ty), map_value_type, public_getter: property_options.public_getter, - public_setter: property_options.public_setter, doc_attributes: field .attrs .iter() @@ -383,33 +328,17 @@ fn property_options(attributes: &[Attribute]) -> syn::Result { PropertyOption::ParseWith(value) => { set_property_option(&mut options.parse_with, value, attribute, "parse_with")? } - PropertyOption::SerializeWith(value) => set_property_option( - &mut options.serialize_with, - value, - attribute, - "serialize_with", - )?, PropertyOption::ParsePropertiesWith(value) => set_property_option( &mut options.parse_properties_with, value, attribute, "parse_properties_with", )?, - PropertyOption::SerializePropertiesWith(value) => set_property_option( - &mut options.serialize_properties_with, - value, - attribute, - "serialize_properties_with", - )?, - PropertyOption::Accessor(accessor) => { - let selected = match accessor { - PublicAccessor::Getter => &mut options.public_getter, - PublicAccessor::Setter => &mut options.public_setter, - }; - if *selected { + PropertyOption::Getter(_) => { + if options.public_getter { return Err(Error::new_spanned(attribute, "duplicate property accessor")); } - *selected = true; + options.public_getter = true; } } } @@ -449,32 +378,27 @@ fn merge_attribute_option( } } -fn field_accessors(field: &PropertyField) -> TokenStream2 { +fn field_getter(field: &PropertyField) -> TokenStream2 { + if !field.public_getter { + return TokenStream2::new(); + } let ident = &field.ident; let ty = &field.ty; let docs = &field.doc_attributes; - let getter = field.public_getter.then(|| { + if is_copy_type(ty) { quote! { #(#docs)* - pub fn #ident(&self) -> &#ty { - &self.#ident + pub fn #ident(&self) -> #ty { + self.#ident } } - }); - let setter = field.public_setter.then(|| { - let setter_ident = format_ident!("set_{}", ident); - let setter_doc = format!("Sets `{ident}`."); + } else { quote! { - #[doc = #setter_doc] - pub fn #setter_ident(&mut self, value: #ty) { - self.#ident = value; + #(#docs)* + pub fn #ident(&self) -> &#ty { + &self.#ident } } - }); - - quote! { - #getter - #setter } } @@ -762,6 +686,55 @@ fn is_bool(ty: &Type) -> bool { is_named_type(ty, "bool") } +fn is_copy_type(ty: &Type) -> bool { + match ty { + Type::Array(array) => is_copy_type(&array.elem), + Type::BareFn(_) | Type::Never(_) | Type::Ptr(_) => true, + Type::Group(group) => is_copy_type(&group.elem), + Type::Paren(paren) => is_copy_type(&paren.elem), + Type::Reference(reference) => reference.mutability.is_none(), + Type::Tuple(tuple) => tuple.elems.iter().all(is_copy_type), + Type::Path(type_path) if type_path.qself.is_none() => { + let Some(segment) = type_path.path.segments.last() else { + return false; + }; + if matches!( + segment.ident.to_string().as_str(), + "bool" + | "char" + | "f32" + | "f64" + | "i8" + | "i16" + | "i32" + | "i64" + | "i128" + | "isize" + | "u8" + | "u16" + | "u32" + | "u64" + | "u128" + | "usize" + ) { + return true; + } + if segment.ident != "Option" && segment.ident != "Result" { + return false; + } + let PathArguments::AngleBracketed(arguments) = &segment.arguments else { + return false; + }; + arguments.args.iter().all(|argument| match argument { + GenericArgument::Lifetime(_) => true, + GenericArgument::Type(ty) => is_copy_type(ty), + _ => false, + }) + } + _ => false, + } +} + fn is_named_type(ty: &Type, name: &str) -> bool { let Type::Path(type_path) = ty else { return false; @@ -773,70 +746,3 @@ fn is_named_type(ty: &Type, name: &str) -> bool { .last() .is_some_and(|segment| segment.ident == name) } - -fn write_field(field: &PropertyField) -> TokenStream2 { - let ident = &field.ident; - if field.nested { - return quote! { - self.#ident.write_properties(properties)?; - }; - } - - let default = typed_default(field); - - if let Some(serialize_properties_with) = &field.serialize_properties_with { - let key = field.key.as_ref().expect("exact-key fields have a key"); - let serialize = match &field.additional_keys { - Some(additional_keys) => { - quote!(#serialize_properties_with(&self.#ident, properties, #key, &[#(#additional_keys),*], &#default)) - } - None => quote!(#serialize_properties_with(&self.#ident, properties, #key, &#default)), - }; - return quote! { - #serialize.map_err(|error| { - format!("Failed to serialize {}: {error}", #key) - })?; - }; - } - - if let Some(prefix) = &field.prefix { - return quote! { - properties.retain(|key, _| !key.starts_with(#prefix)); - if self.#ident != #default { - for (suffix, value) in &self.#ident { - let key = format!("{}{}", #prefix, suffix); - properties.insert(key, ::std::string::ToString::to_string(value)); - } - } - }; - } - - let key = field.key.as_ref().expect("exact-key fields have a key"); - let value = match (&field.serialize_with, &field.option_inner_type) { - (Some(serialize_with), _) => quote!(#serialize_with(&self.#ident).map_err(|error| { - format!("Failed to serialize {}: {error}", #key) - })?), - (None, Some(_)) => quote!(::std::string::ToString::to_string( - self.#ident.as_ref().expect("checked is_some above") - )), - (None, None) => quote!(::std::string::ToString::to_string(&self.#ident)), - }; - let insert = if field.option_inner_type.is_some() { - quote! { - if self.#ident != #default && self.#ident.is_some() { - properties.insert((#key).to_string(), #value); - } - } - } else { - quote! { - if self.#ident != #default { - properties.insert((#key).to_string(), #value); - } - } - }; - - quote! { - properties.remove(#key); - #insert - } -} diff --git a/crates/property-macro/tests/properties.rs b/crates/property-macro/tests/properties.rs index 8c75f2d635..2eecde722e 100644 --- a/crates/property-macro/tests/properties.rs +++ b/crates/property-macro/tests/properties.rs @@ -18,6 +18,7 @@ use std::collections::HashMap; use iceberg_property_macro::Properties; +use serde::{Deserialize, Serialize}; const RETRIES: &str = "commit.retry.num-retries"; const OWNER: &str = "owner"; @@ -52,139 +53,72 @@ fn parse_dimensions( )) } -fn serialize_dimensions( - dimensions: &(u64, u64, u64), - properties: &mut HashMap, - width_key: &str, - additional_keys: &[&str], - default: &(u64, u64, u64), -) -> Result<(), String> { - if additional_keys.len() != 2 { - return Err("dimensions require height and depth keys".to_string()); - } - if dimensions.0 == 0 || dimensions.1 == 0 || dimensions.2 == 0 { - return Err("dimensions must be positive".to_string()); - } - properties.remove(width_key); - properties.remove(additional_keys[0]); - properties.remove(additional_keys[1]); - if dimensions != default { - properties.insert(width_key.to_string(), dimensions.0.to_string()); - properties.insert(additional_keys[0].to_string(), dimensions.1.to_string()); - properties.insert(additional_keys[1].to_string(), dimensions.2.to_string()); - } - Ok(()) -} - #[derive(Debug, Properties)] struct TestProperties { #[key(RETRIES)] #[default(4)] - pub retries: u64, + #[property(pub(getter))] + retries: u64, - #[key(OWNER)] - #[default(None)] - pub owner: Option, + #[property(key = OWNER, default = None, pub(getter))] + owner: Option, - #[key(FORMAT)] - #[default("parquet")] - pub format: String, + #[property(key = FORMAT, default = "parquet", pub(getter))] + format: String, - #[key(FANOUT_ENABLED)] - #[default(true)] - pub fanout_enabled: bool, + #[property(key = FANOUT_ENABLED, default = true, pub(getter))] + fanout_enabled: bool, - #[prefix(COLUMN_FPP_PREFIX)] - #[default(HashMap::new())] - pub column_fpp: HashMap, + #[property( + prefix = COLUMN_FPP_PREFIX, + default = HashMap::new(), + pub(getter) + )] + column_fpp: HashMap, - #[key(WIDTH)] - #[additional_keys(HEIGHT, DEPTH)] - #[default((640, 480, 320))] - #[parse_properties_with(parse_dimensions)] - #[serialize_properties_with(serialize_dimensions)] - pub dimensions: (u64, u64, u64), + #[property( + key = WIDTH, + additional_keys = [HEIGHT, DEPTH], + default = (640, 480, 320), + parse_properties_with = parse_dimensions, + pub(getter) + )] + dimensions: (u64, u64, u64), } #[test] -fn reads_defaults_and_overrides() { - let defaults = TestProperties::from_properties(&HashMap::new()).unwrap(); - assert_eq!(defaults.retries, 4); - assert_eq!(defaults.owner, None); - assert_eq!(defaults.format, "parquet"); - assert!(defaults.fanout_enabled); - assert!(defaults.column_fpp.is_empty()); - assert_eq!(defaults.dimensions, (640, 480, 320)); +fn reads_defaults_through_generated_getters() { + let properties = TestProperties::from_properties(&HashMap::new()).unwrap(); - let properties = HashMap::from([ - (RETRIES.to_string(), "8".to_string()), - (OWNER.to_string(), "iceberg".to_string()), - (FORMAT.to_string(), "orc".to_string()), - (FANOUT_ENABLED.to_string(), "FALSE".to_string()), - (format!("{COLUMN_FPP_PREFIX}id"), "0.01".to_string()), - (WIDTH.to_string(), "1920".to_string()), - (HEIGHT.to_string(), "1080".to_string()), - (DEPTH.to_string(), "720".to_string()), - ]); - let parsed = TestProperties::from_properties(&properties).unwrap(); - - assert_eq!(parsed.retries, 8); - assert_eq!(parsed.owner.as_deref(), Some("iceberg")); - assert_eq!(parsed.format, "orc"); - assert!(!parsed.fanout_enabled); - assert_eq!(parsed.column_fpp["id"], 0.01); - assert_eq!(parsed.dimensions, (1920, 1080, 720)); -} - -#[test] -fn writes_overrides_and_preserves_unrelated_properties() { - let parsed = TestProperties::from_properties(&HashMap::from([ - (RETRIES.to_string(), "8".to_string()), - (OWNER.to_string(), "iceberg".to_string()), - (FORMAT.to_string(), "orc".to_string()), - (FANOUT_ENABLED.to_string(), "false".to_string()), - (format!("{COLUMN_FPP_PREFIX}id"), "0.01".to_string()), - (WIDTH.to_string(), "1920".to_string()), - (HEIGHT.to_string(), "1080".to_string()), - (DEPTH.to_string(), "720".to_string()), - ])) - .unwrap(); - let mut properties = HashMap::from([("unrelated".to_string(), "value".to_string())]); - - parsed.write_properties(&mut properties).unwrap(); - - assert_eq!(properties[RETRIES], "8"); - assert_eq!(properties[OWNER], "iceberg"); - assert_eq!(properties[FORMAT], "orc"); - assert_eq!(properties[FANOUT_ENABLED], "false"); - assert_eq!(properties[&format!("{COLUMN_FPP_PREFIX}id")], "0.01"); - assert_eq!(properties[WIDTH], "1920"); - assert_eq!(properties[HEIGHT], "1080"); - assert_eq!(properties[DEPTH], "720"); - assert_eq!(properties["unrelated"], "value"); + assert_eq!(properties.retries(), 4); + assert_eq!(properties.owner(), &None); + assert_eq!(properties.format(), "parquet"); + assert!(properties.fanout_enabled()); + assert!(properties.column_fpp().is_empty()); + assert_eq!(properties.dimensions(), (640, 480, 320)); } #[test] -fn writing_defaults_removes_modeled_properties() { - let defaults = TestProperties::from_properties(&HashMap::new()).unwrap(); - let mut properties = HashMap::from([ +fn reads_overrides_and_ignores_unknown_properties() { + let raw = HashMap::from([ (RETRIES.to_string(), "8".to_string()), (OWNER.to_string(), "iceberg".to_string()), (FORMAT.to_string(), "orc".to_string()), - (FANOUT_ENABLED.to_string(), "false".to_string()), + (FANOUT_ENABLED.to_string(), "FALSE".to_string()), (format!("{COLUMN_FPP_PREFIX}id"), "0.01".to_string()), (WIDTH.to_string(), "1920".to_string()), (HEIGHT.to_string(), "1080".to_string()), (DEPTH.to_string(), "720".to_string()), - ("unrelated".to_string(), "value".to_string()), + ("unknown".to_string(), "ignored".to_string()), ]); + let properties = TestProperties::from_properties(&raw).unwrap(); - defaults.write_properties(&mut properties).unwrap(); - - assert_eq!( - properties, - HashMap::from([("unrelated".to_string(), "value".to_string())]) - ); + assert_eq!(properties.retries(), 8); + assert_eq!(properties.owner().as_deref(), Some("iceberg")); + assert_eq!(properties.format(), "orc"); + assert!(!properties.fanout_enabled()); + assert_eq!(properties.column_fpp()["id"], 0.01); + assert_eq!(properties.dimensions(), (1920, 1080, 720)); } #[test] @@ -212,34 +146,25 @@ fn reports_the_property_with_an_invalid_value() { assert!(prefix_error.contains(&prefixed_key)); } -#[derive(Clone, Debug, Properties)] +#[derive(Debug, Properties)] struct CommitProperties { - #[key = "commit.retry.num-retries"] - #[default = 4] - pub num_retries: u64, + /// Maximum number of times to retry a commit. + #[property(key = RETRIES, default = 4, pub(getter))] + retries: u64, } #[derive(Debug, Properties)] struct NestedProperties { - #[nested] - pub commit: CommitProperties, + #[property(nested, pub(getter))] + commit: CommitProperties, } #[test] -fn nested_properties_use_a_flat_property_map() { - let mut properties = NestedProperties::from_properties(&HashMap::new()).unwrap(); - assert_eq!(properties.commit.num_retries, 4); +fn nested_properties_read_the_same_flat_map() { + let raw = HashMap::from([(RETRIES.to_string(), "9".to_string())]); + let properties = NestedProperties::from_properties(&raw).unwrap(); - properties.commit.num_retries = 9; - let mut written = HashMap::new(); - properties.write_properties(&mut written).unwrap(); - assert_eq!( - written, - HashMap::from([("commit.retry.num-retries".to_string(), "9".to_string())]) - ); - - let decoded = NestedProperties::from_properties(&written).unwrap(); - assert_eq!(decoded.commit.num_retries, 9); + assert_eq!(properties.commit().retries(), 9); } fn parse_non_empty(value: &str) -> Result { @@ -251,32 +176,25 @@ fn parse_non_empty(value: &str) -> Result { } } -fn serialize_trimmed(value: &str) -> Result { - let value = value.trim(); - if value.is_empty() { - Err("value must not be empty") - } else { - Ok(value.to_string()) - } -} - #[derive(Debug, Properties)] struct ValidatedProperties { - #[key = "location"] - #[default = "default"] - #[parse_with(parse_non_empty)] - #[serialize_with(serialize_trimmed)] + #[property( + key = "location", + default = "default", + parse_with = parse_non_empty, + pub(getter) + )] location: String, } #[test] -fn custom_single_value_hooks_can_validate_and_normalize() { +fn custom_single_value_parser_can_validate_and_normalize() { let parsed = ValidatedProperties::from_properties(&HashMap::from([( "location".to_string(), " path ".to_string(), )])) .unwrap(); - assert_eq!(parsed.location, "path"); + assert_eq!(parsed.location(), "path"); let error = ValidatedProperties::from_properties(&HashMap::from([( "location".to_string(), @@ -284,71 +202,26 @@ fn custom_single_value_hooks_can_validate_and_normalize() { )])) .unwrap_err(); assert_eq!(error, "Invalid value for location: value must not be empty"); +} - let properties = ValidatedProperties { - location: " normalized ".to_string(), - }; - let mut written = HashMap::new(); - properties.write_properties(&mut written).unwrap(); - assert_eq!(written["location"], "normalized"); +#[derive(Debug, Default, Serialize, Deserialize, Properties)] +struct DerivedTraitProperties { + #[property(key = RETRIES, default = 4, pub(getter))] + retries: u64, } #[test] -fn reports_custom_serialization_errors() { - let invalid_location = ValidatedProperties { - location: " ".to_string(), - }; - let error = invalid_location - .write_properties(&mut HashMap::new()) - .unwrap_err(); - assert_eq!( - error, - "Failed to serialize location: value must not be empty" - ); +fn coexists_with_default_serialize_and_deserialize_derives() { + let defaults = DerivedTraitProperties::default(); + assert_eq!(defaults.retries(), 0); - let mut invalid_dimensions = TestProperties::from_properties(&HashMap::new()).unwrap(); - invalid_dimensions.dimensions = (0, 480, 320); - let error = invalid_dimensions - .write_properties(&mut HashMap::new()) - .unwrap_err(); + let properties = DerivedTraitProperties::from_properties(&HashMap::new()).unwrap(); + assert_eq!(properties.retries(), 4); assert_eq!( - error, - "Failed to serialize dimensions.width: dimensions must be positive" + serde_json::to_string(&properties).unwrap(), + r#"{"retries":4}"# ); -} - -mod accessor_fixture { - use iceberg_property_macro::Properties; - - #[derive(Debug, Default, Properties)] - pub struct AccessorProperties { - #[doc = "A property with public read and write access."] - #[property(key = "public.both", default = 0, pub(getter), pub(setter))] - both: u64, - - #[property(key = "public.getter", default = "", pub(getter))] - getter_only: String, - - #[property(key = "public.setter", default = false, pub(setter))] - setter_only: bool, - } - - impl AccessorProperties { - pub fn setter_only_for_test(&self) -> bool { - self.setter_only - } - } -} - -#[test] -fn coexists_with_derived_default_and_generates_opt_in_accessors() { - let mut properties = accessor_fixture::AccessorProperties::default(); - - assert_eq!(*properties.both(), 0); - properties.set_both(2); - assert_eq!(*properties.both(), 2); - assert_eq!(properties.getter_only(), ""); - properties.set_setter_only(true); - assert!(properties.setter_only_for_test()); + let decoded: DerivedTraitProperties = serde_json::from_str(r#"{"retries":7}"#).unwrap(); + assert_eq!(decoded.retries(), 7); } From 5896eea9b90daf1ea1b5b9e0ab4e5fc5cf6ef288 Mon Sep 17 00:00:00 2001 From: Renjie Liu Date: Fri, 7 Aug 2026 17:11:37 +0800 Subject: [PATCH 5/5] Force attributes --- crates/property-macro/README.md | 17 ++- crates/property-macro/src/lib.rs | 14 +- crates/property-macro/src/properties.rs | 166 ++++------------------ crates/property-macro/tests/properties.rs | 4 +- 4 files changed, 35 insertions(+), 166 deletions(-) diff --git a/crates/property-macro/README.md b/crates/property-macro/README.md index 48445bdc1e..7187e5ce26 100644 --- a/crates/property-macro/README.md +++ b/crates/property-macro/README.md @@ -226,15 +226,14 @@ fn main() -> Result<(), Box> { } ``` -Property options may be grouped under `#[property(...)]`, which avoids a -collision between the standalone `#[default(...)]` helper and Rust's `Default` -derive. The original standalone annotations remain supported. - -`#[prefix(...)]` requires `HashMap`. `#[nested]` embeds another -`Properties` struct while reading the same flat map. `#[parse_with(...)]` -customizes parsing for one exact-key field. `#[parse_properties_with(...)]` -receives the complete property map, and `#[additional_keys(...)]` supplies its -list of secondary keys. +All field settings must be grouped under `#[property(...)]`. This keeps `key`, +`default`, `prefix`, `nested`, parser hooks, additional keys, and getter +generation in one attribute and avoids collisions with ordinary Rust derives. + +The `prefix` setting requires `HashMap`. `nested` embeds another +`Properties` struct while reading the same flat map. `parse_with` customizes +parsing for one exact-key field. `parse_properties_with` receives the complete +property map, and `additional_keys` supplies its list of secondary keys. Boolean values are parsed case-insensitively. Other values require `FromStr` unless a custom parser is supplied. String-literal and path defaults are diff --git a/crates/property-macro/src/lib.rs b/crates/property-macro/src/lib.rs index 1e7e9da4f9..e73b48c0f7 100644 --- a/crates/property-macro/src/lib.rs +++ b/crates/property-macro/src/lib.rs @@ -23,19 +23,7 @@ use syn::{DeriveInput, parse_macro_input}; mod properties; /// Derives property-map parsing and opt-in read-only accessors for a struct. -#[proc_macro_derive( - Properties, - attributes( - key, - additional_keys, - prefix, - nested, - default, - parse_with, - parse_properties_with, - property - ) -)] +#[proc_macro_derive(Properties, attributes(property))] pub fn derive_properties(input: TokenStream) -> TokenStream { let input = parse_macro_input!(input as DeriveInput); diff --git a/crates/property-macro/src/properties.rs b/crates/property-macro/src/properties.rs index 67446a7aeb..5416aba19f 100644 --- a/crates/property-macro/src/properties.rs +++ b/crates/property-macro/src/properties.rs @@ -21,7 +21,7 @@ use syn::parse::{Parse, ParseStream}; use syn::punctuated::Punctuated; use syn::{ Attribute, Data, DeriveInput, Error, Expr, ExprLit, ExprPath, Field, Fields, GenericArgument, - Ident, Lit, Meta, Path, PathArguments, Token, Type, parenthesized, + Ident, Lit, Path, PathArguments, Token, Type, parenthesized, }; struct PropertyField { @@ -136,7 +136,7 @@ pub(crate) fn expand_properties(input: DeriveInput) -> syn::Result let fields = fields .iter() - .map(|field| parse_property_field(field, property_options(&field.attrs)?)) + .map(|field| parse_property_field(field, property_options(field)?)) .collect::>>()?; let parses = fields.iter().map(parse_field); let accessors = fields.iter().map(field_getter); @@ -168,56 +168,34 @@ fn parse_property_field( .ident .clone() .ok_or_else(|| Error::new_spanned(field, "Properties fields must be named"))?; - let key = merge_attribute_option( - attribute_expression_value(&field.attrs, "key")?, - property_options.key, - field, - "key", - )?; - let additional_keys = merge_attribute_option( - attribute_expression_list(&field.attrs, "additional_keys")?, - property_options.additional_keys, - field, - "additional_keys", - )?; - let prefix = merge_attribute_option( - attribute_expression_value(&field.attrs, "prefix")?, - property_options.prefix, - field, - "prefix", - )?; - let standalone_nested = marker_attribute(&field.attrs, "nested")?; - if standalone_nested && property_options.nested { - return Err(Error::new_spanned( - field, - "duplicate nested property option", - )); - } - let nested = standalone_nested || property_options.nested; + let PropertyOptions { + key, + additional_keys, + prefix, + nested, + default, + parse_with, + parse_properties_with, + public_getter, + } = property_options; if usize::from(key.is_some()) + usize::from(prefix.is_some()) + usize::from(nested) != 1 { return Err(Error::new_spanned( field, - "Properties fields must declare exactly one of #[key(...)], #[prefix(...)], or #[nested]", + "Properties fields must declare exactly one of key, prefix, or nested in #[property(...)]", )); } - let default = merge_attribute_option( - attribute_expression_value(&field.attrs, "default")?, - property_options.default, - field, - "default", - )?; if nested && default.is_some() { return Err(Error::new_spanned( field, - "#[nested] fields obtain defaults from their own property annotations and cannot declare #[default(...)]", + "nested fields obtain defaults from their own property annotations and cannot declare default in #[property(...)]", )); } if !nested && default.is_none() { return Err(Error::new_spanned( field, - "Properties leaf fields must declare #[default(...)]", + "Properties leaf fields must declare default in #[property(...)]", )); } @@ -225,27 +203,14 @@ fn parse_property_field( if prefix.is_some() && map_value_type.is_none() { return Err(Error::new_spanned( &field.ty, - "#[prefix(...)] fields must have type HashMap", + "property prefix fields must have type HashMap", )); } - let parse_with = merge_attribute_option( - attribute_path_value(&field.attrs, "parse_with")?, - property_options.parse_with, - field, - "parse_with", - )?; - let parse_properties_with = merge_attribute_option( - attribute_path_value(&field.attrs, "parse_properties_with")?, - property_options.parse_properties_with, - field, - "parse_properties_with", - )?; - if additional_keys.is_some() && parse_properties_with.is_none() { return Err(Error::new_spanned( field, - "#[additional_keys(...)] requires parse_properties_with", + "additional_keys requires parse_properties_with in #[property(...)]", )); } if (prefix.is_some() || nested) @@ -253,7 +218,7 @@ fn parse_property_field( { return Err(Error::new_spanned( field, - "#[prefix(...)] and #[nested] fields do not support custom parse functions", + "prefix and nested fields do not support custom parse functions", )); } if parse_with.is_some() && parse_properties_with.is_some() { @@ -274,7 +239,7 @@ fn parse_property_field( parse_properties_with, option_inner_type: option_inner_type(&field.ty), map_value_type, - public_getter: property_options.public_getter, + public_getter, doc_attributes: field .attrs .iter() @@ -284,9 +249,12 @@ fn parse_property_field( }) } -fn property_options(attributes: &[Attribute]) -> syn::Result { - let Some(attribute) = find_attribute(attributes, "property")? else { - return Ok(PropertyOptions::default()); +fn property_options(field: &Field) -> syn::Result { + let Some(attribute) = find_attribute(&field.attrs, "property")? else { + return Err(Error::new_spanned( + field, + "Properties fields must declare #[property(...)]", + )); }; let parsed = @@ -362,22 +330,6 @@ fn set_property_option( Ok(()) } -fn merge_attribute_option( - standalone: Option, - grouped: Option, - field: &Field, - name: &str, -) -> syn::Result> { - match (standalone, grouped) { - (Some(_), Some(_)) => Err(Error::new_spanned( - field, - format!("duplicate {name} property option"), - )), - (Some(value), None) | (None, Some(value)) => Ok(Some(value)), - (None, None) => Ok(None), - } -} - fn field_getter(field: &PropertyField) -> TokenStream2 { if !field.public_getter { return TokenStream2::new(); @@ -402,74 +354,6 @@ fn field_getter(field: &PropertyField) -> TokenStream2 { } } -fn marker_attribute(attributes: &[Attribute], name: &str) -> syn::Result { - let Some(attribute) = find_attribute(attributes, name)? else { - return Ok(false); - }; - - match &attribute.meta { - Meta::Path(_) => Ok(true), - _ => Err(Error::new_spanned( - attribute, - format!("{name} must use the form #[{name}]"), - )), - } -} - -fn attribute_expression_value(attributes: &[Attribute], name: &str) -> syn::Result> { - let Some(attribute) = find_attribute(attributes, name)? else { - return Ok(None); - }; - - match &attribute.meta { - Meta::NameValue(name_value) => Ok(Some(name_value.value.clone())), - Meta::List(_) => attribute.parse_args::().map(Some), - _ => Err(Error::new_spanned( - attribute, - format!("{name} must use the form #[{name}(...)]"), - )), - } -} - -fn attribute_expression_list( - attributes: &[Attribute], - name: &str, -) -> syn::Result>> { - let Some(attribute) = find_attribute(attributes, name)? else { - return Ok(None); - }; - - let expressions = match &attribute.meta { - Meta::NameValue(name_value) => expression_list(name_value.value.clone(), name)?, - Meta::List(_) => attribute - .parse_args_with(Punctuated::::parse_terminated)? - .into_iter() - .collect(), - _ => { - return Err(Error::new_spanned( - attribute, - format!("{name} must contain a non-empty list of keys"), - )); - } - }; - - if expressions.is_empty() { - return Err(Error::new_spanned( - attribute, - format!("{name} must contain at least one key"), - )); - } - Ok(Some(expressions)) -} - -fn attribute_path_value(attributes: &[Attribute], name: &str) -> syn::Result> { - let Some(expression) = attribute_expression_value(attributes, name)? else { - return Ok(None); - }; - - expression_path(expression, name).map(Some) -} - fn expression_path(expression: Expr, name: &str) -> syn::Result { match expression { Expr::Path(ExprPath { path, .. }) => Ok(path), diff --git a/crates/property-macro/tests/properties.rs b/crates/property-macro/tests/properties.rs index 2eecde722e..12bb4f3322 100644 --- a/crates/property-macro/tests/properties.rs +++ b/crates/property-macro/tests/properties.rs @@ -55,9 +55,7 @@ fn parse_dimensions( #[derive(Debug, Properties)] struct TestProperties { - #[key(RETRIES)] - #[default(4)] - #[property(pub(getter))] + #[property(key = RETRIES, default = 4, pub(getter))] retries: u64, #[property(key = OWNER, default = None, pub(getter))]