Skip to content
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

Yaml manifest schemaheader validation for V.1.10.0 and above #5126

Conversation

Madhusudhan-MSFT
Copy link
Contributor

@Madhusudhan-MSFT Madhusudhan-MSFT commented Jan 12, 2025

Enable schema header validation for YAML manifests from version 1.10.0 onwards.

YAML Manifest Validation Changes:

  1. Introduce functions to search, parse, and validate schema headers in YAML manifests.
  2. Add/Update structs and enums to support schema header validation.
  3. Add new error messages and validation options, including treating schema header validation errors as warnings.

Schema Header Validation Includes:

  1. Validate presence of Manifest Schema Header.
  2. Ensure Schema Header format correctness.
  3. Verify Schema Header URL Pattern accuracy.
  4. Confirm Schema Header Manifest Type matches ManifestType Property.
  5. Check Schema Header Manifest Version matches ManifestVersion Property.

WinGet CLI Validation Command Changes:

  • Treat schema header validation errors as warnings for 'winget validate --manifest <>' command
    • Introduced a new option SchemaHeaderValidationAsWarning to the validateOption object and set it to true.
    • This change treats schema header validation issues as warnings instead of errors, making the validation process more lenient.

[NOTE:]

  1. SchemaHeaderValidation errors should be treated as warnings for the winget CLI validate command.
  2. SchemaHeaderValidation errors should be treated as errors for wingetsvc community manifests.

[Test Coverage:]

  • Added new test data files to validate various schema header errors in YAML manifests, including
    • invalid headers,
    • type mismatches,
    • version mismatches,
    • missing headers, and
    • URL pattern mismatches.
  • Added test coverage for WinGet Utils Interop binary and WinGet CLI Validated command

[How Validated:]

  • Compiled the AppInstaller end-to-end solution incorporating the changes.
  • Performed the YAML manifest validation tests, WinGet Utils tests, and WinGet CLI Validate command tests, ensuring all tests passed.
  • Locally validated the winget CLI validation command with various schema header errors, ensuring they are treated as warnings and that no warnings are reported for a valid manifest.

[Test Results:]

YAML Manifest Validation Test Results:
image

WinGet Utils Interop Test Results:
image

Validate Command Test Results:
image


Microsoft Reviewers: Open in CodeFlow

1. Introduce functions to search, parse, and validate schema headers in YAML manifests.
2. Add/Update structs and enums to support schema header validation.
3. Add new error messages and validation options, including treating schema header validation errors as warnings.

Schema Header Validation Includes:
1. Validate presence of Manifest Schema Header.
2. Ensure Schema Header format correctness.
3. Verify Schema Header URL Pattern accuracy.
4. Confirm Schema Header Manifest Type matches ManifestType Property.
5. Check Schema Header Manifest Version matches ManifestVersion Property.
…e --manifest <<path>>' command

1. Introduced a new option `SchemaHeaderValidationAsWarning` to the `validateOption` object and set it to `true`.
2. This change treats schema header validation issues as warnings instead of errors, making the validation process more lenient.

[NOTE:]
1. SchemaHeaderValidation errors should be treated as warnings for the winget CLI validate command.
2. SchemaHeaderValidation errors should be treated as errors for wingetsvc community manifests.
1. Added new test data files to validate various schema header errors in YAML manifests, including
  a. invalid headers,
  b. type mismatches,
  c. version mismatches,
  d. missing headers, and
  e. URL pattern mismatches.
2. Updated project files to include these test data files.
3. Added a new test case in `YamlManifest.cpp` to validate the schema headers of the new test data files and existing v.1.10 schema test files.
…ommand and WinGetUtils Interop binary

1. Added new test manifests with schema headers to validate various scenarios.
2. Updated `ValidateCommand.cs` to include new test cases for extended characters and schema headers.
3. Enhanced `WinGetUtilManifest.cs` with new test cases for schema headers and added a helper method for validation. Also, made minor formatting corrections.
The error message in the test case `TEST_CASE("ManifestV1_10_SchemaHeaderValidations", "[ManifestValidation]")` has been updated to provide a clearer and more specific description. The new message clarifies the mismatch issue between the manifest version in the schema header and the `ManifestVersion` property value in the manifest.

This comment has been minimized.

This comment has been minimized.

The changes correct a typo in the file path for a test manifest file in two different test files. Specifically, the file name `TestWarningManifestV1_10-SchemaHeadeVersionMismatch.yaml` is corrected to `TestWarningManifestV1_10-SchemaHeaderVersionMismatch.yaml` in both `ValidateCommand.cs` and `WinGetUtilManifest.cs`.
- Updated regex pattern for schema URL matching,
- Renamed and revised validation functions, and introduced new helper functions for improved error handling and validation logic.
- These changes ensure accurate validation of schema header URLs against schema definition files.
Updated the URL in the comment for the YAML language server schema to correct the casing of "defaultLocale" to "defaultlocale". This ensures the schema URL is correctly referenced.
…0GoodManifestAndVerifyContents test failure

Corrected the URL in the comment for the YAML language server schema by changing "defaultlocale" to "defaultLocale" to ensure proper casing.
@Madhusudhan-MSFT Madhusudhan-MSFT marked this pull request as ready for review January 12, 2025 19:31
@Madhusudhan-MSFT Madhusudhan-MSFT requested a review from a team as a code owner January 12, 2025 19:31
@Madhusudhan-MSFT Madhusudhan-MSFT marked this pull request as draft January 12, 2025 19:31
@Madhusudhan-MSFT Madhusudhan-MSFT marked this pull request as ready for review January 13, 2025 01:20
…r messages

1. Updated error messages in multiple files to simplify instructions for verifying schema headers.
2.  Removed detailed YAML node verification instructions.
3. Modified function signatures in ManifestSchemaValidation.cpp to remove const qualifiers.
- Simplified schema header handling by directly associating schema headers with YAML documents.
- Removed unnecessary input streams and search functions, resulting in cleaner and more maintainable code.
- Updated relevant structures and methods across multiple files to support the new approach.

This comment has been minimized.

Madhusudhan-MSFT and others added 4 commits January 15, 2025 10:47
- Renamed the parameter `schemHeader` to `schemaHeader` in the
`AddSchemaHeader` method of the `Document` class in `YamlWrapper.cpp`.
… test

Replaced the extended character 'ë' in the file name `TëstExeInstaller.yaml` with a placeholder character '�', resulting in the new file name `T�stExeInstaller.yaml`. This change addresses potential encoding issues or compatibility concerns with systems that may not support the original extended character.
…from master branch to address the encoding issue introduced by editor
@Madhusudhan-MSFT
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

src/AppInstallerSharedLib/YamlWrapper.h Outdated Show resolved Hide resolved
src/AppInstallerSharedLib/Public/winget/Yaml.h Outdated Show resolved Hide resolved
src/AppInstallerSharedLib/Public/winget/Yaml.h Outdated Show resolved Hide resolved
src/AppInstallerSharedLib/Public/winget/Yaml.h Outdated Show resolved Hide resolved
src/AppInstallerSharedLib/YamlWrapper.cpp Outdated Show resolved Hide resolved
src/AppInstallerSharedLib/YamlWrapper.cpp Outdated Show resolved Hide resolved
src/AppInstallerCommonCore/Manifest/YamlParser.cpp Outdated Show resolved Hide resolved
- Refactored and renamed components within `AppInstaller::Manifest::YamlParser`
and `AppInstaller::YAML` namespaces for better clarity.
- Removed `ManifestSchemaHeader` struct, transferring its responsibilities to
`YamlManifestInfo` with a new `DocumentSchemaHeader` member.
- Updated `DocumentSchemaHeader` to rename `SchemaHeaderString` to `SchemaHeader` and added a static constant `YamlLanguageServerKey`.

- Revised `ParseSchemaHeaderString` to use `YamlManifestInfo`.
- Split `ValidateManifestSchemaHeaderDetails` into `ValidateSchemaHeaderType`
and `ValidateSchemaHeaderVersion`.
- Updated `LoadDocument` functions to return `Document` instead of `DocumentRootWithSchema`.

- Simplified `Document` struct by removing schema header methods and
including `DocumentSchemaHeader` directly.
- Added `ExtractSchemaHeaderFromYaml` function.
- Updated `YamlParser.cpp` and `YamlWrapper.cpp` to reflect these changes.
- Removed schema header methods from `YamlWrapper.h`.

- Utilized `Utility::CaseInsensitiveEquals` for case-insensitive comparison of schema header URLs.
@Madhusudhan-MSFT
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

- Improved the efficiency of the `YamlParser` by utilizing move semantics for the `Document` class in `Yaml.h`.
- Updated `YamlParser.cpp` to call `GetRoot` with `std::move(doc).GetRoot()` to avoid unnecessary copying.
- Modified `GetRoot` in `Yaml.h` to return an r-value reference (`Node&&`).
- Also, updated `GetSchemaHeader` to return a constant reference (`const DocumentSchemaHeader&`) to reduce copying.
@Madhusudhan-MSFT
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Madhusudhan-MSFT Madhusudhan-MSFT merged commit b1ac622 into microsoft:master Jan 17, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants