(GH-538) Enhance schema export functionality and add error handling for duplicate paths - #1672
Conversation
…handling for duplicate paths
There was a problem hiding this comment.
Pull request overview
This PR improves the reliability and completeness of the Rust-based schema export pipeline (cargo xtask schema export) so schemas can be generated from source and published in versioned folders without silent collisions.
Changes:
- Add duplicate-path detection during schema export to prevent silent overwrites when two types resolve to the same output path.
- Extend
xtask schema exportto support exporting explicit schema version folders and expanding a--release X.Y.Zinto patch/minor/major version folders. - Improve schema-version parsing ergonomics and align several schema IDs/paths so exported locations match the intended repository layout.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| xtask/src/schemas/export.rs | Adds duplicate export-path detection and expands the exported type list. |
| xtask/src/main.rs | Adds --schema-version / --release resolution logic and tests for version resolution. |
| xtask/src/args.rs | Extends CLI args for schema export to accept repeated schema versions or a release expansion. |
| xtask/locales/en-us.toml | Adds localized help/error strings for the new schema export options and errors. |
| lib/dsc-lib/src/dscresources/invoke_result.rs | Adjusts schema export base name to avoid output collisions. |
| lib/dsc-lib/src/dscresources/adapted_resource_manifest.rs | Fixes schema export folder path and broadens $schema validation to accept relevant URIs. |
| lib/dsc-lib-jsonschema/src/dsc_repo/recognized_schema_version.rs | Implements FromStr + error type for schema version folder parsing, with tests. |
| lib/dsc-lib-jsonschema/src/dsc_repo/mod.rs | Re-exports UnrecognizedSchemaVersion for downstream use. |
| lib/dsc-lib-jsonschema/locales/en-us.toml | Adds i18n strings for schema-version parsing errors. |
| dsc/src/util.rs | Switches CLI/server schema generation to use DscRepoSchema generation (bundled vs canonical based on type). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// The version of the resource manifest schema. | ||
| #[serde(rename = "$schema")] | ||
| #[schemars(schema_with = "ResourceManifest::recognized_schema_uris_subschema")] | ||
| #[schemars(schema_with = "AdaptedDscResourceManifest::recognized_schema_uris_union_subschema")] |
There was a problem hiding this comment.
What is the reason for using this special associated function instead of recognized_schema_uris_subschema() on this type (I see that it was previously using the same subschema as the ResourceManifest, which was incorrect)?
There was a problem hiding this comment.
Mikey Lombardi (He/Him) (@michaeltlombardi) - I looked a bit back in the history, and this function exists to widen the set of $schema URIs the adapted-manifest schema accepts (as far as my understanding goes). It takes the type's own subschema and replaces its enum with the union of the adapted manifest's URI and the ResourceManifest URIs. Using the plain earlier one would restrict the enum to only the new resource/adapted/manifest.json URIs. And I think that would have invalidated every adapted resource manifest that already exists, because until this branch those documents could only declare a resource manifest schema URI.
There was a problem hiding this comment.
Thinking about this for a bit, I'm torn - the prior versions were incorrectly defined and when you defined $schema in a given document the validator uses that URI to retrieve and validate the data.
I don't think we should merge the lists, not least because this schema isn't valid for many of the early resource schema definitions.
There was a problem hiding this comment.
Can't disagree with that. Want me to replace the union with Self::recognized_schema_uris_subschema() and delete recognized_schema_uris_union_subschema? And then treat the old URI in existing files as a bug to migrate (perhaps worth a warn! at load time during a transition period if you want to help people update)?
There was a problem hiding this comment.
A few things:
-
I don't see that we actually exported/published the adapted manifest schema into any existing schema folders. We'll need to get the schemas exported, of course, so people can retrieve them without invoking DSC, but we could correctly export them with the bug fixed.
-
In the current implementation (and previous), we're not actually using a schema registry for lookups/validation. It grabs the schema from the type definition in that release and uses that.
-
So this would only apply to adapted resource manifests that are published with the incorrect schema URI and used in future versions of DSC.
-
I'm open to the idea of a JSON schema for the property like:
$schema: oneOf: - <AdaptedDscResourceManifest::recognized_schema_uris_subschema> - <ResourceManifest::recognized_schema_uris_subschema> deprecated: true deprecationMessage: <explanation>
But there's no good way to raise an info/warning message during schema validation. We would have to special-case this, I think, which is unfortunate.
There was a problem hiding this comment.
Thanks Mikey, I applied the changes. However, while wiring it up for the first point you mentioned, I found that there's actually a third URI family in play.
.../v3/bundled/resource/manifest.json— what the buggy-generated schema declared as valid (the deprecated branch we discussed).../v3/bundled/resource/adapted/manifest.json— the new canonical family from your folder_path suggestion.../v3/bundled/adaptedresource/manifest.json— the URI from the YAML-authored layout (schemas/src/adaptedresource/), which was never published
The catch was with the last one, and these are the ones shipped currently. It was declared, for example, in windows_personalization.dsc.adaptedResource.yaml (also dsctest fixtures, and the PowerShell adapter). Now, we can argue if people have copied this, but yeah, most people copy from examples that are shipped. So, should the deprecated oneOf branch cover family 3 as well?
There was a problem hiding this comment.
I think it should cover that third branch but only the specific URI we shipped those manifests using.
There was a problem hiding this comment.
Done!
…handling for duplicate paths
Co-authored-by: Mikey Lombardi (He/Him) <michael.t.lombardi@gmail.com>
352828d to
a43c5c8
Compare
…ithub.com/Gijsreyn/operation-methods into PowerShellgh-538/main/schema-export-fixes
PR Summary
The primary purpose of this change is to make cargo xtask schema export reliable and complete enough to publish versioned DSC schemas from source code. It fixes two silent output collisions, exports every type that derives DscRepoSchema, and adds version targeting so the exporter can produce any recognized schema version folder instead of only vNext.
As a maintainer or contributor, you can now invoke:
cargo xtask schema export— exports schemas/vNext as before.cargo xtask schema export --schema-version v3.2 --schema-version v3.2.3— exports specific version folders.cargo xtask schema export --release 3.3.0— exports the full set a release must publish: schemas/v3.3.0, schemas/v3.3, and schemas/v3.PR Context
As part of #538 and following up on #1406, the exporter needs to produce correct, complete, versioned schemas from source before it can replace the hand-maintained YAML pipeline under schemas/src.