-
Notifications
You must be signed in to change notification settings - Fork 55
(GH-538) Fully qualified type name #1344
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(GH-538) Fully qualified type name #1344
Conversation
Prior to this change, the `type` field for resource manifests was just
a `String` type, without any inherent validation. When schemars
generates the schema for this field for any struct that has a fully
qualified type name, the generated schema just ensures that the type is
`string`. While we could add the validation pattern to each of these
fields with the `schemars` derive attribute, we would need to add it to
every field. We also can't extract the `definitions/resourceType` schema
from the code because it isn't separaely maintained from the structs
where it is referenced.
This change:
1. Follows the Rust ["parse, don't validate"][01] mantra by using the
newtype pattern to create a `FullyQualifiedTypeName` struct that
wraps a `String` value.
This change implements several traits for the newtype to simplify
using it as much as possible, including referencing and
dereferencing as a string.
1. Ensures that the newtype defines the canonical JSON schema for a
fully qualified type name. This schema can be extracted.
1. Adds the first extended documentation strings for the JSON schemas
in a separate file from the `en-us.toml`, to enable writing many
multiline strings needed by schemas without cluttering the
translations for typical library messages.
Prior to this change, the resource manifest struct defined the `resource_type` field as a `String`. This change updates the type to use the new `FullyQualifiedTypeName` struct instead so that the generated schema will be canonical and correctly validated. This change updates the code where needed to account for this change. Generally this entails calling `to_string()` instead of `clone()` to access the string value or `parse().unwrap()` instead of `to_string()` when constructing the value from a string literal.
This change updates both the `DscExtension` and `ExtensionManifest` types to use `FullyQualifiedTypeName` instead of `String` for the type name field. This change also updates all code as needed to address this change.
This change updates the `DscResource` struct to define the `type_name` field as `FullyQualifiedTypeName` instead of `String`. It also updates the rest of the code as required.
This change updates the following structs to use `FullyQualifiedTypeName` instead of `String` for the relevant field: - `configure::config_doc::Resource` - `configure::config_result::ResourceMessage` - `configure::config_result::ResourceGetResult` - `configure::config_result::ResourceSetResult` - `configure::config_result::ResourceTestResult` It also updates references to those fields as needed.
This change implements `PartialEq` for case-insensitively comparing FQTNs directly and with string types. This obviates the need to manually lowercase both values before comparison elsewhere and simplifies how you compare an FQTN to a string value. This change also adds some maintainer comments to the code to explain the purpose behind each of the hand-implemented traits.
This change updates the `Progress` struct to define the `resource_type` field as `FullyQualifiedTypeName` for consistency and to ensure that the progress data can be canonically schematized.
This change updates the `dscresources::dscresource::DscResource` struct to define the `require_adapter` field to use `FullyQualifiedTypeName` instead of `String` for consistency and correctness. This change also updates the `*adapter` methods for the type to use FQTN for the `adapter` parameters.
This change updates the `target_resource` field of `DscResource` to use `FullyQualifiedTypeName` instead of `String`. It also updates related functions to expect that type instead of `&str`.
This change updates the types and functions in the MCP module to expect and accept the `FullyQualifiedTypeName` type instead of strings.
eed95b9 to
3ac4a8c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request introduces a FullyQualifiedTypeName newtype to enforce type-level validation for DSC resource and extension type names. The change follows Rust's "parse, don't validate" principle by ensuring that any FullyQualifiedTypeName value is guaranteed to be valid by construction.
Key changes:
- Implements
FullyQualifiedTypeNamestruct with comprehensive trait support (Display, FromStr, TryFrom, Deref, AsRef, etc.) - Adds validation pattern and JSON schema for fully qualified type names
- Updates all type name fields from
StringtoFullyQualifiedTypeNamethroughout the codebase - Adds extensive test coverage for the new type
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/dsc-lib/src/types/fully_qualified_type_name.rs | Core implementation of the newtype with validation, trait implementations, and schema support |
| lib/dsc-lib/tests/integration/types/fully_qualified_type_name.rs | Comprehensive test coverage for schema validation, serialization, and trait implementations |
| lib/dsc-lib/src/dscresources/dscresource.rs | Updates DscResource to use FullyQualifiedTypeName for type_name, require_adapter, and target_resource fields |
| lib/dsc-lib/src/dscresources/resource_manifest.rs | Updates ResourceManifest to use FullyQualifiedTypeName for resource_type field |
| lib/dsc-lib/src/extensions/extension_manifest.rs | Updates ExtensionManifest to use FullyQualifiedTypeName for type field |
| lib/dsc-lib/src/dscresources/command_resource.rs | Updates function signatures to accept Option instead of Option<&str> |
| lib/dsc-lib/src/configure/config_doc.rs | Updates Resource struct to use FullyQualifiedTypeName for resource_type field |
| lib/dsc-lib/src/configure/config_result.rs | Updates result structs to use FullyQualifiedTypeName for resource_type fields |
| lib/dsc-lib/src/progress.rs | Updates Progress struct to use FullyQualifiedTypeName for resource_type field |
| lib/dsc-lib/locales/schemas.definitions.yaml | Adds comprehensive schema documentation for fully qualified type names |
| lib/dsc-lib/locales/en-us.toml | Adds error message translations for invalid type names |
| tools/test_group_resource/src/main.rs | Updates test resources to parse type names with .parse().unwrap() |
| dsc/src/subcommand.rs | Updates list operations to call .to_string() on FullyQualifiedTypeName fields |
| dsc/src/mcp/*.rs | Updates MCP-related structs to use FullyQualifiedTypeName |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/dsc-lib/tests/integration/types/fully_qualified_type_name.rs
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
PR Summary
This change:
Follows the Rust "parse, don't validate" mantra by using the newtype pattern to create a
FullyQualifiedTypeNamestruct that wraps aStringvalue.This change implements several traits for the newtype to simplify using it as much as possible, including referencing and dereferencing as a string.
Ensures that the newtype defines the canonical JSON schema for a fully qualified type name. This schema can be extracted.
Adds the first extended documentation strings for the JSON schemas in a separate file from the
en-us.toml, to enable writing many multiline strings needed by schemas without cluttering the translations for typical library messages.Updates the types, methods, and functions to use
FullyQualifiedTypeNameinstead ofString.This change updates smaller sections of the code commit-by-commit to simplify reviewing. This change necessarily modifies many files, types, and functions, so it is easier to review by commit instead of the entire changeset.
PR Context
Prior to this change, the
typefield for resource manifests was just aStringtype, without any inherent validation. When schemars generates the schema for this field for any struct that has a fully qualified type name, the generated schema just ensures that the type isstring. While we could add the validation pattern to each of these fields with theschemarsderive attribute, we would need to add it to every field. We also can't extract thedefinitions/resourceTypeschema from the code because it isn't separaely maintained from the structs where it is referenced.