[meshkit] Broaden schema validator coverage#942
Conversation
Remove the fixed exported schema-type aliases, expose runtime document type discovery, tighten template-based schemaVersion detection, and add coverage for connection validation plus forward credential support. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: marblom007 <158522975+marblom007@users.noreply.github.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the schema validation capabilities by moving from fixed schema type aliases to a more dynamic discovery mechanism. It introduces runtime enumeration of all available schema document types, ensuring that the validator can adapt to new and existing schema assets from the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the schema validation by removing hardcoded schema type aliases and introducing dynamic discovery of document types. It also adds a DocumentTypes() function to enumerate all registered schema types and enhances the test coverage for connection and credential validation. The changes look solid and improve the flexibility and maintainability of the schema validation logic. I have one suggestion to improve the robustness of a new test case.
| instancePaths := make([]string, 0, len(details.Violations)) | ||
| for _, v := range details.Violations { | ||
| instancePaths = append(instancePaths, v.InstancePath) | ||
| } | ||
| foundMissing := false | ||
| for _, missing := range []string{"/sub_type", "/kind", "/status"} { | ||
| for _, p := range instancePaths { | ||
| if p == missing { | ||
| foundMissing = true | ||
| } | ||
| } | ||
| } | ||
| assert.True(t, foundMissing, | ||
| "expected at least one of /sub_type, /kind, /status in violation paths, got: %v", instancePaths) |
There was a problem hiding this comment.
The check for missing fields is a bit weak as it only ensures at least one missing field is reported. A more robust test would verify that all expected missing fields are reported as violations. Additionally, the implementation can be made more concise and readable by using a map for lookups instead of nested loops.
instancePathSet := make(map[string]struct{})
for _, v := range details.Violations {
instancePathSet[v.InstancePath] = struct{}{}
}
for _, missingField := range []string{"/sub_type", "/kind", "/status"} {
assert.Contains(t, instancePathSet, missingField, "expected violation for missing required field %q", missingField)
}There was a problem hiding this comment.
Pull request overview
This PR broadens Meshery schema validator coverage by removing fixed exported document-type aliases, relying on runtime discovery of schema assets, and adding enumeration + additional test coverage around schema discovery and validation.
Changes:
- Remove exported
Type*schema-type constants in favor of usingDocumentType("..."). - Add
DocumentTypes()APIs (package-level +Validatormethod) to enumerate registered document types. - Tighten template-based
schemaVersionextraction to avoid cross-contamination between sibling schemas, and expand tests for connection + forward-looking credential validation.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| schema/validator.go | Removes exported type aliases and adds DocumentTypes() enumeration (sorted snapshot). |
| schema/validator_test.go | Updates tests to use DocumentType("...") instead of removed constants. |
| schema/registry.go | Narrows template matching for schemaVersion extraction to the schema’s exact template name. |
| schema/registry_test.go | Updates existing tests for dynamic DocumentType usage and adds coverage for DocumentTypes() behavior. |
| schema/doc.go | Documents dynamic discovery and the new document-type enumeration API. |
| schema/constructs_test.go | Adds end-to-end validation tests for connection and synthetic credential schema scenarios. |
| schema/compiler_test.go | Removes an obsolete test for a now-nonexistent helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
schema/doc.go
Outdated
| // fmt.Println(dt) | ||
| // } | ||
| // | ||
| // Callers that need a specific type should construct it directly with | ||
| // [DocumentType], for example: | ||
| // | ||
| // ref := schema.Ref{ | ||
| // SchemaVersion: "relationships.meshery.io/v1alpha3", | ||
| // Type: schema.DocumentType("relationship"), |
There was a problem hiding this comment.
In Go package docs, preformatted code blocks require every line in the block to be tab-indented. Only the first line here is tab-indented; the following lines use spaces, so godoc will render them as normal text rather than a code block. Consider switching these lines to tab indentation to keep the example formatted correctly.
| // fmt.Println(dt) | |
| // } | |
| // | |
| // Callers that need a specific type should construct it directly with | |
| // [DocumentType], for example: | |
| // | |
| // ref := schema.Ref{ | |
| // SchemaVersion: "relationships.meshery.io/v1alpha3", | |
| // Type: schema.DocumentType("relationship"), | |
| // fmt.Println(dt) | |
| // } | |
| // | |
| // Callers that need a specific type should construct it directly with | |
| // [DocumentType], for example: | |
| // | |
| // ref := schema.Ref{ | |
| // SchemaVersion: "relationships.meshery.io/v1alpha3", | |
| // Type: schema.DocumentType("relationship"), |
| // "connection_page.yaml"). We only read templates whose name begins with | ||
| // that exact name so that sibling schemas sharing a templates/ directory | ||
| // (such as connection.yaml and connection_page.yaml) cannot inherit each | ||
| // other's schemaVersion. | ||
| schemaBase := strings.TrimSuffix(path.Base(assetPath), path.Ext(assetPath)) | ||
| templatePrefix := schemaBase + "_template" | ||
|
|
||
| values := map[string]struct{}{} | ||
| for _, entry := range entries { | ||
| if entry.IsDir() || !isDiscoverableSchemaAsset(entry.Name()) { | ||
| continue | ||
| } | ||
|
|
||
| entryBase := strings.TrimSuffix(entry.Name(), path.Ext(entry.Name())) | ||
| if entryBase != templatePrefix { | ||
| continue |
There was a problem hiding this comment.
The comment says templates are read when their name "begins with" the derived prefix, but the implementation requires an exact match on the basename (no extra suffixes). If exact match is intended per the documented convention {schema_base}_template.{ext}, consider updating the wording to avoid implying prefix matching.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Lee Calcote <leecalcote@gmail.com>
Summary
DocumentTypes()enumeration for all schema assets discovered from github.com/meshery/schemasTesting