From eb67be6a87fb66c8bb39d9217fffe587368ec289 Mon Sep 17 00:00:00 2001 From: Mikey Lombardi Date: Wed, 25 Mar 2026 14:48:15 -0500 Subject: [PATCH 01/15] (GH-538) Refactor `FullyQualifiedTypeName` Prior to this change, the `FullyQualifiedTypeName` struct relied entirely on the JSON Schema validation pattern for parsing an input string into a fully qualified type name. This change refactors the implementation to take advantage of the improved diagnostics and erroring model used for the version types. This change also updates the test suite for the type to mirror types that were developed later. --- lib/dsc-lib/locales/en-us.toml | 10 + lib/dsc-lib/locales/schemas.definitions.yaml | 73 ++- lib/dsc-lib/src/dscerror.rs | 4 +- .../src/types/fully_qualified_type_name.rs | 555 ++++++++++++++++-- lib/dsc-lib/src/types/mod.rs | 2 +- .../types/fully_qualified_type_name.rs | 475 ++++++++++++--- 6 files changed, 950 insertions(+), 169 deletions(-) diff --git a/lib/dsc-lib/locales/en-us.toml b/lib/dsc-lib/locales/en-us.toml index 3c87d081e..56e0e36b2 100644 --- a/lib/dsc-lib/locales/en-us.toml +++ b/lib/dsc-lib/locales/en-us.toml @@ -833,6 +833,16 @@ invalidDate = "unable to parse '%{text}' as a date version - %{errors}" successText = "Success" failureText = "Error" +[types.fully_qualified_type_name] +emptyNamespaceSegment = "namespace segment %{index} is empty" +emptyOwnerSegment = "owner segment is empty" +emptyTypeName = "fully qualified type name cannot be an empty string" +invalidNameSegment = "name segment '%{name}' contains invalid characters" +invalidNamespaceSegment = "namespace segment '%{namespace}' contains invalid characters" +invalidOwnerSegment = "owner segment '%{owner}' contains invalid characters" +invalidTypeName = "invalid fully qualified type name '%{name}': %{err}" +missingNameSegment = "missing required name segment following forward slash (`/`) character" + [types.resource_version] unparseableVersion = "unable to parse '%{text}' as resource version - input doesn't seem to be a semantic or date version" invalidDateVersion = "invalid date resource version: %{err}" diff --git a/lib/dsc-lib/locales/schemas.definitions.yaml b/lib/dsc-lib/locales/schemas.definitions.yaml index e1562c968..9fa0c5f17 100644 --- a/lib/dsc-lib/locales/schemas.definitions.yaml +++ b/lib/dsc-lib/locales/schemas.definitions.yaml @@ -88,37 +88,48 @@ schemas: signed integer, like `5` or `-2147024891`. resourceType: - title: Fully qualified type name - description: >- - Uniquely identifies a DSC resource or extension. - markdownDescription: |- - The fully qualified type name of a DSC resource or extension uniquely identifies a resource - or extension. - - Fully qualified type names use the following syntax: - - ```yaml - [....]/ - ``` - - Where the type may have zero or more namespace segments for organizing the type. The - `owner`, `namespace`, and `name` segments must consist only of alphanumeric characters and - underscores. - - Conventionally, the first character of each segment is capitalized. When a segment - contains a brand or proper name, use the correct casing for that word, like - `TailspinToys/Settings`, not `Tailspintoys/Settings`. - - Example fully qualified type names include: - - - `Microsoft/OSInfo` - - `Microsoft.SqlServer/Database` - - `Microsoft.Windows.IIS/WebApp` - patternErrorMessage: >- - Invalid type name. Valid resource type names always define an owner and a name separated by - a slash, like `Microsoft/OSInfo`. Type names may optionally include the group, area, and - subarea segments to namespace the resource under the owner, like - `Microsoft.Windows/Registry`. + title: + en-us: Fully qualified type name + description: + en-us: >- + Uniquely identifies a DSC resource or extension. + markdownDescription: + en-us: |- + The fully qualified type name of a DSC resource or extension uniquely identifies a + resource or extension. + + Fully qualified type names use the following syntax: + + ```yaml + [....]/ + ``` + + Where: + + 1. The `owner` segment is mandatory and indicates the party responsible for publishing + and maintaining the type. + 1. The type may define any number of `namespace` segments for organizing the type. + Namespaces must be separated from the `owner` segment and other `namespace` segments + by a single dot (`.`). + 1. The `name` segment is mandatory and indicates the specific name of the type. It must + be separated from the preceding segment by a forward slash (`/`). + 1. Every segment must consist only of unicode alphanumeric characters and underscores. + + Conventionally, the first character of each segment is capitalized. When a segment + contains a brand or proper name, use the correct casing for that word, like + `TailspinToys/Settings`, not `Tailspintoys/Settings`. + + Example fully qualified type names include: + + - `Microsoft/OSInfo` + - `Microsoft.SqlServer/Database` + - `Microsoft.Windows.IIS/WebApp` + patternErrorMessage: + en-us: >- + Invalid type name. Valid resource type names always define an owner and a name separated by + a slash, like `Microsoft/OSInfo`. Type names may optionally include the group, area, and + subarea segments to namespace the resource under the owner, like + `Microsoft.Windows/Registry`. resourceVersion: title: diff --git a/lib/dsc-lib/src/dscerror.rs b/lib/dsc-lib/src/dscerror.rs index 17cd040d2..624556fec 100644 --- a/lib/dsc-lib/src/dscerror.rs +++ b/lib/dsc-lib/src/dscerror.rs @@ -71,8 +71,8 @@ pub enum DscError { #[error("{t} '{0}' - {t2}: '{1}'", t = t!("dscerror.invalidTagPrefix"), t2 = t!("dscerror.invalidTagSuffix"))] InvalidTag(String, String), - #[error("{t} '{0}' - {t2}: '{1}'", t = t!("dscerror.invalidTypeNamePrefix"), t2 = t!("dscerror.invalidTypeNameSuffix"))] - InvalidTypeName(String, String), + #[error(transparent)] + FullyQualifiedTypeName(#[from] crate::types::FullyQualifiedTypeNameError), #[error("IO: {0}")] Io(#[from] std::io::Error), diff --git a/lib/dsc-lib/src/types/fully_qualified_type_name.rs b/lib/dsc-lib/src/types/fully_qualified_type_name.rs index 101f4bb8a..19e34c12d 100644 --- a/lib/dsc-lib/src/types/fully_qualified_type_name.rs +++ b/lib/dsc-lib/src/types/fully_qualified_type_name.rs @@ -2,33 +2,125 @@ // Licensed under the MIT License. use std::fmt::{Display, Formatter}; +use std::hash::Hash; use std::ops::Deref; use std::str::FromStr; use std::sync::OnceLock; +use miette::Diagnostic; use regex::Regex; use rust_i18n::t; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; +use thiserror::Error; -use crate::dscerror::DscError; use crate::schemas::dsc_repo::DscRepoSchema; /// Defines the fully qualified type name for a DSC resource or extension. The fully qualified name /// uniquely identifies each resource and extension. +/// +/// # Syntax +/// +/// Fully qualified type names use the following syntax: +/// +/// ```yaml +/// [....]/ +/// ``` +/// +/// Where: +/// +/// 1. The `owner` segment is mandatory and indicates the party responsible for publishing and +/// maintaining the type. +/// 1. The type may define any number of `namespace` segments for organizing the type. Namespaces +/// must be separated from the `owner` segment and other `namespace` segments by a single dot +/// (`.`). +/// 1. The `name` segment is mandatory and indicates the specific name of the type. It must be +/// separated from the preceding segment by a forward slash (`/`). +/// 1. Every segment must consist only of unicode alphanumeric characters and underscores. +/// +/// Conventionally, the first character of each segment is capitalized. When a segment +/// contains a brand or proper name, use the correct casing for that word, like +/// `TailspinToys/Settings`, not `Tailspintoys/Settings`. +/// +/// Example fully qualified type names include: +/// +/// - `Microsoft/OSInfo` +/// - `Microsoft.SqlServer/Database` +/// - `Microsoft.Windows.IIS/WebApp` +/// +/// # Comparisons +/// +/// For equivalency, Fully qualified types are compared case-insensitively as strings. Instances +/// [`FullyQualifiedTypeName`] are equal if their string representations are equal when lowercased. +/// +/// ```rust +/// # use dsc_lib::types::FullyQualifiedTypeName; +/// # use pretty_assertions::assert_eq; +/// assert_eq!( +/// FullyQualifiedTypeName::parse("Microsoft/OSInfo").unwrap(), +/// FullyQualifiedTypeName::parse("microsoft/osinfo").unwrap() +/// ); +/// ``` +/// +/// For ordering, fully qualified type names are ordered by actual string representations _without_ +/// lowercasing. The ordering is [lexicographic][01], reusing the underlying Rust ordering for +/// strings. +/// +/// ```rust +/// # use dsc_lib::types::FullyQualifiedTypeName; +/// # use pretty_assertions::assert_eq; +/// +/// let mut names = vec![ +/// FullyQualifiedTypeName::parse("Microsoft/OSInfo").unwrap(), +/// FullyQualifiedTypeName::parse("Contoso/Resource").unwrap(), +/// FullyQualifiedTypeName::parse("TailspinToys/Settings").unwrap(), +/// ]; +/// names.sort(); +/// +/// assert_eq!( +/// names, +/// vec![ +/// FullyQualifiedTypeName::parse("Contoso/Resource").unwrap(), +/// FullyQualifiedTypeName::parse("Microsoft/OSInfo").unwrap(), +/// FullyQualifiedTypeName::parse("TailspinToys/Settings").unwrap(), +/// ] +/// ); +/// ``` +/// +/// # JSON Schema Validation +/// +/// For JSON schema validation, the fully qualified type name must be defined as a string and is +/// validated against the following regular expression pattern: +/// +/// ```regex +/// "^\w+(\.\w+)*\/\w+$" +/// ``` +/// +/// This pattern enforces the following rules: +/// +/// 1. The `owner` segment must be defined and consist of one or more unicode word characters +/// (alphanumeric or underscores). +/// 1. The `namespace` segments are optional, but if defined, each must consist of one or more +/// unicode word characters and be separated by a single dot (`.`). Consecutive dots or dots at +/// the beginning or end of the owner and namespace portion are forbidden. +/// 1. The `name` segment must be defined and consist of one or more unicode word characters. +/// +/// Note that validation for the JSON Schema is necessarily less expressive than the parsing errors +/// from the [`parse()`] method, because the input either validates against the regular expression +/// or it fails entirely without specific diagnostics. +/// +/// [01]: https://doc.rust-lang.org/std/cmp/trait.Ord.html#lexicographical-comparison +/// [`parse()`]: Self::parse() #[derive( Clone, Debug, Eq, - PartialOrd, - Ord, - Hash, Serialize, Deserialize, JsonSchema, DscRepoSchema, )] -#[serde(try_from = "String")] +#[serde(try_from = "String", into = "String")] #[schemars( title = t!("schemas.definitions.resourceType.title"), description = t!("schemas.definitions.resourceType.description"), @@ -41,56 +133,387 @@ use crate::schemas::dsc_repo::DscRepoSchema; #[dsc_repo_schema(base_name = "resourceType", folder_path = "definitions")] pub struct FullyQualifiedTypeName(String); +/// Defines the various errors that can occur when parsing and working with instances of +/// [`FullyQualifiedTypeName`]. +#[derive(Debug, Clone, PartialEq, Error, Diagnostic)] +pub enum FullyQualifiedTypeNameError { + /// Indicates that the provided fully qualified type name is invalid for + /// one or more reasons. + /// + /// This error can occur for multiple reasons, such as invalid characters in the owner, + /// namespace, or name segments, or if the input string is empty or missing required + /// segments. The `errors` field contains a list of specific validation errors that were + /// encountered while parsing the fully qualified type name to enable reviewing all issues + /// rather than just the first one encountered. + #[error("{t}", t = t!( + "types.fully_qualified_type_name.invalidTypeName", + name = text, + err = errors.iter().map(|e| e.to_string()).collect::>().join(", ") + ))] + InvalidTypeName { + /// The input text that failed to parse as a fully qualified type name. + text: String, + /// A list of specific validation errors that were encountered while parsing the fully + /// qualified type name. + /// + /// Each error in this list indicates a specific issue with the input text, such as invalid + /// characters in a segment or missing required segments. + #[related] + errors: Vec, + }, + + /// Indicates that the provided fully qualified type name is an empty string. + #[error("{t}", t = t!( + "types.fully_qualified_type_name.emptyTypeName" + ))] + EmptyTypeName, + + /// Indicates that the provided fully qualified type name is invalid because it contains + /// invalid characters in the owner segment (the first segment before any dots or slashes). + /// + /// The owner segment must contain only unicode alphanumeric characters and underscores. If it + /// contains any other characters, validation raises this error in the `errors` field of the + /// main [`InvalidTypeName`] error. + /// + /// [`InvalidTypeName`]: Self::InvalidTypeName + #[error("{t}", t = t!( + "types.fully_qualified_type_name.invalidOwnerSegment", + "owner" => segment_text + ))] + InvalidOwnerSegment { + /// The owner segment text that contains invalid characters. + segment_text: String, + }, + + /// Indicates that the provided fully qualified type name is invalid because it defines a + /// namespace segment without defining a non-dot character before it, like + /// `.Contoso.Example/Resource`. + /// + /// The owner segment must be defined before any namespace segments. It must contain only + /// unicode alphanumeric characters and underscores. If the input string contains a namespace + /// segment that is not preceded by a valid owner segment, validation raises this error in the + /// `errors` field of the main [`InvalidTypeName`] error. + /// + /// [`InvalidTypeName`]: Self::InvalidTypeName + #[error("{t}", t = t!( + "types.fully_qualified_type_name.emptyOwnerSegment" + ))] + EmptyOwnerSegment, + + /// Indicates that the provided fully qualified type name is invalid because it contains + /// invalid characters in a namespace segment (any segments between the owner and the name, + /// separated by dots (`.`)). + /// + /// Each namespace segment must contain only unicode alphanumeric characters and underscores. + /// If it contains any other characters, validation raises this error in the `errors` field of + /// the main [`InvalidTypeName`] error. + /// + /// [`InvalidTypeName`]: Self::InvalidTypeName + #[error("{t}", t = t!( + "types.fully_qualified_type_name.invalidNamespaceSegment", + "namespace" => segment_text + ))] + InvalidNamespaceSegment { + /// The namespace segment text that contains invalid characters. + segment_text: String, + }, + + /// Indicates that the provided fully qualified type name is invalid because it contains + /// an empty namespace segment (i.e., two consecutive dots with no characters in between). + /// + /// If the fully qualified type name contains any empty namespace segments, validation raises + /// this error in the `errors` field of the main [`InvalidTypeName`] error. + /// + /// [`InvalidTypeName`]: Self::InvalidTypeName + #[error("{t}", t = t!( + "types.fully_qualified_type_name.emptyNamespaceSegment", + "index" => index + ))] + EmptyNamespaceSegment { + /// The 1-based index of the empty namespace segment in the fully qualified type name. + index: usize, + }, + + /// Indicates that the provided fully qualified type name is invalid because it contains + /// invalid characters in the name segment (the last segment after the forward slash (`/`)). + /// + /// The name segment must contain only unicode alphanumeric characters and underscores. If it + /// contains any other characters, validation raises this error in the `errors` field of the + /// main [`InvalidTypeName`] error. + /// + /// [`InvalidTypeName`]: Self::InvalidTypeName + #[error("{t}", t = t!( + "types.fully_qualified_type_name.invalidNameSegment", + "name" => segment_text + ))] + InvalidNameSegment { + segment_text: String, + }, + + /// Indicates that the provided fully qualified type name is invalid because it is missing the + /// required name segment (i.e., the segment after the forward slash (`/`)). + /// + /// A fully qualified type name must include a name segment. If the input string is missing the + /// forward slash or if there are no characters after the forward slash, validation raises this + /// error in the `errors` field of the main [`InvalidTypeName`] error. + /// + /// [`InvalidTypeName`]: Self::InvalidTypeName + #[error("{t}", t = t!( + "types.fully_qualified_type_name.missingNameSegment" + ))] + MissingNameSegment, +} + /// This static lazily defines the validating regex for [`FullyQualifiedTypeName`]. It enables the /// [`Regex`] instance to be constructed once, the first time it's used, and then reused on all /// subsequent validation calls. It's kept private, since the API usage is to invoke the /// [`FullyQualifiedTypeName::validate()`] method for direct validation or to leverage this static /// from within the constructor for [`FullyQualifiedTypeName`]. -static VALIDATING_REGEX: OnceLock = OnceLock::new(); +static VALIDATING_SEGMENT_REGEX: OnceLock = OnceLock::new(); impl FullyQualifiedTypeName { - /// Defines the regular expression for validating a string as a fully qualified type name. + /// Parses a given string into a [`FullyQualifiedTypeName`] instance. /// - /// The string must begin with one or more alphanumeric characters and underscores that define - /// the `owner` for the type. Following the `owner` segment, the string may include any number - /// of `namespace` segments, which must be separated from the previous segment by a single - /// period (`.`). Finally, the string must include a forward slash (`/`) followed by one or - /// more alphanumeric characters and underscores to define the `name` segment. - pub const VALIDATING_PATTERN: &str = r"^\w+(\.\w+)*\/\w+$"; - - /// Returns the [`Regex`] for [`Self::VALIDATING_PATTERN`]. + /// # Arguments /// - /// This private method is used to initialize the [`VALIDATING_REGEX`] private static to reduce - /// the number of times the regular expression is compiled from the pattern string. - fn init_pattern() -> Regex { - Regex::new(Self::VALIDATING_PATTERN).expect("pattern is valid") - } - - /// Validates a given string as a fully qualified name. + /// - `text` - The input string to parse as a fully qualified type name. + /// + /// # Errors + /// + /// This function returns a [`FullyQualifiedTypeNameError`] if the input string is not a valid + /// fully qualified type name. The error will indicate which validation rules were violated and + /// include diagnostics for every validation error encountered during parsing. + /// + /// # Returns + /// + /// A result containing the parsed [`FullyQualifiedTypeName`] or a + /// [`FullyQualifiedTypeNameError`] if the input string is invalid. + /// + /// # Examples + /// + /// The following snippets shows how various valid input strings are parsed: + /// + /// - A minimal valid fully qualified type name defines only the owner and name segments. + /// ```rust + /// # use dsc_lib::types::FullyQualifiedTypeName; + /// let _ = FullyQualifiedTypeName::parse("Contoso/Resource").unwrap(); + /// ``` + /// + /// - A fully qualified type name can include namespaces between the owner and name segments, + /// separated by a dot (`.`). + /// + /// ```rust + /// # use dsc_lib::types::FullyQualifiedTypeName; + /// let _ = FullyQualifiedTypeName::parse("Contoso.Example/Resource").unwrap(); + /// ``` + /// + /// - A fully qualified type name can include multiple namespaces between the owner and name + /// segments. + /// + /// ```rust + /// # use dsc_lib::types::FullyQualifiedTypeName; + /// let _ = FullyQualifiedTypeName::parse("Contoso.Example.SubExample/Resource").unwrap(); + /// ``` + /// + /// The following snippets shows how invalid input strings result in errors: + /// + /// - An empty string is not a valid fully qualified type name. + /// + /// ```rust + /// # use dsc_lib::types::{FullyQualifiedTypeName, FullyQualifiedTypeNameError}; + /// # use pretty_assertions::assert_eq; + /// assert_eq!( + /// FullyQualifiedTypeName::parse("").unwrap_err(), + /// FullyQualifiedTypeNameError::EmptyTypeName + /// ); + /// ``` + /// + /// - A missing name segment is not valid, regardless of whether the `/` is included. + /// + /// ```rust + /// # use dsc_lib::types::{FullyQualifiedTypeName, FullyQualifiedTypeNameError}; + /// # use pretty_assertions::assert_eq; + /// for input in ["Contoso", "Contoso/", "Contoso.Example", "Contoso.Example/"] { + /// assert_eq!( + /// FullyQualifiedTypeName::parse(input).unwrap_err(), + /// FullyQualifiedTypeNameError::InvalidTypeName { + /// text: input.to_string(), + /// errors: vec![FullyQualifiedTypeNameError::MissingNameSegment] + /// } + /// ); + /// } + /// ``` + /// - An empty owner segment is not valid. /// - /// A string is valid if it matches the [`VALIDATING_PATTERN`]. If the string is invalid, DSC - /// raises the [`DscError::InvalidTypeName`] error. + /// ```rust + /// # use dsc_lib::types::{FullyQualifiedTypeName, FullyQualifiedTypeNameError}; + /// # use pretty_assertions::assert_eq; + /// for input in [".Contoso.Example/Resource", "/Resource"] { + /// assert_eq!( + /// FullyQualifiedTypeName::parse(input).unwrap_err(), + /// FullyQualifiedTypeNameError::InvalidTypeName { + /// text: input.to_string(), + /// errors: vec![FullyQualifiedTypeNameError::EmptyOwnerSegment] + /// } + /// ); + /// } + /// ``` /// - /// [`VALIDATING_PATTERN`]: Self::VALIDATING_PATTERN - pub fn validate(name: &str) -> Result<(), DscError> { - let pattern = VALIDATING_REGEX.get_or_init(Self::init_pattern); - match pattern.is_match(name) { - true => Ok(()), - false => Err(DscError::InvalidTypeName( - name.to_string(), - pattern.to_string(), - )), + /// - Characters other than unicode alphanumeric characters and underscores in any segment are + /// not valid. + /// + /// ```rust + /// # use dsc_lib::types::{FullyQualifiedTypeName, FullyQualifiedTypeNameError}; + /// # use pretty_assertions::assert_eq; + /// let input = "Contoso&Invalid.Example!Invalid/Resource-Invalid"; + /// assert_eq!( + /// FullyQualifiedTypeName::parse(input).unwrap_err(), + /// FullyQualifiedTypeNameError::InvalidTypeName { + /// text: input.to_string(), + /// errors: vec![ + /// FullyQualifiedTypeNameError::InvalidOwnerSegment { + /// segment_text: "Contoso&Invalid".to_string(), + /// }, + /// FullyQualifiedTypeNameError::InvalidNamespaceSegment { + /// segment_text: "Example!Invalid".to_string(), + /// }, + /// FullyQualifiedTypeNameError::InvalidNameSegment { + /// segment_text: "Resource-Invalid".to_string(), + /// }, + /// ], + /// } + /// ); + /// ``` + /// + /// - An empty namespace segment is not valid. + /// + /// ```rust + /// # use dsc_lib::types::{FullyQualifiedTypeName, FullyQualifiedTypeNameError}; + /// # use pretty_assertions::assert_eq; + /// let input = "Contoso.Example.With.Empty..Namespace/Resource"; + /// assert_eq!( + /// FullyQualifiedTypeName::parse(input).unwrap_err(), + /// FullyQualifiedTypeNameError::InvalidTypeName { + /// text: input.to_string(), + /// errors: vec![FullyQualifiedTypeNameError::EmptyNamespaceSegment { + /// index: 4 + /// }], + /// } + /// ); + /// ``` + pub fn parse(text: &str) -> Result { + // If the input text is empty, return an error immediately to avoid unnecessary processing. + if text.is_empty() { + return Err(FullyQualifiedTypeNameError::EmptyTypeName); + } + + let errors = &mut Vec::::new(); + let owner: String; + let namespaces: Vec; + let name: String; + let validating_segment_regex = Self::init_validating_segment_regex(); + + if let Some((owner_and_namespaces, name_segment)) = text.rsplit_once('/') { + name = name_segment.to_string(); + let mut segments = owner_and_namespaces + .split('.') + .map(|s| s.to_string()) + .collect::>(); + owner = segments.remove(0); + namespaces = segments; + } else if text.contains('.') { + let mut segments = text + .split('.') + .map(|s| s.to_string()) + .collect::>(); + owner = segments.remove(0); + namespaces = segments; + name = String::new(); + } else { + owner = text.to_string(); + namespaces = Vec::new(); + name = String::new(); + } + + if owner.is_empty() { + errors.push(FullyQualifiedTypeNameError::EmptyOwnerSegment); + } else if !validating_segment_regex.is_match(&owner) { + errors.push(FullyQualifiedTypeNameError::InvalidOwnerSegment { + segment_text: owner.clone(), + }); + } + + for (index, namespace) in namespaces.into_iter().enumerate() { + if namespace.is_empty() { + errors.push(FullyQualifiedTypeNameError::EmptyNamespaceSegment { + // Insert the index as 1-based for more user-friendly error messages + index: index + 1 + }); + } else if !validating_segment_regex.is_match(&namespace) { + errors.push(FullyQualifiedTypeNameError::InvalidNamespaceSegment { + segment_text: namespace.clone(), + }); + } + } + + if name.is_empty() { + errors.push(FullyQualifiedTypeNameError::MissingNameSegment); + } else if !validating_segment_regex.is_match(&name) { + errors.push(FullyQualifiedTypeNameError::InvalidNameSegment { + segment_text: name.clone() + }); + } + + if errors.is_empty() { + Ok(Self(text.to_string())) + } else { + Err(FullyQualifiedTypeNameError::InvalidTypeName { + text: text.to_string(), + errors: errors.clone(), + }) } } - /// Creates a new instance of [`FullyQualifiedTypeName`] from a string if the input is valid for the - /// [`VALIDATING_PATTERN`]. If the string is invalid, the method raises the - /// [`DscError::InvalidTypeName`] error. + /// Defines the regular expression for validating a string as a fully qualified type name. + /// + /// This pattern is only used for the JSON Schema validation of the entire type name string. + /// When parsing and validating a type name string, the implementation slices the string into + /// its segments (owner, namespaces, and name) and validates each segment individually against + /// the [`VALIDATING_SEGMENT_PATTERN`] for more specific error messages indicating which + /// segment(s) are invalid and how. + /// + /// The string must begin with one or more unicode alphanumeric characters and underscores that + /// define the `owner` for the type. Following the `owner` segment, the string may include any + /// number of `namespace` segments, which must be separated from the previous segment by a + /// single period (`.`). Finally, the string must include a forward slash (`/`) followed by one + /// or more unicode alphanumeric characters and underscores to define the `name` segment. + /// + /// [`VALIDATING_SEGMENT_PATTERN`]: Self::VALIDATING_SEGMENT_PATTERN + pub const VALIDATING_PATTERN: &str = r"^\w+(\.\w+)*\/\w+$"; + + /// Defines the regular expression for validating a segment in a fully qualified type name. + /// + /// Each segment must contain only unicode alphanumeric characters and underscores. This + /// regular expression is applied to each individual segment of the type name (owner, + /// namespaces, and name) rather than the entire type name string to provide more specific + /// error messages indicating which segment is invalid when a type name fails validation. + /// For example, if the type name is `Owner.Namespace/Name` and the `Namespace` segment + /// contains an invalid character, the error message can specifically indicate that the + /// `Namespace` segment is invalid rather than just indicating that the entire type name + /// is invalid. + /// + /// This also obviates needing to check for the namespace separator (`.`) and type/name + /// separator (`/`) in the regex pattern, since the segments are validated individually after + /// slicing the input string based on those separators. + pub const VALIDATING_SEGMENT_PATTERN: &'static str = r"^\w+$"; + + /// Initializes and returns the validating regex for type name segments and returns a reference + /// to the compiled regex. /// - /// [`VALIDATING_PATTERN`]: Self::VALIDATING_PATTERN - pub fn new(name: &str) -> Result { - Self::validate(name)?; - Ok(Self(name.to_string())) + /// This helps avoid recompiling the same regex on every validation call by constructing it + /// once and then reusing it for subsequent validations. + fn init_validating_segment_regex() -> &'static Regex { + VALIDATING_SEGMENT_REGEX.get_or_init(|| Regex::new(Self::VALIDATING_SEGMENT_PATTERN).unwrap()) } } @@ -116,32 +539,70 @@ impl PartialEq for FullyQualifiedTypeName { } } +impl PartialEq for String { + fn eq(&self, other: &FullyQualifiedTypeName) -> bool { + self.to_lowercase() == other.0.to_lowercase() + } +} + impl PartialEq for FullyQualifiedTypeName { fn eq(&self, other: &str) -> bool { self.0.to_lowercase() == other.to_lowercase() } } +impl PartialEq for str { + fn eq(&self, other: &FullyQualifiedTypeName) -> bool { + self.to_lowercase() == other.0.to_lowercase() + } +} + impl PartialEq<&str> for FullyQualifiedTypeName { fn eq(&self, other: &&str) -> bool { self.0.to_lowercase() == other.to_lowercase() } } +impl PartialEq for &str { + fn eq(&self, other: &FullyQualifiedTypeName) -> bool { + self.to_lowercase() == other.0.to_lowercase() + } +} + +// Implement `Ord` and `PartialOrd` by hand to ignore case sensitivity. +impl Ord for FullyQualifiedTypeName { + fn cmp(&self, other: &Self) -> std::cmp::Ordering { + self.0.to_lowercase().cmp(&other.0.to_lowercase()) + } +} + +impl PartialOrd for FullyQualifiedTypeName { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + // Enables using the construct `"Owner/Name".parse()` to convert a literal string into an FQTN. impl FromStr for FullyQualifiedTypeName { - type Err = DscError; + type Err = FullyQualifiedTypeNameError; fn from_str(s: &str) -> Result { - Self::new(s) + Self::parse(s) } } // Enables converting from a `String` and raising the appropriate error message for an invalid // FQTN. impl TryFrom for FullyQualifiedTypeName { - type Error = DscError; + type Error = FullyQualifiedTypeNameError; fn try_from(value: String) -> Result { - Self::new(value.as_str()) + Self::parse(value.as_str()) + } +} + +impl TryFrom<&str> for FullyQualifiedTypeName { + type Error = FullyQualifiedTypeNameError; + fn try_from(value: &str) -> Result { + Self::parse(value) } } @@ -174,3 +635,9 @@ impl Deref for FullyQualifiedTypeName { &self.0 } } + +impl Hash for FullyQualifiedTypeName { + fn hash(&self, state: &mut H) { + self.0.to_lowercase().hash(state); + } +} diff --git a/lib/dsc-lib/src/types/mod.rs b/lib/dsc-lib/src/types/mod.rs index a555e71e8..9a54a2161 100644 --- a/lib/dsc-lib/src/types/mod.rs +++ b/lib/dsc-lib/src/types/mod.rs @@ -8,7 +8,7 @@ pub use exit_code::ExitCode; mod exit_codes_map; pub use exit_codes_map::ExitCodesMap; mod fully_qualified_type_name; -pub use fully_qualified_type_name::FullyQualifiedTypeName; +pub use fully_qualified_type_name::{FullyQualifiedTypeName, FullyQualifiedTypeNameError}; mod resource_version; pub use resource_version::{ResourceVersion, ResourceVersionError}; mod resource_version_req; diff --git a/lib/dsc-lib/tests/integration/types/fully_qualified_type_name.rs b/lib/dsc-lib/tests/integration/types/fully_qualified_type_name.rs index 961494d12..2f07df1fd 100644 --- a/lib/dsc-lib/tests/integration/types/fully_qualified_type_name.rs +++ b/lib/dsc-lib/tests/integration/types/fully_qualified_type_name.rs @@ -1,113 +1,406 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -use jsonschema::Validator; -use schemars::schema_for; -use serde_json::{json, Value}; - -use dsc_lib::{dscerror::DscError, types::FullyQualifiedTypeName}; - -#[test] -fn test_schema_without_segments() { - let schema = Validator::new(schema_for!(FullyQualifiedTypeName).as_value()).unwrap(); - let name = "invalid_type_name"; - - assert!(schema - .validate(&json!(name)) - .unwrap_err() - .to_string() - .starts_with(format!(r#""{name}" does not match"#).as_str())) +#[cfg(test)] +mod methods { + #[cfg(test)] + mod parse { + use dsc_lib::types::{FullyQualifiedTypeName, FullyQualifiedTypeNameError}; + use test_case::test_case; + + #[test_case("Owner/Name" => matches Ok(_); "valid type name without namespace segments")] + #[test_case("Owner.Namespace/Name" => matches Ok(_); "valid type name with one namespace segment")] + #[test_case("Owner.A.B.C/Name" => matches Ok(_); "valid type name with multiple namespace segments")] + fn valid(text: &str) -> Result { + FullyQualifiedTypeName::parse(text) + } + + #[test_case("" => + FullyQualifiedTypeNameError::EmptyTypeName; + "empty string" + )] + #[test_case("Owner.MissingName" => + FullyQualifiedTypeNameError::InvalidTypeName { + text: "Owner.MissingName".to_string(), + errors: vec![ + FullyQualifiedTypeNameError::MissingNameSegment + ] + }; "missing forward slash" + )] + #[test_case("Owner/" => + FullyQualifiedTypeNameError::InvalidTypeName{ + text: "Owner/".to_string(), + errors: vec![ + FullyQualifiedTypeNameError::MissingNameSegment + ] + }; "missing name segment after forward slash" + )] + #[test_case("Owner/Invalid&Name" => + FullyQualifiedTypeNameError::InvalidTypeName{ + text: "Owner/Invalid&Name".to_string(), + errors: vec![ + FullyQualifiedTypeNameError::InvalidNameSegment { + segment_text: "Invalid&Name".to_string() + } + ] + }; "invalid characters in name segment" + )] + #[test_case("Owner.ValidNamespace.Invalid&Namespace.ValidNamespace/Name" => + FullyQualifiedTypeNameError::InvalidTypeName{ + text: "Owner.ValidNamespace.Invalid&Namespace.ValidNamespace/Name".to_string(), + errors: vec![ + FullyQualifiedTypeNameError::InvalidNamespaceSegment { + segment_text: "Invalid&Namespace".to_string() + } + ] + }; "invalid characters in namespace segment" + )] + #[test_case(".Missing.Owner/Name" => + FullyQualifiedTypeNameError::InvalidTypeName{ + text: ".Missing.Owner/Name".to_string(), + errors: vec![ + FullyQualifiedTypeNameError::EmptyOwnerSegment, + ] + }; "empty owner segment before first namespace" + )] + #[test_case("/Name" => + FullyQualifiedTypeNameError::InvalidTypeName{ + text: "/Name".to_string(), + errors: vec![ + FullyQualifiedTypeNameError::EmptyOwnerSegment, + ] + }; "empty owner segment before slash" + )] + #[test_case("Owner.Empty.Namespace..Segment/Name" => + FullyQualifiedTypeNameError::InvalidTypeName{ + text: "Owner.Empty.Namespace..Segment/Name".to_string(), + errors: vec![ + FullyQualifiedTypeNameError::EmptyNamespaceSegment { + index: 3 + } + ] + }; "empty namespace segment" + )] + #[test_case("Invalid&Owner/Name" => + FullyQualifiedTypeNameError::InvalidTypeName{ + text: "Invalid&Owner/Name".to_string(), + errors: vec![ + FullyQualifiedTypeNameError::InvalidOwnerSegment { + segment_text: "Invalid&Owner".to_string() + } + ] + }; "invalid characters in owner segment" + )] + #[test_case("Invalid&Owner.Empty..Namespace.Invalid&Namespace.MissingName" => + FullyQualifiedTypeNameError::InvalidTypeName{ + text: "Invalid&Owner.Empty..Namespace.Invalid&Namespace.MissingName".to_string(), + errors: vec![ + FullyQualifiedTypeNameError::InvalidOwnerSegment { + segment_text: "Invalid&Owner".to_string() + }, + FullyQualifiedTypeNameError::EmptyNamespaceSegment { + index: 2 + }, + FullyQualifiedTypeNameError::InvalidNamespaceSegment { + segment_text: "Invalid&Namespace".to_string() + }, + FullyQualifiedTypeNameError::MissingNameSegment + ] + }; "validation reports all errors for an invalid type name" + )] + fn invalid(text: &str) -> FullyQualifiedTypeNameError { + FullyQualifiedTypeName::parse(text).unwrap_err() + } + } } -#[test] -fn test_schema_with_invalid_character() { - let schema = Validator::new(schema_for!(FullyQualifiedTypeName).as_value()).unwrap(); - let name = "With&Invalid/Character"; +#[cfg(test)] +mod schema { + use std::sync::LazyLock; - assert!(schema - .validate(&json!(name)) - .unwrap_err() - .to_string() - .starts_with(format!(r#""{name}" does not match"#).as_str())) -} + use dsc_lib::types::FullyQualifiedTypeName; + use dsc_lib_jsonschema::schema_utility_extensions::SchemaUtilityExtensions; + use jsonschema::Validator; + use regex::Regex; + use schemars::{schema_for, Schema}; + use serde_json::{json, Value}; + use test_case::test_case; -#[test] -fn test_schema_without_namespaces() { - let schema = Validator::new(schema_for!(FullyQualifiedTypeName).as_value()).unwrap(); - let name = "Owner/Name"; + static SCHEMA: LazyLock = LazyLock::new(|| schema_for!(FullyQualifiedTypeName)); + static VALIDATOR: LazyLock = + LazyLock::new(|| Validator::new((&*SCHEMA).as_value()).unwrap()); + static KEYWORD_PATTERN: LazyLock = + LazyLock::new(|| Regex::new(r"^\w+(\.\w+)+$").expect("pattern is valid")); - assert!(schema.validate(&json!(name)).is_ok()) -} + #[test_case("title")] + #[test_case("description")] + #[test_case("markdownDescription")] + #[test_case("patternErrorMessage")] + fn has_documentation_keyword(keyword: &str) { + let schema = &*SCHEMA; + let value = schema + .get_keyword_as_str(keyword) + .expect(format!("expected keyword '{keyword}' to be defined").as_str()); -#[test] -fn test_schema_with_one_namespace() { - let schema = Validator::new(schema_for!(FullyQualifiedTypeName).as_value()).unwrap(); - let name = "Owner.Namespace/Name"; + assert!( + !(&*KEYWORD_PATTERN).is_match(value), + "Expected keyword '{keyword}' to be defined in translation, but was set to i18n key '{value}'" + ); + } - assert!(schema.validate(&json!(name)).is_ok()) + #[test_case(&json!("Owner/Name") => true; "valid type name without namespaces is valid")] + #[test_case(&json!("Owner.Namespace/Name") => true; "valid type name with one namespace is valid")] + #[test_case(&json!("Owner.A.B.C/Name") => true; "valid type name with multiple namespaces is valid")] + #[test_case(&json!("") => false; "empty string is invalid")] + #[test_case(&json!("Owner.MissingName") => false; "missing forward slash is invalid")] + #[test_case(&json!("Owner/") => false; "missing name segment after forward slash is invalid")] + #[test_case(&json!("Owner/Invalid&Name") => false; "invalid characters in name segment are invalid")] + #[test_case(&json!("Owner.ValidNamespace.Invalid&Namespace.ValidNamespace/Name") => false; "invalid characters in namespace segment are invalid")] + #[test_case(&json!("Owner.Empty.Namespace..Segment/Name") => false; "empty namespace segment is invalid")] + #[test_case(&json!("Invalid&Owner/Name") => false; "invalid characters in owner segment are invalid")] + #[test_case(&json!("Invalid&Owner.Empty..Namespace.Invalid&Namespace.MissingName") => false; "multiple errors are all invalid")] + #[test_case(&json!(true) => false; "boolean value is invalid")] + #[test_case(&json!(1) => false; "integer value is invalid")] + #[test_case(&json!(1.2) => false; "float value is invalid")] + #[test_case(&json!({"type": "Owner/Name"}) => false; "object value is invalid")] + #[test_case(&json!(["Owner/Name"]) => false; "array value is invalid")] + #[test_case(&serde_json::Value::Null => false; "null value is invalid")] + fn validation(input_json: &Value) -> bool { + (&*VALIDATOR).validate(input_json).is_ok() + } } -#[test] -fn test_schema_with_many_namespaces() { - let schema = Validator::new(schema_for!(FullyQualifiedTypeName).as_value()).unwrap(); - let name = "Owner.A.B.C.D.E.F/Name"; +#[cfg(test)] +mod serde { + use dsc_lib::types::FullyQualifiedTypeName; + use serde_json::{json, Value}; + use test_case::test_case; - assert!(schema.validate(&json!(name)).is_ok()) -} + #[test_case(json!("Owner/Name") => matches Ok(_); "valid type name without namespace segments deserializes")] + #[test_case(json!("Owner.Namespace/Name") => matches Ok(_); "valid type name with one namespace segment deserializes")] + #[test_case(json!("Owner.A.B.C/Name") => matches Ok(_); "valid type name with multiple namespace segments deserializes")] + #[test_case(json!("invalid_name") => matches Err(_); "invalid type name fails")] + #[test_case(json!("") => matches Err(_); "empty string fails")] + #[test_case(json!(true) => matches Err(_); "boolean value fails")] + #[test_case(json!(1) => matches Err(_); "integer value fails")] + #[test_case(json!(1.2) => matches Err(_); "float value fails")] + #[test_case(json!({"type": "Contoso.Example/Resource"}) => matches Err(_); "object value fails")] + #[test_case(json!(["Contoso.Example/Resource"]) => matches Err(_); "array value fails")] + #[test_case(serde_json::Value::Null => matches Err(_); "null value fails")] + fn deserialize(value: Value) -> Result { + serde_json::from_value(json!(value)) + } -#[test] -fn test_deserialize_valid() { - let name = "Owner/Name"; - let deserialized: FullyQualifiedTypeName = serde_json::from_value(json!(name)).unwrap(); - assert_eq!(deserialized.to_string(), name.to_string()) + #[test_case("Owner/Name" => json!("Owner/Name"); "valid type name without namespace segments serializes")] + #[test_case("Owner.Namespace/Name" => json!("Owner.Namespace/Name"); "valid type name with one namespace segment serializes")] + #[test_case("Owner.A.B.C/Name" => json!("Owner.A.B.C/Name"); "valid type name with multiple namespace segments serializes")] + fn serialize(text: &str) -> Value { + let instance = FullyQualifiedTypeName::parse(text).unwrap(); + serde_json::to_value(instance).unwrap() + } } -#[test] -fn test_deserialize_invalid() { - let name = "invalid_name"; - let deserializing_error = serde_json::from_value::(json!(name)) - .unwrap_err() - .to_string(); - let expected_error = DscError::InvalidTypeName( - name.to_string(), - FullyQualifiedTypeName::VALIDATING_PATTERN.to_string(), - ) - .to_string(); - - assert_eq!(deserializing_error, expected_error) -} +#[cfg(test)] +mod traits { + #[cfg(test)] + mod default { + use dsc_lib::types::FullyQualifiedTypeName; -#[test] -fn test_serialize_valid() { - let name = "Owner/Name"; - let instance = FullyQualifiedTypeName::new(name).unwrap(); - let serialized: Value = serde_json::to_value(instance).unwrap(); - assert_eq!(serialized, json!(name)) -} + #[test] + fn default_is_empty() { + let instance = FullyQualifiedTypeName::default(); + assert!(instance.is_empty()) + } + } -#[test] -fn test_display() { - let name = "Owner/Name"; - let instance = FullyQualifiedTypeName::new(name).unwrap(); - assert_eq!(format!("{instance}"), format!("{name}")) -} + #[cfg(test)] + mod display { + use dsc_lib::types::FullyQualifiedTypeName; + use test_case::test_case; -#[test] -fn test_as_ref() { - let name = "Owner/Name"; - let instance = FullyQualifiedTypeName::new(name).unwrap(); - assert_eq!(name, instance.as_ref()) -} + #[test_case("Owner/Name", "Owner/Name"; "valid type name without namespace segments")] + #[test_case("Owner.Namespace/Name", "Owner.Namespace/Name"; "valid type name with one namespace segment")] + #[test_case("Owner.A.B.C/Name", "Owner.A.B.C/Name"; "valid type name with multiple namespace segments")] + fn format(type_name: &str, expected: &str) { + pretty_assertions::assert_eq!( + format!("type name: '{}'", FullyQualifiedTypeName::parse(type_name).unwrap()), + format!("type name: '{}'", expected) + ) + } -#[test] -fn test_deref() { - let name = "Owner/Name"; - let instance = FullyQualifiedTypeName::new(name).unwrap(); - assert_eq!(name, &*instance) -} + #[test_case("Owner/Name", "Owner/Name"; "valid type name without namespace segments")] + #[test_case("Owner.Namespace/Name", "Owner.Namespace/Name"; "valid type name with one namespace segment")] + #[test_case("Owner.A.B.C/Name", "Owner.A.B.C/Name"; "valid type name with multiple namespace segments")] + fn to_string(text: &str, expected: &str) { + pretty_assertions::assert_eq!( + FullyQualifiedTypeName::parse(text).unwrap().to_string(), + expected.to_string() + ) + } + } + + #[cfg(test)] + mod from_str { + use std::str::FromStr; + + use dsc_lib::types::{FullyQualifiedTypeName, FullyQualifiedTypeNameError}; + use test_case::test_case; + + #[test_case("Owner/Name" => matches Ok(_); "valid type name without namespace segments parses from string")] + #[test_case("Owner.Namespace/Name" => matches Ok(_); "valid type name with one namespace segment parses from string")] + #[test_case("Owner.A.B.C/Name" => matches Ok(_); "valid type name with multiple namespace segments parses from string")] + #[test_case("invalid_name" => matches Err(_); "invalid type name fails to parse from string")] + fn from_str(text: &str) -> Result { + FullyQualifiedTypeName::from_str(text) + } + + #[test_case("Owner/Name" => matches Ok(_); "valid type name without namespace segments parses from string")] + #[test_case("Owner.Namespace/Name" => matches Ok(_); "valid type name with one namespace segment parses from string")] + #[test_case("Owner.A.B.C/Name" => matches Ok(_); "valid type name with multiple namespace segments parses from string")] + #[test_case("invalid_name" => matches Err(_); "invalid type name fails to parse from string")] + fn parse(text: &str) -> Result { + text.parse() + } + } + + #[cfg(test)] + mod try_from { + use dsc_lib::types::{FullyQualifiedTypeName, FullyQualifiedTypeNameError}; + use test_case::test_case; + + #[test_case("Owner/Name" => matches Ok(_); "valid type name without namespace segments converts from string")] + #[test_case("Owner.Namespace/Name" => matches Ok(_); "valid type name with one namespace segment converts from string")] + #[test_case("Owner.A.B.C/Name" => matches Ok(_); "valid type name with multiple namespace segments converts from string")] + #[test_case("invalid_name" => matches Err(_); "invalid type name fails to convert from string")] + fn string(text: &str) -> Result { + FullyQualifiedTypeName::try_from(text.to_string()) + } + + #[test_case("Owner/Name" => matches Ok(_); "valid type name without namespace segments converts from string")] + #[test_case("Owner.Namespace/Name" => matches Ok(_); "valid type name with one namespace segment converts from string")] + #[test_case("Owner.A.B.C/Name" => matches Ok(_); "valid type name with multiple namespace segments converts from string")] + #[test_case("invalid_name" => matches Err(_); "invalid type name fails to convert from string")] + fn str(text: &str) -> Result { + FullyQualifiedTypeName::try_from(text) + } + } + + #[cfg(test)] + mod into { + use dsc_lib::types::FullyQualifiedTypeName; + + #[test] + fn string() { + let _: String = FullyQualifiedTypeName::parse("Owner/Name").unwrap().into(); + } + } + + #[cfg(test)] + mod as_ref { + use dsc_lib::types::FullyQualifiedTypeName; + + #[test] + fn as_ref() { + let _: &str = FullyQualifiedTypeName::parse("Owner/Name").unwrap().as_ref(); + } + } + + #[cfg(test)] + mod deref { + use dsc_lib::types::FullyQualifiedTypeName; + + #[test] + fn to_lowercase() { + let n = FullyQualifiedTypeName::parse("Owner.Namespace/Name").unwrap(); + assert_eq!(n.to_lowercase(), "owner.namespace/name".to_string()); + } + } + + #[cfg(test)] + mod partial_eq { + use dsc_lib::types::FullyQualifiedTypeName; + use test_case::test_case; + + #[test_case("Owner/Name", "Owner/Name", true; "identical type names are equal")] + #[test_case("Owner.Namespace/Name", "owner.namespace/name", true; "type names with different casing are equal")] + #[test_case("Owner/Name", "Owner.Namespace/Name", false; "type names with different namespaces are not equal")] + #[test_case("Owner/Name", "Owner/OtherName", false; "type names with different name segments are not equal")] + #[test_case("Owner.Namespace/Name", "OtherOwner.Namespace/Name", false; "type names with different owner segments are not equal")] + fn fully_qualified_type_name(lhs: &str, rhs: &str, should_be_equal: bool) { + if should_be_equal { + assert_eq!( + FullyQualifiedTypeName::parse(lhs).unwrap(), + FullyQualifiedTypeName::parse(rhs).unwrap() + ); + } else { + assert_ne!( + FullyQualifiedTypeName::parse(lhs).unwrap(), + FullyQualifiedTypeName::parse(rhs).unwrap() + ); + } + } + + #[test_case("Owner/Name", "Owner/Name", true; "identical type names are equal")] + #[test_case("Owner.Namespace/Name", "owner.namespace/name", true; "type names with different casing are equal")] + #[test_case("Owner/Name", "Owner.Namespace/Name", false; "type names with different namespaces are not equal")] + #[test_case("Owner/Name", "Owner/OtherName", false; "type names with different name segments are not equal")] + #[test_case("Owner.Namespace/Name", "OtherOwner.Namespace/Name", false; "type names with different owner segments are not equal")] + #[test_case("Owner.Namespace/Name", "Not a FQTN", false; "type names are never equal to strings that can't parse as FQTNs")] + fn string(type_name_string: &str, string_slice: &str, should_be_equal: bool) { + let name = FullyQualifiedTypeName::parse(type_name_string).unwrap(); + let string = string_slice.to_string(); + + // Test equivalency bidirectionally + pretty_assertions::assert_eq!( + name == string, + should_be_equal, + "expected comparison of {name} and {string} to be {should_be_equal}" + ); + + pretty_assertions::assert_eq!( + string == name, + should_be_equal, + "expected comparison of {string} and {name} to be {should_be_equal}" + ); + } + + #[test_case("Owner/Name", "Owner/Name", true; "identical type names are equal")] + #[test_case("Owner.Namespace/Name", "owner.namespace/name", true; "type names with different casing are equal")] + #[test_case("Owner/Name", "Owner.Namespace/Name", false; "type names with different namespaces are not equal")] + #[test_case("Owner/Name", "Owner/OtherName", false; "type names with different name segments are not equal")] + #[test_case("Owner.Namespace/Name", "OtherOwner.Namespace/Name", false; "type names with different owner segments are not equal")] + #[test_case("Owner.Namespace/Name", "Not a FQTN", false; "type names are never equal to strings that can't parse as FQTNs")] + fn str(type_name_string: &str, string_slice: &str, should_be_equal: bool) { + let name = FullyQualifiedTypeName::parse(type_name_string).unwrap(); + + // Test equivalency bidirectionally + pretty_assertions::assert_eq!( + name == string_slice, + should_be_equal, + "expected comparison of {name} and {string_slice} to be {should_be_equal}" + ); + + pretty_assertions::assert_eq!( + string_slice == name, + should_be_equal, + "expected comparison of {string_slice} and {name} to be {should_be_equal}" + ); + } + } + + #[cfg(test)] + mod ord { + use dsc_lib::types::FullyQualifiedTypeName; + use test_case::test_case; -#[test] -fn test_default_is_empty() { - let instance = FullyQualifiedTypeName::default(); - assert!(instance.is_empty()) + #[test_case("Owner/Name", "Owner/Name" => std::cmp::Ordering::Equal; "identical type names are equal")] + #[test_case("owner/name", "Owner/Name" => std::cmp::Ordering::Equal; "differently cased type names are equal")] + #[test_case("Owner.A/Name", "Owner.B/Name" => std::cmp::Ordering::Less; "type name with lexicographically smaller namespace is less")] + #[test_case("owner.a/name", "Owner.B/Name" => std::cmp::Ordering::Less; "downcased type name with lexicographically smaller namespace is less")] + #[test_case("Owner.B/Name", "Owner.A/Name" => std::cmp::Ordering::Greater; "type name with lexicographically larger namespace is greater")] + #[test_case("owner.b/name", "Owner.A/Name" => std::cmp::Ordering::Greater; "downcased type name with lexicographically larger namespace is greater")] + fn fully_qualified_type_name(lhs: &str, rhs: &str) -> std::cmp::Ordering { + FullyQualifiedTypeName::parse(lhs).unwrap().cmp(&FullyQualifiedTypeName::parse(rhs).unwrap()) + } + } } From 91b3b5dd3b9a56bc28e3973f86f0bd492db419a2 Mon Sep 17 00:00:00 2001 From: Mikey Lombardi Date: Wed, 25 Mar 2026 15:00:59 -0500 Subject: [PATCH 02/15] (GH-538) Define `WildcardTypeName` struct Prior to this change, the logic for filtering type names used in discovery relied on simple strings converted to regex patterns for wildcard matching. This approach prevented defining either the caching `BTreeMap`s or the filter itself from using `FullyQualifiedTypeName` instances, which are the canonically identifying fields for resources, extensions, and adapters. It also meant that the code couldn't take advantage of the "parse don't validate" approach where we move validation errors earlier in the process and guarantee the safety for any instance after parsing. This change defines a new `WildcardTypeName` struct that represents a type name with one or more wildcards to use for filtering. It provides parsing and validation logic to ensure that the created instance _can_ match a `FullyQualifiedTypeName`, and it also provides a method for checking if a given `FullyQualifiedTypeName` matches the filter. --- lib/dsc-lib/locales/en-us.toml | 12 + lib/dsc-lib/src/dscerror.rs | 3 + lib/dsc-lib/src/types/mod.rs | 2 + lib/dsc-lib/src/types/wildcard_type_name.rs | 716 ++++++++++++++++++ lib/dsc-lib/tests/integration/types/mod.rs | 2 + .../integration/types/wildcard_type_name.rs | 398 ++++++++++ 6 files changed, 1133 insertions(+) create mode 100644 lib/dsc-lib/src/types/wildcard_type_name.rs create mode 100644 lib/dsc-lib/tests/integration/types/wildcard_type_name.rs diff --git a/lib/dsc-lib/locales/en-us.toml b/lib/dsc-lib/locales/en-us.toml index 56e0e36b2..a9452f3ea 100644 --- a/lib/dsc-lib/locales/en-us.toml +++ b/lib/dsc-lib/locales/en-us.toml @@ -868,3 +868,15 @@ forbiddenBuildMetadata = "comparator '%{comparator}' is defined with forbidden b missingOperator = "comparator '%{comparator}' doesn't define an operator" invalidWildcards = "comparator '%{comparator}' has invalid wildcard characters - must define wildcards as asterisks (`*`), not `x` or `X`" wildcardMajorVersion = "comparator '%{comparator}' defines the major version segment as a wildcard `%{wildcard}` instead of a literal number" + +[types.wildcard_type_name] +emptyNamespaceSegment = "namespace segment %{index} is empty" +emptyOwnerSegment = "owner segment is empty" +emptyTypeName = "wildcard type name cannot be an empty string" +invalidNameSegment = "name segment '%{name}' contains invalid characters" +invalidNamespaceSegment = "namespace segment '%{namespace}' contains invalid characters" +invalidOwnerSegment = "owner segment '%{owner}' contains invalid characters" +invalidRegex = "wildcard type name '%{name}' could not be converted into a valid regex pattern: %{err}" +invalidTypeName = "invalid wildcard type name '%{text}': %{err}" +missingNameSegment = "missing name segment when no wildcard in the pattern can match a name segment" +noWildcard = "wildcard type name '%{text}' doesn't contain any wildcard characters (`*`)" diff --git a/lib/dsc-lib/src/dscerror.rs b/lib/dsc-lib/src/dscerror.rs index 624556fec..05f8d7a7c 100644 --- a/lib/dsc-lib/src/dscerror.rs +++ b/lib/dsc-lib/src/dscerror.rs @@ -182,6 +182,9 @@ pub enum DscError { #[error("{t}: {0}", t = t!("dscerror.validation"))] Validation(String), + #[error(transparent)] + WildcardTypeName(#[from] crate::types::WildcardTypeNameError), + #[error("YAML: {0}")] Yaml(#[from] serde_yaml::Error), diff --git a/lib/dsc-lib/src/types/mod.rs b/lib/dsc-lib/src/types/mod.rs index 9a54a2161..94c1c9c90 100644 --- a/lib/dsc-lib/src/types/mod.rs +++ b/lib/dsc-lib/src/types/mod.rs @@ -21,3 +21,5 @@ mod tag; pub use tag::Tag; mod tag_list; pub use tag_list::TagList; +mod wildcard_type_name; +pub use wildcard_type_name::{WildcardTypeName, WildcardTypeNameError}; diff --git a/lib/dsc-lib/src/types/wildcard_type_name.rs b/lib/dsc-lib/src/types/wildcard_type_name.rs new file mode 100644 index 000000000..e8ad63478 --- /dev/null +++ b/lib/dsc-lib/src/types/wildcard_type_name.rs @@ -0,0 +1,716 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +use std::fmt::{Display, Formatter}; +use std::hash::Hash; +use std::str::FromStr; +use std::sync::OnceLock; + +use miette::Diagnostic; +use regex::{Regex, RegexBuilder}; +use rust_i18n::t; +use serde::{Deserialize, Serialize}; +use thiserror::Error; + +use crate::types::FullyQualifiedTypeName; +use crate::util::convert_wildcard_to_regex; + +/// Defines the wildcard type name for filtering DSC resources or extensions by name. +/// +/// The wildcard type name is used to match against [`FullyQualifiedTypeName`]s in order to filter +/// for specific resources or extensions based on their type names, while allowing for flexible +/// patterns using wildcard characters (`*`). +/// +/// The wildcard type name supports the same overall structure as a fully qualified type name +/// (owner, optional namespaces, and name) but allows for the presence of wildcard characters in any +/// segment. The presence of wildcard characters means that the validation rules for fully qualified +/// type names are relaxed to allow for wildcard characters. +/// +/// The default value for a wildcard type name is `*`, which matches any fully qualified type name. +/// This provides a convenient default for cases where the user wants to match all type names +/// without needing to specify a wildcard pattern. +#[derive(Clone, Debug, Serialize, Deserialize)] +#[serde(try_from = "String", into = "String")] +pub struct WildcardTypeName { + /// The original input text for the wildcard type name, which must contain at least one + /// wildcard character (`*`). + text: String, + /// The compiled regex pattern that corresponds to the wildcard type name, used for matching + /// candidate fully qualified type names against the wildcard pattern. + /// + /// This regex is generated by converting the wildcard type name into a regex pattern, where + /// the wildcard characters are replaced with `.*` to match any sequence of characters, and + /// other characters are escaped as needed to ensure they are treated as literals in the regex + /// pattern. + /// + /// The regex is compiled during the parsing/validation of the wildcard type name, and if the + /// resulting regex pattern is invalid, an error is returned. This allows for early detection + /// of invalid wildcard patterns that would not be usable for matching + /// [`FullyQualifiedTypeName`]s. + regex: Regex, +} + +/// Defines the various errors that can occur when parsing and working with instances of +/// [`WildcardTypeName`]. +#[derive(Debug, Clone, PartialEq, Error, Diagnostic)] +pub enum WildcardTypeNameError { + /// Indicates that the provided wildcard type name is invalid because it doesn't contain any + /// wildcard characters. + #[error("{t}", t = t!( + "types.wildcard_type_name.noWildcard", + "text" => text + ))] + NoWildcard { + /// The input text that failed to parse as a wildcard type name. + text: String, + }, + + /// Indicates that the provided wildcard type name is an empty string. + #[error("{t}", t = t!( + "types.wildcard_type_name.emptyTypeName" + ))] + EmptyTypeName, + + /// Indicates that the provided wildcard type name is invalid for one or more reasons. + /// + /// This error can occur for multiple reasons, such as invalid characters in the owner, + /// namespace, or name segments, or invalid regex patterns resulting from the wildcard + /// characters. The `errors` field contains a list of specific validation errors that were + /// encountered while parsing the wildcard type name, to enable reviewing all issues rather + /// than just the first one encountered. + #[error("{t}", t = t!( + "types.wildcard_type_name.invalidTypeName", + "text" => text, + "err" => errors.iter().map(|e| e.to_string()).collect::>().join(", ") + ))] + InvalidTypeName { + /// The input text that failed to parse as a wildcard type name. + text: String, + /// A list of specific validation errors that were encountered while parsing the wildcard + /// type name. + /// + /// Each error in this list indicates a specific issue with the input text, such as invalid + /// characters in a segment or an invalid regex pattern. + #[related] + errors: Vec, + }, + + /// Indicates that the provided wildcard type name is invalid because it contains invalid + /// characters in the owner segment (the first segment before any dots or slashes). + /// + /// The owner segment must contain only unicode alphanumeric characters, underscores, or + /// wildcard characters (`*`). If it contains any other characters, validation raises this + /// error in the `errors` field of the main [`InvalidTypeName`] error. + /// + /// [`InvalidTypeName`]: Self::InvalidTypeName + #[error("{t}", t = t!( + "types.wildcard_type_name.invalidOwnerSegment", + "owner" => segment_text + ))] + InvalidOwnerSegment { + /// The owner segment text that contains invalid characters. + segment_text: String, + }, + + /// Indicates that the provided wildcard type name is invalid because it defines a namespace + /// segment without defining a non-dot character before it, like `.Contoso.Example/Resource`. + /// + /// The owner segment must be defined before any namespace segments. It must contain only + /// unicode alphanumeric characters, underscores, and wildcard characters (`*`). If the input + /// string contains a namespace segment that is not preceded by a valid owner segment, + /// validation raises this error in the `errors` field of the main [`InvalidTypeName`] error. + /// + /// [`InvalidTypeName`]: Self::InvalidTypeName + #[error("{t}", t = t!( + "types.wildcard_type_name.emptyOwnerSegment" + ))] + EmptyOwnerSegment, + + /// Indicates that the provided wildcard type name is invalid because it contains invalid + /// characters in a namespace segment (any segments between the owner and the name, separated by + /// dots (`.`)). + /// + /// Each namespace segment must contain only unicode alphanumeric characters, underscores, or + /// wildcard characters (`*`). If it contains any other characters, validation raises this + /// error in the `errors` field of the main [`InvalidTypeName`] error. + /// + /// [`InvalidTypeName`]: Self::InvalidTypeName + #[error("{t}", t = t!( + "types.wildcard_type_name.invalidNamespaceSegment", + "namespace" => segment_text + ))] + InvalidNamespaceSegment { + /// The namespace segment text that contains invalid characters. + segment_text: String, + }, + + /// Indicates that the provided wildcard type name is invalid because it contains + /// an empty namespace segment (i.e., two consecutive dots with no characters in between). + /// + /// If the wildcard type name contains any empty namespace segments, validation raises + /// this error in the `errors` field of the main [`InvalidTypeName`] error. + /// + /// [`InvalidTypeName`]: Self::InvalidTypeName + #[error("{t}", t = t!( + "types.wildcard_type_name.emptyNamespaceSegment", + "index" => index + ))] + EmptyNamespaceSegment { + /// The 1-based index of the empty namespace segment in the wildcard type name. + index: usize, + }, + + /// Indicates that the provided wildcard type name is invalid because it contains invalid + /// characters in the name segment (the last segment, which follows a forward slash (`/`)). + /// + /// The name segment must contain only unicode alphanumeric characters, underscores, or wildcard + /// characters (`*`). If it contains any other characters, validation raises this error in the + /// `errors` field of the main [`InvalidTypeName`] error. + /// + /// [`InvalidTypeName`]: Self::InvalidTypeName + #[error("{t}", t = t!( + "types.wildcard_type_name.invalidNameSegment", + "name" => segment_text + ))] + InvalidNameSegment { + /// The name segment text that contains invalid characters. + segment_text: String, + }, + + /// Indicates that the provided wildcard type name is invalid because it is missing the + /// required name segment (i.e., the segment after the forward slash (`/`)). + /// + /// A fully qualified type name must include a name segment. For a wildcard type name, the + /// name segment can only be empty when a prior segment contains a wildcard that can match the + /// name segment. + /// + /// For example, `Contoso.*.Example` would not be valid because the literal + /// namespace segment after the wildcard can never match a [`FullyQualifiedTypeName`] where + /// the last namespace is always followed by a slash and the name segment. + /// + /// The input strings `Contoso*` and `Contoso.Example*` are valid because there are no literal + /// matches after the wildcards that would prevent matching a name segment. + /// + /// If the input string is defined in a way that can't match a valid [`FullyQualifiedTypeName`] + /// because of a missing name segment, validation raises this error in the `errors` field of + /// the main [`InvalidTypeName`] error. + /// + /// [`InvalidTypeName`]: Self::InvalidTypeName + #[error("{t}", t = t!( + "types.wildcard_type_name.missingNameSegment" + ))] + MissingNameSegment, + + /// Indicates that the provided wildcard type name is invalid because it could not be converted + /// into a valid regex pattern, which is necessary for matching candidate + /// [`FullyQualifiedTypeName`]s against the wildcard pattern. + /// + /// This can occur if the wildcard characters are used in a way that results in an invalid + /// regex pattern when converted, such as defining consecutive wildcard characters without + /// proper escaping. + #[error("{t}", t = t!( + "types.wildcard_type_name.invalidRegex", + "name" => text, + "err" => err + ))] + InvalidRegex { + /// The input text that failed to parse as a wildcard type name. + text: String, + /// The specific regex error that was encountered when attempting to convert the wildcard + /// type name into a regex pattern for matching. + /// + /// This error provides details about why the regex pattern was invalid, which can help + /// with diagnosing issues in the wildcard type name format. + #[source] + err: regex::Error, + }, +} + +/// This static lazily defines the validating regex for [`WildcardTypeName`]. It enables the +/// [`Regex`] instance to be constructed once, the first time it's used, and then reused on all +/// subsequent validation calls. It's kept private, since the API usage is to invoke the +/// [`WildcardTypeName::parse()`] method for direct validation or to leverage this static from +/// within the constructor for [`WildcardTypeName`]. +static VALIDATING_SEGMENT_REGEX: OnceLock = OnceLock::new(); + +impl WildcardTypeName { + /// Parses a string into a [`WildcardTypeName`] instance. + /// + /// # Arguments + /// + /// - `text` - The input string to parse as a wildcard type name. + /// + /// # Errors + /// + /// This function returns a [`WildcardTypeNameError`] if the input string is not a valid + /// wildcard type name. The error will indicate which validation rules were violated and + /// include diagnostics for every validation error encountered during parsing. + /// + /// # Returns + /// + /// A result containing the parsed [`WildcardTypeName`] or an error if the input is invalid. + /// + /// # Examples + /// + /// The following snippets show how various valid input strings are parsed: + /// + /// - Single wildcard character, matching any type name: + /// + /// ```rust + /// # use dsc_lib::types::WildcardTypeName; + /// let _ = WildcardTypeName::parse("*").unwrap(); + /// ``` + /// + /// - Wildcard character in the name segment, matching any type name with the specified owner + /// and namespaces: + /// + /// ```rust + /// # use dsc_lib::types::WildcardTypeName; + /// let _ = WildcardTypeName::parse("Contoso.Example/*").unwrap(); + /// ``` + /// + /// - Wildcard character in the namespace segment, matching any type name with the specified + /// owner and first namespace: + /// + /// ```rust + /// # use dsc_lib::types::WildcardTypeName; + /// let _ = WildcardTypeName::parse("Contoso.Example.*").unwrap(); + /// ``` + /// + /// The following snippets show examples of invalid input strings and the corresponding errors: + /// + /// - An empty input string: + /// + /// ```rust + /// # use dsc_lib::types::{WildcardTypeName, WildcardTypeNameError}; + /// # use pretty_assertions::assert_eq; + /// assert_eq!( + /// WildcardTypeName::parse("").unwrap_err(), + /// WildcardTypeNameError::EmptyTypeName + /// ); + /// ``` + /// + /// - A string without any wildcard characters: + /// + /// ```rust + /// # use dsc_lib::types::{WildcardTypeName, WildcardTypeNameError}; + /// # use pretty_assertions::assert_eq; + /// assert_eq!( + /// WildcardTypeName::parse("Contoso.Example/Resource").unwrap_err(), + /// WildcardTypeNameError::NoWildcard { + /// text: "Contoso.Example/Resource".to_string() + /// } + /// ); + /// ``` + /// + /// - An empty owner segment before the first namespace: + /// + /// ```rust + /// # use dsc_lib::types::{WildcardTypeName, WildcardTypeNameError}; + /// # use pretty_assertions::assert_eq; + /// assert_eq!( + /// WildcardTypeName::parse(".Example/Resource*").unwrap_err(), + /// WildcardTypeNameError::InvalidTypeName { + /// text: ".Example/Resource*".to_string(), + /// errors: vec![ + /// WildcardTypeNameError::EmptyOwnerSegment + /// ], + /// } + /// ); + /// ``` + /// + /// - An empty owner segment before the slash: + /// + /// ```rust + /// # use dsc_lib::types::{WildcardTypeName, WildcardTypeNameError}; + /// # use pretty_assertions::assert_eq; + /// assert_eq!( + /// WildcardTypeName::parse("/Resource*").unwrap_err(), + /// WildcardTypeNameError::InvalidTypeName { + /// text: "/Resource*".to_string(), + /// errors: vec![ + /// WildcardTypeNameError::EmptyOwnerSegment + /// ], + /// } + /// ); + /// ``` + /// + /// - An empty namespace segment: + /// + /// ```rust + /// # use dsc_lib::types::{WildcardTypeName, WildcardTypeNameError}; + /// # use pretty_assertions::assert_eq; + /// assert_eq!( + /// WildcardTypeName::parse("Contoso..Example/Resource*").unwrap_err(), + /// WildcardTypeNameError::InvalidTypeName { + /// text: "Contoso..Example/Resource*".to_string(), + /// errors: vec![ + /// WildcardTypeNameError::EmptyNamespaceSegment { + /// index: 1 + /// } + /// ], + /// } + /// ); + /// ``` + /// + /// - Missing name segment when required because no prior wildcard can match the name segment: + /// + /// ```rust + /// # use dsc_lib::types::{WildcardTypeName, WildcardTypeNameError}; + /// # use pretty_assertions::assert_eq; + /// assert_eq!( + /// WildcardTypeName::parse("Contoso.*.Example/").unwrap_err(), + /// WildcardTypeNameError::InvalidTypeName { + /// text: "Contoso.*.Example/".to_string(), + /// errors: vec![ + /// WildcardTypeNameError::MissingNameSegment + /// ], + /// } + /// ); + /// ``` + /// + /// - Invalid characters in the any segment: + /// + /// ```rust + /// # use dsc_lib::types::{WildcardTypeName, WildcardTypeNameError}; + /// # use pretty_assertions::assert_eq; + /// assert_eq!( + /// WildcardTypeName::parse("Conto$o.Ex@mple/Re$ource*").unwrap_err(), + /// WildcardTypeNameError::InvalidTypeName { + /// text: "Conto$o.Ex@mple/Re$ource*".to_string(), + /// errors: vec![ + /// WildcardTypeNameError::InvalidOwnerSegment { + /// segment_text: "Conto$o".to_string(), + /// }, + /// WildcardTypeNameError::InvalidNamespaceSegment { + /// segment_text: "Ex@mple".to_string(), + /// }, + /// WildcardTypeNameError::InvalidNameSegment { + /// segment_text: "Re$ource*".to_string(), + /// }, + /// ], + /// } + /// ); + /// ``` + pub fn parse(text: &str) -> Result { + // If the text is empty, it's not a valid wildcard type name. Fail fast in this case to + // avoid unnecessary processing and provide a clearer error message. + if text.is_empty() { + return Err(WildcardTypeNameError::EmptyTypeName); + } + + // If the text doesn't contain any wildcard characters, it's not a valid wildcard type name, + // even if it would otherwise match the fully qualified type name pattern. Fail fast in + // this case to avoid unnecessary regex validation and to provide a clearer error message + // about the specific issue. + if !text.contains('*') { + return Err(WildcardTypeNameError::NoWildcard { + text: text.to_string(), + }); + } + + // We need to validate each segment of the type name separately to provide better error + // messages about which specific segment(s) contain invalid characters. We also need to + // allow for the presence of wildcard characters in any segment, which means we can't rely + // on the same regex pattern used for fully qualified type names and instead need a simpler + // pattern that just checks for valid characters within each segment. + // + // We also want to collect all segment errors rather than failing on the first invalid + // segment, to provide more comprehensive feedback to the user. + let errors = &mut Vec::::new(); + let owner: String; + let namespaces: Vec; + let require_name: bool; + let name: String; + let regex: Regex; + let validating_segment_regex = Self::init_validating_segment_regex(); + + // Split the input into owner/namespaces and name segments + if let Some((owner_and_namespaces, name_segment)) = text.rsplit_once('/') { + name = name_segment.to_string(); + let mut segments = owner_and_namespaces + .split('.') + .map(|s| s.to_string()) + .collect::>(); + owner = segments.remove(0); + namespaces = segments; + } else if text.contains('.') { + let mut segments = text + .split('.') + .map(|s| s.to_string()) + .collect::>(); + owner = segments.remove(0); + namespaces = segments; + name = String::new(); + } else { + owner = text.to_string(); + namespaces = Vec::new(); + name = String::new(); + } + + // Validate the owner segment, which _must_ be defined. + if owner.is_empty() { + errors.push(WildcardTypeNameError::EmptyOwnerSegment); + } else if !validating_segment_regex.is_match(&owner) { + errors.push(WildcardTypeNameError::InvalidOwnerSegment { + segment_text: owner.clone(), + }); + } + + // Validate every defined namespace segment + for (index, namespace) in (&namespaces).into_iter().enumerate() { + if namespace.is_empty() { + errors.push(WildcardTypeNameError::EmptyNamespaceSegment { + // Insert the index as 1-based for more user-friendly error messages + index: index + 1 + }); + } else if !validating_segment_regex.is_match(&namespace) { + errors.push(WildcardTypeNameError::InvalidNamespaceSegment { + segment_text: namespace.clone(), + }); + } + } + + // The name segment is required if no wildcard is defined that can match the name + // segment. For example, `Contoso.*.Example` would require a name segment because it + // defines a wildcard in a prior namespace segment with a following literal namespace + // segment. + require_name = if namespaces.is_empty() { + !owner.contains('*') + } else { + !namespaces.last().unwrap().contains('*') + }; + if name.is_empty() && require_name { + errors.push(WildcardTypeNameError::MissingNameSegment); + } + + // Validate the name segment for invalid characters if it's defined. + if !name.is_empty() && !validating_segment_regex.is_match(&name) { + errors.push(WildcardTypeNameError::InvalidNameSegment { + segment_text: name.clone(), + }); + } + + // Construct the regular expression. + let pattern = convert_wildcard_to_regex(text); + regex = match RegexBuilder::new(&pattern).case_insensitive(true).build() { + Ok(r) => r, + Err(err) => { + errors.push(WildcardTypeNameError::InvalidRegex { + text: text.to_string(), + err + }); + + // Placeholder regex since we have to return something, but this value won't be + // used since we'll return an error due to the invalid regex pattern. WIthout this + // placeholder, the function would fail to compile since the `regex` variable would + // be potentially uninitialized in the error case and we can't tell the compiler to + // ignore that even though the code logic guarantees it won't be used. + Regex::new("").unwrap() + } + }; + + // Return result and ensure all failures are bubbled up in the error case. + if errors.is_empty() { + Ok(Self { text: text.to_string(), regex }) + } else { + return Err(WildcardTypeNameError::InvalidTypeName { + text: text.to_string(), + errors: errors.clone(), + }); + } + } + + /// Returns `true` if the wildcard type name has a length of zero and otherwise `false`. + pub fn is_empty(&self) -> bool { + self.text.is_empty() + } + + /// Determines if a given candidate [`FullyQualifiedTypeName`] matches the wildcard pattern + /// defined by this [`WildcardTypeName`]. + /// + /// This method uses the compiled regex pattern corresponding to the wildcard type name to + /// check if the candidate fully qualified type name matches the wildcard pattern. + /// + /// # Arguments + /// + /// - `candidate` - The [`FullyQualifiedTypeName`] to check. + /// + /// # Returns + /// + /// `true` if the candidate fully qualified type name matches the wildcard pattern, and `false` + /// otherwise. + /// + /// # Examples + /// + /// The following snippet shows how a wildcard type name can be used to match candidate + /// fully qualified type names: + /// + /// ```rust + /// # use dsc_lib::types::{WildcardTypeName, FullyQualifiedTypeName}; + /// let wildcard = WildcardTypeName::parse("Contoso.*.Example/*").unwrap(); + /// assert!(wildcard.is_match( + /// &FullyQualifiedTypeName::parse("Contoso.A.Example/Resource").unwrap() + /// )); + /// assert!(wildcard.is_match( + /// &FullyQualifiedTypeName::parse("Contoso.B.Example/AnotherResource").unwrap() + /// )); + /// assert!(!wildcard.is_match( + /// &FullyQualifiedTypeName::parse("Contoso.Example/Resource").unwrap()) + /// ); + /// ``` + pub fn is_match(&self, candidate: &FullyQualifiedTypeName) -> bool { + self.regex.is_match(candidate.as_ref()) + } + + /// Returns a reference to the compiled regex pattern that corresponds to this wildcard type + /// name. + /// + /// The regular expression replaces the wildcard characters (`*`) in the original text with + /// the subexpression `.*?` to enable matching any sequence of characters in place of the + /// wildcard. Any other special regex characters in the original text are escaped to ensure + /// they are treated as literals in the regex pattern. The resulting regex pattern is anchored + /// to match the entire input string. + /// + /// # Returns + /// + /// A reference to the compiled regex pattern. + /// + /// # Examples + /// + /// The following snippet shows the regular expression for a [`WildcardTypeName`] defined as + /// a single wildcard character, which should match any fully qualified type name: + /// + /// ```rust + /// # use dsc_lib::types::WildcardTypeName; + /// # use pretty_assertions::assert_eq; + /// assert_eq!( + /// WildcardTypeName::parse("*").unwrap().as_regex().as_str(), + /// r"^.*?$" + /// ); + /// ``` + /// + /// The following snippet shows the regular expression for a [`WildcardTypeName`] defined as + /// `Contoso.*.Example/*`: + /// + /// ```rust + /// # use dsc_lib::types::WildcardTypeName; + /// # use pretty_assertions::assert_eq; + /// assert_eq!( + /// WildcardTypeName::parse("Contoso.*.Example/*").unwrap().as_regex().as_str(), + /// r"^Contoso\..*?\.Example/.*?$" + /// ); + /// + pub fn as_regex(&self) -> &Regex { + &self.regex + } + + /// Defines the regular expression for validating a segment in a wildcard type name. + /// + /// Each segment must contain only alphanumeric characters, underscores, or wildcard characters + /// (`*`). This regular expression is applied to each individual segment of the type name + /// (owner, namespaces, and name) rather than the entire type name string, since the presence + /// of wildcard characters means that the overall structure of the type name cannot be strictly + /// validated with a single regex pattern. + /// + /// This also obviates needing to check for the namespace separator (`.`) and type/name + /// separator (`/`) in the regex pattern, since the segments are validated individually after + /// slicing the input string based on those separators. + pub const VALIDATING_SEGMENT_PATTERN: &'static str = r"^(?:\w|\*)+$"; + + /// Initializes and returns the validating regex for wildcard type name segments. + fn init_validating_segment_regex() -> &'static Regex { + VALIDATING_SEGMENT_REGEX.get_or_init(|| Regex::new(Self::VALIDATING_SEGMENT_PATTERN).unwrap()) + } +} + +// Default to a single wildcard. +impl Default for WildcardTypeName { + fn default() -> Self { + Self { + text: String::from("*"), + regex: RegexBuilder::new(&convert_wildcard_to_regex("*")) + .case_insensitive(true) + .build() + .unwrap() + } + } +} + + +// We implement `PartialEq` by hand for various types because WCTNs should be compared +// case insensitively. This obviates the need to `.to_string().to_lowercase()` for comparisons. +impl Eq for WildcardTypeName {} +impl PartialEq for WildcardTypeName { + fn eq(&self, other: &Self) -> bool { + self.text.to_lowercase() == other.text.to_lowercase() + } +} + +impl PartialEq for WildcardTypeName { + fn eq(&self, other: &String) -> bool { + self.text.to_lowercase() == other.to_lowercase() + } +} + +impl PartialEq for WildcardTypeName { + fn eq(&self, other: &str) -> bool { + self.text.to_lowercase() == other.to_lowercase() + } +} + +impl PartialEq<&str> for WildcardTypeName { + fn eq(&self, other: &&str) -> bool { + self.text.to_lowercase() == other.to_lowercase() + } +} + +// Enables using the construct `"Owner/Name".parse()` to convert a literal string into a WCTN. +impl FromStr for WildcardTypeName { + type Err = WildcardTypeNameError; + fn from_str(s: &str) -> Result { + Self::parse(s) + } +} + +// Enables converting from a `String` and raising the appropriate error message for an invalid +// FQTN. +impl TryFrom for WildcardTypeName { + type Error = WildcardTypeNameError; + fn try_from(value: String) -> Result { + Self::parse(value.as_str()) + } +} + +// Enables converting an FQTN into a string. +impl From for String { + fn from(value: WildcardTypeName) -> Self { + value.text + } +} + +impl From for Regex { + fn from(value: WildcardTypeName) -> Self { + value.regex + } +} + +// Enables using WCTNs in `format!()` and similar macros. +impl Display for WildcardTypeName { + fn fmt(&self, f: &mut Formatter) -> std::fmt::Result { + write!(f, "{}", self.text) + } +} + +// Enables passing a WCTN as `&str` +impl AsRef for WildcardTypeName { + fn as_ref(&self) -> &str { + &self.text + } +} + +impl Hash for WildcardTypeName { + fn hash(&self, state: &mut H) { + self.text.to_lowercase().hash(state); + } +} diff --git a/lib/dsc-lib/tests/integration/types/mod.rs b/lib/dsc-lib/tests/integration/types/mod.rs index b3b1a3f06..fe25a25fc 100644 --- a/lib/dsc-lib/tests/integration/types/mod.rs +++ b/lib/dsc-lib/tests/integration/types/mod.rs @@ -21,3 +21,5 @@ mod semantic_version_req; mod tag; #[cfg(test)] mod tag_list; +#[cfg(test)] +mod wildcard_type_name; diff --git a/lib/dsc-lib/tests/integration/types/wildcard_type_name.rs b/lib/dsc-lib/tests/integration/types/wildcard_type_name.rs new file mode 100644 index 000000000..ccaecccb2 --- /dev/null +++ b/lib/dsc-lib/tests/integration/types/wildcard_type_name.rs @@ -0,0 +1,398 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +#[cfg(test)] +mod methods { + use dsc_lib::types::WildcardTypeName; + use test_case::test_case; + + #[cfg(test)] + mod parse { + use dsc_lib::types::{WildcardTypeName, WildcardTypeNameError}; + use test_case::test_case; + + #[test_case("*" => matches Ok(_); "single wildcard parses successfully")] + #[test_case("**" => matches Ok(_); "consecutive wildcards parse successfully")] + #[test_case("Contoso.Example/*" => matches Ok(_); "wildcard name parses successfully")] + #[test_case("Contoso.*/Resource" => matches Ok(_); "wildcard namespace parses successfully")] + #[test_case("*.Example/Resource" => matches Ok(_); "wildcard owner parses successfully")] + #[test_case("Contoso.*.Example/*" => matches Ok(_); "multiple wildcards parse successfully")] + #[test_case("Contoso*Owner" => matches Ok(_); "wildcard in owner without other segments parses successfully")] + #[test_case("Contoso.Example*Namespace" => matches Ok(_); "wildcard in last namespace without name segment parses successfully")] + fn valid(text: &str) -> Result { + WildcardTypeName::parse(text) + } + + #[test_case("" => + WildcardTypeNameError::EmptyTypeName; + "empty string" + )] + #[test_case("Type.Name.Without/Wildcards" => + WildcardTypeNameError::NoWildcard { + text: "Type.Name.Without/Wildcards".to_string() + }; "missing wildcard character" + )] + #[test_case("Invalid&Characters/In*Owner" => + WildcardTypeNameError::InvalidTypeName { + text: "Invalid&Characters/In*Owner".to_string(), + errors: vec![ + WildcardTypeNameError::InvalidOwnerSegment { + segment_text: "Invalid&Characters".to_string(), + }, + ] + }; "invalid characters in owner segment" + )] + #[test_case(".Empty.Owner/*" => + WildcardTypeNameError::InvalidTypeName { + text: ".Empty.Owner/*".to_string(), + errors: vec![ + WildcardTypeNameError::EmptyOwnerSegment, + ] + }; "empty owner segment" + )] + #[test_case("Owner.Invalid&Characters.*/InNameSpace" => + WildcardTypeNameError::InvalidTypeName { + text: "Owner.Invalid&Characters.*/InNameSpace".to_string(), + errors: vec![ + WildcardTypeNameError::InvalidNamespaceSegment { + segment_text: "Invalid&Characters".to_string(), + }, + ] + }; "invalid characters in namespace segment" + )] + #[test_case("Owner.With.Empty..Namespace/*" => + WildcardTypeNameError::InvalidTypeName { + text: "Owner.With.Empty..Namespace/*".to_string(), + errors: vec![ + WildcardTypeNameError::EmptyNamespaceSegment { + index: 3, + }, + ] + }; "empty namespace segment" + )] + #[test_case("Owner.*/Invalid&CharactersInName" => + WildcardTypeNameError::InvalidTypeName { + text: "Owner.*/Invalid&CharactersInName".to_string(), + errors: vec![ + WildcardTypeNameError::InvalidNameSegment { + segment_text: "Invalid&CharactersInName".to_string(), + }, + ] + }; "invalid characters in name segment" + )] + #[test_case("Owner.*.NamespaceWithoutNameSegment" => + WildcardTypeNameError::InvalidTypeName { + text: "Owner.*.NamespaceWithoutNameSegment".to_string(), + errors: vec![ + WildcardTypeNameError::MissingNameSegment, + ] + }; "missing name segment when wildcard is in prior namespace segment" + )] + #[test_case("Owner*.Namespace" => + WildcardTypeNameError::InvalidTypeName { + text: "Owner*.Namespace".to_string(), + errors: vec![ + WildcardTypeNameError::MissingNameSegment, + ] + }; "missing name segment when wildcard is in owner segment succeeded by a namespace" + )] + #[test_case("Invalid&Owner.With.Empty..Namespace/And&Invalid*Name" => + WildcardTypeNameError::InvalidTypeName { + text: "Invalid&Owner.With.Empty..Namespace/And&Invalid*Name".to_string(), + errors: vec![ + WildcardTypeNameError::InvalidOwnerSegment { + segment_text: "Invalid&Owner".to_string(), + }, + WildcardTypeNameError::EmptyNamespaceSegment { + index: 3, + }, + WildcardTypeNameError::InvalidNameSegment { + segment_text: "And&Invalid*Name".to_string(), + }, + ] + }; "reports all errors in the wildcard type name" + )] + fn invalid(text: &str) -> WildcardTypeNameError { + WildcardTypeName::parse(text).unwrap_err() + } + } + + #[test_case(&WildcardTypeName::default() => false; "default instance is not empty")] + #[test_case(&WildcardTypeName::parse("Contoso.Example/*").unwrap() => false; "wildcard name is not empty")] + fn is_empty(instance: &WildcardTypeName) -> bool { + instance.is_empty() + } + + #[test_case( + &WildcardTypeName::parse("Contoso.Example/*").unwrap(), + vec!["Contoso.Example/Resource", "Contoso.Example/OtherResource"], + true; + "matches candidate with same owner and namespace" + )] + #[test_case( + &WildcardTypeName::parse("Contoso.Example/*").unwrap(), + vec!["Contoso.OtherExample/Resource", "OtherContoso.Example/Resource"], + false; + "not matches candidate with different owner or namespace" + )] + #[test_case( + &WildcardTypeName::parse("Contoso.*").unwrap(), + vec!["Contoso.Example/Resource", "Contoso.OtherExample/Resource"], + true; + "matches candidate with same owner and any namespace and name" + )] + #[test_case( + &WildcardTypeName::parse("Contoso.*").unwrap(), + vec!["OtherContoso.Example/Resource", "OtherContoso.OtherExample/Resource"], + false; + "not matches candidate with different owner" + )] + fn is_match(filter: &WildcardTypeName, candidates: Vec<&str>, should_match: bool) { + for candidate in candidates { + pretty_assertions::assert_eq!( + filter.is_match(&candidate.parse().unwrap()), + should_match, + "expected filter {filter} to {}match candidate {candidate}", + if should_match { "" } else { "not " } + ); + } + } + + #[test_case("*", r"^.*?$"; "regex for single wildcard")] + #[test_case("Contoso.Example/*", r"^Contoso\.Example/.*?$"; "regex for wildcard name")] + #[test_case("Contoso.*/Resource", r"^Contoso\..*?/Resource$"; "regex for wildcard namespace")] + #[test_case("*.Example/Resource", r"^.*?\.Example/Resource$"; "regex for wildcard owner")] + #[test_case("Contoso.*.Example/*", r"^Contoso\..*?\.Example/.*?$"; "regex for multiple wildcards")] + fn as_regex(text: &str, expected: &str) { + pretty_assertions::assert_eq!( + WildcardTypeName::parse(text).unwrap().as_regex().as_str(), + expected, + "expected wildcard type name '{text}' to convert to regex `{expected}`" + ); + } +} + +#[cfg(test)] +mod serde { + use dsc_lib::types::WildcardTypeName; + use serde_json::{json, Value}; + use test_case::test_case; + + #[test_case(json!("*") => matches Ok(_); "single wildcard string deserializes")] + #[test_case(json!("**") => matches Ok(_); "consecutive wildcards string deserializes")] + #[test_case(json!("Contoso.Example/*") => matches Ok(_); "wildcard name string deserializes")] + #[test_case(json!("Contoso.*/Resource") => matches Ok(_); "wildcard namespace string deserializes")] + #[test_case(json!("*.Example/Resource") => matches Ok(_); "wildcard owner string deserializes")] + #[test_case(json!("Contoso.*.Example/*") => matches Ok(_); "multiple wildcards string deserializes")] + #[test_case(json!("invalid_name") => matches Err(_); "invalid type name string fails")] + #[test_case(json!("") => matches Err(_); "empty string fails")] + #[test_case(json!(true) => matches Err(_); "boolean value fails")] + #[test_case(json!(1) => matches Err(_); "integer value fails")] + #[test_case(json!(1.2) => matches Err(_); "float value fails")] + #[test_case(json!({"filter": "*"}) => matches Err(_); "object value fails")] + #[test_case(json!(["*"]) => matches Err(_); "array value fails")] + #[test_case(serde_json::Value::Null => matches Err(_); "null value fails")] + fn deserialize(text: Value) -> Result { + serde_json::from_value(json!(text)) + } + + #[test_case("*" => json!("*"); "single wildcard serializes")] + #[test_case("**" => json!("**"); "consecutive wildcards serializes")] + #[test_case("Contoso.Example/*" => json!("Contoso.Example/*"); "wildcard name serializes")] + #[test_case("Contoso.*/Resource" => json!("Contoso.*/Resource"); "wildcard namespace serializes")] + #[test_case("*.Example/Resource" => json!("*.Example/Resource"); "wildcard owner serializes")] + #[test_case("Contoso.*.Example/*" => json!("Contoso.*.Example/*"); "multiple wildcards serializes")] + fn serialize(text: &str) -> Value { + let instance = WildcardTypeName::parse(text).unwrap(); + serde_json::to_value(instance).unwrap() + } +} + +// #[cfg(test)] +// mod traits { +// #[cfg(test)] +// mod default { +// use dsc_lib::types::WildcardTypeName; + +// #[test] +// fn default_is_empty() { +// let instance = WildcardTypeName::default(); +// assert!(instance.is_empty()) +// } +// } + +// #[cfg(test)] +// mod display { +// use dsc_lib::types::WildcardTypeName; +// use test_case::test_case; + +// #[test_case("Owner/Name", "Owner/Name"; "valid type name without namespace segments")] +// #[test_case("Owner.Namespace/Name", "Owner.Namespace/Name"; "valid type name with one namespace segment")] +// #[test_case("Owner.A.B.C/Name", "Owner.A.B.C/Name"; "valid type name with multiple namespace segments")] +// fn format(type_name: &str, expected: &str) { +// pretty_assertions::assert_eq!( +// format!("type name: '{}'", WildcardTypeName::parse(type_name).unwrap()), +// format!("type name: '{}'", expected) +// ) +// } + +// #[test_case("Owner/Name", "Owner/Name"; "valid type name without namespace segments")] +// #[test_case("Owner.Namespace/Name", "Owner.Namespace/Name"; "valid type name with one namespace segment")] +// #[test_case("Owner.A.B.C/Name", "Owner.A.B.C/Name"; "valid type name with multiple namespace segments")] +// fn to_string(text: &str, expected: &str) { +// pretty_assertions::assert_eq!( +// WildcardTypeName::parse(text).unwrap().to_string(), +// expected.to_string() +// ) +// } +// } + +// #[cfg(test)] +// mod from_str { +// use std::str::FromStr; + +// use dsc_lib::types::{WildcardTypeName, WildcardTypeNameError}; +// use test_case::test_case; + +// #[test_case("Owner/Name" => matches Ok(_); "valid type name without namespace segments parses from string")] +// #[test_case("Owner.Namespace/Name" => matches Ok(_); "valid type name with one namespace segment parses from string")] +// #[test_case("Owner.A.B.C/Name" => matches Ok(_); "valid type name with multiple namespace segments parses from string")] +// #[test_case("invalid_name" => matches Err(_); "invalid type name fails to parse from string")] +// fn from_str(text: &str) -> Result { +// WildcardTypeName::from_str(text) +// } + +// #[test_case("Owner/Name" => matches Ok(_); "valid type name without namespace segments parses from string")] +// #[test_case("Owner.Namespace/Name" => matches Ok(_); "valid type name with one namespace segment parses from string")] +// #[test_case("Owner.A.B.C/Name" => matches Ok(_); "valid type name with multiple namespace segments parses from string")] +// #[test_case("invalid_name" => matches Err(_); "invalid type name fails to parse from string")] +// fn parse(text: &str) -> Result { +// text.parse() +// } +// } + +// #[cfg(test)] +// mod try_from { +// use dsc_lib::types::{WildcardTypeName, WildcardTypeNameError}; +// use test_case::test_case; + +// #[test_case("Owner/Name" => matches Ok(_); "valid type name without namespace segments converts from string")] +// #[test_case("Owner.Namespace/Name" => matches Ok(_); "valid type name with one namespace segment converts from string")] +// #[test_case("Owner.A.B.C/Name" => matches Ok(_); "valid type name with multiple namespace segments converts from string")] +// #[test_case("invalid_name" => matches Err(_); "invalid type name fails to convert from string")] +// fn string(text: &str) -> Result { +// WildcardTypeName::try_from(text.to_string()) +// } + +// #[test_case("Owner/Name" => matches Ok(_); "valid type name without namespace segments converts from string")] +// #[test_case("Owner.Namespace/Name" => matches Ok(_); "valid type name with one namespace segment converts from string")] +// #[test_case("Owner.A.B.C/Name" => matches Ok(_); "valid type name with multiple namespace segments converts from string")] +// #[test_case("invalid_name" => matches Err(_); "invalid type name fails to convert from string")] +// fn str(text: &str) -> Result { +// WildcardTypeName::try_from(text) +// } +// } + +// #[cfg(test)] +// mod into { +// use dsc_lib::types::WildcardTypeName; + +// #[test] +// fn string() { +// let _: String = WildcardTypeName::parse("Owner/Name").unwrap().into(); +// } +// } + +// #[cfg(test)] +// mod as_ref { +// use dsc_lib::types::WildcardTypeName; + +// #[test] +// fn as_ref() { +// let _: &str = WildcardTypeName::parse("Owner/Name").unwrap().as_ref(); +// } +// } + +// #[cfg(test)] +// mod deref { +// use dsc_lib::types::WildcardTypeName; + +// #[test] +// fn to_lowercase() { +// let n = WildcardTypeName::parse("Owner.Namespace/Name").unwrap(); +// assert_eq!(n.to_lowercase(), "owner.namespace/name".to_string()); +// } +// } + +// #[cfg(test)] +// mod partial_eq { +// use dsc_lib::types::WildcardTypeName; +// use test_case::test_case; + +// #[test_case("Owner/Name", "Owner/Name", true; "identical type names are equal")] +// #[test_case("Owner.Namespace/Name", "owner.namespace/name", true; "type names with different casing are equal")] +// #[test_case("Owner/Name", "Owner.Namespace/Name", false; "type names with different namespaces are not equal")] +// #[test_case("Owner/Name", "Owner/OtherName", false; "type names with different name segments are not equal")] +// #[test_case("Owner.Namespace/Name", "OtherOwner.Namespace/Name", false; "type names with different owner segments are not equal")] +// fn fully_qualified_type_name(lhs: &str, rhs: &str, should_be_equal: bool) { +// if should_be_equal { +// assert_eq!( +// WildcardTypeName::parse(lhs).unwrap(), +// WildcardTypeName::parse(rhs).unwrap() +// ); +// } else { +// assert_ne!( +// WildcardTypeName::parse(lhs).unwrap(), +// WildcardTypeName::parse(rhs).unwrap() +// ); +// } +// } + +// #[test_case("Owner/Name", "Owner/Name", true; "identical type names are equal")] +// #[test_case("Owner.Namespace/Name", "owner.namespace/name", true; "type names with different casing are equal")] +// #[test_case("Owner/Name", "Owner.Namespace/Name", false; "type names with different namespaces are not equal")] +// #[test_case("Owner/Name", "Owner/OtherName", false; "type names with different name segments are not equal")] +// #[test_case("Owner.Namespace/Name", "OtherOwner.Namespace/Name", false; "type names with different owner segments are not equal")] +// #[test_case("Owner.Namespace/Name", "Not a FQTN", false; "type names are never equal to strings that can't parse as FQTNs")] +// fn string(type_name_string: &str, string_slice: &str, should_be_equal: bool) { +// let name = WildcardTypeName::parse(type_name_string).unwrap(); +// let string = string_slice.to_string(); + +// // Test equivalency bidirectionally +// pretty_assertions::assert_eq!( +// name == string, +// should_be_equal, +// "expected comparison of {name} and {string} to be {should_be_equal}" +// ); + +// pretty_assertions::assert_eq!( +// string == name, +// should_be_equal, +// "expected comparison of {string} and {name} to be {should_be_equal}" +// ); +// } + +// #[test_case("Owner/Name", "Owner/Name", true; "identical type names are equal")] +// #[test_case("Owner.Namespace/Name", "owner.namespace/name", true; "type names with different casing are equal")] +// #[test_case("Owner/Name", "Owner.Namespace/Name", false; "type names with different namespaces are not equal")] +// #[test_case("Owner/Name", "Owner/OtherName", false; "type names with different name segments are not equal")] +// #[test_case("Owner.Namespace/Name", "OtherOwner.Namespace/Name", false; "type names with different owner segments are not equal")] +// #[test_case("Owner.Namespace/Name", "Not a FQTN", false; "type names are never equal to strings that can't parse as FQTNs")] +// fn str(type_name_string: &str, string_slice: &str, should_be_equal: bool) { +// let name = WildcardTypeName::parse(type_name_string).unwrap(); + +// // Test equivalency bidirectionally +// pretty_assertions::assert_eq!( +// name == string_slice, +// should_be_equal, +// "expected comparison of {name} and {string_slice} to be {should_be_equal}" +// ); + +// pretty_assertions::assert_eq!( +// string_slice == name, +// should_be_equal, +// "expected comparison of {string_slice} and {name} to be {should_be_equal}" +// ); +// } +// } +// } From 7c97895b52ed3905b6abc4e96f535af16f2af7aa Mon Sep 17 00:00:00 2001 From: Mikey Lombardi Date: Wed, 25 Mar 2026 15:06:47 -0500 Subject: [PATCH 03/15] (GH-538) Define `TypeNameFilter` This change defines the new `TypeNameFilter` enum which has variants for literal type names (`FullyQualifiedTypeName`) and wildcard type names (`WildcardTypeName`). This change is required for updating the discovery components to cache discovered types with their `FullyQualifiedTypeName` and incorporate both precise and wildcard lookups for those types. Updates to the discovery implementation follow in a subsequent change. --- lib/dsc-lib/locales/en-us.toml | 8 +- lib/dsc-lib/src/dscerror.rs | 3 + lib/dsc-lib/src/types/mod.rs | 2 + lib/dsc-lib/src/types/type_name_filter.rs | 284 ++++++++++++++ lib/dsc-lib/tests/integration/types/mod.rs | 2 + .../integration/types/type_name_filter.rs | 347 ++++++++++++++++++ 6 files changed, 644 insertions(+), 2 deletions(-) create mode 100644 lib/dsc-lib/src/types/type_name_filter.rs create mode 100644 lib/dsc-lib/tests/integration/types/type_name_filter.rs diff --git a/lib/dsc-lib/locales/en-us.toml b/lib/dsc-lib/locales/en-us.toml index a9452f3ea..500b02a7a 100644 --- a/lib/dsc-lib/locales/en-us.toml +++ b/lib/dsc-lib/locales/en-us.toml @@ -771,8 +771,6 @@ invalidTagPrefix = "Invalid tag" invalidTagSuffix = "valid tags must match the following pattern" invalidExitCode = "Invalid key in 'exitCodes' map" invalidExitCodePlusPrefix = "Exit codes must not begin with a plus sign (+)" -invalidTypeNamePrefix = "Invalid type name" -invalidTypeNameSuffix = "valid resource type names must match the following pattern" unsupportedManifestVersion = "Unsupported manifest version" mustBe = "Must be" invalidFunctionParameterCount = "Invalid function parameter count for" @@ -869,6 +867,12 @@ missingOperator = "comparator '%{comparator}' doesn't define an operator" invalidWildcards = "comparator '%{comparator}' has invalid wildcard characters - must define wildcards as asterisks (`*`), not `x` or `X`" wildcardMajorVersion = "comparator '%{comparator}' defines the major version segment as a wildcard `%{wildcard}` instead of a literal number" +[types.type_name_filter] +invalidConversionToFullyQualifiedTypeName = "cannot convert wildcard type name filter '%{type_name}' to a fully qualified type name" +invalidConversionToWildcardTypeName = "cannot convert literal type name filter '%{type_name}' to a wildcard type name" +invalidLiteralTypeNameFilter = "invalid literal type name filter: %{err}" +invalidWildcardTypeNameFilter = "invalid wildcard type name filter: %{err}" + [types.wildcard_type_name] emptyNamespaceSegment = "namespace segment %{index} is empty" emptyOwnerSegment = "owner segment is empty" diff --git a/lib/dsc-lib/src/dscerror.rs b/lib/dsc-lib/src/dscerror.rs index 05f8d7a7c..18e488efa 100644 --- a/lib/dsc-lib/src/dscerror.rs +++ b/lib/dsc-lib/src/dscerror.rs @@ -161,6 +161,9 @@ pub enum DscError { #[error(transparent)] SemverReq(#[from] crate::types::SemanticVersionReqError), + #[error(transparent)] + TypeNameFilter(#[from] crate::types::TypeNameFilterError), + #[error("{t}: {0}", t = t!("dscerror.utf16Conversion"))] Utf16Conversion(#[from] std::string::FromUtf16Error), diff --git a/lib/dsc-lib/src/types/mod.rs b/lib/dsc-lib/src/types/mod.rs index 94c1c9c90..35814a048 100644 --- a/lib/dsc-lib/src/types/mod.rs +++ b/lib/dsc-lib/src/types/mod.rs @@ -21,5 +21,7 @@ mod tag; pub use tag::Tag; mod tag_list; pub use tag_list::TagList; +mod type_name_filter; +pub use type_name_filter::{TypeNameFilter, TypeNameFilterError}; mod wildcard_type_name; pub use wildcard_type_name::{WildcardTypeName, WildcardTypeNameError}; diff --git a/lib/dsc-lib/src/types/type_name_filter.rs b/lib/dsc-lib/src/types/type_name_filter.rs new file mode 100644 index 000000000..6de969184 --- /dev/null +++ b/lib/dsc-lib/src/types/type_name_filter.rs @@ -0,0 +1,284 @@ +use std::{fmt::Display, str::FromStr}; + +use miette::Diagnostic; +use rust_i18n::t; +use serde::{Deserialize, Serialize}; +use thiserror::Error; + +use crate::types::{FullyQualifiedTypeName, FullyQualifiedTypeNameError, WildcardTypeName, WildcardTypeNameError}; + +#[derive(Debug, Clone, Hash, PartialEq, Eq, Serialize, Deserialize)] +#[serde(try_from = "String", into = "String")] +pub enum TypeNameFilter { + /// Filter by exact type name. + Literal(FullyQualifiedTypeName), + /// Filter by wildcard type name, where `*` can be used as a wildcard character. + Wildcard(WildcardTypeName), +} + +/// Defines errors that can occur when parsing or working with a [`TypeNameFilter`]. +/// +/// This includes errors from parsing both [`FullyQualifiedTypeName`]s and [`WildcardTypeName`]s. +#[derive(Debug, Clone, Error, Diagnostic, PartialEq)] +pub enum TypeNameFilterError { + /// Indicates a parsing error for a [`Wildcard`] type name filter. + /// + /// [`Wildcard`]: TypeNameFilter::Wildcard + #[error("{t}", t = t!( + "types.type_name_filter.invalidWildcardTypeNameFilter", + "err" => source + ))] + InvalidWildcardTypeNameFilter{ + /// The source error that occurred while parsing the wildcard type name filter. + #[from] + source: WildcardTypeNameError, + }, + + /// Indicates a parsing error for a [`Literal`] type name filter. + /// + /// [`Literal`]: TypeNameFilter::Literal + #[error("{t}", t = t!( + "types.type_name_filter.invalidLiteralTypeNameFilter", + "err" => source + ))] + InvalidLiteralTypeNameFilter{ + /// The source error that occurred while parsing the literal type name filter. + #[from] + source: FullyQualifiedTypeNameError, + }, + + /// Indicates that conversion failed for a [`Literal`] instance into a [`WildcardTypeName`]. + /// + /// [`Literal`]: TypeNameFilter::Literal + #[error("{t}", t = t!( + "types.type_name_filter.invalidConversionToWildcardTypeName", + "type_name" => type_name + ))] + InvalidConversionToWildcardTypeName{ + /// The inner literal type name that failed to convert to a wildcard type name. + type_name: FullyQualifiedTypeName, + }, + + /// Indicates that conversion failed for a [`Wildcard`] instance into a + /// [`FullyQualifiedTypeName`]. + /// + /// [`Wildcard`]: TypeNameFilter::Wildcard + #[error("{t}", t = t!( + "types.type_name_filter.invalidConversionToFullyQualifiedTypeName", + "type_name" => type_name + ))] + InvalidConversionToFullyQualifiedTypeName{ + /// The inner wildcard type name that failed to convert to a fully qualified type name. + type_name: WildcardTypeName, + }, +} + +impl TypeNameFilter { + /// Parses a string into a `TypeNameFilter`. The string can be either a literal fully qualified + /// type name or a wildcard type name. + /// + /// If the input string contains a `*`, it will be parsed as a `WildcardTypeName`. Otherwise, + /// it will be parsed as a `FullyQualifiedTypeName`. + /// + /// # Arguments + /// + /// - `text` - The input string to parse. + /// + /// # Errors + /// + /// This function returns a [`TypeNameFilterError`] if the input string is not a valid literal + /// fully qualified type name ([`FullyQualifiedTypeName`]) or a valid wildcard type name + /// ([`WildcardTypeName`]). The error will indicate which type of parsing failed and include + /// diagnostics for every validation error encountered during parsing. + /// + /// # Returns + /// + /// A result containing the parsed [`TypeNameFilter`] or an error if the input is invalid. + /// + /// # Examples + /// + /// The following snippet shows how various valid input strings are parsed: + /// + /// ```rust + /// use dsc_lib::types::{TypeNameFilter, TypeNameFilterError}; + /// let literal = TypeNameFilter::parse("Contoso.Example/Resource").unwrap(); + /// assert!(matches!(literal, TypeNameFilter::Literal(_))); + /// let wildcard_name = TypeNameFilter::parse("Contoso.Example/*").unwrap(); + /// assert!(matches!(wildcard_name, TypeNameFilter::Wildcard(_))); + /// let wildcard_namespace = TypeNameFilter::parse("Contoso.*").unwrap(); + /// assert!(matches!(wildcard_namespace, TypeNameFilter::Wildcard(_))); + /// let wildcard_owner = TypeNameFilter::parse("*/Resource").unwrap(); + /// assert!(matches!(wildcard_owner, TypeNameFilter::Wildcard(_))); + /// ``` + /// + /// The following snippet shows how invalid input strings result in errors: + /// + /// ```rust + /// use dsc_lib::types::{TypeNameFilter, TypeNameFilterError}; + /// let invalid = TypeNameFilter::parse("Invalid/Name/With/Too/Many/Segments").unwrap_err(); + /// assert!(matches!(invalid, TypeNameFilterError::InvalidLiteralTypeNameFilter{..})); + /// let invalid_wildcard = TypeNameFilter::parse("Invalid*Wildcard!").unwrap_err(); + /// assert!(matches!(invalid_wildcard, TypeNameFilterError::InvalidWildcardTypeNameFilter{..})); + /// ``` + pub fn parse(text: &str) -> Result { + // If the text contains a '*', attempt to parse it as a WildcardTypeName. Otherwise, parse + // it as a FullyQualifiedTypeName. + if text.contains('*') { + let wildcard = WildcardTypeName::parse(text)?; + Ok(TypeNameFilter::Wildcard(wildcard)) + } else { + let literal = FullyQualifiedTypeName::parse(text)?; + Ok(TypeNameFilter::Literal(literal)) + } + } + + /// Checks if the filter is empty. A filter is considered empty if it does not contain any + /// valid type name information. + #[must_use] + pub fn is_empty(&self) -> bool { + match self { + TypeNameFilter::Literal(literal) => literal.is_empty(), + TypeNameFilter::Wildcard(wildcard) => wildcard.is_empty(), + } + } + + /// Checks if a given candidate [`FullyQualifiedTypeName`] matches the filter. + /// + /// For a literal filter, the match is exact. A candidate matches a literal filter only if it's + /// exactly equal to the literal type name. [`FullyQualifiedTypeName`] comparisons are + /// case-insensitive, so the candidate `Contoso.Example/Resource` would match a literal filter + /// defined as `contoso.example/resource`. + /// + /// For a wildcard filter, the match is based on the wildcard pattern. The wildcard filter is + /// converted to a regular expression where `*` matches any sequence of characters, and the + /// regex is anchored to match the entire candidate string. For example, a wildcard filter of + /// `Contoso*` would match candidates like `Contoso.Example/Resource` and `Contoso/Resource`. + /// + /// # Arguments + /// + /// - `candidate` - The fully qualified type name to check against the filter. + /// + /// # Returns + /// + /// `true` if the candidate matches the filter, `false` otherwise. + /// + /// # Examples + /// + /// The following snippet shows how candidates match against a literal filter: + /// + /// ```rust + /// use dsc_lib::types::{TypeNameFilter, FullyQualifiedTypeName}; + /// let filter = TypeNameFilter::Literal("Contoso.Example/Resource".parse().unwrap()); + /// // The candidate matches the filter exactly, including casing. + /// assert!(filter.is_match(&"Contoso.Example/Resource".parse().unwrap())); + /// // The candidate still matches the filter, even with different casing. + /// assert!(filter.is_match(&"contoso.example/resource".parse().unwrap())); + /// // The candidate does not match the filter if the text varies beyond casing. + /// assert!(!filter.is_match(&"Example.Contoso/Resource".parse().unwrap())); + /// ``` + /// + /// The following snippet shows how candidates match against a wildcard filter: + /// + /// ```rust + /// use dsc_lib::types::{TypeNameFilter, FullyQualifiedTypeName}; + /// let filter = TypeNameFilter::Wildcard("Contoso*".parse().unwrap()); + /// // The candidate matches the filter if it starts with "Contoso". + /// assert!(filter.is_match(&"Contoso.Example/Resource".parse().unwrap())); + /// assert!(filter.is_match(&"Contoso/Resource".parse().unwrap())); + /// // The candidate does not match the filter if it does not start with "Contoso". + /// assert!(!filter.is_match(&"Example.Contoso/Resource".parse().unwrap())); + /// ``` + pub fn is_match(&self, candidate: &FullyQualifiedTypeName) -> bool { + match self { + TypeNameFilter::Literal(literal) => literal == candidate, + TypeNameFilter::Wildcard(wildcard) => wildcard.is_match(candidate), + } + } +} + +impl Default for TypeNameFilter { + fn default() -> Self { + TypeNameFilter::Wildcard(WildcardTypeName::default()) + } +} + +impl Display for TypeNameFilter { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + TypeNameFilter::Literal(literal) => literal.fmt(f), + TypeNameFilter::Wildcard(wildcard) => wildcard.fmt(f), + } + } +} + +impl FromStr for TypeNameFilter { + type Err = TypeNameFilterError; + + fn from_str(s: &str) -> Result { + Self::parse(s) + } +} + +impl TryFrom for TypeNameFilter { + type Error = TypeNameFilterError; + + fn try_from(s: String) -> Result { + Self::parse(&s) + } +} + +impl From for String { + fn from(filter: TypeNameFilter) -> Self { + filter.to_string() + } +} + +impl TryFrom<&str> for TypeNameFilter { + type Error = TypeNameFilterError; + + fn try_from(s: &str) -> Result { + Self::parse(s) + } +} + +impl From for TypeNameFilter { + fn from(wildcard: WildcardTypeName) -> Self { + TypeNameFilter::Wildcard(wildcard) + } +} + +impl From for TypeNameFilter { + fn from(literal: FullyQualifiedTypeName) -> Self { + TypeNameFilter::Literal(literal) + } +} + +impl TryFrom for FullyQualifiedTypeName { + type Error = TypeNameFilterError; + + fn try_from(filter: TypeNameFilter) -> Result { + match filter { + TypeNameFilter::Literal(literal) => Ok(literal), + TypeNameFilter::Wildcard(wildcard) => Err( + TypeNameFilterError::InvalidConversionToFullyQualifiedTypeName{ + type_name: wildcard + } + ), + } + } +} + +impl TryFrom for WildcardTypeName { + type Error = TypeNameFilterError; + + fn try_from(filter: TypeNameFilter) -> Result { + match filter { + TypeNameFilter::Wildcard(wildcard) => Ok(wildcard), + TypeNameFilter::Literal(literal) => Err( + TypeNameFilterError::InvalidConversionToWildcardTypeName{ + type_name: literal + } + ), + } + } +} diff --git a/lib/dsc-lib/tests/integration/types/mod.rs b/lib/dsc-lib/tests/integration/types/mod.rs index fe25a25fc..76e5e35f5 100644 --- a/lib/dsc-lib/tests/integration/types/mod.rs +++ b/lib/dsc-lib/tests/integration/types/mod.rs @@ -22,4 +22,6 @@ mod tag; #[cfg(test)] mod tag_list; #[cfg(test)] +mod type_name_filter; +#[cfg(test)] mod wildcard_type_name; diff --git a/lib/dsc-lib/tests/integration/types/type_name_filter.rs b/lib/dsc-lib/tests/integration/types/type_name_filter.rs new file mode 100644 index 000000000..0583c749a --- /dev/null +++ b/lib/dsc-lib/tests/integration/types/type_name_filter.rs @@ -0,0 +1,347 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +macro_rules! literal { + ($text:literal) => { + dsc_lib::types::TypeNameFilter::Literal( + $text.parse().unwrap() + ) + }; +} + +macro_rules! wildcard { + ($text:literal) => { + dsc_lib::types::TypeNameFilter::Wildcard( + $text.parse().unwrap() + ) + }; +} + +macro_rules! fqtn { + ($text:literal) => { + dsc_lib::types::FullyQualifiedTypeName::parse($text).unwrap() + }; +} + +#[cfg(test)] +mod methods { + use dsc_lib::types::{FullyQualifiedTypeName, TypeNameFilter}; + use test_case::test_case; + + #[cfg(test)] + mod parse { + use dsc_lib::types::{ + TypeNameFilter, + TypeNameFilter::*, + TypeNameFilterError, + TypeNameFilterError::*, + }; + use test_case::test_case; + + #[test_case("Owner/Name" => + matches Literal(_); + "literal filter without namespace segments" + )] + #[test_case("Owner.Namespace/Name" => + matches Literal(_); + "literal filter with one namespace segment" + )] + #[test_case("Owner.A.B.C/Name" => + matches Literal(_); + "literal filter with multiple namespace segments" + )] + #[test_case("*" => + matches Wildcard(_); + "wildcard filter with single wildcard" + )] + #[test_case("**" => + matches Wildcard(_); + "wildcard filter with consecutive wildcards" + )] + #[test_case("Contoso.Example/*" => + matches Wildcard(_); + "wildcard filter with wildcard name" + )] + #[test_case("Contoso.*/Resource" => + matches Wildcard(_); + "wildcard filter with wildcard namespace" + )] + #[test_case("*.Example/Resource" => + matches Wildcard(_); + "wildcard filter with wildcard owner" + )] + #[test_case("Contoso.*.Example/*" => + matches Wildcard(_); + "wildcard filter with multiple wildcards" + )] + #[test_case("Contoso*Owner" => + matches Wildcard(_); + "wildcard filter with wildcard in owner without other segments" + )] + #[test_case("Contoso.Example*Namespace" => + matches Wildcard(_); + "wildcard filter with wildcard in last namespace without name segment" + )] + fn valid(text: &str) -> TypeNameFilter { + TypeNameFilter::parse(text).expect(&format!( + "Expected '{}' to be a valid type name filter, but parsing failed", + text + )) + } + + #[test_case("" => + matches InvalidLiteralTypeNameFilter{..}; + "empty string is not a valid literal filter" + )] + #[test_case("Owner.MissingName" => + matches InvalidLiteralTypeNameFilter{..}; + "literal filter missing forward slash" + )] + #[test_case("Owner.MissingName/" => + matches InvalidLiteralTypeNameFilter{..}; + "literal filter missing name after forward slash" + )] + #[test_case("Owner/Invalid&Name" => + matches InvalidLiteralTypeNameFilter{..}; + "literal filter with invalid character in name segment" + )] + #[test_case("Owner.ValidNamespace.Invalid&Namespace.ValidNamespace/Name" => + matches InvalidLiteralTypeNameFilter{..}; + "literal filter with invalid character in namespace segment" + )] + #[test_case(".Missing.Owner/Name" => + matches InvalidLiteralTypeNameFilter{..}; + "literal filter with missing owner segment before first namespace segment" + )] + #[test_case("/Name" => + matches InvalidLiteralTypeNameFilter{..}; + "literal filter with missing owner segment and leading slash" + )] + #[test_case("Owner.Empty.Namespace..Segment/Name" => + matches InvalidLiteralTypeNameFilter{..}; + "literal filter with empty namespace segment" + )] + #[test_case("Invalid&Owner/Name" => + matches InvalidLiteralTypeNameFilter{..}; + "literal filter with invalid character in owner segment" + )] + #[test_case("Invalid&Characters/In*Owner" => + matches InvalidWildcardTypeNameFilter{..}; + "wildcard filter with invalid characters in owner segment" + )] + #[test_case(".Empty.Owner/*" => + matches InvalidWildcardTypeNameFilter{..}; + "wildcard filter with missing owner segment before first namespace segment" + )] + #[test_case("Owner.Invalid&Characters.*/InNameSpace" => + matches InvalidWildcardTypeNameFilter{..}; + "wildcard filter with invalid characters in namespace segment" + )] + #[test_case("Owner.With.Empty..Namespace/*" => + matches InvalidWildcardTypeNameFilter{..}; + "wildcard filter with empty namespace segment" + )] + #[test_case("Owner.*/Invalid&CharactersInName" => + matches InvalidWildcardTypeNameFilter{..}; + "wildcard filter with invalid characters in name segment" + )] + #[test_case("Owner.*.NamespaceWithoutNameSegment" => + matches InvalidWildcardTypeNameFilter{..}; + "wildcard filter with wildcard in namespace but missing name segment" + )] + #[test_case("Owner*.Namespace" => + matches InvalidWildcardTypeNameFilter{..}; + "wildcard filter with wildcard in owner but missing name segment" + )] + fn invalid(text: &str) -> TypeNameFilterError { + TypeNameFilter::parse(text).expect_err(&format!( + "Expected '{}' to be an invalid type name filter, but parsing succeeded", + text + )) + } + } + + #[test_case(&TypeNameFilter::Literal(FullyQualifiedTypeName::default()) => true; "only the default literal filter is empty")] + #[test_case(&literal!("Contoso.Example/Resource") => false; "literal filter is never empty")] + #[test_case(&wildcard!("Contoso.Example/*") => false; "wildcard filter is never empty")] + fn is_empty(filter: &TypeNameFilter) -> bool { + filter.is_empty() + } + + #[test_case(&literal!("Contoso.Example/Resource"), &fqtn!("Contoso.Example/Resource") => true; "candidate matches literal filter exactly")] + #[test_case(&literal!("Contoso.Example/Resource"), &fqtn!("contoso.example/resource") => true; "candidate matches literal filter with different casing")] + #[test_case(&literal!("Contoso.Example/Resource"), &fqtn!("Example.Contoso/Resource") => false; "candidate does not match literal filter when text varies beyond casing")] + #[test_case(&wildcard!("Contoso*"), &fqtn!("Contoso.Example/Resource") => true; "candidate matches wildcard filter when it starts with the wildcard text")] + #[test_case(&wildcard!("Contoso*"), &fqtn!("Contoso/Resource") => true; "candidate matches wildcard filter when it starts with the wildcard text even without additional segments")] + #[test_case(&wildcard!("Contoso*"), &fqtn!("Example.Contoso/Resource") => false; "candidate does not match wildcard filter when it does not start with the wildcard text")] + fn is_match(filter: &TypeNameFilter, candidate: &FullyQualifiedTypeName) -> bool { + filter.is_match(candidate) + } +} + +#[cfg(test)] +mod serde { + use dsc_lib::types::TypeNameFilter; + use serde_json::{json, Value}; + use test_case::test_case; + + #[test_case(json!("Contoso.Example/Resource") => matches Ok(_); "literal filter string deserializes")] + #[test_case(json!("Contoso*") => matches Ok(_); "wildcard filter string deserializes")] + #[test_case(json!("") => matches Err(_); "empty string fails")] + #[test_case(json!(true) => matches Err(_); "boolean value fails")] + #[test_case(json!(1) => matches Err(_); "integer value fails")] + #[test_case(json!(1.2) => matches Err(_); "float value fails")] + #[test_case(json!({"filter": "*"}) => matches Err(_); "object value fails")] + #[test_case(json!(["*"]) => matches Err(_); "array value fails")] + #[test_case(serde_json::Value::Null => matches Err(_); "null value fails")] + fn deserialize(value: Value) -> Result { + serde_json::from_value(value) + } + + #[test_case(&literal!("Contoso.Example/Resource") => + json!("Contoso.Example/Resource"); + "literal filter serializes to string" + )] + #[test_case(&wildcard!("Contoso*") => + json!("Contoso*"); + "wildcard filter serializes to string" + )] + fn serialize(filter: &TypeNameFilter) -> Value { + serde_json::to_value(filter).expect("serialize should never fail") + } +} + +#[cfg(test)] +mod traits { + #[cfg(test)] + mod default { + use dsc_lib::types::TypeNameFilter; + + #[test] + fn default() { + let default_filter = TypeNameFilter::default(); + assert_eq!( + default_filter, + TypeNameFilter::Wildcard(dsc_lib::types::WildcardTypeName::default()) + ); + } + } + + #[cfg(test)] + mod display { + use dsc_lib::types::TypeNameFilter; + use test_case::test_case; + + #[test_case(&literal!("Contoso/Resource") => "Contoso/Resource".to_string(); "literal filter text")] + #[test_case(&wildcard!("Contoso*") => "Contoso*".to_string(); "wildcard filter text")] + fn to_string(filter: &TypeNameFilter) -> String { + filter.to_string() + } + + #[test_case(&literal!("Contoso/Resource") => "Contoso/Resource".to_string(); "literal filter text")] + #[test_case(&wildcard!("Contoso*") => "Contoso*".to_string(); "wildcard filter text")] + fn format(filter: &TypeNameFilter) -> String { + format!("{}", filter) + } + } + + #[cfg(test)] + mod from_str { + use dsc_lib::types::{TypeNameFilter, TypeNameFilterError}; + use test_case::test_case; + + #[test_case("Contoso/Resource" => matches Ok(_); "literal filter string parses")] + #[test_case("Contoso*" => matches Ok(_); "wildcard filter string parses")] + #[test_case("" => matches Err(_); "empty string fails to parse")] + #[test_case("Invalid&Filter" => matches Err(_); "string with invalid characters fails to parse")] + fn from_str(text: &str) -> Result { + text.parse() + } + + #[test_case("Contoso/Resource" => matches Ok(_); "literal filter string parses")] + #[test_case("Contoso*" => matches Ok(_); "wildcard filter string parses")] + #[test_case("" => matches Err(_); "empty string fails to parse")] + #[test_case("Invalid&Filter" => matches Err(_); "string with invalid characters fails to parse")] + fn parse(text: &str) -> Result { + TypeNameFilter::parse(text) + } + } + + #[cfg(test)] + mod try_from { + use dsc_lib::types::{TypeNameFilter, TypeNameFilterError}; + use std::convert::TryFrom; + use test_case::test_case; + + #[test_case("Contoso/Resource" => matches Ok(_); "literal filter string parses")] + #[test_case("Contoso*" => matches Ok(_); "wildcard filter string parses")] + #[test_case("" => matches Err(_); "empty string fails to parse")] + #[test_case("Invalid&Filter" => matches Err(_); "string with invalid characters fails to parse")] + fn str(text: &str) -> Result { + TypeNameFilter::try_from(text) + } + + #[test_case("Contoso/Resource" => matches Ok(_); "literal filter string parses")] + #[test_case("Contoso*" => matches Ok(_); "wildcard filter string parses")] + #[test_case("" => matches Err(_); "empty string fails to parse")] + #[test_case("Invalid&Filter" => matches Err(_); "string with invalid characters fails to parse")] + fn string(text: &str) -> Result { + TypeNameFilter::try_from(text.to_string()) + } + } + + #[cfg(test)] + mod from { + use dsc_lib::types::{FullyQualifiedTypeName, TypeNameFilter, WildcardTypeName}; + + #[test] + fn fully_qualified_type_name() { + let filter = TypeNameFilter::from( + FullyQualifiedTypeName::parse("Contoso/Resource").unwrap() + ); + + assert!(matches!(filter, TypeNameFilter::Literal(_))); + assert_eq!(filter.to_string(), "Contoso/Resource"); + } + + #[test] + fn wildcard_type_name() { + let filter = TypeNameFilter::from( + WildcardTypeName::parse("Contoso*").unwrap() + ); + + assert!(matches!(filter, TypeNameFilter::Wildcard(_))); + assert_eq!(filter.to_string(), "Contoso*"); + } + } + + #[cfg(test)] + mod into { + use dsc_lib::types::TypeNameFilter; + use test_case::test_case; + + #[test_case(literal!("Contoso/Resource") => "Contoso/Resource".to_string(); "literal filter into string")] + #[test_case(wildcard!("Contoso*") => "Contoso*".to_string(); "wildcard filter into string")] + fn string(filter: TypeNameFilter) -> String { + filter.into() + } + } + + #[cfg(test)] + mod try_into { + use dsc_lib::types::{FullyQualifiedTypeName, TypeNameFilter, TypeNameFilterError, WildcardTypeName}; + use test_case::test_case; + + #[test_case(literal!("Contoso/Resource") => matches Ok(_); "literal filter converts")] + #[test_case(wildcard!("Contoso*") => matches Err(_); "wildcard filter fails")] + fn fully_qualified_type_name(filter: TypeNameFilter) -> Result { + filter.try_into() + } + + #[test_case(literal!("Contoso/Resource") => matches Err(_); "literal filter fails")] + #[test_case(wildcard!("Contoso*") => matches Ok(_); "wildcard filter converts")] + fn wildcard_type_name(filter: TypeNameFilter) -> Result { + filter.try_into() + } + } +} From 13e849f85db14341133e12b14158da90bbf6ef06 Mon Sep 17 00:00:00 2001 From: Mikey Lombardi Date: Wed, 25 Mar 2026 15:14:14 -0500 Subject: [PATCH 04/15] (GH-538) Define type aliases for caching `BTreeMap`s Prior to this change, the discovery implementation used `BTreeMap` directly for caching discovered resources and extensions. This made using those cache types verbose and difficult to update, requiring every output type and initialization to specify the full `BTreeMap` type signature and be modified everywhere. In preparation for updating the filtering logic work more idiomatically and safely on the version and type name structs, this change defines type aliases for each of the btree maps. This change _only_ affects the ergonomics, readability, and maintainability of the code. It doesn't affect the behavior at all. --- .../src/discovery/command_discovery.rs | 38 +++++++++---------- lib/dsc-lib/src/discovery/discovery_trait.rs | 16 +++++--- lib/dsc-lib/src/discovery/mod.rs | 23 ++++++++--- 3 files changed, 47 insertions(+), 30 deletions(-) diff --git a/lib/dsc-lib/src/discovery/command_discovery.rs b/lib/dsc-lib/src/discovery/command_discovery.rs index cdb899744..5f1877ef3 100644 --- a/lib/dsc-lib/src/discovery/command_discovery.rs +++ b/lib/dsc-lib/src/discovery/command_discovery.rs @@ -1,7 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -use crate::{discovery::{discovery_trait::{DiscoveryFilter, DiscoveryKind, ResourceDiscovery}, matches_adapter_requirement}, dscresources::adapted_resource_manifest::AdaptedDscResourceManifest, parser::Statement}; +use crate::{discovery::{DiscoveryExtensionCache, DiscoveryManifestCache, DiscoveryResourceCache, discovery_trait::{DiscoveryFilter, DiscoveryKind, ResourceDiscovery}, matches_adapter_requirement}, dscresources::adapted_resource_manifest::AdaptedDscResourceManifest, parser::Statement}; use crate::{locked_clear, locked_is_empty, locked_extend, locked_clone, locked_get}; use crate::configure::{config_doc::ResourceDiscoveryMode, context::Context}; use crate::dscresources::dscresource::{Capability, DscResource, ImplementedAs}; @@ -18,7 +18,7 @@ use rust_i18n::t; use semver::{Version, VersionReq}; use schemars::JsonSchema; use serde::Deserialize; -use std::{collections::{BTreeMap, HashMap, HashSet}, sync::{LazyLock, RwLock}}; +use std::{collections::{HashMap, HashSet}, sync::{LazyLock, RwLock}}; use std::env; use std::ffi::OsStr; use std::fs::{create_dir_all, read, read_to_string, write}; @@ -35,10 +35,10 @@ const DSC_MANIFEST_LIST_EXTENSIONS: [&str; 3] = [".dsc.manifests.json", ".dsc.ma const DSC_RESOURCE_EXTENSIONS: [&str; 3] = [".dsc.resource.json", ".dsc.resource.yaml", ".dsc.resource.yml"]; // use BTreeMap so that the results are sorted by the typename, the Vec is sorted by version -static ADAPTERS: LazyLock>>> = LazyLock::new(|| RwLock::new(BTreeMap::new())); -static RESOURCES: LazyLock>>> = LazyLock::new(|| RwLock::new(BTreeMap::new())); -static EXTENSIONS: LazyLock>> = LazyLock::new(|| RwLock::new(BTreeMap::new())); -static ADAPTED_RESOURCES: LazyLock>>> = LazyLock::new(|| RwLock::new(BTreeMap::new())); +static ADAPTERS: LazyLock> = LazyLock::new(|| RwLock::new(DiscoveryResourceCache::new())); +static RESOURCES: LazyLock> = LazyLock::new(|| RwLock::new(DiscoveryResourceCache::new())); +static EXTENSIONS: LazyLock> = LazyLock::new(|| RwLock::new(DiscoveryExtensionCache::new())); +static ADAPTED_RESOURCES: LazyLock> = LazyLock::new(|| RwLock::new(DiscoveryResourceCache::new())); #[derive(Deserialize, JsonSchema)] pub struct ManifestList { @@ -94,7 +94,7 @@ impl CommandDiscovery { } #[must_use] - pub fn get_extensions(&self) -> BTreeMap { locked_clone!(EXTENSIONS) } + pub fn get_extensions(&self) -> DiscoveryExtensionCache { locked_clone!(EXTENSIONS) } fn get_resource_path_setting() -> Result { @@ -246,9 +246,9 @@ impl ResourceDiscovery for CommandDiscovery { } } - let mut adapters = BTreeMap::>::new(); - let mut resources = BTreeMap::>::new(); - let mut extensions = BTreeMap::::new(); + let mut adapters = DiscoveryResourceCache::new(); + let mut resources = DiscoveryResourceCache::new(); + let mut extensions = DiscoveryExtensionCache::new(); if let Ok(paths) = CommandDiscovery::get_resource_paths() { for path in paths { @@ -394,7 +394,7 @@ impl ResourceDiscovery for CommandDiscovery { let mut progress = ProgressBar::new(adapters.len() as u64, self.progress_format)?; progress.write_activity("Searching for adapted resources"); - let mut adapted_resources = BTreeMap::>::new(); + let mut adapted_resources = DiscoveryResourceCache::new(); let mut found_adapter: bool = false; for (adapter_name, adapters) in &adapters { @@ -464,8 +464,8 @@ impl ResourceDiscovery for CommandDiscovery { Ok(()) } - fn list_available(&mut self, kind: &DiscoveryKind, type_name_filter: &str, adapter_name_filter: &str) -> Result>, DscError> { - let mut resources = BTreeMap::>::new(); + fn list_available(&mut self, kind: &DiscoveryKind, type_name_filter: &str, adapter_name_filter: &str) -> Result { + let mut resources = DiscoveryManifestCache::new(); if *kind == DiscoveryKind::Resource { if adapter_name_filter.is_empty() { self.discover(kind, type_name_filter)?; @@ -497,12 +497,12 @@ impl ResourceDiscovery for CommandDiscovery { Ok(resources) } - fn find_resources(&mut self, required_resource_types: &[DiscoveryFilter]) -> Result>, DscError> { + fn find_resources(&mut self, required_resource_types: &[DiscoveryFilter]) -> Result { debug!("{}", t!("discovery.commandDiscovery.searchingForResources", resources = required_resource_types : {:?})); if self.discovery_mode == ResourceDiscoveryMode::DuringDeployment || locked_is_empty!(RESOURCES) { self.discover(&DiscoveryKind::Resource, "*")?; } - let mut found_resources = BTreeMap::>::new(); + let mut found_resources = DiscoveryResourceCache::new(); let mut required_resources = HashMap::::new(); for filter in required_resource_types { required_resources.insert(filter.clone(), false); @@ -556,7 +556,7 @@ impl ResourceDiscovery for CommandDiscovery { Ok(found_resources) } - fn get_extensions(&mut self) -> Result, DscError> { + fn get_extensions(&mut self) -> Result { if locked_is_empty!(EXTENSIONS) { self.discover(&DiscoveryKind::Extension, "*")?; } @@ -568,7 +568,7 @@ impl ResourceDiscovery for CommandDiscovery { } } -fn filter_resources(found_resources: &mut BTreeMap>, required_resources: &mut HashMap, resources: &[DscResource], filter: &DiscoveryFilter) { +fn filter_resources(found_resources: &mut DiscoveryResourceCache, required_resources: &mut HashMap, resources: &[DscResource], filter: &DiscoveryFilter) { for resource in resources { if let Some(required_version) = filter.version() { if let Ok(resource_version) = Version::parse(&resource.version) { @@ -603,7 +603,7 @@ fn filter_resources(found_resources: &mut BTreeMap>, re } /// Inserts a resource into tree adding to vector if already exists -fn insert_resource(resources: &mut BTreeMap>, resource: &DscResource) { +fn insert_resource(resources: &mut DiscoveryResourceCache, resource: &DscResource) { if let Some(resource_versions) = resources.get_mut(&resource.type_name.to_lowercase()) { // compare the resource versions and insert newest to oldest using semver let mut insert_index = resource_versions.len(); @@ -921,7 +921,7 @@ fn verify_executable(resource: &str, operation: &str, executable: &str, director } } -fn add_resources_to_lookup_table(adapted_resources: &BTreeMap>) +fn add_resources_to_lookup_table(adapted_resources: &DiscoveryResourceCache) { let mut lookup_table = load_adapted_resources_lookup_table(); diff --git a/lib/dsc-lib/src/discovery/discovery_trait.rs b/lib/dsc-lib/src/discovery/discovery_trait.rs index ddc007198..3978d60bf 100644 --- a/lib/dsc-lib/src/discovery/discovery_trait.rs +++ b/lib/dsc-lib/src/discovery/discovery_trait.rs @@ -1,9 +1,8 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -use crate::{configure::config_doc::ResourceDiscoveryMode, dscerror::DscError, dscresources::dscresource::DscResource, extensions::dscextension::DscExtension}; -use std::collections::BTreeMap; -use super::{command_discovery::ImportedManifest, fix_semver}; +use crate::{configure::config_doc::ResourceDiscoveryMode, discovery::{DiscoveryExtensionCache, DiscoveryManifestCache, DiscoveryResourceCache}, dscerror::DscError}; +use super::fix_semver; #[derive(Debug, PartialEq)] pub enum DiscoveryKind { @@ -93,7 +92,12 @@ pub trait ResourceDiscovery { /// # Errors /// /// This function will return an error if the underlying discovery fails. - fn list_available(&mut self, kind: &DiscoveryKind, type_name_filter: &str, adapter_name_filter: &str) -> Result>, DscError>; + fn list_available( + &mut self, + kind: &DiscoveryKind, + type_name_filter: &str, + adapter_name_filter: &str + ) -> Result; /// Find resources based on the required resource types. /// This is not applicable for extensions. @@ -109,7 +113,7 @@ pub trait ResourceDiscovery { /// # Errors /// /// This function will return an error if the underlying discovery fails. - fn find_resources(&mut self, required_resource_types: &[DiscoveryFilter]) -> Result>, DscError>; + fn find_resources(&mut self, required_resource_types: &[DiscoveryFilter]) -> Result; /// Get the available extensions. /// @@ -120,7 +124,7 @@ pub trait ResourceDiscovery { /// # Errors /// /// This function will return an error if the underlying discovery fails. - fn get_extensions(&mut self) -> Result, DscError>; + fn get_extensions(&mut self) -> Result; /// Set the discovery mode. /// diff --git a/lib/dsc-lib/src/discovery/mod.rs b/lib/dsc-lib/src/discovery/mod.rs index 38c76113f..26625ea1c 100644 --- a/lib/dsc-lib/src/discovery/mod.rs +++ b/lib/dsc-lib/src/discovery/mod.rs @@ -15,10 +15,17 @@ use std::collections::BTreeMap; use command_discovery::{CommandDiscovery, ImportedManifest}; use tracing::error; +/// Defines the caching [`BTreeMap`] for discovered DSC extensions. +type DiscoveryExtensionCache = BTreeMap; +/// Defines the caching [`BTreeMap`] for discovered DSC manifests of any type. +type DiscoveryManifestCache = BTreeMap>; +/// Defines the caching [`BTreeMap`] for discovered DSC resources. +type DiscoveryResourceCache = BTreeMap>; + #[derive(Clone)] pub struct Discovery { - pub resources: BTreeMap>, - pub extensions: BTreeMap, + pub resources: DiscoveryResourceCache, + pub extensions: DiscoveryExtensionCache, pub refresh_cache: bool, } @@ -32,8 +39,8 @@ impl Discovery { #[must_use] pub fn new() -> Self { Self { - resources: BTreeMap::new(), - extensions: BTreeMap::new(), + resources: DiscoveryResourceCache::new(), + extensions: DiscoveryExtensionCache::new(), refresh_cache: false, } } @@ -49,7 +56,13 @@ impl Discovery { /// # Returns /// /// A vector of `DscResource` instances. - pub fn list_available(&mut self, kind: &DiscoveryKind, type_name_filter: &str, adapter_name_filter: &str, progress_format: ProgressFormat) -> Vec { + pub fn list_available( + &mut self, + kind: &DiscoveryKind, + type_name_filter: &str, + adapter_name_filter: &str, + progress_format: ProgressFormat + ) -> Vec { let discovery_types: Vec> = vec![ Box::new(command_discovery::CommandDiscovery::new(progress_format)), ]; From 4404cfb64355a859309547673c13a4e887bacb9c Mon Sep 17 00:00:00 2001 From: Mikey Lombardi Date: Wed, 25 Mar 2026 15:48:01 -0500 Subject: [PATCH 05/15] (GH-538) Use `FullyQualifiedTypeName` for discovery This change updates the `DiscoveryFilter` to use the parsed and validated `FullyQualifiedTypeName` instead of a raw string for the resource type and optional required adapter type. This more accurately reflects the underlying data for the caches and lookups and enables us to take advantage of the builtin parsing, comparison, and validation logic of `FullyQualifiedTypeName` instead of re-implementing that logic in the discovery code and on every access to these fields. --- .../src/discovery/command_discovery.rs | 28 +++++++++---------- lib/dsc-lib/src/discovery/discovery_trait.rs | 19 ++++++++----- lib/dsc-lib/src/discovery/mod.rs | 11 ++++---- 3 files changed, 32 insertions(+), 26 deletions(-) diff --git a/lib/dsc-lib/src/discovery/command_discovery.rs b/lib/dsc-lib/src/discovery/command_discovery.rs index 5f1877ef3..e1215d409 100644 --- a/lib/dsc-lib/src/discovery/command_discovery.rs +++ b/lib/dsc-lib/src/discovery/command_discovery.rs @@ -1,7 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -use crate::{discovery::{DiscoveryExtensionCache, DiscoveryManifestCache, DiscoveryResourceCache, discovery_trait::{DiscoveryFilter, DiscoveryKind, ResourceDiscovery}, matches_adapter_requirement}, dscresources::adapted_resource_manifest::AdaptedDscResourceManifest, parser::Statement}; +use crate::{discovery::{DiscoveryExtensionCache, DiscoveryManifestCache, DiscoveryResourceCache, discovery_trait::{DiscoveryFilter, DiscoveryKind, ResourceDiscovery}, matches_adapter_requirement}, dscresources::adapted_resource_manifest::AdaptedDscResourceManifest, parser::Statement, types::FullyQualifiedTypeName}; use crate::{locked_clear, locked_is_empty, locked_extend, locked_clone, locked_get}; use crate::configure::{config_doc::ResourceDiscoveryMode, context::Context}; use crate::dscresources::dscresource::{Capability, DscResource, ImplementedAs}; @@ -289,7 +289,7 @@ impl ResourceDiscovery for CommandDiscovery { if regex.is_match(&extension.type_name) { trace!("{}", t!("discovery.commandDiscovery.extensionFound", extension = extension.type_name, version = extension.version)); // we only keep newest version of the extension so compare the version and only keep the newest - if let Some(existing_extension) = extensions.get_mut(extension.type_name.as_ref()) { + if let Some(existing_extension) = extensions.get_mut(&extension.type_name) { let Ok(existing_version) = Version::parse(&existing_extension.version) else { return Err(DscError::Operation(t!("discovery.commandDiscovery.extensionInvalidVersion", extension = existing_extension.type_name, version = existing_extension.version).to_string())); }; @@ -297,10 +297,10 @@ impl ResourceDiscovery for CommandDiscovery { return Err(DscError::Operation(t!("discovery.commandDiscovery.extensionInvalidVersion", extension = extension.type_name, version = extension.version).to_string())); }; if new_version > existing_version { - extensions.insert(extension.type_name.to_string(), extension.clone()); + extensions.insert(extension.type_name.clone(), extension.clone()); } } else { - extensions.insert(extension.type_name.to_string(), extension.clone()); + extensions.insert(extension.type_name.clone(), extension.clone()); } } }, @@ -409,7 +409,7 @@ impl ResourceDiscovery for CommandDiscovery { let mut adapter_progress = ProgressBar::new(1, self.progress_format)?; adapter_progress.write_activity(format!("Enumerating resources for adapter '{adapter_name}'").as_str()); let Some(manifest) = &adapter.manifest else { - return Err(DscError::MissingManifest(adapter_name.clone())); + return Err(DscError::MissingManifest(adapter_name.to_string())); }; let mut adapter_resources_count = 0; @@ -435,7 +435,7 @@ impl ResourceDiscovery for CommandDiscovery { match serde_json::from_str::(line){ Result::Ok(resource) => { if resource.require_adapter.is_none() { - warn!("{}", DscError::MissingRequires(adapter_name.clone(), resource.type_name.to_string()).to_string()); + warn!("{}", DscError::MissingRequires(adapter_name.to_string(), resource.type_name.to_string()).to_string()); continue; } @@ -523,17 +523,17 @@ impl ResourceDiscovery for CommandDiscovery { } // store the keys of the ADAPTERS into a vec - let mut adapters: Vec = locked_clone!(ADAPTERS).keys().cloned().collect(); + let mut adapters: Vec = locked_clone!(ADAPTERS).keys().cloned().collect(); // sort the adapters by ones specified in the required resources first for filter in required_resource_types { if let Some(required_adapter) = filter.require_adapter() { - if !adapters.contains(&required_adapter.to_string()) { + if !adapters.contains(&required_adapter) { return Err(DscError::AdapterNotFound(required_adapter.to_string())); } // otherwise insert at the front of the list adapters.retain(|a| a != required_adapter); - adapters.insert(0, required_adapter.to_string()); + adapters.insert(0, required_adapter.clone()); } } @@ -574,7 +574,7 @@ fn filter_resources(found_resources: &mut DiscoveryResourceCache, required_resou if let Ok(resource_version) = Version::parse(&resource.version) { if let Ok(version_req) = VersionReq::parse(required_version) { if version_req.matches(&resource_version) && matches_adapter_requirement(resource, filter) { - found_resources.entry(filter.resource_type().to_string()).or_default().push(resource.clone()); + found_resources.entry(filter.resource_type().clone()).or_default().push(resource.clone()); required_resources.insert(filter.clone(), true); debug!("{}", t!("discovery.commandDiscovery.foundResourceWithVersion", resource = resource.type_name, version = resource.version)); break; @@ -583,7 +583,7 @@ fn filter_resources(found_resources: &mut DiscoveryResourceCache, required_resou } else { // if not semver, we do a string comparison if resource.version == *required_version && matches_adapter_requirement(resource, filter) { - found_resources.entry(filter.resource_type().to_string()).or_default().push(resource.clone()); + found_resources.entry(filter.resource_type().clone()).or_default().push(resource.clone()); required_resources.insert(filter.clone(), true); debug!("{}", t!("discovery.commandDiscovery.foundResourceWithVersion", resource = resource.type_name, version = resource.version)); break; @@ -591,7 +591,7 @@ fn filter_resources(found_resources: &mut DiscoveryResourceCache, required_resou } } else { if matches_adapter_requirement(resource, filter) { - found_resources.entry(filter.resource_type().to_string()).or_default().push(resource.clone()); + found_resources.entry(filter.resource_type().clone()).or_default().push(resource.clone()); required_resources.insert(filter.clone(), true); break; } @@ -604,7 +604,7 @@ fn filter_resources(found_resources: &mut DiscoveryResourceCache, required_resou /// Inserts a resource into tree adding to vector if already exists fn insert_resource(resources: &mut DiscoveryResourceCache, resource: &DscResource) { - if let Some(resource_versions) = resources.get_mut(&resource.type_name.to_lowercase()) { + if let Some(resource_versions) = resources.get_mut(&resource.type_name) { // compare the resource versions and insert newest to oldest using semver let mut insert_index = resource_versions.len(); for (index, resource_instance) in resource_versions.iter().enumerate() { @@ -628,7 +628,7 @@ fn insert_resource(resources: &mut DiscoveryResourceCache, resource: &DscResourc } resource_versions.insert(insert_index, resource.clone()); } else { - resources.insert(resource.type_name.to_lowercase(), vec![resource.clone()]); + resources.insert(resource.type_name.clone(), vec![resource.clone()]); } } diff --git a/lib/dsc-lib/src/discovery/discovery_trait.rs b/lib/dsc-lib/src/discovery/discovery_trait.rs index 3978d60bf..1a9b4b108 100644 --- a/lib/dsc-lib/src/discovery/discovery_trait.rs +++ b/lib/dsc-lib/src/discovery/discovery_trait.rs @@ -1,7 +1,12 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -use crate::{configure::config_doc::ResourceDiscoveryMode, discovery::{DiscoveryExtensionCache, DiscoveryManifestCache, DiscoveryResourceCache}, dscerror::DscError}; +use crate::{ + configure::config_doc::ResourceDiscoveryMode, + discovery::{DiscoveryExtensionCache, DiscoveryManifestCache, DiscoveryResourceCache}, + dscerror::DscError, + types::FullyQualifiedTypeName +}; use super::fix_semver; #[derive(Debug, PartialEq)] @@ -12,8 +17,8 @@ pub enum DiscoveryKind { #[derive(Debug, Clone, Eq, Hash, PartialEq)] pub struct DiscoveryFilter { - require_adapter: Option, - r#type: String, + require_adapter: Option, + r#type: FullyQualifiedTypeName, version: Option, } @@ -22,19 +27,19 @@ impl DiscoveryFilter { pub fn new(resource_type: &str, version: Option<&str>, adapter: Option<&str>) -> Self { let version = version.map(|v| fix_semver(&v)); Self { - require_adapter: adapter.map(|a| a.to_lowercase()), - r#type: resource_type.to_lowercase(), + require_adapter: adapter.map(|a| a.parse().unwrap()), + r#type: resource_type.parse().unwrap(), version, } } #[must_use] - pub fn require_adapter(&self) -> Option<&String> { + pub fn require_adapter(&self) -> Option<&FullyQualifiedTypeName> { self.require_adapter.as_ref() } #[must_use] - pub fn resource_type(&self) -> &str { + pub fn resource_type(&self) -> &FullyQualifiedTypeName { &self.r#type } diff --git a/lib/dsc-lib/src/discovery/mod.rs b/lib/dsc-lib/src/discovery/mod.rs index 26625ea1c..be677792a 100644 --- a/lib/dsc-lib/src/discovery/mod.rs +++ b/lib/dsc-lib/src/discovery/mod.rs @@ -8,6 +8,7 @@ use crate::configure::config_doc::ResourceDiscoveryMode; use crate::discovery::discovery_trait::{DiscoveryKind, ResourceDiscovery, DiscoveryFilter}; use crate::dscerror::DscError; use crate::extensions::dscextension::{Capability, DscExtension}; +use crate::types::FullyQualifiedTypeName; use crate::{dscresources::dscresource::DscResource, progress::ProgressFormat}; use core::result::Result::Ok; use semver::{Version, VersionReq}; @@ -16,11 +17,11 @@ use command_discovery::{CommandDiscovery, ImportedManifest}; use tracing::error; /// Defines the caching [`BTreeMap`] for discovered DSC extensions. -type DiscoveryExtensionCache = BTreeMap; +type DiscoveryExtensionCache = BTreeMap; /// Defines the caching [`BTreeMap`] for discovered DSC manifests of any type. -type DiscoveryManifestCache = BTreeMap>; +type DiscoveryManifestCache = BTreeMap>; /// Defines the caching [`BTreeMap`] for discovered DSC resources. -type DiscoveryResourceCache = BTreeMap>; +type DiscoveryResourceCache = BTreeMap>; #[derive(Clone)] pub struct Discovery { @@ -109,8 +110,8 @@ impl Discovery { self.find_resources(&[filter.clone()], ProgressFormat::None)?; } - let type_name = filter.resource_type().to_lowercase(); - if let Some(resources) = self.resources.get(&type_name) { + let type_name = filter.resource_type(); + if let Some(resources) = self.resources.get(type_name) { if let Some(version) = filter.version() { let version = fix_semver(version); if let Ok(version_req) = VersionReq::parse(&version) { From f7d5003e6cb0d24e038918e48ee1c89d99648769 Mon Sep 17 00:00:00 2001 From: Mikey Lombardi Date: Wed, 25 Mar 2026 16:13:06 -0500 Subject: [PATCH 06/15] (GH-538) Initial plumbing for `ResourceVersion` This change updates the following struct fields to use the `ResourceVersion` type instead of a string: - `dsc_lib::dscresources::dscresource::DscResource.version` - `dsc_lib::dscresources::resource_manifest::ResourceManifest.version` - `dsc_lib::dscresources::adapted_resource_manifest::AdaptedDscResourceManifest.version` - `dsc::mcp::show_dsc_resource::DscResource.version` This change _minimally_ updates the code to use `ResourceVersion` type, especially in discovery, but it doesn't yet take advantage of the richer semantics. This change is limited to updating the struct fields and making the code continue to compile for ease of reviewing. --- dsc/src/mcp/show_dsc_resource.rs | 4 ++-- dsc/src/subcommand.rs | 2 +- lib/dsc-lib/src/discovery/command_discovery.rs | 18 +++++++++--------- lib/dsc-lib/src/discovery/mod.rs | 2 +- .../dscresources/adapted_resource_manifest.rs | 9 ++++----- lib/dsc-lib/src/dscresources/dscresource.rs | 6 +++--- .../src/dscresources/resource_manifest.rs | 8 ++++---- tools/test_group_resource/src/main.rs | 10 +++++----- 8 files changed, 29 insertions(+), 30 deletions(-) diff --git a/dsc/src/mcp/show_dsc_resource.rs b/dsc/src/mcp/show_dsc_resource.rs index ae9dc50d4..d41d3b12f 100644 --- a/dsc/src/mcp/show_dsc_resource.rs +++ b/dsc/src/mcp/show_dsc_resource.rs @@ -8,7 +8,7 @@ use dsc_lib::{ dscresources::{ dscresource::{Capability, Invoke}, resource_manifest::Kind - }, types::FullyQualifiedTypeName, + }, types::{FullyQualifiedTypeName, ResourceVersion}, }; use rmcp::{ErrorData as McpError, Json, tool, tool_router, handler::server::wrapper::Parameters}; use rust_i18n::t; @@ -25,7 +25,7 @@ pub struct DscResource { /// The kind of resource. pub kind: Kind, /// The version of the resource. - pub version: String, + pub version: ResourceVersion, /// The capabilities of the resource. pub capabilities: Vec, /// The description of the resource. diff --git a/dsc/src/subcommand.rs b/dsc/src/subcommand.rs index fcdaf471c..a552c6a40 100644 --- a/dsc/src/subcommand.rs +++ b/dsc/src/subcommand.rs @@ -830,7 +830,7 @@ pub fn list_resources(dsc: &mut DscManager, resource_name: Option<&String>, adap table.add_row(vec![ resource.type_name.to_string(), format!("{:?}", resource.kind), - resource.version, + resource.version.to_string(), capabilities, resource.require_adapter.unwrap_or_default().to_string(), resource.description.unwrap_or_default() diff --git a/lib/dsc-lib/src/discovery/command_discovery.rs b/lib/dsc-lib/src/discovery/command_discovery.rs index e1215d409..1e6dfbf1a 100644 --- a/lib/dsc-lib/src/discovery/command_discovery.rs +++ b/lib/dsc-lib/src/discovery/command_discovery.rs @@ -571,7 +571,7 @@ impl ResourceDiscovery for CommandDiscovery { fn filter_resources(found_resources: &mut DiscoveryResourceCache, required_resources: &mut HashMap, resources: &[DscResource], filter: &DiscoveryFilter) { for resource in resources { if let Some(required_version) = filter.version() { - if let Ok(resource_version) = Version::parse(&resource.version) { + if let Some(resource_version) = resource.version.as_semver() { if let Ok(version_req) = VersionReq::parse(required_version) { if version_req.matches(&resource_version) && matches_adapter_requirement(resource, filter) { found_resources.entry(filter.resource_type().clone()).or_default().push(resource.clone()); @@ -608,15 +608,15 @@ fn insert_resource(resources: &mut DiscoveryResourceCache, resource: &DscResourc // compare the resource versions and insert newest to oldest using semver let mut insert_index = resource_versions.len(); for (index, resource_instance) in resource_versions.iter().enumerate() { - let resource_instance_version = match Version::parse(&resource_instance.version) { - Ok(v) => v, - Err(_err) => { + let resource_instance_version = match resource_instance.version.as_semver() { + Some(v) => v, + None => { continue; }, }; - let resource_version = match Version::parse(&resource.version) { - Ok(v) => v, - Err(_err) => { + let resource_version = match resource.version.as_semver() { + Some(v) => v, + None => { continue; }, }; @@ -785,7 +785,7 @@ pub fn load_manifest(path: &Path) -> Result, DscError> { } fn load_adapted_resource_manifest(path: &Path, manifest: &AdaptedDscResourceManifest) -> Result { - if let Err(err) = validate_semver(&manifest.version) { + if let Err(err) = validate_semver(&manifest.version.to_string()) { warn!("{}", t!("discovery.commandDiscovery.invalidManifestVersion", path = path.to_string_lossy(), err = err).to_string()); } @@ -813,7 +813,7 @@ fn load_adapted_resource_manifest(path: &Path, manifest: &AdaptedDscResourceMani } fn load_resource_manifest(path: &Path, manifest: &ResourceManifest) -> Result { - if let Err(err) = validate_semver(&manifest.version) { + if let Err(err) = validate_semver(&manifest.version.to_string()) { warn!("{}", t!("discovery.commandDiscovery.invalidManifestVersion", path = path.to_string_lossy(), err = err).to_string()); } diff --git a/lib/dsc-lib/src/discovery/mod.rs b/lib/dsc-lib/src/discovery/mod.rs index be677792a..3ec242719 100644 --- a/lib/dsc-lib/src/discovery/mod.rs +++ b/lib/dsc-lib/src/discovery/mod.rs @@ -116,7 +116,7 @@ impl Discovery { let version = fix_semver(version); if let Ok(version_req) = VersionReq::parse(&version) { for resource in resources { - if let Ok(resource_version) = Version::parse(&resource.version) { + if let Some(resource_version) = resource.version.as_semver() { if version_req.matches(&resource_version) && matches_adapter_requirement(resource, filter) { return Ok(Some(resource)); } diff --git a/lib/dsc-lib/src/dscresources/adapted_resource_manifest.rs b/lib/dsc-lib/src/dscresources/adapted_resource_manifest.rs index e9c3f7593..906c3af86 100644 --- a/lib/dsc-lib/src/dscresources/adapted_resource_manifest.rs +++ b/lib/dsc-lib/src/dscresources/adapted_resource_manifest.rs @@ -1,10 +1,9 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -use crate::dscresources::{ - resource_manifest::{Kind, ResourceManifest}, - dscresource::Capability, -}; +use crate::{dscresources::{ + dscresource::Capability, resource_manifest::{Kind, ResourceManifest} +}, types::ResourceVersion}; use crate::{ schemas::dsc_repo::DscRepoSchema, types::FullyQualifiedTypeName, @@ -38,7 +37,7 @@ pub struct AdaptedDscResourceManifest { /// The kind of resource. pub kind: Kind, /// The version of the resource. - pub version: String, + pub version: ResourceVersion, /// The capabilities of the resource. pub capabilities: Vec, /// An optional condition for the resource to be active. diff --git a/lib/dsc-lib/src/dscresources/dscresource.rs b/lib/dsc-lib/src/dscresources/dscresource.rs index 8c8b747b1..b748090bf 100644 --- a/lib/dsc-lib/src/dscresources/dscresource.rs +++ b/lib/dsc-lib/src/dscresources/dscresource.rs @@ -1,7 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -use crate::{configure::{Configurator, config_doc::{Configuration, ExecutionKind, Resource}, context::ProcessMode, parameters::{SECURE_VALUE_REDACTED, is_secure_value}}, dscresources::resource_manifest::{AdapterInputKind, Kind}, types::FullyQualifiedTypeName}; +use crate::{configure::{Configurator, config_doc::{Configuration, ExecutionKind, Resource}, context::ProcessMode, parameters::{SECURE_VALUE_REDACTED, is_secure_value}}, dscresources::resource_manifest::{AdapterInputKind, Kind}, types::{FullyQualifiedTypeName, ResourceVersion}}; use crate::discovery::discovery_trait::DiscoveryFilter; use crate::dscresources::invoke_result::{ResourceGetResponse, ResourceSetResponse}; use crate::schemas::transforms::idiomaticize_string_enum; @@ -36,7 +36,7 @@ pub struct DscResource { /// The kind of resource. pub kind: Kind, /// The version of the resource. - pub version: String, + pub version: ResourceVersion, /// The capabilities of the resource. pub capabilities: Vec, /// An optional message indicating the resource is deprecated. If provided, the message will be shown when the resource is used. @@ -101,7 +101,7 @@ impl DscResource { Self { type_name: FullyQualifiedTypeName::default(), kind: Kind::Resource, - version: String::new(), + version: ResourceVersion::default(), capabilities: Vec::new(), deprecation_message: None, description: None, diff --git a/lib/dsc-lib/src/dscresources/resource_manifest.rs b/lib/dsc-lib/src/dscresources/resource_manifest.rs index 5b3f5632f..96cbdcef2 100644 --- a/lib/dsc-lib/src/dscresources/resource_manifest.rs +++ b/lib/dsc-lib/src/dscresources/resource_manifest.rs @@ -10,7 +10,7 @@ use serde_json::{Map, Value}; use crate::{ configure::config_doc::SecurityContextKind, schemas::{dsc_repo::DscRepoSchema, transforms::idiomaticize_string_enum}, - types::{ExitCodesMap, FullyQualifiedTypeName, TagList}, + types::{ExitCodesMap, FullyQualifiedTypeName, ResourceVersion, TagList}, }; #[derive(Debug, Clone, PartialEq, Deserialize, Serialize, JsonSchema, DscRepoSchema)] @@ -55,7 +55,7 @@ pub struct ResourceManifest { #[serde(skip_serializing_if = "Option::is_none")] pub kind: Option, /// The version of the resource using semantic versioning. - pub version: String, + pub version: ResourceVersion, /// The description of the resource. #[serde(skip_serializing_if = "Option::is_none")] pub description: Option, @@ -383,7 +383,7 @@ mod test { let manifest = ResourceManifest{ schema_version: invalid_uri.clone(), resource_type: "Microsoft.Dsc.Test/InvalidSchemaUri".parse().unwrap(), - version: "0.1.0".to_string(), + version: "0.1.0".parse().unwrap(), ..Default::default() }; @@ -404,7 +404,7 @@ mod test { let manifest = ResourceManifest{ schema_version: ResourceManifest::default_schema_id_uri(), resource_type: "Microsoft.Dsc.Test/ValidSchemaUri".parse().unwrap(), - version: "0.1.0".to_string(), + version: "0.1.0".parse().unwrap(), ..Default::default() }; diff --git a/tools/test_group_resource/src/main.rs b/tools/test_group_resource/src/main.rs index e6ed15de7..31b8520c9 100644 --- a/tools/test_group_resource/src/main.rs +++ b/tools/test_group_resource/src/main.rs @@ -17,7 +17,7 @@ fn main() { let resource1 = DscResource { type_name: "Test/TestResource1".parse().unwrap(), kind: Kind::Resource, - version: "1.0.0".to_string(), + version: "1.0.0".parse().unwrap(), capabilities: vec![Capability::Get, Capability::Set], deprecation_message: None, description: Some("This is a test resource.".to_string()), @@ -34,7 +34,7 @@ fn main() { schema_version: dsc_lib::dscresources::resource_manifest::ResourceManifest::default_schema_id_uri(), resource_type: "Test/TestResource1".parse().unwrap(), kind: Some(Kind::Resource), - version: "1.0.0".to_string(), + version: "1.0.0".parse().unwrap(), get: Some(GetMethod { executable: String::new(), ..Default::default() @@ -45,7 +45,7 @@ fn main() { let resource2 = DscResource { type_name: "Test/TestResource2".parse().unwrap(), kind: Kind::Resource, - version: "1.0.1".to_string(), + version: "1.0.1".parse().unwrap(), capabilities: vec![Capability::Get, Capability::Set], deprecation_message: None, description: Some("This is a test resource.".to_string()), @@ -62,7 +62,7 @@ fn main() { schema_version: dsc_lib::dscresources::resource_manifest::ResourceManifest::default_schema_id_uri(), resource_type: "Test/TestResource2".parse().unwrap(), kind: Some(Kind::Resource), - version: "1.0.1".to_string(), + version: "1.0.1".parse().unwrap(), get: Some(GetMethod { executable: String::new(), ..Default::default() @@ -77,7 +77,7 @@ fn main() { let resource1 = DscResource { type_name: "Test/InvalidResource".parse().unwrap(), kind: Kind::Resource, - version: "1.0.0".to_string(), + version: "1.0.0".parse().unwrap(), capabilities: vec![Capability::Get], deprecation_message: None, description: Some("This is a test resource.".to_string()), From 30ae3f2768818d79073f5ed4f3d243348d233e88 Mon Sep 17 00:00:00 2001 From: Mikey Lombardi Date: Wed, 25 Mar 2026 16:25:13 -0500 Subject: [PATCH 07/15] (GH-538) Initial plumbing for `SemanticVersion` in extensions This change updates the following struct fields to use the `SemanticVersion` type instead of a string: - `dsc_lib::extensions::dscextension::DscExtension.version` - `dsc_lib::extensions::extension_manifest::ExtensionManifest.version` This change _minimally_ updates the code to use `SemanticVersion` type, especially in discovery, but it doesn't yet take advantage of the richer semantics. This change is limited to updating the struct fields and making the code continue to compile for ease of reviewing. --- dsc/src/subcommand.rs | 2 +- lib/dsc-lib/src/discovery/command_discovery.rs | 12 +++--------- lib/dsc-lib/src/extensions/dscextension.rs | 6 +++--- lib/dsc-lib/src/extensions/extension_manifest.rs | 8 ++++---- 4 files changed, 11 insertions(+), 17 deletions(-) diff --git a/dsc/src/subcommand.rs b/dsc/src/subcommand.rs index a552c6a40..f6329ce9a 100644 --- a/dsc/src/subcommand.rs +++ b/dsc/src/subcommand.rs @@ -637,7 +637,7 @@ fn list_extensions(dsc: &mut DscManager, extension_name: Option<&String>, format if write_table { table.add_row(vec![ extension.type_name.to_string(), - extension.version, + extension.version.to_string(), capabilities, extension.description.unwrap_or_default() ]); diff --git a/lib/dsc-lib/src/discovery/command_discovery.rs b/lib/dsc-lib/src/discovery/command_discovery.rs index 1e6dfbf1a..602562f5f 100644 --- a/lib/dsc-lib/src/discovery/command_discovery.rs +++ b/lib/dsc-lib/src/discovery/command_discovery.rs @@ -15,7 +15,7 @@ use crate::util::convert_wildcard_to_regex; use crate::schemas::transforms::idiomaticize_externally_tagged_enum; use regex::RegexBuilder; use rust_i18n::t; -use semver::{Version, VersionReq}; +use semver::VersionReq; use schemars::JsonSchema; use serde::Deserialize; use std::{collections::{HashMap, HashSet}, sync::{LazyLock, RwLock}}; @@ -290,13 +290,7 @@ impl ResourceDiscovery for CommandDiscovery { trace!("{}", t!("discovery.commandDiscovery.extensionFound", extension = extension.type_name, version = extension.version)); // we only keep newest version of the extension so compare the version and only keep the newest if let Some(existing_extension) = extensions.get_mut(&extension.type_name) { - let Ok(existing_version) = Version::parse(&existing_extension.version) else { - return Err(DscError::Operation(t!("discovery.commandDiscovery.extensionInvalidVersion", extension = existing_extension.type_name, version = existing_extension.version).to_string())); - }; - let Ok(new_version) = Version::parse(&extension.version) else { - return Err(DscError::Operation(t!("discovery.commandDiscovery.extensionInvalidVersion", extension = extension.type_name, version = extension.version).to_string())); - }; - if new_version > existing_version { + if extension.version > existing_extension.version { extensions.insert(extension.type_name.clone(), extension.clone()); } } else { @@ -873,7 +867,7 @@ fn load_resource_manifest(path: &Path, manifest: &ResourceManifest) -> Result Result { - if let Err(err) = validate_semver(&manifest.version) { + if let Err(err) = validate_semver(&manifest.version.to_string()) { warn!("{}", t!("discovery.commandDiscovery.invalidManifestVersion", path = path.to_string_lossy(), err = err).to_string()); } diff --git a/lib/dsc-lib/src/extensions/dscextension.rs b/lib/dsc-lib/src/extensions/dscextension.rs index 8c4d65e22..3d30d155c 100644 --- a/lib/dsc-lib/src/extensions/dscextension.rs +++ b/lib/dsc-lib/src/extensions/dscextension.rs @@ -3,7 +3,7 @@ use crate::extensions::import::ImportMethod; use crate::schemas::{dsc_repo::DscRepoSchema, transforms::idiomaticize_string_enum}; -use crate::types::FullyQualifiedTypeName; +use crate::types::{FullyQualifiedTypeName, SemanticVersion}; use serde::{Deserialize, Serialize}; use serde_json::Value; use schemars::JsonSchema; @@ -18,7 +18,7 @@ pub struct DscExtension { #[serde(rename="type")] pub type_name: FullyQualifiedTypeName, /// The version of the extension. - pub version: String, + pub version: SemanticVersion, /// The capabilities of the extension. pub capabilities: Vec, /// The import specifics. @@ -65,7 +65,7 @@ impl DscExtension { pub fn new() -> Self { Self { type_name: FullyQualifiedTypeName::default(), - version: String::new(), + version: SemanticVersion::default(), capabilities: Vec::new(), import: None, deprecation_message: None, diff --git a/lib/dsc-lib/src/extensions/extension_manifest.rs b/lib/dsc-lib/src/extensions/extension_manifest.rs index a799537b2..91a3a6686 100644 --- a/lib/dsc-lib/src/extensions/extension_manifest.rs +++ b/lib/dsc-lib/src/extensions/extension_manifest.rs @@ -10,7 +10,7 @@ use serde_json::{Map, Value}; use crate::dscerror::DscError; use crate::extensions::{discover::DiscoverMethod, import::ImportMethod, secret::SecretMethod}; use crate::schemas::dsc_repo::DscRepoSchema; -use crate::types::{ExitCodesMap, FullyQualifiedTypeName, TagList}; +use crate::types::{ExitCodesMap, FullyQualifiedTypeName, SemanticVersion, TagList}; #[derive(Debug, Default, Clone, PartialEq, Deserialize, Serialize, JsonSchema, DscRepoSchema)] #[serde(deny_unknown_fields, rename_all = "camelCase")] @@ -33,7 +33,7 @@ pub struct ExtensionManifest { #[serde(rename = "type")] pub r#type: FullyQualifiedTypeName, /// The version of the extension using semantic versioning. - pub version: String, + pub version: SemanticVersion, /// An optional condition for the extension to be active. If the condition evaluates to false, the extension is skipped. #[serde(skip_serializing_if = "Option::is_none")] pub condition: Option, @@ -110,7 +110,7 @@ mod test { let manifest = ExtensionManifest{ schema_version: invalid_uri.clone(), r#type: "Microsoft.Dsc.Test/InvalidSchemaUri".parse().unwrap(), - version: "0.1.0".to_string(), + version: "0.1.0".parse().unwrap(), ..Default::default() }; @@ -131,7 +131,7 @@ mod test { let manifest = ExtensionManifest{ schema_version: ExtensionManifest::default_schema_id_uri(), r#type: "Microsoft.Dsc.Test/ValidSchemaUri".parse().unwrap(), - version: "0.1.0".to_string(), + version: "0.1.0".parse().unwrap(), ..Default::default() }; From a5142211b1d0f8693927949060be29ac9495bcfb Mon Sep 17 00:00:00 2001 From: Mikey Lombardi Date: Wed, 25 Mar 2026 19:09:53 -0500 Subject: [PATCH 08/15] (GH-538) Use correct types in config doc and discovery This change continues the canonicalization work by updating the the following types and fields: - `dsc_lib::configure::config_doc::ConfigDirective.version` is now an optional `SemanticVersionReq` instead of a string. - `dsc_lib::configure::config_doc::ResourceDirective.require_adapter` is now an optional `FullyQualifiedTypeName` instead of a string. - `dsc_lib::configure::config_doc::Resource.require_version` is now an optional `ResourceVersionReq` instead of a string. This change makes additional enhancements for maintainability: - Defines the `new_for_resource` and `new_for_extension` constructors for `DiscoveryFilter` to ensure that the filter can be correctly created from existing valid data instead of needing to convert to strings and reparse on every construction. - Defines the `find_resource_or_error!` macro for the configurator to simplify the common pattern of looking up a resource and returning an error if it is not found or does not match the filter. - Simplifies the version and type name matching logic in discovery to take advantage of the canonical types, making the logic more readable and keeping the parsing and matching implementation details encapsulated in the appropriate types. --- lib/dsc-lib/src/configure/config_doc.rs | 25 +++--- lib/dsc-lib/src/configure/mod.rs | 90 ++++++++++++++----- .../src/discovery/command_discovery.rs | 40 ++------- lib/dsc-lib/src/discovery/discovery_trait.rs | 65 ++++++++++++-- lib/dsc-lib/src/discovery/mod.rs | 26 ++---- lib/dsc-lib/src/dscresources/dscresource.rs | 2 +- 6 files changed, 157 insertions(+), 91 deletions(-) diff --git a/lib/dsc-lib/src/configure/config_doc.rs b/lib/dsc-lib/src/configure/config_doc.rs index a4d2b42fb..6fbc8b4c9 100644 --- a/lib/dsc-lib/src/configure/config_doc.rs +++ b/lib/dsc-lib/src/configure/config_doc.rs @@ -8,10 +8,13 @@ use serde::{Deserialize, Serialize}; use serde_json::{Map, Value}; use std::{collections::HashMap, fmt::Display}; -use crate::{schemas::{ - dsc_repo::DscRepoSchema, - transforms::{idiomaticize_externally_tagged_enum, idiomaticize_string_enum} -}, types::FullyQualifiedTypeName}; +use crate::{ + schemas::{ + dsc_repo::DscRepoSchema, + transforms::{idiomaticize_externally_tagged_enum, idiomaticize_string_enum} + }, + types::{FullyQualifiedTypeName, ResourceVersionReq, SemanticVersionReq} +}; #[derive(Debug, Clone, PartialEq, Deserialize, Serialize, JsonSchema, DscRepoSchema)] #[serde(rename_all = "camelCase")] @@ -226,7 +229,7 @@ pub struct ConfigDirective { pub security_context: Option, /// Required version of DSC #[serde(skip_serializing_if = "Option::is_none")] - pub version: Option, + pub version: Option, } #[derive(Debug, Default, Clone, PartialEq, Deserialize, Serialize, JsonSchema, DscRepoSchema)] @@ -235,7 +238,7 @@ pub struct ConfigDirective { pub struct ResourceDirective { /// Specify specific adapter type used for implicit operations #[serde(skip_serializing_if = "Option::is_none")] - pub require_adapter: Option, + pub require_adapter: Option, /// The required security context of the configuration operation #[serde(skip_serializing_if = "Option::is_none")] pub security_context: Option, @@ -473,7 +476,7 @@ pub struct Resource { #[serde(rename = "type")] pub resource_type: FullyQualifiedTypeName, #[serde(skip_serializing_if = "Option::is_none", rename = "requireVersion", alias = "apiVersion")] - pub require_version: Option, + pub require_version: Option, /// A friendly name for the resource instance #[serde(default)] pub name: String, // friendly unique instance name @@ -644,12 +647,12 @@ mod test { { "type": "Microsoft.DSC.Debug/Echo", "name": "echoResource", - "requireVersion": "1.0.0" + "requireVersion": "=1.0.0" }, { "type": "Microsoft/Process", "name": "processResource", - "requireVersion": "0.1.0" + "requireVersion": "=0.1.0" } ] }"#; @@ -659,11 +662,11 @@ mod test { assert_eq!(config.resources.len(), 2); assert_eq!(config.resources[0].name, "echoResource"); assert_eq!(config.resources[0].resource_type, "Microsoft.DSC.Debug/Echo"); - assert_eq!(config.resources[0].require_version.as_deref(), Some("1.0.0")); + assert_eq!(config.resources[0].require_version.as_ref().map(|r| r.to_string()), Some("=1.0.0".to_string())); assert_eq!(config.resources[1].name, "processResource"); assert_eq!(config.resources[1].resource_type, "Microsoft/Process"); - assert_eq!(config.resources[1].require_version.as_deref(), Some("0.1.0")); + assert_eq!(config.resources[1].require_version.as_ref().map(|r| r.to_string()), Some("=0.1.0".to_string())); } } diff --git a/lib/dsc-lib/src/configure/mod.rs b/lib/dsc-lib/src/configure/mod.rs index 3dd32ab43..f1661b83e 100644 --- a/lib/dsc-lib/src/configure/mod.rs +++ b/lib/dsc-lib/src/configure/mod.rs @@ -16,7 +16,7 @@ use crate::DscResource; use crate::discovery::Discovery; use crate::parser::Statement; use crate::progress::{Failure, ProgressBar, ProgressFormat}; -use crate::types::{SemanticVersion, SemanticVersionReq}; +use crate::types::{FullyQualifiedTypeName, SemanticVersion}; use crate::util::resource_id; use self::config_doc::{Configuration, DataType, MicrosoftDscMetadata, Operation, SecurityContextKind}; use self::depends_on::get_resource_invocation_order; @@ -44,6 +44,58 @@ pub struct Configurator { progress_format: ProgressFormat, } +/// Invokes the [`Discovery::find_resource`] method to retrieve a specific resource or raise a +/// [`DscError::ResourceNotFound`] if the resource cannot be found. +/// +/// # Arguments +/// +/// * `variable` - The variable to bind the found resource to. +/// * `discovery` - The discovery instance to use for finding the resource. +/// * `resource` - The resource for which to construct the discovery filter. +/// * `adapter` - An optional adapter requirement to include in the discovery filter. +/// +/// # Examples +/// +/// The following snippet shows how the `find_resource_or_error!` macro can be used within a method +/// of the `Configurator` struct to find a resource and return an error if it's not found: +/// +/// ```ignore +/// find_resource_or_error!(dsc_resource, discovery, resource, adapter); +/// ``` +/// +/// Which expands to: +/// +/// ```ignore +/// let Some(dsc_resource) = discovery.find_resource( +/// &DiscoveryFilter::new_for_resource( +/// &resource.resource_type, +/// resource.require_version.clone(), +/// adapter +/// ) +/// )? else { +/// return Err(DscError::ResourceNotFound( +/// resource.resource_type.to_string(), +/// resource.require_version.as_ref().map(|r| r.to_string()).unwrap_or("".to_string()) +/// )); +/// }; +/// ``` +macro_rules! find_resource_or_error { + ($variable:ident, $discovery: ident, $resource:ident, $adapter:ident) => { + let Some($variable) = $discovery.find_resource( + &DiscoveryFilter::new_for_resource( + &$resource.resource_type, + $resource.require_version.clone(), + $adapter + ) + )? else { + return Err(DscError::ResourceNotFound( + $resource.resource_type.to_string(), + $resource.require_version.as_ref().map(|r| r.to_string()).unwrap_or("".to_string()) + )); + }; + }; +} + /// Add the results of an export operation to a configuration. /// /// # Arguments @@ -226,7 +278,7 @@ fn add_metadata(dsc_resource: &DscResource, mut properties: Option) -> Option { +fn get_require_adapter_from_directive(resource_directives: &Option) -> Option { if let Some(directives) = resource_directives { if let Some(require_adapter) = &directives.require_adapter { return Some(require_adapter.clone()); @@ -485,9 +537,7 @@ impl Configurator { let directive_security_context = resource.directives.as_ref().and_then(|d| d.security_context.as_ref()); check_security_context(resource.metadata.as_ref(), directive_security_context)?; let adapter = get_require_adapter_from_directive(&resource.directives); - let Some(dsc_resource) = discovery.find_resource(&DiscoveryFilter::new(&resource.resource_type, resource.require_version.as_deref(), adapter.as_deref()))? else { - return Err(DscError::ResourceNotFound(resource.resource_type.to_string(), resource.require_version.as_deref().unwrap_or("").to_string())); - }; + find_resource_or_error!(dsc_resource, discovery, resource, adapter); let properties = self.get_properties(&resource, &dsc_resource.kind)?; let filter = add_metadata(&dsc_resource, properties, resource.metadata.clone())?; let start_datetime = chrono::Local::now(); @@ -578,9 +628,7 @@ impl Configurator { let directive_security_context = resource.directives.as_ref().and_then(|d| d.security_context.as_ref()); check_security_context(resource.metadata.as_ref(), directive_security_context)?; let adapter = get_require_adapter_from_directive(&resource.directives); - let Some(dsc_resource) = discovery.find_resource(&DiscoveryFilter::new(&resource.resource_type, resource.require_version.as_deref(), adapter.as_deref()))? else { - return Err(DscError::ResourceNotFound(resource.resource_type.to_string(), resource.require_version.as_deref().unwrap_or("").to_string())); - }; + find_resource_or_error!(dsc_resource, discovery, resource, adapter); let properties = self.get_properties(&resource, &dsc_resource.kind)?; debug!("resource_type {}", &resource.resource_type); // see if the properties contains `_exist` and is false @@ -771,9 +819,7 @@ impl Configurator { let directive_security_context = resource.directives.as_ref().and_then(|d| d.security_context.as_ref()); check_security_context(resource.metadata.as_ref(), directive_security_context)?; let adapter = get_require_adapter_from_directive(&resource.directives); - let Some(dsc_resource) = discovery.find_resource(&DiscoveryFilter::new(&resource.resource_type, resource.require_version.as_deref(), adapter.as_deref()))? else { - return Err(DscError::ResourceNotFound(resource.resource_type.to_string(), resource.require_version.as_deref().unwrap_or("").to_string())); - }; + find_resource_or_error!(dsc_resource, discovery, resource, adapter); let properties = self.get_properties(&resource, &dsc_resource.kind)?; debug!("resource_type {}", &resource.resource_type); let expected = add_metadata(&dsc_resource, properties, resource.metadata.clone())?; @@ -863,9 +909,7 @@ impl Configurator { let directive_security_context = resource.directives.as_ref().and_then(|d| d.security_context.as_ref()); check_security_context(resource.metadata.as_ref(), directive_security_context)?; let adapter = get_require_adapter_from_directive(&resource.directives); - let Some(dsc_resource) = discovery.find_resource(&DiscoveryFilter::new(&resource.resource_type, resource.require_version.as_deref(), adapter.as_deref()))? else { - return Err(DscError::ResourceNotFound(resource.resource_type.to_string(), resource.require_version.as_deref().unwrap_or("").to_string())); - }; + find_resource_or_error!(dsc_resource, discovery, resource, adapter); let properties = self.get_properties(resource, &dsc_resource.kind)?; debug!("resource_type {}", &resource.resource_type); let input = add_metadata(&dsc_resource, properties, resource.metadata.clone())?; @@ -1164,11 +1208,10 @@ impl Configurator { check_security_context(config.metadata.as_ref(), config_security_context.as_ref())?; if let Some(directives) = &config.directives { - if let Some(version) = &directives.version { + if let Some(version_req) = &directives.version { let dsc_version = SemanticVersion::parse(env!("CARGO_PKG_VERSION"))?; - let version_req = SemanticVersionReq::parse(&version)?; if !version_req.matches(&dsc_version) { - return Err(DscError::Validation(t!("configure.mod.versionNotSatisfied", required_version = version, current_version = env!("CARGO_PKG_VERSION")).to_string())); + return Err(DscError::Validation(t!("configure.mod.versionNotSatisfied", required_version = version_req, current_version = env!("CARGO_PKG_VERSION")).to_string())); } } } @@ -1190,7 +1233,11 @@ impl Configurator { let config_copy = config.clone(); for resource in config_copy.resources { let adapter = get_require_adapter_from_directive(&resource.directives); - let filter = DiscoveryFilter::new(&resource.resource_type, resource.require_version.as_deref(), adapter.as_deref()); + let filter = DiscoveryFilter::new_for_resource( + &resource.resource_type, + resource.require_version.clone(), + adapter + ); if !discovery_filter.contains(&filter) { discovery_filter.push(filter); } @@ -1210,8 +1257,11 @@ impl Configurator { // now check that each resource in the config was found for resource in config.resources.iter() { let adapter = get_require_adapter_from_directive(&resource.directives); - let Some(_dsc_resource) = self.discovery.find_resource(&DiscoveryFilter::new(&resource.resource_type, resource.require_version.as_deref(), adapter.as_deref()))? else { - return Err(DscError::ResourceNotFound(resource.resource_type.to_string(), resource.require_version.as_deref().unwrap_or("").to_string())); + let Some(_dsc_resource) = self.discovery.find_resource(&DiscoveryFilter::new_for_resource(&resource.resource_type, resource.require_version.clone(), adapter))? else { + return Err(DscError::ResourceNotFound( + resource.resource_type.to_string(), + resource.require_version.as_ref().map(|r| r.to_string()).unwrap_or("".to_string()) + )); }; } } diff --git a/lib/dsc-lib/src/discovery/command_discovery.rs b/lib/dsc-lib/src/discovery/command_discovery.rs index 602562f5f..4f3b8da16 100644 --- a/lib/dsc-lib/src/discovery/command_discovery.rs +++ b/lib/dsc-lib/src/discovery/command_discovery.rs @@ -15,7 +15,6 @@ use crate::util::convert_wildcard_to_regex; use crate::schemas::transforms::idiomaticize_externally_tagged_enum; use regex::RegexBuilder; use rust_i18n::t; -use semver::VersionReq; use schemars::JsonSchema; use serde::Deserialize; use std::{collections::{HashMap, HashSet}, sync::{LazyLock, RwLock}}; @@ -564,24 +563,12 @@ impl ResourceDiscovery for CommandDiscovery { fn filter_resources(found_resources: &mut DiscoveryResourceCache, required_resources: &mut HashMap, resources: &[DscResource], filter: &DiscoveryFilter) { for resource in resources { - if let Some(required_version) = filter.version() { - if let Some(resource_version) = resource.version.as_semver() { - if let Ok(version_req) = VersionReq::parse(required_version) { - if version_req.matches(&resource_version) && matches_adapter_requirement(resource, filter) { - found_resources.entry(filter.resource_type().clone()).or_default().push(resource.clone()); - required_resources.insert(filter.clone(), true); - debug!("{}", t!("discovery.commandDiscovery.foundResourceWithVersion", resource = resource.type_name, version = resource.version)); - break; - } - } - } else { - // if not semver, we do a string comparison - if resource.version == *required_version && matches_adapter_requirement(resource, filter) { - found_resources.entry(filter.resource_type().clone()).or_default().push(resource.clone()); - required_resources.insert(filter.clone(), true); - debug!("{}", t!("discovery.commandDiscovery.foundResourceWithVersion", resource = resource.type_name, version = resource.version)); - break; - } + if let Some(required_version) = filter.require_version() { + if required_version.matches(&resource.version) && matches_adapter_requirement(resource, filter) { + found_resources.entry(filter.resource_type().clone()).or_default().push(resource.clone()); + required_resources.insert(filter.clone(), true); + debug!("{}", t!("discovery.commandDiscovery.foundResourceWithVersion", resource = resource.type_name, version = resource.version)); + break; } } else { if matches_adapter_requirement(resource, filter) { @@ -602,20 +589,7 @@ fn insert_resource(resources: &mut DiscoveryResourceCache, resource: &DscResourc // compare the resource versions and insert newest to oldest using semver let mut insert_index = resource_versions.len(); for (index, resource_instance) in resource_versions.iter().enumerate() { - let resource_instance_version = match resource_instance.version.as_semver() { - Some(v) => v, - None => { - continue; - }, - }; - let resource_version = match resource.version.as_semver() { - Some(v) => v, - None => { - continue; - }, - }; - - if resource_instance_version < resource_version { + if resource_instance.version < resource.version { insert_index = index; break; } diff --git a/lib/dsc-lib/src/discovery/discovery_trait.rs b/lib/dsc-lib/src/discovery/discovery_trait.rs index 1a9b4b108..cf6799dab 100644 --- a/lib/dsc-lib/src/discovery/discovery_trait.rs +++ b/lib/dsc-lib/src/discovery/discovery_trait.rs @@ -5,9 +5,8 @@ use crate::{ configure::config_doc::ResourceDiscoveryMode, discovery::{DiscoveryExtensionCache, DiscoveryManifestCache, DiscoveryResourceCache}, dscerror::DscError, - types::FullyQualifiedTypeName + types::{FullyQualifiedTypeName, ResourceVersionReq, SemanticVersionReq} }; -use super::fix_semver; #[derive(Debug, PartialEq)] pub enum DiscoveryKind { @@ -19,17 +18,69 @@ pub enum DiscoveryKind { pub struct DiscoveryFilter { require_adapter: Option, r#type: FullyQualifiedTypeName, - version: Option, + require_version: Option, } impl DiscoveryFilter { #[must_use] pub fn new(resource_type: &str, version: Option<&str>, adapter: Option<&str>) -> Self { - let version = version.map(|v| fix_semver(&v)); Self { require_adapter: adapter.map(|a| a.parse().unwrap()), r#type: resource_type.parse().unwrap(), - version, + require_version: version.map(|v| v.parse().unwrap()), + } + } + + /// Construct a [`DiscoveryFilter`] for a resource with the specified type name, optional + /// version requirement, and optional adapter requirement. + /// + /// # Arguments + /// + /// - `type_name` - The [`FullyQualifiedTypeName`] of the resource. + /// - `require_version` - An optional [`ResourceVersionReq`] specifying the version requirement + /// for the resource. The version requirement can be semantic or date-based, depending on the + /// resource's versioning scheme. + /// - `require_adapter` - An optional [`FullyQualifiedTypeName`] specifying the adapter that + /// the resource is expected to require. + /// + /// # Returns + /// + /// A new instance of [`DiscoveryFilter`] initialized with the provided parameters. + pub fn new_for_resource( + type_name: &FullyQualifiedTypeName, + require_version: Option, + require_adapter: Option + ) -> Self { + Self { + require_adapter, + r#type: type_name.clone(), + require_version, + } + } + + /// Construct a [`DiscoveryFilter`] for an extension with the specified type name and optional + /// version requirement. + /// + /// # Arguments + /// + /// - `type_name` - The [`FullyQualifiedTypeName`] of the extension. + /// - `require_version` - An optional [`SemanticVersionReq`] specifying the semantic version + /// requirement for the extension. + /// + /// # Returns + /// + /// A new instance of [`DiscoveryFilter`] initialized with the provided parameters. + /// + /// Note that extensions do not have an adapter requirement, so the `require_adapter` field is + /// always set to `None`. + pub fn new_for_extension( + type_name: &FullyQualifiedTypeName, + require_version: Option, + ) -> Self { + Self { + require_adapter: None, + r#type: type_name.clone(), + require_version: require_version.map(|r| r.into()), } } @@ -44,8 +95,8 @@ impl DiscoveryFilter { } #[must_use] - pub fn version(&self) -> Option<&String> { - self.version.as_ref() + pub fn require_version(&self) -> Option<&ResourceVersionReq> { + self.require_version.as_ref() } } diff --git a/lib/dsc-lib/src/discovery/mod.rs b/lib/dsc-lib/src/discovery/mod.rs index 3ec242719..31e1d3bad 100644 --- a/lib/dsc-lib/src/discovery/mod.rs +++ b/lib/dsc-lib/src/discovery/mod.rs @@ -11,7 +11,7 @@ use crate::extensions::dscextension::{Capability, DscExtension}; use crate::types::FullyQualifiedTypeName; use crate::{dscresources::dscresource::DscResource, progress::ProgressFormat}; use core::result::Result::Ok; -use semver::{Version, VersionReq}; +use semver::Version; use std::collections::BTreeMap; use command_discovery::{CommandDiscovery, ImportedManifest}; use tracing::error; @@ -112,25 +112,13 @@ impl Discovery { let type_name = filter.resource_type(); if let Some(resources) = self.resources.get(type_name) { - if let Some(version) = filter.version() { - let version = fix_semver(version); - if let Ok(version_req) = VersionReq::parse(&version) { - for resource in resources { - if let Some(resource_version) = resource.version.as_semver() { - if version_req.matches(&resource_version) && matches_adapter_requirement(resource, filter) { - return Ok(Some(resource)); - } - } - } - Ok(None) - } else { - for resource in resources { - if resource.version == version && matches_adapter_requirement(resource, filter) { - return Ok(Some(resource)); - } + if let Some(version_req) = filter.require_version() { + for resource in resources { + if version_req.matches(&resource.version) && matches_adapter_requirement(resource, filter) { + return Ok(Some(resource)); } - Ok(None) } + Ok(None) } else { for resource in resources { if matches_adapter_requirement(resource, filter) { @@ -190,7 +178,7 @@ impl Discovery { pub fn matches_adapter_requirement(resource: &DscResource, filter: &DiscoveryFilter) -> bool { if let Some(required_adapter) = filter.require_adapter() { if let Some(resource_adapter) = &resource.require_adapter { - required_adapter.to_lowercase() == resource_adapter.to_lowercase() + required_adapter == resource_adapter } else { false } diff --git a/lib/dsc-lib/src/dscresources/dscresource.rs b/lib/dsc-lib/src/dscresources/dscresource.rs index b748090bf..24b2f7a48 100644 --- a/lib/dsc-lib/src/dscresources/dscresource.rs +++ b/lib/dsc-lib/src/dscresources/dscresource.rs @@ -291,7 +291,7 @@ impl DscResource { } fn get_adapter_resource(configurator: &mut Configurator, adapter: &FullyQualifiedTypeName) -> Result { - if let Some(adapter_resource) = configurator.discovery().find_resource(&DiscoveryFilter::new(adapter, None, None))? { + if let Some(adapter_resource) = configurator.discovery().find_resource(&DiscoveryFilter::new_for_resource(adapter, None, None))? { return Ok(adapter_resource.clone()); } Err(DscError::Operation(t!("dscresources.dscresource.adapterResourceNotFound", adapter = adapter).to_string())) From 5f1132ed5e92ba61573829922e4f8f1f8031a443 Mon Sep 17 00:00:00 2001 From: Mikey Lombardi Date: Thu, 26 Mar 2026 09:32:38 -0500 Subject: [PATCH 09/15] (GH-538) Initial plumbing for `TypeNameFilter` Prior to this change, the `discover()`, `discover_adapted()`, and `list_available()` methods for discovery used string slices as input for type name filters. This change updates the signatures for those methods to use the new `TypeNameFilter` struct instead. This helped to simplify the implementation for the methods and removed the need to continually construct an appropriate regex, since that is now encapsulated within the `WildcardTypeName` struct as a variant of `TypeNameFilter`. This change updates the callers of these methods to construct a `TypeNameFilter` from the input strings. This is temporary to enable updating the callers without modifying the CLI arguments, which will be addressed in a following change. --- dsc/src/mcp/list_dsc_resources.rs | 12 +-- dsc/src/mcp/show_dsc_resource.rs | 4 +- dsc/src/subcommand.rs | 48 ++++++++++- .../src/discovery/command_discovery.rs | 86 ++++++++----------- lib/dsc-lib/src/discovery/discovery_trait.rs | 25 +++--- lib/dsc-lib/src/discovery/mod.rs | 8 +- lib/dsc-lib/src/lib.rs | 9 +- 7 files changed, 117 insertions(+), 75 deletions(-) diff --git a/dsc/src/mcp/list_dsc_resources.rs b/dsc/src/mcp/list_dsc_resources.rs index 77e93599d..f9df20a5a 100644 --- a/dsc/src/mcp/list_dsc_resources.rs +++ b/dsc/src/mcp/list_dsc_resources.rs @@ -6,7 +6,7 @@ use dsc_lib::{ DscManager, discovery::{ command_discovery::ImportedManifest::Resource, discovery_trait::{DiscoveryFilter, DiscoveryKind}, - }, dscresources::resource_manifest::Kind, progress::ProgressFormat, types::FullyQualifiedTypeName + }, dscresources::resource_manifest::Kind, progress::ProgressFormat, types::{FullyQualifiedTypeName, TypeNameFilter} }; use rmcp::{ErrorData as McpError, Json, tool, tool_router, handler::server::wrapper::Parameters}; use rust_i18n::t; @@ -56,15 +56,15 @@ impl McpServer { if resource.kind != Kind::Adapter { return Err(McpError::invalid_params(t!("mcp.list_dsc_resources.resourceNotAdapter", adapter = adapter), None)); } - adapter + Some(&TypeNameFilter::Literal(resource.type_name.clone())) } else { return Err(McpError::invalid_params(t!("mcp.list_dsc_resources.adapterNotFound", adapter = adapter), None)); } }, - None => String::new(), + None => None, }; - let mut resources = BTreeMap::::new(); - for resource in dsc.list_available(&DiscoveryKind::Resource, "*", &adapter_filter, ProgressFormat::None) { + let mut resources = BTreeMap::::new(); + for resource in dsc.list_available(&DiscoveryKind::Resource, &TypeNameFilter::default(), adapter_filter, ProgressFormat::None) { if let Resource(resource) = resource { let summary = ResourceSummary { r#type: resource.type_name.clone(), @@ -72,7 +72,7 @@ impl McpServer { description: resource.description.clone(), require_adapter: resource.require_adapter, }; - resources.insert(resource.type_name.to_lowercase(), summary); + resources.insert(resource.type_name.clone(), summary); } } Ok(ResourceListResult { resources: resources.into_values().collect() }) diff --git a/dsc/src/mcp/show_dsc_resource.rs b/dsc/src/mcp/show_dsc_resource.rs index d41d3b12f..0c725a2ef 100644 --- a/dsc/src/mcp/show_dsc_resource.rs +++ b/dsc/src/mcp/show_dsc_resource.rs @@ -41,7 +41,7 @@ pub struct DscResource { #[derive(Deserialize, JsonSchema)] pub struct ShowResourceRequest { #[schemars(description = "The type name of the resource to get detailed information.")] - pub r#type: String, + pub r#type: FullyQualifiedTypeName, } #[tool_router(router = show_dsc_resource_router, vis = "pub")] @@ -59,7 +59,7 @@ impl McpServer { pub async fn show_dsc_resource(&self, Parameters(ShowResourceRequest { r#type }): Parameters) -> Result, McpError> { let result = task::spawn_blocking(move || { let mut dsc = DscManager::new(); - let Some(resource) = dsc.find_resource(&DiscoveryFilter::new(&r#type, None, None)).unwrap_or(None) else { + let Some(resource) = dsc.find_resource(&DiscoveryFilter::new_for_resource(&r#type, None, None)).unwrap_or(None) else { return Err(McpError::invalid_params(t!("mcp.show_dsc_resource.resourceNotFound", type_name = r#type), None)) }; let schema = match resource.schema() { diff --git a/dsc/src/subcommand.rs b/dsc/src/subcommand.rs index f6329ce9a..e2bf85139 100644 --- a/dsc/src/subcommand.rs +++ b/dsc/src/subcommand.rs @@ -7,6 +7,7 @@ use crate::resource_command::{get_resource, self}; use crate::tablewriter::Table; use crate::util::{get_input, get_schema, in_desired_state, set_dscconfigroot, write_object, DSC_CONFIG_ROOT, EXIT_DSC_ASSERTION_FAILED, EXIT_DSC_ERROR, EXIT_INVALID_ARGS, EXIT_INVALID_INPUT, EXIT_JSON_ERROR}; use dsc_lib::functions::FunctionArgKind; +use dsc_lib::types::TypeNameFilter; use dsc_lib::{ configure::{ config_doc::{ @@ -619,7 +620,18 @@ fn list_extensions(dsc: &mut DscManager, extension_name: Option<&String>, format write_table = true; } let mut include_separator = false; - for manifest_resource in dsc.list_available(&DiscoveryKind::Extension, extension_name.unwrap_or(&String::from("*")), "", progress_format) { + // Temporary workaround to convert input string into type name filter: + let ref extension_name = match extension_name { + Some(name_text) => match TypeNameFilter::parse(name_text) { + Ok(filter) => filter, + Err(err) => { + error!("{}: {err}", t!("subcommand.invalidResourceFilter")); + exit(EXIT_INVALID_ARGS); + } + }, + None => TypeNameFilter::default(), + }; + for manifest_resource in dsc.list_available(&DiscoveryKind::Extension, extension_name, None, progress_format) { if let ImportedManifest::Extension(extension) = manifest_resource { let capability_types = [ (ExtensionCapability::Discover, "d"), @@ -763,7 +775,15 @@ fn list_functions(functions: &FunctionDispatcher, function_name: Option<&String> } } -pub fn list_resources(dsc: &mut DscManager, resource_name: Option<&String>, adapter_name: Option<&String>, description: Option<&String>, tags: Option<&Vec>, format: Option<&ListOutputFormat>, progress_format: ProgressFormat) { +pub fn list_resources( + dsc: &mut DscManager, + resource_name: Option<&String>, + adapter_name: Option<&String>, + description: Option<&String>, + tags: Option<&Vec>, + format: Option<&ListOutputFormat>, + progress_format: ProgressFormat +) { let mut write_table = false; let mut table = Table::new(&[ t!("subcommand.tableHeader_type").to_string().as_ref(), @@ -778,7 +798,29 @@ pub fn list_resources(dsc: &mut DscManager, resource_name: Option<&String>, adap write_table = true; } let mut include_separator = false; - for manifest_resource in dsc.list_available(&DiscoveryKind::Resource, resource_name.unwrap_or(&String::from("*")), adapter_name.unwrap_or(&String::new()), progress_format) { + // Temporary workaround to convert input strings to type name filters: + let ref resource_name = match resource_name { + Some(name_text) => match TypeNameFilter::parse(name_text) { + Ok(filter) => filter, + Err(err) => { + error!("{}: {err}", t!("subcommand.invalidResourceFilter")); + exit(EXIT_INVALID_ARGS); + } + }, + None => TypeNameFilter::default(), + }; + let ref adapter_name = match adapter_name { + Some(name_text) => match TypeNameFilter::parse(name_text) { + Ok(filter) => Some(filter), + Err(err) => { + error!("{}: {err}", t!("subcommand.invalidAdapterFilter")); + exit(EXIT_INVALID_ARGS); + } + }, + None => None, + }; + + for manifest_resource in dsc.list_available(&DiscoveryKind::Resource, resource_name, adapter_name.as_ref(), progress_format) { if let ImportedManifest::Resource(resource) = manifest_resource { let capability_types = [ (Capability::Get, "g"), diff --git a/lib/dsc-lib/src/discovery/command_discovery.rs b/lib/dsc-lib/src/discovery/command_discovery.rs index 4f3b8da16..324171294 100644 --- a/lib/dsc-lib/src/discovery/command_discovery.rs +++ b/lib/dsc-lib/src/discovery/command_discovery.rs @@ -1,7 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -use crate::{discovery::{DiscoveryExtensionCache, DiscoveryManifestCache, DiscoveryResourceCache, discovery_trait::{DiscoveryFilter, DiscoveryKind, ResourceDiscovery}, matches_adapter_requirement}, dscresources::adapted_resource_manifest::AdaptedDscResourceManifest, parser::Statement, types::FullyQualifiedTypeName}; +use crate::{discovery::{DiscoveryExtensionCache, DiscoveryManifestCache, DiscoveryResourceCache, discovery_trait::{DiscoveryFilter, DiscoveryKind, ResourceDiscovery}, matches_adapter_requirement}, dscresources::adapted_resource_manifest::AdaptedDscResourceManifest, parser::Statement, types::{FullyQualifiedTypeName, TypeNameFilter}}; use crate::{locked_clear, locked_is_empty, locked_extend, locked_clone, locked_get}; use crate::configure::{config_doc::ResourceDiscoveryMode, context::Context}; use crate::dscresources::dscresource::{Capability, DscResource, ImplementedAs}; @@ -11,9 +11,7 @@ use crate::dscerror::DscError; use crate::extensions::dscextension::{self, DscExtension, Capability as ExtensionCapability}; use crate::extensions::extension_manifest::ExtensionManifest; use crate::progress::{ProgressBar, ProgressFormat}; -use crate::util::convert_wildcard_to_regex; use crate::schemas::transforms::idiomaticize_externally_tagged_enum; -use regex::RegexBuilder; use rust_i18n::t; use schemars::JsonSchema; use serde::Deserialize; @@ -212,7 +210,7 @@ impl Default for CommandDiscovery { impl ResourceDiscovery for CommandDiscovery { #[allow(clippy::too_many_lines)] - fn discover(&mut self, kind: &DiscoveryKind, filter: &str) -> Result<(), DscError> { + fn discover(&mut self, kind: &DiscoveryKind, filter: &TypeNameFilter) -> Result<(), DscError> { if self.discovery_mode == ResourceDiscoveryMode::PreDeployment && !locked_is_empty!(RESOURCES) { return Ok(()); } else if self.discovery_mode == ResourceDiscoveryMode::DuringDeployment { @@ -222,19 +220,12 @@ impl ResourceDiscovery for CommandDiscovery { // if kind is DscResource, we need to discover extensions first if *kind == DiscoveryKind::Resource && (self.discovery_mode == ResourceDiscoveryMode::DuringDeployment || locked_is_empty!(EXTENSIONS)){ - self.discover(&DiscoveryKind::Extension, "*")?; + self.discover(&DiscoveryKind::Extension, &TypeNameFilter::default())?; } - info!("{}", t!("discovery.commandDiscovery.discoverResources", kind = kind : {:?}, filter = filter)); - - let regex_str = convert_wildcard_to_regex(filter); - debug!("Using regex {regex_str} as filter for adapter name"); - let mut regex_builder = RegexBuilder::new(®ex_str); - regex_builder.case_insensitive(true); - let Ok(regex) = regex_builder.build() else { - return Err(DscError::Operation(t!("discovery.commandDiscovery.invalidAdapterFilter").to_string())); - }; + info!("{}", t!("discovery.commandDiscovery.discoverResources", kind = kind : {:?}, filter = filter.to_string())); + debug!("Using type name filter '{filter}' for adapter name"); let mut progress = ProgressBar::new(1, self.progress_format)?; match kind { DiscoveryKind::Resource => { @@ -285,7 +276,7 @@ impl ResourceDiscovery for CommandDiscovery { for imported_manifest in imported_manifests { match imported_manifest { ImportedManifest::Extension(extension) => { - if regex.is_match(&extension.type_name) { + if filter.is_match(&extension.type_name) { trace!("{}", t!("discovery.commandDiscovery.extensionFound", extension = extension.type_name, version = extension.version)); // we only keep newest version of the extension so compare the version and only keep the newest if let Some(existing_extension) = extensions.get_mut(&extension.type_name) { @@ -298,7 +289,7 @@ impl ResourceDiscovery for CommandDiscovery { } }, ImportedManifest::Resource(resource) => { - if regex.is_match(&resource.type_name) { + if filter.is_match(&resource.type_name) { if let Some(manifest) = &resource.manifest { if manifest.kind == Some(Kind::Adapter) { trace!("{}", t!("discovery.commandDiscovery.adapterFound", adapter = resource.type_name, version = resource.version)); @@ -340,7 +331,7 @@ impl ResourceDiscovery for CommandDiscovery { }; debug!("{}", t!("discovery.commandDiscovery.extensionFoundResources", extension = extension.type_name, count = discovered_resources.len())); for resource in discovered_resources { - if regex.is_match(&resource.type_name) { + if filter.is_match(&resource.type_name) { trace!("{}", t!("discovery.commandDiscovery.extensionResourceFound", resource = resource.type_name)); insert_resource(&mut resources, &resource); } @@ -358,9 +349,13 @@ impl ResourceDiscovery for CommandDiscovery { Ok(()) } - fn discover_adapted_resources(&mut self, name_filter: &str, adapter_filter: &str) -> Result<(), DscError> { + fn discover_adapted_resources( + &mut self, + name_filter: &TypeNameFilter, + adapter_filter: &TypeNameFilter + ) -> Result<(), DscError> { if self.discovery_mode == ResourceDiscoveryMode::DuringDeployment || (locked_is_empty!(RESOURCES) && locked_is_empty!(ADAPTERS)) { - self.discover(&DiscoveryKind::Resource, "*")?; + self.discover(&DiscoveryKind::Resource, &TypeNameFilter::default())?; } if locked_is_empty!(ADAPTERS) { @@ -368,21 +363,9 @@ impl ResourceDiscovery for CommandDiscovery { } let adapters = locked_clone!(ADAPTERS); - let regex_str = convert_wildcard_to_regex(adapter_filter); - debug!("Using regex {regex_str} as filter for adapter name"); - let mut regex_builder = RegexBuilder::new(®ex_str); - regex_builder.case_insensitive(true); - let Ok(regex) = regex_builder.build() else { - return Err(DscError::Operation("Could not build Regex filter for adapter name".to_string())); - }; + debug!("Using type name filter '{adapter_filter}' as filter for adapter name"); - let name_regex_str = convert_wildcard_to_regex(name_filter); - debug!("Using regex {name_regex_str} as filter for resource name"); - let mut name_regex_builder = RegexBuilder::new(&name_regex_str); - name_regex_builder.case_insensitive(true); - let Ok(name_regex) = name_regex_builder.build() else { - return Err(DscError::Operation("Could not build Regex filter for resource name".to_string())); - }; + debug!("Using type name filter '{name_filter}' as filter for resource name"); let mut progress = ProgressBar::new(adapters.len() as u64, self.progress_format)?; progress.write_activity("Searching for adapted resources"); @@ -394,7 +377,7 @@ impl ResourceDiscovery for CommandDiscovery { for adapter in adapters { progress.write_increment(1); - if !regex.is_match(adapter_name) { + if !adapter_filter.is_match(adapter_name) { continue; } @@ -432,7 +415,7 @@ impl ResourceDiscovery for CommandDiscovery { continue; } - if name_regex.is_match(&resource.type_name) { + if name_filter.is_match(&resource.type_name) { insert_resource(&mut adapted_resources, &resource); adapter_resources_count += 1; } @@ -457,19 +440,16 @@ impl ResourceDiscovery for CommandDiscovery { Ok(()) } - fn list_available(&mut self, kind: &DiscoveryKind, type_name_filter: &str, adapter_name_filter: &str) -> Result { + fn list_available( + &mut self, + kind: &DiscoveryKind, + type_name_filter: &TypeNameFilter, + adapter_name_filter: Option<&TypeNameFilter> + ) -> Result { let mut resources = DiscoveryManifestCache::new(); if *kind == DiscoveryKind::Resource { - if adapter_name_filter.is_empty() { - self.discover(kind, type_name_filter)?; - for (resource_name, resources_vec) in &locked_clone!(RESOURCES) { - resources.insert(resource_name.clone(), resources_vec.iter().map(|r| ImportedManifest::Resource(r.clone())).collect()); - } - for (adapter_name, adapter_vec) in &locked_clone!(ADAPTERS) { - resources.insert(adapter_name.clone(), adapter_vec.iter().map(|r| ImportedManifest::Resource(r.clone())).collect()); - } - } else { - self.discover(kind, "*")?; + if let Some(adapter_name_filter) = adapter_name_filter { + self.discover(kind, &TypeNameFilter::default())?; self.discover_adapted_resources(type_name_filter, adapter_name_filter)?; // add/update found adapted resources to the lookup_table @@ -479,6 +459,14 @@ impl ResourceDiscovery for CommandDiscovery { for (adapted_name, adapted_vec) in &adapted_resources { resources.insert(adapted_name.clone(), adapted_vec.iter().map(|r| ImportedManifest::Resource(r.clone())).collect()); } + } else { + self.discover(kind, type_name_filter)?; + for (resource_name, resources_vec) in &locked_clone!(RESOURCES) { + resources.insert(resource_name.clone(), resources_vec.iter().map(|r| ImportedManifest::Resource(r.clone())).collect()); + } + for (adapter_name, adapter_vec) in &locked_clone!(ADAPTERS) { + resources.insert(adapter_name.clone(), adapter_vec.iter().map(|r| ImportedManifest::Resource(r.clone())).collect()); + } } } else { self.discover(kind, type_name_filter)?; @@ -493,7 +481,7 @@ impl ResourceDiscovery for CommandDiscovery { fn find_resources(&mut self, required_resource_types: &[DiscoveryFilter]) -> Result { debug!("{}", t!("discovery.commandDiscovery.searchingForResources", resources = required_resource_types : {:?})); if self.discovery_mode == ResourceDiscoveryMode::DuringDeployment || locked_is_empty!(RESOURCES) { - self.discover(&DiscoveryKind::Resource, "*")?; + self.discover(&DiscoveryKind::Resource, &TypeNameFilter::default())?; } let mut found_resources = DiscoveryResourceCache::new(); let mut required_resources = HashMap::::new(); @@ -531,7 +519,7 @@ impl ResourceDiscovery for CommandDiscovery { } for adapter_name in &adapters { - self.discover_adapted_resources("*", adapter_name)?; + self.discover_adapted_resources(&TypeNameFilter::default(), &adapter_name.clone().into())?; add_resources_to_lookup_table(&locked_clone!(ADAPTED_RESOURCES)); for filter in required_resource_types { if let Some(adapted_resources) = locked_get!(ADAPTED_RESOURCES, filter.resource_type()) { @@ -551,7 +539,7 @@ impl ResourceDiscovery for CommandDiscovery { fn get_extensions(&mut self) -> Result { if locked_is_empty!(EXTENSIONS) { - self.discover(&DiscoveryKind::Extension, "*")?; + self.discover(&DiscoveryKind::Extension, &TypeNameFilter::default())?; } Ok(locked_clone!(EXTENSIONS)) } diff --git a/lib/dsc-lib/src/discovery/discovery_trait.rs b/lib/dsc-lib/src/discovery/discovery_trait.rs index cf6799dab..418e027b4 100644 --- a/lib/dsc-lib/src/discovery/discovery_trait.rs +++ b/lib/dsc-lib/src/discovery/discovery_trait.rs @@ -5,7 +5,7 @@ use crate::{ configure::config_doc::ResourceDiscoveryMode, discovery::{DiscoveryExtensionCache, DiscoveryManifestCache, DiscoveryResourceCache}, dscerror::DscError, - types::{FullyQualifiedTypeName, ResourceVersionReq, SemanticVersionReq} + types::{FullyQualifiedTypeName, ResourceVersionReq, SemanticVersionReq, TypeNameFilter} }; #[derive(Debug, PartialEq)] @@ -115,7 +115,7 @@ pub trait ResourceDiscovery { /// # Errors /// /// This function will return an error if the underlying discovery fails. - fn discover(&mut self, kind: &DiscoveryKind, filter: &str) -> Result<(), DscError>; + fn discover(&mut self, kind: &DiscoveryKind, filter: &TypeNameFilter) -> Result<(), DscError>; /// Discover adapted resources based on the provided filters. /// @@ -131,15 +131,19 @@ pub trait ResourceDiscovery { /// # Errors /// /// This function will return an error if the underlying discovery fails. - fn discover_adapted_resources(&mut self, name_filter: &str, adapter_filter: &str) -> Result<(), DscError>; + fn discover_adapted_resources( + &mut self, + name_filter: &TypeNameFilter, + adapter_filter: &TypeNameFilter + ) -> Result<(), DscError>; /// List available resources based on the provided filters. /// /// # Arguments /// - /// * `kind` - The kind of discovery (e.g., Resource). - /// * `type_name_filter` - The filter for the resource type name. - /// * `adapter_name_filter` - The filter for the adapter name (only applies to resources). + /// - `kind` - The kind of discovery (e.g., Resource). + /// - `type_name_filter` - The filter for the resource type name. + /// - `adapter_name_filter` - The filter for the adapter name (only applies to resources). /// /// # Returns /// @@ -151,8 +155,8 @@ pub trait ResourceDiscovery { fn list_available( &mut self, kind: &DiscoveryKind, - type_name_filter: &str, - adapter_name_filter: &str + type_name_filter: &TypeNameFilter, + adapter_name_filter: Option<&TypeNameFilter> ) -> Result; /// Find resources based on the required resource types. @@ -160,7 +164,8 @@ pub trait ResourceDiscovery { /// /// # Arguments /// - /// * `required_resource_types` - A slice of strings representing the required resource types. + /// - `required_resource_types` - A slice of `DiscoveryFilter` instances representing the + /// required resource types. /// /// # Returns /// @@ -186,6 +191,6 @@ pub trait ResourceDiscovery { /// /// # Arguments /// - /// * `mode` - The resource discovery mode to set. + /// - `mode` - The resource discovery mode to set. fn set_discovery_mode(&mut self, mode: &ResourceDiscoveryMode); } diff --git a/lib/dsc-lib/src/discovery/mod.rs b/lib/dsc-lib/src/discovery/mod.rs index 31e1d3bad..1faf04978 100644 --- a/lib/dsc-lib/src/discovery/mod.rs +++ b/lib/dsc-lib/src/discovery/mod.rs @@ -8,7 +8,7 @@ use crate::configure::config_doc::ResourceDiscoveryMode; use crate::discovery::discovery_trait::{DiscoveryKind, ResourceDiscovery, DiscoveryFilter}; use crate::dscerror::DscError; use crate::extensions::dscextension::{Capability, DscExtension}; -use crate::types::FullyQualifiedTypeName; +use crate::types::{FullyQualifiedTypeName, TypeNameFilter}; use crate::{dscresources::dscresource::DscResource, progress::ProgressFormat}; use core::result::Result::Ok; use semver::Version; @@ -60,8 +60,8 @@ impl Discovery { pub fn list_available( &mut self, kind: &DiscoveryKind, - type_name_filter: &str, - adapter_name_filter: &str, + type_name_filter: &TypeNameFilter, + adapter_name_filter: Option<&TypeNameFilter>, progress_format: ProgressFormat ) -> Vec { let discovery_types: Vec> = vec![ @@ -96,7 +96,7 @@ impl Discovery { pub fn get_extensions(&mut self, capability: &Capability) -> Vec { if self.extensions.is_empty() { - self.list_available(&DiscoveryKind::Extension, "*", "", ProgressFormat::None); + self.list_available(&DiscoveryKind::Extension, &TypeNameFilter::default(), None, ProgressFormat::None); } self.extensions.values() .filter(|ext| ext.capabilities.contains(capability)) diff --git a/lib/dsc-lib/src/lib.rs b/lib/dsc-lib/src/lib.rs index 093500963..5dce75744 100644 --- a/lib/dsc-lib/src/lib.rs +++ b/lib/dsc-lib/src/lib.rs @@ -4,6 +4,7 @@ use crate::discovery::{command_discovery::ImportedManifest, discovery_trait::DiscoveryFilter}; use crate::discovery::discovery_trait::DiscoveryKind; use crate::progress::ProgressFormat; +use crate::types::TypeNameFilter; use configure::config_doc::ExecutionKind; use dscerror::DscError; @@ -55,7 +56,13 @@ impl DscManager { self.discovery.find_resource(filter) } - pub fn list_available(&mut self, kind: &DiscoveryKind, type_name_filter: &str, adapter_name_filter: &str, progress_format: ProgressFormat) -> Vec { + pub fn list_available( + &mut self, + kind: &DiscoveryKind, + type_name_filter: &TypeNameFilter, + adapter_name_filter: Option<&TypeNameFilter>, + progress_format: ProgressFormat + ) -> Vec { self.discovery.list_available(kind, type_name_filter, adapter_name_filter, progress_format) } From c2ca59a3e7e71a246891d27c3a31f8dbfa1edc52 Mon Sep 17 00:00:00 2001 From: Mikey Lombardi Date: Thu, 26 Mar 2026 10:17:02 -0500 Subject: [PATCH 10/15] (GH-538) Use `TypeNameFilter` for CLI arguments This change finishes the transition to using `TypeNameFilter` for wildcard searching resources and extensions by updating the CLI argument types and updating the implementation to match. This change enables DSC to parse the filter _once_ as an argument to the CLI and then use the parsed and validated `TypeNameFilter` instead of constructing the regular expression on demand every time it's used. --- dsc/src/args.rs | 9 ++++++--- dsc/src/subcommand.rs | 45 +++++++------------------------------------ 2 files changed, 13 insertions(+), 41 deletions(-) diff --git a/dsc/src/args.rs b/dsc/src/args.rs index add67b0bb..313d056cb 100644 --- a/dsc/src/args.rs +++ b/dsc/src/args.rs @@ -5,6 +5,7 @@ use clap::{Parser, Subcommand, ValueEnum}; use clap_complete::Shell; use dsc_lib::dscresources::command_resource::TraceLevel; use dsc_lib::progress::ProgressFormat; +use dsc_lib::types::TypeNameFilter; use rust_i18n::t; use serde::Deserialize; @@ -178,7 +179,8 @@ pub enum ExtensionSubCommand { #[clap(name = "list", about = t!("args.listExtensionAbout").to_string())] List { /// Optional extension name to filter the list - extension_name: Option, + #[clap(default_value_t)] + extension_name: TypeNameFilter, #[clap(short = 'o', long, help = t!("args.outputFormat").to_string())] output_format: Option, }, @@ -200,10 +202,11 @@ pub enum ResourceSubCommand { #[clap(name = "list", about = t!("args.listAbout").to_string())] List { /// Optional resource name to filter the list - resource_name: Option, + #[clap(default_value_t)] + resource_name: TypeNameFilter, /// Optional adapter filter to apply to the list of resources #[clap(short = 'a', long = "adapter", help = t!("args.adapter").to_string())] - adapter_name: Option, + adapter_name: Option, #[clap(short, long, help = t!("args.description").to_string())] description: Option, #[clap(short, long, help = t!("args.tags").to_string())] diff --git a/dsc/src/subcommand.rs b/dsc/src/subcommand.rs index e2bf85139..fd5364e6e 100644 --- a/dsc/src/subcommand.rs +++ b/dsc/src/subcommand.rs @@ -526,7 +526,7 @@ pub fn extension(subcommand: &ExtensionSubCommand, progress_format: ProgressForm match subcommand { ExtensionSubCommand::List{extension_name, output_format} => { - list_extensions(&mut dsc, extension_name.as_ref(), output_format.as_ref(), progress_format); + list_extensions(&mut dsc, extension_name, output_format.as_ref(), progress_format); }, } } @@ -546,7 +546,7 @@ pub fn resource(subcommand: &ResourceSubCommand, progress_format: ProgressFormat match subcommand { ResourceSubCommand::List { resource_name, adapter_name, description, tags, output_format } => { - list_resources(&mut dsc, resource_name.as_ref(), adapter_name.as_ref(), description.as_ref(), tags.as_ref(), output_format.as_ref(), progress_format); + list_resources(&mut dsc, resource_name, adapter_name.as_ref(), description.as_ref(), tags.as_ref(), output_format.as_ref(), progress_format); }, ResourceSubCommand::Schema { resource , version, output_format } => { if let Err(err) = dsc.find_resources(&[DiscoveryFilter::new(resource, version.as_deref(), None)], progress_format) { @@ -607,7 +607,7 @@ pub fn resource(subcommand: &ResourceSubCommand, progress_format: ProgressFormat } } -fn list_extensions(dsc: &mut DscManager, extension_name: Option<&String>, format: Option<&ListOutputFormat>, progress_format: ProgressFormat) { +fn list_extensions(dsc: &mut DscManager, extension_name: &TypeNameFilter, format: Option<&ListOutputFormat>, progress_format: ProgressFormat) { let mut write_table = false; let mut table = Table::new(&[ t!("subcommand.tableHeader_type").to_string().as_ref(), @@ -620,17 +620,7 @@ fn list_extensions(dsc: &mut DscManager, extension_name: Option<&String>, format write_table = true; } let mut include_separator = false; - // Temporary workaround to convert input string into type name filter: - let ref extension_name = match extension_name { - Some(name_text) => match TypeNameFilter::parse(name_text) { - Ok(filter) => filter, - Err(err) => { - error!("{}: {err}", t!("subcommand.invalidResourceFilter")); - exit(EXIT_INVALID_ARGS); - } - }, - None => TypeNameFilter::default(), - }; + for manifest_resource in dsc.list_available(&DiscoveryKind::Extension, extension_name, None, progress_format) { if let ImportedManifest::Extension(extension) = manifest_resource { let capability_types = [ @@ -777,8 +767,8 @@ fn list_functions(functions: &FunctionDispatcher, function_name: Option<&String> pub fn list_resources( dsc: &mut DscManager, - resource_name: Option<&String>, - adapter_name: Option<&String>, + resource_name: &TypeNameFilter, + adapter_name: Option<&TypeNameFilter>, description: Option<&String>, tags: Option<&Vec>, format: Option<&ListOutputFormat>, @@ -798,29 +788,8 @@ pub fn list_resources( write_table = true; } let mut include_separator = false; - // Temporary workaround to convert input strings to type name filters: - let ref resource_name = match resource_name { - Some(name_text) => match TypeNameFilter::parse(name_text) { - Ok(filter) => filter, - Err(err) => { - error!("{}: {err}", t!("subcommand.invalidResourceFilter")); - exit(EXIT_INVALID_ARGS); - } - }, - None => TypeNameFilter::default(), - }; - let ref adapter_name = match adapter_name { - Some(name_text) => match TypeNameFilter::parse(name_text) { - Ok(filter) => Some(filter), - Err(err) => { - error!("{}: {err}", t!("subcommand.invalidAdapterFilter")); - exit(EXIT_INVALID_ARGS); - } - }, - None => None, - }; - for manifest_resource in dsc.list_available(&DiscoveryKind::Resource, resource_name, adapter_name.as_ref(), progress_format) { + for manifest_resource in dsc.list_available(&DiscoveryKind::Resource, resource_name, adapter_name, progress_format) { if let ImportedManifest::Resource(resource) = manifest_resource { let capability_types = [ (Capability::Get, "g"), From 88877bddb957bf693b1aef1abc3028ea6e488a23 Mon Sep 17 00:00:00 2001 From: Mikey Lombardi Date: Thu, 26 Mar 2026 11:27:49 -0500 Subject: [PATCH 11/15] (GH-538) Use defined types for CLI args This change updates the CLI arguments for resource type names to use the `FullyQualifiedTypeName` type, and for resource version requirements to use the `ResourceVersionReq` type. This enables DSC to fail fast and clearly on invalid input instead of propagating invalid strings to parse and validate later. This change also updates the `get_resource()` function to accept the defined types instead of string slices and updates the callers as needed. --- dsc/src/args.rs | 26 +++++++++++++------------- dsc/src/resource_command.rs | 33 +++++++++++++++++---------------- dsc/src/subcommand.rs | 35 ++++++++++++++++++++--------------- 3 files changed, 50 insertions(+), 44 deletions(-) diff --git a/dsc/src/args.rs b/dsc/src/args.rs index 313d056cb..3475871df 100644 --- a/dsc/src/args.rs +++ b/dsc/src/args.rs @@ -5,7 +5,7 @@ use clap::{Parser, Subcommand, ValueEnum}; use clap_complete::Shell; use dsc_lib::dscresources::command_resource::TraceLevel; use dsc_lib::progress::ProgressFormat; -use dsc_lib::types::TypeNameFilter; +use dsc_lib::types::{FullyQualifiedTypeName, ResourceVersionReq, TypeNameFilter}; use rust_i18n::t; use serde::Deserialize; @@ -219,9 +219,9 @@ pub enum ResourceSubCommand { #[clap(short, long, help = t!("args.getAll").to_string())] all: bool, #[clap(short, long, help = t!("args.resource").to_string())] - resource: String, + resource: FullyQualifiedTypeName, #[clap(short, long, help = t!("args.version").to_string())] - version: Option, + version: Option, #[clap(short, long, help = t!("args.input").to_string(), conflicts_with = "file")] input: Option, #[clap(short = 'f', long, help = t!("args.file").to_string(), conflicts_with = "input")] @@ -232,9 +232,9 @@ pub enum ResourceSubCommand { #[clap(name = "set", about = "Invoke the set operation to a resource", arg_required_else_help = true)] Set { #[clap(short, long, help = t!("args.resource").to_string())] - resource: String, + resource: FullyQualifiedTypeName, #[clap(short, long, help = t!("args.version").to_string())] - version: Option, + version: Option, #[clap(short, long, help = t!("args.input").to_string(), conflicts_with = "file")] input: Option, #[clap(short = 'f', long, help = t!("args.file").to_string(), conflicts_with = "input")] @@ -247,9 +247,9 @@ pub enum ResourceSubCommand { #[clap(name = "test", about = "Invoke the test operation to a resource", arg_required_else_help = true)] Test { #[clap(short, long, help = t!("args.resource").to_string())] - resource: String, + resource: FullyQualifiedTypeName, #[clap(short, long, help = t!("args.version").to_string())] - version: Option, + version: Option, #[clap(short, long, help = t!("args.input").to_string(), conflicts_with = "file")] input: Option, #[clap(short = 'f', long, help = t!("args.file").to_string(), conflicts_with = "input")] @@ -260,9 +260,9 @@ pub enum ResourceSubCommand { #[clap(name = "delete", about = "Invoke the delete operation to a resource", arg_required_else_help = true)] Delete { #[clap(short, long, help = t!("args.resource").to_string())] - resource: String, + resource: FullyQualifiedTypeName, #[clap(short, long, help = t!("args.version").to_string())] - version: Option, + version: Option, #[clap(short, long, help = t!("args.input").to_string(), conflicts_with = "file")] input: Option, #[clap(short = 'f', long, help = t!("args.file").to_string(), conflicts_with = "input")] @@ -275,18 +275,18 @@ pub enum ResourceSubCommand { #[clap(name = "schema", about = "Get the JSON schema for a resource", arg_required_else_help = true)] Schema { #[clap(short, long, help = t!("args.resource").to_string())] - resource: String, + resource: FullyQualifiedTypeName, #[clap(short, long, help = t!("args.version").to_string())] - version: Option, + version: Option, #[clap(short = 'o', long, help = t!("args.outputFormat").to_string())] output_format: Option, }, #[clap(name = "export", about = "Retrieve all resource instances", arg_required_else_help = true)] Export { #[clap(short, long, help = t!("args.resource").to_string())] - resource: String, + resource: FullyQualifiedTypeName, #[clap(short, long, help = t!("args.version").to_string())] - version: Option, + version: Option, #[clap(short, long, help = t!("args.input").to_string(), conflicts_with = "file")] input: Option, #[clap(short = 'f', long, help = t!("args.file").to_string(), conflicts_with = "input")] diff --git a/dsc/src/resource_command.rs b/dsc/src/resource_command.rs index cc4c0216a..e9fda17e9 100644 --- a/dsc/src/resource_command.rs +++ b/dsc/src/resource_command.rs @@ -9,6 +9,7 @@ use dsc_lib::discovery::discovery_trait::DiscoveryFilter; use dsc_lib::dscresources::{resource_manifest::Kind, invoke_result::{DeleteResultKind, GetResult, ResourceGetResponse, ResourceSetResponse, SetResult}}; use dsc_lib::dscresources::dscresource::{Capability, get_diff}; use dsc_lib::dscerror::DscError; +use dsc_lib::types::{FullyQualifiedTypeName, ResourceVersionReq}; use rust_i18n::t; use serde_json::Value; use tracing::{debug, error, info}; @@ -19,9 +20,9 @@ use dsc_lib::{ }; use std::process::exit; -pub fn get(dsc: &mut DscManager, resource_type: &str, version: Option<&str>, input: &str, format: Option<&GetOutputFormat>) { +pub fn get(dsc: &mut DscManager, resource_type: &FullyQualifiedTypeName, version: Option<&ResourceVersionReq>, input: &str, format: Option<&GetOutputFormat>) { let Some(resource) = get_resource(dsc, resource_type, version) else { - error!("{}", DscError::ResourceNotFound(resource_type.to_string(), version.unwrap_or("").to_string()).to_string()); + error!("{}", DscError::ResourceNotFound(resource_type.to_string(), version.map_or(String::new(), |v| v.to_string()))); exit(EXIT_DSC_RESOURCE_NOT_FOUND); }; @@ -70,10 +71,10 @@ pub fn get(dsc: &mut DscManager, resource_type: &str, version: Option<&str>, inp } } -pub fn get_all(dsc: &mut DscManager, resource_type: &str, version: Option<&str>, format: Option<&GetOutputFormat>) { +pub fn get_all(dsc: &mut DscManager, resource_type: &FullyQualifiedTypeName, version: Option<&ResourceVersionReq>, format: Option<&GetOutputFormat>) { let input = String::new(); let Some(resource) = get_resource(dsc, resource_type, version) else { - error!("{}", DscError::ResourceNotFound(resource_type.to_string(), version.unwrap_or("").to_string()).to_string()); + error!("{}", DscError::ResourceNotFound(resource_type.to_string(), version.map_or(String::new(), |r| r.to_string()))); exit(EXIT_DSC_RESOURCE_NOT_FOUND); }; @@ -128,14 +129,14 @@ pub fn get_all(dsc: &mut DscManager, resource_type: &str, version: Option<&str>, } } -pub fn set(dsc: &mut DscManager, resource_type: &str, version: Option<&str>, input: &str, format: Option<&OutputFormat>, what_if: bool) { +pub fn set(dsc: &mut DscManager, resource_type: &FullyQualifiedTypeName, version: Option<&ResourceVersionReq>, input: &str, format: Option<&OutputFormat>, what_if: bool) { if input.is_empty() { error!("{}", t!("resource_command.setInputEmpty")); exit(EXIT_INVALID_ARGS); } let Some(resource) = get_resource(dsc, resource_type, version) else { - error!("{}", DscError::ResourceNotFound(resource_type.to_string(), version.unwrap_or("").to_string()).to_string()); + error!("{}", DscError::ResourceNotFound(resource_type.to_string(), version.map_or(String::new(), |v| v.to_string()))); exit(EXIT_DSC_RESOURCE_NOT_FOUND); }; @@ -222,14 +223,14 @@ pub fn set(dsc: &mut DscManager, resource_type: &str, version: Option<&str>, inp } } -pub fn test(dsc: &mut DscManager, resource_type: &str, version: Option<&str>, input: &str, format: Option<&OutputFormat>) { +pub fn test(dsc: &mut DscManager, resource_type: &FullyQualifiedTypeName, version: Option<&ResourceVersionReq>, input: &str, format: Option<&OutputFormat>) { if input.is_empty() { error!("{}", t!("resource_command.testInputEmpty")); exit(EXIT_INVALID_ARGS); } let Some(resource) = get_resource(dsc, resource_type, version) else { - error!("{}", DscError::ResourceNotFound(resource_type.to_string(), version.unwrap_or("").to_string()).to_string()); + error!("{}", DscError::ResourceNotFound(resource_type.to_string(), version.map_or(String::new(), |v| v.to_string()))); exit(EXIT_DSC_RESOURCE_NOT_FOUND); }; @@ -258,9 +259,9 @@ pub fn test(dsc: &mut DscManager, resource_type: &str, version: Option<&str>, in } } -pub fn delete(dsc: &mut DscManager, resource_type: &str, version: Option<&str>, input: &str, format: Option<&OutputFormat>, what_if: bool) { +pub fn delete(dsc: &mut DscManager, resource_type: &FullyQualifiedTypeName, version: Option<&ResourceVersionReq>, input: &str, format: Option<&OutputFormat>, what_if: bool) { let Some(resource) = get_resource(dsc, resource_type, version) else { - error!("{}", DscError::ResourceNotFound(resource_type.to_string(), version.unwrap_or("").to_string()).to_string()); + error!("{}", DscError::ResourceNotFound(resource_type.to_string(), version.map_or(String::new(), |v| v.to_string()))); exit(EXIT_DSC_RESOURCE_NOT_FOUND); }; @@ -298,9 +299,9 @@ pub fn delete(dsc: &mut DscManager, resource_type: &str, version: Option<&str>, } } -pub fn schema(dsc: &mut DscManager, resource_type: &str, version: Option<&str>, format: Option<&OutputFormat>) { +pub fn schema(dsc: &mut DscManager, resource_type: &FullyQualifiedTypeName, version: Option<&ResourceVersionReq>, format: Option<&OutputFormat>) { let Some(resource) = get_resource(dsc, resource_type, version) else { - error!("{}", DscError::ResourceNotFound(resource_type.to_string(), version.unwrap_or("").to_string()).to_string()); + error!("{}", DscError::ResourceNotFound(resource_type.to_string(), version.map_or(String::new(), |v| v.to_string()))); exit(EXIT_DSC_RESOURCE_NOT_FOUND); }; if resource.kind == Kind::Adapter { @@ -327,9 +328,9 @@ pub fn schema(dsc: &mut DscManager, resource_type: &str, version: Option<&str>, } } -pub fn export(dsc: &mut DscManager, resource_type: &str, version: Option<&str>, input: &str, format: Option<&OutputFormat>) { +pub fn export(dsc: &mut DscManager, resource_type: &FullyQualifiedTypeName, version: Option<&ResourceVersionReq>, input: &str, format: Option<&OutputFormat>) { let Some(dsc_resource) = get_resource(dsc, resource_type, version) else { - error!("{}", DscError::ResourceNotFound(resource_type.to_string(), version.unwrap_or("").to_string()).to_string()); + error!("{}", DscError::ResourceNotFound(resource_type.to_string(), version.map_or(String::new(), |v| v.to_string())).to_string()); exit(EXIT_DSC_RESOURCE_NOT_FOUND); }; @@ -355,7 +356,7 @@ pub fn export(dsc: &mut DscManager, resource_type: &str, version: Option<&str>, } #[must_use] -pub fn get_resource<'a>(dsc: &'a mut DscManager, resource: &str, version: Option<&str>) -> Option<&'a DscResource> { +pub fn get_resource<'a>(dsc: &'a mut DscManager, resource: &FullyQualifiedTypeName, version: Option<&ResourceVersionReq>) -> Option<&'a DscResource> { //TODO: add dynamically generated resource to dsc - dsc.find_resource(&DiscoveryFilter::new(resource, version, None)).unwrap_or(None) + dsc.find_resource(&DiscoveryFilter::new_for_resource(resource, version.cloned(), None)).unwrap_or(None) } diff --git a/dsc/src/subcommand.rs b/dsc/src/subcommand.rs index fd5364e6e..6310ca216 100644 --- a/dsc/src/subcommand.rs +++ b/dsc/src/subcommand.rs @@ -7,7 +7,7 @@ use crate::resource_command::{get_resource, self}; use crate::tablewriter::Table; use crate::util::{get_input, get_schema, in_desired_state, set_dscconfigroot, write_object, DSC_CONFIG_ROOT, EXIT_DSC_ASSERTION_FAILED, EXIT_DSC_ERROR, EXIT_INVALID_ARGS, EXIT_INVALID_INPUT, EXIT_JSON_ERROR}; use dsc_lib::functions::FunctionArgKind; -use dsc_lib::types::TypeNameFilter; +use dsc_lib::types::{FullyQualifiedTypeName, ResourceVersionReq, TypeNameFilter}; use dsc_lib::{ configure::{ config_doc::{ @@ -504,11 +504,16 @@ pub fn validate_config(config: &Configuration, progress_format: ProgressFormat) let Some(type_name) = resource_block["type"].as_str() else { return Err(DscError::Validation(t!("subcommand.resourceTypeNotSpecified").to_string())); }; + let type_name = &FullyQualifiedTypeName::parse(type_name)?; + let require_version = resource_block["requireVersion"] + .as_str() + .map(|r| ResourceVersionReq::parse(r)) + .transpose()?; trace!("{} '{}'", t!("subcommand.validatingResource"), resource_block["name"].as_str().unwrap_or_default()); // get the actual resource - let Some(resource) = get_resource(&mut dsc, type_name, resource_block["requireVersion"].as_str()) else { + let Some(resource) = get_resource(&mut dsc, type_name, require_version.as_ref()) else { return Err(DscError::Validation(format!("{}: '{type_name}'", t!("subcommand.resourceNotFound")))); }; @@ -549,27 +554,27 @@ pub fn resource(subcommand: &ResourceSubCommand, progress_format: ProgressFormat list_resources(&mut dsc, resource_name, adapter_name.as_ref(), description.as_ref(), tags.as_ref(), output_format.as_ref(), progress_format); }, ResourceSubCommand::Schema { resource , version, output_format } => { - if let Err(err) = dsc.find_resources(&[DiscoveryFilter::new(resource, version.as_deref(), None)], progress_format) { + if let Err(err) = dsc.find_resources(&[DiscoveryFilter::new_for_resource(resource, version.clone(), None)], progress_format) { error!("{}: {err}", t!("subcommand.failedDiscoverResource")); exit(EXIT_DSC_ERROR); } - resource_command::schema(&mut dsc, resource, version.as_deref(), output_format.as_ref()); + resource_command::schema(&mut dsc, resource, version.as_ref(), output_format.as_ref()); }, ResourceSubCommand::Export { resource, version, input, file, output_format } => { - if let Err(err) = dsc.find_resources(&[DiscoveryFilter::new(resource, version.as_deref(), None)], progress_format) { + if let Err(err) = dsc.find_resources(&[DiscoveryFilter::new_for_resource(resource, version.clone(), None)], progress_format) { error!("{}: {err}", t!("subcommand.failedDiscoverResource")); exit(EXIT_DSC_ERROR); } let parsed_input = get_input(input.as_ref(), file.as_ref()); - resource_command::export(&mut dsc, resource, version.as_deref(), &parsed_input, output_format.as_ref()); + resource_command::export(&mut dsc, resource, version.as_ref(), &parsed_input, output_format.as_ref()); }, ResourceSubCommand::Get { resource, version, input, file: path, all, output_format } => { - if let Err(err) = dsc.find_resources(&[DiscoveryFilter::new(resource, version.as_deref(), None)], progress_format) { + if let Err(err) = dsc.find_resources(&[DiscoveryFilter::new_for_resource(resource, version.clone(), None)], progress_format) { error!("{}: {err}", t!("subcommand.failedDiscoverResource")); exit(EXIT_DSC_ERROR); } if *all { - resource_command::get_all(&mut dsc, resource, version.as_deref(), output_format.as_ref()); + resource_command::get_all(&mut dsc, resource, version.as_ref(), output_format.as_ref()); } else { if *output_format == Some(GetOutputFormat::JsonArray) { @@ -577,32 +582,32 @@ pub fn resource(subcommand: &ResourceSubCommand, progress_format: ProgressFormat exit(EXIT_INVALID_ARGS); } let parsed_input = get_input(input.as_ref(), path.as_ref()); - resource_command::get(&mut dsc, resource, version.as_deref(), &parsed_input, output_format.as_ref()); + resource_command::get(&mut dsc, resource, version.as_ref(), &parsed_input, output_format.as_ref()); } }, ResourceSubCommand::Set { resource, version, input, file: path, output_format, what_if } => { - if let Err(err) = dsc.find_resources(&[DiscoveryFilter::new(resource, version.as_deref(), None)], progress_format) { + if let Err(err) = dsc.find_resources(&[DiscoveryFilter::new_for_resource(resource, version.clone(), None)], progress_format) { error!("{}: {err}", t!("subcommand.failedDiscoverResource")); exit(EXIT_DSC_ERROR); } let parsed_input = get_input(input.as_ref(), path.as_ref()); - resource_command::set(&mut dsc, resource, version.as_deref(), &parsed_input, output_format.as_ref(), *what_if); + resource_command::set(&mut dsc, resource, version.as_ref(), &parsed_input, output_format.as_ref(), *what_if); }, ResourceSubCommand::Test { resource, version, input, file: path, output_format } => { - if let Err(err) = dsc.find_resources(&[DiscoveryFilter::new(resource, version.as_deref(), None)], progress_format) { + if let Err(err) = dsc.find_resources(&[DiscoveryFilter::new_for_resource(resource, version.clone(), None)], progress_format) { error!("{}: {err}", t!("subcommand.failedDiscoverResource")); exit(EXIT_DSC_ERROR); } let parsed_input = get_input(input.as_ref(), path.as_ref()); - resource_command::test(&mut dsc, resource, version.as_deref(), &parsed_input, output_format.as_ref()); + resource_command::test(&mut dsc, resource, version.as_ref(), &parsed_input, output_format.as_ref()); }, ResourceSubCommand::Delete { resource, version, input, file: path, output_format, what_if } => { - if let Err(err) = dsc.find_resources(&[DiscoveryFilter::new(resource, version.as_deref(), None)], progress_format) { + if let Err(err) = dsc.find_resources(&[DiscoveryFilter::new_for_resource(resource, version.clone(), None)], progress_format) { error!("{}: {err}", t!("subcommand.failedDiscoverResource")); exit(EXIT_DSC_ERROR); } let parsed_input = get_input(input.as_ref(), path.as_ref()); - resource_command::delete(&mut dsc, resource, version.as_deref(), &parsed_input, output_format.as_ref(), *what_if); + resource_command::delete(&mut dsc, resource, version.as_ref(), &parsed_input, output_format.as_ref(), *what_if); }, } } From e09661e0e98c76ae27aafcd241849a2a7c1d2a6f Mon Sep 17 00:00:00 2001 From: Mikey Lombardi Date: Thu, 26 Mar 2026 13:09:26 -0500 Subject: [PATCH 12/15] (MAINT) Address copilot review --- dsc/src/mcp/invoke_dsc_resource.rs | 2 +- dsc/src/mcp/list_dsc_resources.rs | 4 ++-- dsc/src/subcommand.rs | 7 +++++- lib/dsc-lib/locales/en-us.toml | 2 +- .../src/discovery/command_discovery.rs | 22 +++++++++++-------- lib/dsc-lib/src/discovery/discovery_trait.rs | 2 ++ 6 files changed, 25 insertions(+), 14 deletions(-) diff --git a/dsc/src/mcp/invoke_dsc_resource.rs b/dsc/src/mcp/invoke_dsc_resource.rs index 5cd171e97..15d4c619b 100644 --- a/dsc/src/mcp/invoke_dsc_resource.rs +++ b/dsc/src/mcp/invoke_dsc_resource.rs @@ -71,7 +71,7 @@ impl McpServer { pub async fn invoke_dsc_resource(&self, Parameters(InvokeDscResourceRequest { operation, resource_type, properties_json }): Parameters) -> Result, McpError> { let result = task::spawn_blocking(move || { let mut dsc = DscManager::new(); - let Some(resource) = dsc.find_resource(&DiscoveryFilter::new(&resource_type, None, None)).unwrap_or(None) else { + let Some(resource) = dsc.find_resource(&DiscoveryFilter::new_for_resource(&resource_type, None, None)).unwrap_or(None) else { return Err(McpError::invalid_request(t!("mcp.invoke_dsc_resource.resourceNotFound", resource = resource_type), None)); }; match operation { diff --git a/dsc/src/mcp/list_dsc_resources.rs b/dsc/src/mcp/list_dsc_resources.rs index f9df20a5a..0948e18a2 100644 --- a/dsc/src/mcp/list_dsc_resources.rs +++ b/dsc/src/mcp/list_dsc_resources.rs @@ -32,7 +32,7 @@ pub struct ResourceSummary { #[derive(Deserialize, JsonSchema)] pub struct ListResourcesRequest { #[schemars(description = "Filter adapted resources to only those requiring the specified adapter type. If not specified, all non-adapted resources are returned.")] - pub adapter: Option, + pub adapter: Option, } #[tool_router(router = list_dsc_resources_router, vis = "pub")] @@ -52,7 +52,7 @@ impl McpServer { let mut dsc = DscManager::new(); let adapter_filter = match adapter { Some(adapter) => { - if let Some(resource) = dsc.find_resource(&DiscoveryFilter::new(&adapter, None, None)).unwrap_or(None) { + if let Some(resource) = dsc.find_resource(&DiscoveryFilter::new_for_resource(&adapter, None, None)).unwrap_or(None) { if resource.kind != Kind::Adapter { return Err(McpError::invalid_params(t!("mcp.list_dsc_resources.resourceNotAdapter", adapter = adapter), None)); } diff --git a/dsc/src/subcommand.rs b/dsc/src/subcommand.rs index 6310ca216..b7ac43bef 100644 --- a/dsc/src/subcommand.rs +++ b/dsc/src/subcommand.rs @@ -496,7 +496,12 @@ pub fn validate_config(config: &Configuration, progress_format: ProgressFormat) let Some(type_name) = resource_block["type"].as_str() else { return Err(DscError::Validation(t!("subcommand.resourceTypeNotSpecified").to_string())); }; - resource_types.push(DiscoveryFilter::new(type_name, resource_block["requireVersion"].as_str(), None)); + let type_name = &FullyQualifiedTypeName::parse(type_name)?; + let require_version = resource_block["requireVersion"] + .as_str() + .map(|r| ResourceVersionReq::parse(r)) + .transpose()?; + resource_types.push(DiscoveryFilter::new_for_resource(type_name, require_version, None)); } dsc.find_resources(&resource_types, progress_format)?; diff --git a/lib/dsc-lib/locales/en-us.toml b/lib/dsc-lib/locales/en-us.toml index 500b02a7a..b11bed104 100644 --- a/lib/dsc-lib/locales/en-us.toml +++ b/lib/dsc-lib/locales/en-us.toml @@ -129,7 +129,7 @@ invalidManifestFile = "Invalid manifest file '%{resource}': %{err}" extensionResourceFound = "Extension found resource '%{resource}'" callingExtension = "Calling extension '%{extension}' to discover resources" extensionFoundResources = "Extension '%{extension}' found %{count} resources" -invalidManifestVersion = "Manifest '%{path}' does not use semver: %{err}" +invalidManifestVersion = "Manifest '%{path}' is defined with non-semantic version '%{version}'" importExtensionsEmpty = "Import extension '%{extension}' has no import extensions defined" searchingForResources = "Searching for resources: %{resources}" foundResourceWithVersion = "Found matching resource '%{resource}' version %{version}" diff --git a/lib/dsc-lib/src/discovery/command_discovery.rs b/lib/dsc-lib/src/discovery/command_discovery.rs index 324171294..b4f9022a5 100644 --- a/lib/dsc-lib/src/discovery/command_discovery.rs +++ b/lib/dsc-lib/src/discovery/command_discovery.rs @@ -5,7 +5,7 @@ use crate::{discovery::{DiscoveryExtensionCache, DiscoveryManifestCache, Discove use crate::{locked_clear, locked_is_empty, locked_extend, locked_clone, locked_get}; use crate::configure::{config_doc::ResourceDiscoveryMode, context::Context}; use crate::dscresources::dscresource::{Capability, DscResource, ImplementedAs}; -use crate::dscresources::resource_manifest::{validate_semver, Kind, ResourceManifest, SchemaKind}; +use crate::dscresources::resource_manifest::{Kind, ResourceManifest, SchemaKind}; use crate::dscresources::command_resource::invoke_command; use crate::dscerror::DscError; use crate::extensions::dscextension::{self, DscExtension, Capability as ExtensionCapability}; @@ -741,8 +741,12 @@ pub fn load_manifest(path: &Path) -> Result, DscError> { } fn load_adapted_resource_manifest(path: &Path, manifest: &AdaptedDscResourceManifest) -> Result { - if let Err(err) = validate_semver(&manifest.version.to_string()) { - warn!("{}", t!("discovery.commandDiscovery.invalidManifestVersion", path = path.to_string_lossy(), err = err).to_string()); + if manifest.version.is_date_version() { + warn!("{}", t!( + "discovery.commandDiscovery.invalidManifestVersion", + path = path.to_string_lossy(), + version = manifest.version + )); } let directory = path.parent().unwrap(); @@ -769,8 +773,12 @@ fn load_adapted_resource_manifest(path: &Path, manifest: &AdaptedDscResourceMani } fn load_resource_manifest(path: &Path, manifest: &ResourceManifest) -> Result { - if let Err(err) = validate_semver(&manifest.version.to_string()) { - warn!("{}", t!("discovery.commandDiscovery.invalidManifestVersion", path = path.to_string_lossy(), err = err).to_string()); + if manifest.version.is_date_version() { + warn!("{}", t!( + "discovery.commandDiscovery.invalidManifestVersion", + path = path.to_string_lossy(), + version = manifest.version + )); } let kind = if let Some(kind) = manifest.kind.clone() { @@ -829,10 +837,6 @@ fn load_resource_manifest(path: &Path, manifest: &ResourceManifest) -> Result Result { - if let Err(err) = validate_semver(&manifest.version.to_string()) { - warn!("{}", t!("discovery.commandDiscovery.invalidManifestVersion", path = path.to_string_lossy(), err = err).to_string()); - } - let mut capabilities: Vec = vec![]; if let Some(discover) = &manifest.discover { verify_executable(&manifest.r#type, "discover", &discover.executable, path.parent().unwrap()); diff --git a/lib/dsc-lib/src/discovery/discovery_trait.rs b/lib/dsc-lib/src/discovery/discovery_trait.rs index 418e027b4..e199c24dd 100644 --- a/lib/dsc-lib/src/discovery/discovery_trait.rs +++ b/lib/dsc-lib/src/discovery/discovery_trait.rs @@ -24,6 +24,8 @@ pub struct DiscoveryFilter { impl DiscoveryFilter { #[must_use] pub fn new(resource_type: &str, version: Option<&str>, adapter: Option<&str>) -> Self { + // Keeping this implementation as parse-and-fail to avoid needing to update bicep; + // this should be addressed more fully in a follow-up change. Self { require_adapter: adapter.map(|a| a.parse().unwrap()), r#type: resource_type.parse().unwrap(), From a6f7c6486ac8df0a6721618ebb92905a6ccc3404 Mon Sep 17 00:00:00 2001 From: Mikey Lombardi Date: Thu, 26 Mar 2026 13:49:16 -0500 Subject: [PATCH 13/15] (GH-538) Ensure PS adapters return semver Prior to this change, the PowerShell adapters returned non-semantic versions when discovering resources. This change updates the implementations to munge the `[Version]` type to a semantic version string. If the `Build` segment is defined, it is used as the patch segment. Otherwise the patch segment is set to `0`. --- .../TestAdapter/testadapter.resource.ps1 | 2 +- .../Tests/powershellgroup.resource.tests.ps1 | 18 +++++++-------- .../powershell/psDscAdapter/psDscAdapter.psm1 | 19 +++++++++++++--- .../psDscAdapter/win_psDscAdapter.psm1 | 22 +++++++++++++++++-- 4 files changed, 46 insertions(+), 15 deletions(-) diff --git a/adapters/powershell/Tests/TestAdapter/testadapter.resource.ps1 b/adapters/powershell/Tests/TestAdapter/testadapter.resource.ps1 index 85d5de051..359f0d92c 100644 --- a/adapters/powershell/Tests/TestAdapter/testadapter.resource.ps1 +++ b/adapters/powershell/Tests/TestAdapter/testadapter.resource.ps1 @@ -37,7 +37,7 @@ switch ($Operation) { @{ type = "Test/TestCase" kind = 'resource' - version = '1' + version = '1.0.0' capabilities = @('get', 'set', 'test', 'export') path = $PSScriptRoot directory = Split-Path $PSScriptRoot diff --git a/adapters/powershell/Tests/powershellgroup.resource.tests.ps1 b/adapters/powershell/Tests/powershellgroup.resource.tests.ps1 index ec63ed9a7..97bcc5897 100644 --- a/adapters/powershell/Tests/powershellgroup.resource.tests.ps1 +++ b/adapters/powershell/Tests/powershellgroup.resource.tests.ps1 @@ -195,9 +195,9 @@ Describe 'PowerShell adapter resource tests' { $srcPath = Join-Path $PSScriptRoot 'TestClassResource' $pathRoot1 = Join-Path $TestDrive 'A' $pathRoot2 = Join-Path $TestDrive 'B' - $path1 = Join-Path $pathRoot1 'TestClassResource' '1.0' - $path2 = Join-Path $pathRoot1 'TestClassResource' '1.1' - $path3 = Join-Path $pathRoot2 'TestClassResource' '2.0' + $path1 = Join-Path $pathRoot1 'TestClassResource' '1.0.0' + $path2 = Join-Path $pathRoot1 'TestClassResource' '1.1.0' + $path3 = Join-Path $pathRoot2 'TestClassResource' '2.0.0' $path4 = Join-Path $pathRoot2 'TestClassResource' '2.0.1' New-Item -ItemType Directory -Force -Path $path1 | Out-Null @@ -212,11 +212,11 @@ Describe 'PowerShell adapter resource tests' { $files | Copy-Item -Destination $path4 $filePath = Join-Path $path1 'TestClassResource.psd1' - (Get-Content -Raw $filePath).Replace("ModuleVersion = `'0.0.1`'", "ModuleVersion = `'1.0`'") | Set-Content $filePath + (Get-Content -Raw $filePath).Replace("ModuleVersion = `'0.0.1`'", "ModuleVersion = `'1.0.0`'") | Set-Content $filePath $filePath = Join-Path $path2 'TestClassResource.psd1' - (Get-Content -Raw $filePath).Replace("ModuleVersion = `'0.0.1`'", "ModuleVersion = `'1.1`'") | Set-Content $filePath + (Get-Content -Raw $filePath).Replace("ModuleVersion = `'0.0.1`'", "ModuleVersion = `'1.1.0`'") | Set-Content $filePath $filePath = Join-Path $path3 'TestClassResource.psd1' - (Get-Content -Raw $filePath).Replace("ModuleVersion = `'0.0.1`'", "ModuleVersion = `'2.0`'") | Set-Content $filePath + (Get-Content -Raw $filePath).Replace("ModuleVersion = `'0.0.1`'", "ModuleVersion = `'2.0.0`'") | Set-Content $filePath $filePath = Join-Path $path4 'TestClassResource.psd1' (Get-Content -Raw $filePath).Replace("ModuleVersion = `'0.0.1`'", "ModuleVersion = `'2.0.1`'") | Set-Content $filePath @@ -377,15 +377,15 @@ Describe 'PowerShell adapter resource tests' { } It 'Specifying version works' { - $out = dsc resource get -r TestClassResource/TestClassResource --version 0.0.1 | ConvertFrom-Json + $out = dsc resource get -r TestClassResource/TestClassResource --version '=0.0.1' | ConvertFrom-Json $LASTEXITCODE | Should -Be 0 $out.actualState.Ensure | Should -BeExactly 'Present' } It 'Specifying a non-existent version returns an error' { - $null = dsc resource get -r TestClassResource/TestClassResource --version 0.0.2 2> $TestDrive/error.log + $null = dsc resource get -r TestClassResource/TestClassResource --version '=0.0.2' 2> $TestDrive/error.log $LASTEXITCODE | Should -Be 7 - (Get-Content -Raw -Path $TestDrive/error.log) | Should -BeLike '*Resource not found: TestClassResource/TestClassResource 0.0.2*' -Because (Get-Content -Raw -Path $TestDrive/error.log) + (Get-Content -Raw -Path $TestDrive/error.log) | Should -BeLike '*Resource not found: TestClassResource/TestClassResource =0.0.2*' -Because (Get-Content -Raw -Path $TestDrive/error.log) } It 'Can process SecureString property' { diff --git a/adapters/powershell/psDscAdapter/psDscAdapter.psm1 b/adapters/powershell/psDscAdapter/psDscAdapter.psm1 index 3fb07c51c..5555be8c7 100644 --- a/adapters/powershell/psDscAdapter/psDscAdapter.psm1 +++ b/adapters/powershell/psDscAdapter/psDscAdapter.psm1 @@ -86,13 +86,26 @@ function Add-AstMembers { } } +function ConvertTo-SemanticVersionString { + [cmdletbinding()] + param([version]$version) + + $patch = if ($version.Build -ne -1) { $version.Build } else { 0 } + + "{0}.{1}.{2}" -f @( + $version.Major, + $version.Minor, + $patch + ) +} + function FindAndParseResourceDefinitions { [CmdletBinding(HelpUri = '')] param( [Parameter(Mandatory = $true)] [string]$filePath, [Parameter(Mandatory = $true)] - [string]$moduleVersion + [version]$moduleVersion ) if (-not (Test-Path $filePath)) { @@ -134,7 +147,7 @@ function FindAndParseResourceDefinitions { #TODO: ModuleName, Version and ParentPath should be taken from psd1 contents $DscResourceInfo.ModuleName = [System.IO.Path]::GetFileNameWithoutExtension($filePath) $DscResourceInfo.ParentPath = [System.IO.Path]::GetDirectoryName($filePath) - $DscResourceInfo.Version = $moduleVersion + $DscResourceInfo.Version = ConvertTo-SemanticVersionString $moduleVersion $DscResourceInfo.Properties = [System.Collections.Generic.List[DscResourcePropertyInfo]]::new() $DscResourceInfo.Capabilities = GetClassBasedCapabilities $typeDefinitionAst.Members @@ -206,7 +219,7 @@ function LoadPowerShellClassResourcesFromModule { $scriptPath = $moduleInfo.Path; } - $version = if ($moduleInfo.Version) { $moduleInfo.Version.ToString() } else { '0.0.0' } + $version = if ($moduleInfo.Version) { $moduleInfo.Version } else { [version]'0.0.0' } $Resources = FindAndParseResourceDefinitions $scriptPath $version if ($moduleInfo.NestedModules) { diff --git a/adapters/powershell/psDscAdapter/win_psDscAdapter.psm1 b/adapters/powershell/psDscAdapter/win_psDscAdapter.psm1 index 4562f4ba7..2317e5e0a 100644 --- a/adapters/powershell/psDscAdapter/win_psDscAdapter.psm1 +++ b/adapters/powershell/psDscAdapter/win_psDscAdapter.psm1 @@ -26,6 +26,19 @@ if ($PSVersionTable.PSVersion.Major -gt 5) { } } +function ConvertTo-SemanticVersionString { + [cmdletbinding()] + param([version]$version) + + $patch = if ($version.Build -ne -1) { $version.Build } else { 0 } + + "{0}.{1}.{2}" -f @( + $version.Major, + $version.Minor, + $patch + ) +} + <# public function Invoke-DscCacheRefresh .SYNOPSIS This function caches the results of the Get-DscResource call to optimize performance. @@ -187,7 +200,12 @@ function Invoke-DscCacheRefresh { $DscResourceInfo = [DscResourceInfo]::new() $dscResource.PSObject.Properties | ForEach-Object -Process { if ($null -ne $_.Value) { - $DscResourceInfo.$($_.Name) = $_.Value + # Handle version specially to munge as semantic version + if ($_.Name -eq 'Version') { + $DscResourceInfo.$($_.Name) = ConvertTo-SemanticVersionString $_.Value + } else { + $DscResourceInfo.$($_.Name) = $_.Value + } } else { $DscResourceInfo.$($_.Name) = '' } @@ -212,7 +230,7 @@ function Invoke-DscCacheRefresh { # workaround: populate module version from psmoduleinfo if available if ($moduleInfo = $Modules | Where-Object { $_.Name -eq $moduleName }) { $moduleInfo = $moduleInfo | Sort-Object -Property Version -Descending | Select-Object -First 1 - $DscResourceInfo.Version = $moduleInfo.Version.ToString() + $DscResourceInfo.Version = ConvertTo-SemanticVersionString $moduleInfo.Version } } From 1ef96688445479bf165df895347ac14d48bb19ea Mon Sep 17 00:00:00 2001 From: Mikey Lombardi Date: Thu, 26 Mar 2026 13:49:54 -0500 Subject: [PATCH 14/15] (GH-538) Fix acceptance tests This change addresses failing acceptance tests caused by the update to version parsing and handling. --- dsc/tests/dsc_config_version.tests.ps1 | 21 ++++++++++----------- dsc/tests/dsc_discovery.tests.ps1 | 2 +- dsc/tests/dsc_resource_get.tests.ps1 | 2 +- dsc/tests/dsc_resource_set.tests.ps1 | 2 +- dsc/tests/dsc_resource_test.tests.ps1 | 2 +- dsc/tests/dsc_tracing.tests.ps1 | 10 +++++----- lib/dsc-lib/locales/en-us.toml | 2 -- 7 files changed, 19 insertions(+), 22 deletions(-) diff --git a/dsc/tests/dsc_config_version.tests.ps1 b/dsc/tests/dsc_config_version.tests.ps1 index 672c0dae0..bd109edd7 100644 --- a/dsc/tests/dsc_config_version.tests.ps1 +++ b/dsc/tests/dsc_config_version.tests.ps1 @@ -19,7 +19,7 @@ Describe 'Tests for resource versioning' { resources: - name: Test Version type: Test/Version - requireVersion: $version + requireVersion: '=$version' properties: version: $version "@ @@ -34,13 +34,12 @@ Describe 'Tests for resource versioning' { @{ req = '<1.3' ; expected = '1.1.3' } @{ req = '>1,<=2.0.0' ; expected = '2.0.0' } @{ req = '>1.0.0,<2.0.0' ; expected = '1.1.3' } - @{ req = '1'; expected = '1.1.3' } - @{ req = '1.1' ; expected = '1.1.3' } + @{ req = '=1'; expected = '1.1.3' } + @{ req = '=1.1' ; expected = '1.1.3' } @{ req = '^1.0' ; expected = '1.1.3' } @{ req = '~1.1' ; expected = '1.1.3' } - @{ req = '*' ; expected = '2.0.0' } - @{ req = '1.*' ; expected = '1.1.3' } - @{ req = '2.1.0-preview.2' ; expected = '2.1.0-preview.2' } + @{ req = '=1.*' ; expected = '1.1.3' } + @{ req = '=2.1.0-preview.2' ; expected = '2.1.0-preview.2' } ) { param($req, $expected) $config_yaml = @" @@ -63,13 +62,13 @@ Describe 'Tests for resource versioning' { resources: - name: Test Version 1 type: Test/Version - requireVersion: '1.1.2' + requireVersion: '=1.1.2' - name: Test Version 2 type: Test/Version - requireVersion: '1.1.0' + requireVersion: '=1.1.0' - name: Test Version 3 type: Test/Version - requireVersion: '2' + requireVersion: '=2' "@ $out = dsc -l trace config get -i $config_yaml 2> $TestDrive/error.log | ConvertFrom-Json $LASTEXITCODE | Should -Be 0 -Because (Get-Content $TestDrive/error.log -Raw) @@ -84,9 +83,9 @@ Describe 'Tests for resource versioning' { resources: - name: Test Version type: Test/Version - apiVersion: '1.1.2' + apiVersion: '=1.1.2' properties: - version: '1.1.2' + version: '=1.1.2' "@ $out = dsc -l trace config get -i $config_yaml 2> $TestDrive/error.log | ConvertFrom-Json $LASTEXITCODE | Should -Be 0 -Because (Get-Content $TestDrive/error.log -Raw) diff --git a/dsc/tests/dsc_discovery.tests.ps1 b/dsc/tests/dsc_discovery.tests.ps1 index a6809ec60..2227491d2 100644 --- a/dsc/tests/dsc_discovery.tests.ps1 +++ b/dsc/tests/dsc_discovery.tests.ps1 @@ -102,7 +102,7 @@ Describe 'tests for resource discovery' { '@ Set-Content -Path "$testdrive/test.dsc.resource.json" -Value $manifest $null = dsc resource list 2> "$testdrive/error.txt" - "$testdrive/error.txt" | Should -FileContentMatchExactly 'WARN.*?does not use semver' -Because (Get-Content -Raw "$testdrive/error.txt") + "$testdrive/error.txt" | Should -FileContentMatchExactly 'WARN.*?invalid semantic version' -Because (Get-Content -Raw "$testdrive/error.txt") } } diff --git a/dsc/tests/dsc_resource_get.tests.ps1 b/dsc/tests/dsc_resource_get.tests.ps1 index d39be27dc..aab5f4806 100644 --- a/dsc/tests/dsc_resource_get.tests.ps1 +++ b/dsc/tests/dsc_resource_get.tests.ps1 @@ -72,7 +72,7 @@ Describe 'resource get tests' { } It 'version works' { - $out = dsc resource get -r Test/Version --version 1.1.2 | ConvertFrom-Json + $out = dsc resource get -r Test/Version --version '=1.1.2' | ConvertFrom-Json $LASTEXITCODE | Should -Be 0 $out.actualState.version | Should -BeExactly '1.1.2' } diff --git a/dsc/tests/dsc_resource_set.tests.ps1 b/dsc/tests/dsc_resource_set.tests.ps1 index 61b91da2c..75acab7a1 100644 --- a/dsc/tests/dsc_resource_set.tests.ps1 +++ b/dsc/tests/dsc_resource_set.tests.ps1 @@ -9,7 +9,7 @@ Describe 'Invoke a resource set directly' { } It 'version works' { - $out = dsc resource set -r Test/Version --version 1.1.2 --input '{"version":"1.1.2"}' | ConvertFrom-Json + $out = dsc resource set -r Test/Version --version '=1.1.2' --input '{"version":"1.1.2"}' | ConvertFrom-Json $LASTEXITCODE | Should -Be 0 $out.afterState.version | Should -BeExactly '1.1.2' $out.changedProperties | Should -BeNullOrEmpty diff --git a/dsc/tests/dsc_resource_test.tests.ps1 b/dsc/tests/dsc_resource_test.tests.ps1 index 8eee35991..e692a49d7 100644 --- a/dsc/tests/dsc_resource_test.tests.ps1 +++ b/dsc/tests/dsc_resource_test.tests.ps1 @@ -28,7 +28,7 @@ Describe 'Invoke a resource test directly' { } It 'version works' { - $out = dsc resource test -r Test/Version --version 1.1.2 --input '{"version":"1.1.2"}' | ConvertFrom-Json + $out = dsc resource test -r Test/Version --version '=1.1.2' --input '{"version":"1.1.2"}' | ConvertFrom-Json $LASTEXITCODE | Should -Be 0 $out.actualState.version | Should -BeExactly '1.1.2' $out.inDesiredState | Should -Be $true diff --git a/dsc/tests/dsc_tracing.tests.ps1 b/dsc/tests/dsc_tracing.tests.ps1 index 322e281dc..68a04a56c 100644 --- a/dsc/tests/dsc_tracing.tests.ps1 +++ b/dsc/tests/dsc_tracing.tests.ps1 @@ -12,14 +12,14 @@ Describe 'tracing tests' { param($level) $logPath = "$TestDrive/dsc_trace.log" - $null = dsc -l $level resource get -r 'DoesNotExist' 2> $logPath + $null = dsc -l $level resource get -r 'Does.Not/Exist' 2> $logPath $log = Get-Content $logPath -Raw $log | Should -BeLikeExactly "* $($level.ToUpper()) *" } It 'trace level error does not emit other levels' { $logPath = "$TestDrive/dsc_trace.log" - $null = dsc --trace-level error resource list 'DoesNotExist' 2> $logPath + $null = dsc --trace-level error resource list 'Does.Not/Exist' 2> $logPath $log = Get-Content $logPath -Raw $log | Should -Not -BeLikeExactly "* WARNING *" $log | Should -Not -BeLikeExactly "* INFO *" @@ -29,14 +29,14 @@ Describe 'tracing tests' { It 'trace format plaintext does not emit ANSI' { $logPath = "$TestDrive/dsc_trace.log" - $null = dsc --trace-format plaintext resource list 'DoesNotExist' 2> $logPath + $null = dsc --trace-format plaintext resource list 'Does.Not/Exist' 2> $logPath $log = Get-Content $logPath -Raw $log | Should -Not -BeLikeExactly "*``[0m*" } It 'trace format json emits json' { $logPath = "$TestDrive/dsc_trace.log" - $null = dsc --trace-format json resource list 'DoesNotExist' 2> $logPath + $null = dsc --trace-format json resource list 'Does.Not/Exist' 2> $logPath foreach ($line in (Get-Content $logPath)) { $trace = $line | ConvertFrom-Json -Depth 10 $trace.timestamp | Should -Not -BeNullOrEmpty @@ -55,7 +55,7 @@ Describe 'tracing tests' { param($level, $sourceExpected) $logPath = "$TestDrive/dsc_trace.log" - $null = dsc -l $level resource list 'DoesNotExist' 2> $logPath + $null = dsc -l $level resource list 'Does.Not/Exist' 2> $logPath $log = Get-Content $logPath -Raw if ($sourceExpected) { $log | Should -BeLike "*dsc_lib*: *" diff --git a/lib/dsc-lib/locales/en-us.toml b/lib/dsc-lib/locales/en-us.toml index b11bed104..3cf098a20 100644 --- a/lib/dsc-lib/locales/en-us.toml +++ b/lib/dsc-lib/locales/en-us.toml @@ -111,7 +111,6 @@ exeHomeAlreadyInPath = "Exe home is already in path: %{path}" addExeHomeToPath = "Adding exe home to path: %{path}" usingResourcePath = "Using Resource Path: %{path}" discoverResources = "Discovering '%{kind}' using filter: %{filter}" -invalidAdapterFilter = "Could not build Regex filter for adapter name" progressSearching = "Searching for resources" extensionSearching = "Searching for extensions" foundManifest = "Found manifest: %{path}" @@ -120,7 +119,6 @@ adapterFound = "Resource adapter '%{adapter}' version %{version} found" resourceFound = "Resource '%{resource}' version %{version} found" adaptedResourceFound = "Adapted resource '%{resource}' version %{version} found" executableNotFound = "Executable '%{executable}' not found for operation '%{operation}' for resource '%{resource}'" -extensionInvalidVersion = "Extension '%{extension}' version '%{version}' is invalid" invalidResourceManifest = "Invalid manifest for resource '%{resource}': %{err}" invalidExtensionManifest = "Invalid manifest for extension '%{extension}': %{err}" invalidAdaptedResourceManifest = "Invalid manifest for adapted resource '%{resource}': %{err}" From 82200da2e3e7749f5f48a7e101eee20263f7f530 Mon Sep 17 00:00:00 2001 From: Mikey Lombardi Date: Thu, 26 Mar 2026 17:04:06 -0500 Subject: [PATCH 15/15] (GH-538) Fix versioning for WMI adapter Prior to this change, the WMI adapter incorrectly set the version for the adapted resources to an empty string. This wasn't caught by prior implementations because invalid version fields were ignored, rather than raising a validation error. This change updates the adapter to set the version for the resources to `1.0.0`, mirroring the version of the adapter. --- adapters/wmi/wmi.resource.ps1 | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/adapters/wmi/wmi.resource.ps1 b/adapters/wmi/wmi.resource.ps1 index 102db4d34..190f91016 100644 --- a/adapters/wmi/wmi.resource.ps1 +++ b/adapters/wmi/wmi.resource.ps1 @@ -28,8 +28,8 @@ switch ($Operation) { 'List' { $clases = Get-CimClass - foreach ($r in $clases) { - $version_string = "" + foreach ($r in $clases) {` + $version_string = "1.0.0" # WMI resources don't have a version, default to 1.0.0 $author_string = "" $description = ""