Skip to content

Commit

Permalink
PRFeedback: Refactor YAML schema handling and improve maintainability
Browse files Browse the repository at this point in the history 
- 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.
  • Loading branch information
@Madhusudhan-MSFT
Madhusudhan-MSFT committed Jan 17, 2025
1 parent 001e610 commit 812f547
Show file tree
Hide file tree
Showing 7 changed files with 99 additions and 137 deletions.
82 changes: 34 additions & 48 deletions src/AppInstallerCommonCore/Manifest/ManifestSchemaValidation.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,6 @@ namespace AppInstaller::Manifest::YamlParser

namespace
{
constexpr std::string_view YamlLanguageServerKey = "yaml-language-server"sv;

struct ManifestSchemaHeader
{
std::string SchemaHeaderString;
std::string ManifestType;
std::string ManifestVersion;
std::string FileName;
size_t Line;
size_t column;
};

enum class YamlScalarType
{
String,
Expand Down Expand Up @@ -111,10 +99,10 @@ namespace AppInstaller::Manifest::YamlParser
return result;
}

std::vector<ValidationError> ParseSchemaHeaderString(const ManifestSchemaHeader& manifestSchemaHeader, const ValidationError::Level& errorLevel, std::string& schemaHeaderUrlString)
std::vector<ValidationError> ParseSchemaHeaderString(const YamlManifestInfo& manifestInfo, const ValidationError::Level& errorLevel, std::string& schemaHeaderUrlString)
{
std::vector<ValidationError> errors;
std::string schemaHeader = manifestSchemaHeader.SchemaHeaderString;
std::string schemaHeader = manifestInfo.DocumentSchemaHeader.SchemaHeader;

// Remove the leading '#' and any leading/trailing whitespaces
if (schemaHeader[0] == '#')
Expand All @@ -124,27 +112,26 @@ namespace AppInstaller::Manifest::YamlParser
}

// Parse the schema header string as YAML string to get the schema header URL

try
{
auto root = YAML::Load(schemaHeader);

if (root.IsNull() || (!root.IsNull() && !root.IsDefined()))
{
errors.emplace_back(ValidationError::MessageContextValueLineLevelWithFile(ManifestError::InvalidSchemaHeader, "", schemaHeader, manifestSchemaHeader.Line, manifestSchemaHeader.column, errorLevel, manifestSchemaHeader.FileName));
errors.emplace_back(ValidationError::MessageContextValueLineLevelWithFile(ManifestError::InvalidSchemaHeader, "", schemaHeader, manifestInfo.DocumentSchemaHeader.Mark.line, manifestInfo.DocumentSchemaHeader.Mark.column, errorLevel, manifestInfo.FileName));
}
else
{
schemaHeaderUrlString = root[YamlLanguageServerKey].as<std::string>();
schemaHeaderUrlString = root[YAML::DocumentSchemaHeader::YamlLanguageServerKey].as<std::string>();
}
}
catch (const YAML::Exception&)
{
errors.emplace_back(ValidationError::MessageContextValueLineLevelWithFile(ManifestError::InvalidSchemaHeader, "", schemaHeader, manifestSchemaHeader.Line, manifestSchemaHeader.column, errorLevel, manifestSchemaHeader.FileName));
errors.emplace_back(ValidationError::MessageContextValueLineLevelWithFile(ManifestError::InvalidSchemaHeader, "", schemaHeader, manifestInfo.DocumentSchemaHeader.Mark.line, manifestInfo.DocumentSchemaHeader.Mark.column, errorLevel, manifestInfo.FileName));
}
catch (const std::exception&)
{
errors.emplace_back(ValidationError::MessageContextValueLineLevelWithFile(ManifestError::InvalidSchemaHeader, "", schemaHeader, manifestSchemaHeader.Line, manifestSchemaHeader.column, errorLevel, manifestSchemaHeader.FileName));
errors.emplace_back(ValidationError::MessageContextValueLineLevelWithFile(ManifestError::InvalidSchemaHeader, "", schemaHeader, manifestInfo.DocumentSchemaHeader.Mark.line, manifestInfo.DocumentSchemaHeader.Mark.column, errorLevel, manifestInfo.FileName));
}

return errors;
Expand All @@ -166,23 +153,29 @@ namespace AppInstaller::Manifest::YamlParser
return false;
}

std::vector<ValidationError> ValidateManifestSchemaHeaderDetails(const ManifestSchemaHeader& schemaHeader, const ManifestTypeEnum& expectedManifestType, const ManifestVer& expectedManifestVersion, ValidationError::Level errorLevel)
std::vector<ValidationError> ValidateSchemaHeaderType(const std::string& headerManifestType, const ManifestTypeEnum& expectedManifestType, const YamlManifestInfo& manifestInfo, ValidationError::Level errorLevel)
{
std::vector<ValidationError> errors;
ManifestTypeEnum actualManifestType = ConvertToManifestTypeEnum(schemaHeader.ManifestType);
ManifestVer actualHeaderVersion(schemaHeader.ManifestVersion);

size_t actualManifestTypeIndex = schemaHeader.SchemaHeaderString.find(schemaHeader.ManifestType) + 1;
size_t actualHeaderVersionIndex = schemaHeader.SchemaHeaderString.find(schemaHeader.ManifestVersion) + 1;
ManifestTypeEnum actualManifestType = ConvertToManifestTypeEnum(headerManifestType);
size_t schemaHeaderTypeIndex = manifestInfo.DocumentSchemaHeader.SchemaHeader.find(headerManifestType) + 1;

if (actualManifestType != expectedManifestType)
{
errors.emplace_back(ValidationError::MessageContextValueLineLevelWithFile(ManifestError::SchemaHeaderManifestTypeMismatch, "", schemaHeader.ManifestType, schemaHeader.Line, actualManifestTypeIndex, errorLevel, schemaHeader.FileName));
errors.emplace_back(ValidationError::MessageContextValueLineLevelWithFile(ManifestError::SchemaHeaderManifestTypeMismatch, "", headerManifestType, manifestInfo.DocumentSchemaHeader.Mark.line, schemaHeaderTypeIndex, errorLevel, manifestInfo.FileName));
}

return errors;
}

std::vector<ValidationError> ValidateSchemaHeaderVersion(const std::string& headerManifestVersion, const ManifestVer& expectedManifestVersion, const YamlManifestInfo& manifestInfo, ValidationError::Level errorLevel)
{
std::vector<ValidationError> errors;
ManifestVer actualHeaderVersion(headerManifestVersion);
size_t schemaHeaderVersionIndex = manifestInfo.DocumentSchemaHeader.SchemaHeader.find(headerManifestVersion) + 1;

if (actualHeaderVersion != expectedManifestVersion)
{
errors.emplace_back(ValidationError::MessageContextValueLineLevelWithFile(ManifestError::SchemaHeaderManifestVersionMismatch, "", schemaHeader.ManifestVersion, schemaHeader.Line, actualHeaderVersionIndex, errorLevel, schemaHeader.FileName));
errors.emplace_back(ValidationError::MessageContextValueLineLevelWithFile(ManifestError::SchemaHeaderManifestVersionMismatch, "", headerManifestVersion, manifestInfo.DocumentSchemaHeader.Mark.line, schemaHeaderVersionIndex, errorLevel, manifestInfo.FileName));
}

return errors;
Expand All @@ -200,8 +193,7 @@ namespace AppInstaller::Manifest::YamlParser
// Prefix schema ID with "schema=" to match the schema header URL pattern and compare it with the schema header URL
schemaId = "$schema=" + schemaId;

if (std::equal(schemaId.begin(), schemaId.end(), schemaHeaderUrlString.begin(), schemaHeaderUrlString.end(),
[](char a, char b) { return tolower(a) == tolower(b); }))
if (Utility::CaseInsensitiveEquals(schemaId, schemaHeaderUrlString))
{
return true;
}
Expand All @@ -210,20 +202,20 @@ namespace AppInstaller::Manifest::YamlParser
return false;
}

ValidationError GetSchemaHeaderUrlPatternMismatchError(const std::string& schemaHeaderUrlString,const ManifestSchemaHeader& manifestSchemaHeader, const ValidationError::Level& errorLevel)
ValidationError GetSchemaHeaderUrlPatternMismatchError(const std::string& schemaHeaderUrlString, const YamlManifestInfo& manifestInfo, const ValidationError::Level& errorLevel)
{
size_t schemaHeaderUrlIndex = manifestSchemaHeader.SchemaHeaderString.find(schemaHeaderUrlString) + 1;
size_t schemaHeaderUrlIndex = manifestInfo.DocumentSchemaHeader.SchemaHeader.find(schemaHeaderUrlString) + 1;

return ValidationError::MessageContextValueLineLevelWithFile(ManifestError::SchemaHeaderUrlPatternMismatch, "", manifestSchemaHeader.SchemaHeaderString, manifestSchemaHeader.Line, schemaHeaderUrlIndex, errorLevel, manifestSchemaHeader.FileName);
return ValidationError::MessageContextValueLineLevelWithFile(ManifestError::SchemaHeaderUrlPatternMismatch, "", manifestInfo.DocumentSchemaHeader.SchemaHeader, manifestInfo.DocumentSchemaHeader.Mark.line, schemaHeaderUrlIndex, errorLevel, manifestInfo.FileName);
}

std::vector<ValidationError> ValidateSchemaHeaderUrl(const YamlManifestInfo& manifestInfo, const ManifestVer& manifestVersion, ManifestSchemaHeader& manifestSchemaHeader, const ValidationError::Level& errorLevel)
std::vector<ValidationError> ValidateSchemaHeaderUrl(const YamlManifestInfo& manifestInfo, const ManifestVer& manifestVersion, const ValidationError::Level& errorLevel)
{
std::vector<ValidationError> errors;

std::string schemaHeaderUrlString;
// Parse the schema header string to get the schema header URL
auto parserErrors = ParseSchemaHeaderString(manifestSchemaHeader, errorLevel, schemaHeaderUrlString);
auto parserErrors = ParseSchemaHeaderString(manifestInfo, errorLevel, schemaHeaderUrlString);
std::move(parserErrors.begin(), parserErrors.end(), std::inserter(errors, errors.end()));

if (!errors.empty())
Expand All @@ -237,21 +229,21 @@ namespace AppInstaller::Manifest::YamlParser
// Parse the schema header URL to get the manifest type and version
if (ParseSchemaHeaderUrl(schemaHeaderUrlString, manifestTypeString, manifestVersionString))
{
manifestSchemaHeader.ManifestType = std::move(manifestTypeString);
manifestSchemaHeader.ManifestVersion = std::move(manifestVersionString);
auto headerManifestTypeErrors = ValidateSchemaHeaderType(manifestTypeString, manifestInfo.ManifestType, manifestInfo, errorLevel);
std::move(headerManifestTypeErrors.begin(), headerManifestTypeErrors.end(), std::inserter(errors, errors.end()));

auto compareErrors = ValidateManifestSchemaHeaderDetails(manifestSchemaHeader, manifestInfo.ManifestType, manifestVersion, errorLevel);
std::move(compareErrors.begin(), compareErrors.end(), std::inserter(errors, errors.end()));
auto headerManifestVersionErrors = ValidateSchemaHeaderVersion(manifestVersionString, manifestVersion, manifestInfo, errorLevel);
std::move(headerManifestVersionErrors.begin(), headerManifestVersionErrors.end(), std::inserter(errors, errors.end()));

// Finally, match the entire schema header URL with the schema ID in the schema file to ensure the URL domain matches the schema definition file.
if (!IsValidSchemaHeaderUrl(schemaHeaderUrlString, manifestInfo, manifestVersion))
{
errors.emplace_back(GetSchemaHeaderUrlPatternMismatchError(schemaHeaderUrlString, manifestSchemaHeader, errorLevel));
errors.emplace_back(GetSchemaHeaderUrlPatternMismatchError(schemaHeaderUrlString, manifestInfo, errorLevel));
}
}
else
{
errors.emplace_back(GetSchemaHeaderUrlPatternMismatchError(schemaHeaderUrlString, manifestSchemaHeader, errorLevel));
errors.emplace_back(GetSchemaHeaderUrlPatternMismatchError(schemaHeaderUrlString, manifestInfo, errorLevel));
}

return errors;
Expand All @@ -262,19 +254,13 @@ namespace AppInstaller::Manifest::YamlParser
std::vector<ValidationError> errors;
std::string schemaHeaderString;

ManifestSchemaHeader schemaHeader{};
schemaHeader.FileName = manifestInfo.FileName;
schemaHeader.SchemaHeaderString = manifestInfo.SchemaHeader.SchemaHeaderString;
schemaHeader.Line = manifestInfo.SchemaHeader.Mark.line;
schemaHeader.column = manifestInfo.SchemaHeader.Mark.column;

if (schemaHeader.SchemaHeaderString.empty())
if (manifestInfo.DocumentSchemaHeader.SchemaHeader.empty())
{
errors.emplace_back(ValidationError::MessageLevelWithFile(ManifestError::SchemaHeaderNotFound, errorLevel, manifestInfo.FileName));
return errors;
}

auto parserErrors = ValidateSchemaHeaderUrl(manifestInfo, manifestVersion, schemaHeader, errorLevel);
auto parserErrors = ValidateSchemaHeaderUrl(manifestInfo, manifestVersion, errorLevel);
std::move(parserErrors.begin(), parserErrors.end(), std::inserter(errors, errors.end()));

return errors;
Expand Down
12 changes: 6 additions & 6 deletions src/AppInstallerCommonCore/Manifest/YamlParser.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -527,19 +527,19 @@ namespace AppInstaller::Manifest::YamlParser
THROW_HR_IF_MSG(HRESULT_FROM_WIN32(ERROR_DIRECTORY_NOT_SUPPORTED), std::filesystem::is_directory(file.path()), "Subdirectory not supported in manifest path");

YamlManifestInfo manifestInfo;
YAML::DocumentRootWithSchema doc = YAML::LoadDocument(file.path());
YAML::Document doc = YAML::LoadDocument(file.path());
manifestInfo.Root = doc.GetRoot();
manifestInfo.SchemaHeader = doc.GetSchemaHeader();
manifestInfo.DocumentSchemaHeader = doc.GetSchemaHeader();
manifestInfo.FileName = file.path().filename().u8string();
docList.emplace_back(std::move(manifestInfo));
}
}
else
{
YamlManifestInfo manifestInfo;
YAML::DocumentRootWithSchema doc = YAML::LoadDocument(inputPath, manifestInfo.StreamSha256);
YAML::Document doc = YAML::LoadDocument(inputPath, manifestInfo.StreamSha256);
manifestInfo.Root = doc.GetRoot();
manifestInfo.SchemaHeader = doc.GetSchemaHeader();
manifestInfo.DocumentSchemaHeader = doc.GetSchemaHeader();
manifestInfo.FileName = inputPath.filename().u8string();
docList.emplace_back(std::move(manifestInfo));
}
Expand All @@ -562,9 +562,9 @@ namespace AppInstaller::Manifest::YamlParser
try
{
YamlManifestInfo manifestInfo;
YAML::DocumentRootWithSchema doc = YAML::LoadDocument(input);
YAML::Document doc = YAML::LoadDocument(input);
manifestInfo.Root = doc.GetRoot();
manifestInfo.SchemaHeader = doc.GetSchemaHeader();
manifestInfo.DocumentSchemaHeader = doc.GetSchemaHeader();
docList.emplace_back(std::move(manifestInfo));
}
catch (const std::exception& e)
Expand Down
2 changes: 1 addition & 1 deletion src/AppInstallerCommonCore/Public/winget/ManifestYamlParser.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ namespace AppInstaller::Manifest::YamlParser
std::string FileName;

// Schema header string found in the manifest file
YAML::DocumentSchemaHeader SchemaHeader;
YAML::DocumentSchemaHeader DocumentSchemaHeader;

// The SHA256 hash of the stream
Utility::SHA256::HashBuffer StreamSha256;
Expand Down
22 changes: 12 additions & 10 deletions src/AppInstallerSharedLib/Public/winget/Yaml.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -241,19 +241,21 @@ namespace AppInstaller::YAML
struct DocumentSchemaHeader
{
DocumentSchemaHeader() = default;
DocumentSchemaHeader(std::string schemaHeaderString, const Mark& mark) : SchemaHeaderString(std::move(schemaHeaderString)), Mark(mark) {}
DocumentSchemaHeader(std::string schemaHeaderString, const Mark& mark) : SchemaHeader(std::move(schemaHeaderString)), Mark(mark) {}

std::string SchemaHeaderString;
std::string SchemaHeader;
Mark Mark;
static constexpr std::string_view YamlLanguageServerKey = "yaml-language-server";
};

struct DocumentRootWithSchema
struct Document
{
DocumentRootWithSchema() = default;
DocumentRootWithSchema(Node root, DocumentSchemaHeader schemaHeader) : m_root(std::move(root)), m_schemaHeader(std::move(schemaHeader)) {}
Document() = default;
Document(Node root, DocumentSchemaHeader schemaHeader) : m_root(std::move(root)), m_schemaHeader(std::move(schemaHeader)) {}

// Return r-values for move semantics
Node GetRoot() const { return m_root; }
const DocumentSchemaHeader& GetSchemaHeader() const { return m_schemaHeader; }
DocumentSchemaHeader GetSchemaHeader() const { return m_schemaHeader; }

private:
Node m_root;
Expand All @@ -267,10 +269,10 @@ namespace AppInstaller::YAML
}

// Loads from the input; returns the root node of the first document.
DocumentRootWithSchema LoadDocument(std::string_view input);
DocumentRootWithSchema LoadDocument(const std::string& input);
DocumentRootWithSchema LoadDocument(const std::filesystem::path& input);
DocumentRootWithSchema LoadDocument(const std::filesystem::path& input, Utility::SHA256::HashBuffer& hashOut);
Document LoadDocument(std::string_view input);
Document LoadDocument(const std::string& input);
Document LoadDocument(const std::filesystem::path& input);
Document LoadDocument(const std::filesystem::path& input, Utility::SHA256::HashBuffer& hashOut);

// A YAML emitter.
struct Emitter
Expand Down
Loading

0 comments on commit 812f547

Please sign in to comment.