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

Add support for subschemas #99

Open
wants to merge 34 commits into
base: main
Choose a base branch
from
Open

Conversation

omissis
Copy link
Owner

@omissis omissis commented Jul 21, 2023

Open Source Saturday

This PR introduces support for anyOf and allOf subschemas.

@omissis omissis added the enhancement New feature or request label Jul 21, 2023
@omissis omissis self-assigned this Jul 21, 2023
@omissis omissis force-pushed the feature/implement-subschemas branch 2 times, most recently from 1c44284 to 4684bc6 Compare October 7, 2023 13:56
@codecov
Copy link

codecov bot commented Oct 7, 2023

Codecov Report

Attention: Patch coverage is 70.50147% with 100 lines in your changes are missing coverage. Please review.

Project coverage is 75.25%. Comparing base (d963216) to head (7bc36c4).
Report is 29 commits behind head on main.

❗ Current head 7bc36c4 differs from pull request most recent head 66cf143. Consider uploading reports for the commit 66cf143 to get more accurate results

Files Patch % Lines
pkg/generator/schema_generator.go 71.11% 17 Missing and 9 partials ⚠️
pkg/schemas/model.go 70.37% 12 Missing and 4 partials ⚠️
tests/data/validation/maxLength/maxLength.go 37.50% 13 Missing and 2 partials ⚠️
tests/data/validation/minLength/minLength.go 37.50% 13 Missing and 2 partials ⚠️
pkg/generator/output.go 54.16% 9 Missing and 2 partials ⚠️
internal/x/text/cases.go 0.00% 2 Missing and 1 partial ⚠️
pkg/generator/generate.go 62.50% 2 Missing and 1 partial ⚠️
pkg/generator/json_formatter.go 81.25% 2 Missing and 1 partial ⚠️
pkg/generator/yaml_formatter.go 76.92% 2 Missing and 1 partial ⚠️
tests/data/extraImports/gopkgYAMLv3/gopkgYAMLv3.go 0.00% 3 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #99      +/-   ##
==========================================
- Coverage   76.58%   75.25%   -1.33%     
==========================================
  Files          24       25       +1     
  Lines        1892     2138     +246     
==========================================
+ Hits         1449     1609     +160     
- Misses        354      425      +71     
- Partials       89      104      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@omissis omissis force-pushed the feature/implement-subschemas branch from 4684bc6 to 43dd923 Compare November 8, 2023 00:57
@omissis omissis marked this pull request as ready for review November 8, 2023 01:04
@omissis omissis added this to the v0.16.0 milestone Jan 17, 2024
@omissis omissis force-pushed the feature/implement-subschemas branch from 046ba39 to 7bc36c4 Compare January 18, 2024 23:29
@omissis omissis modified the milestones: v0.16.0, v0.17.0 Apr 20, 2024
@omissis omissis force-pushed the feature/implement-subschemas branch 2 times, most recently from 18e8e38 to 827b886 Compare April 20, 2024 23:41
omissis added 9 commits May 16, 2024 08:25
- fix: add guard against loading non-json files when reading references in schemas
- feat: introduce basic allOf support
- fix: handle struct slices prop merging for allOf types
- feat: introduce basic anyOf support, fix default value issue.
- fix: make anyOf properties of primitive types dump interface instead of first type
- fix: reduce duplicate types
- fix: furhter reduce duplicate types
- chore: refactor cmp Opts utility
- fix: add graceful handling of some edge cases in code generation
    - empty enum type name
    - missing unmarshal methods for map types
    - "Plain" type naming collisions
    - schema.Type new properties potential (de)serialization
- chore: rename test files after rebase
- chore: cleanup go deps
- chore: fix go linting issues
- chore: integrate new formatter changes after rebase
- fix: set consistent var names in unmarshallers, introduce support for both yaml and json in validators
- chore: remove dead code
- fix: use '>' instead of '>=' to perform max length validation.
- chore: remove 'two' constant
- chore: disable gomnd linter
@omissis omissis force-pushed the feature/implement-subschemas branch from cb3f1ad to 66cf143 Compare May 16, 2024 06:28
@@ -373,7 +415,7 @@ func (g *schemaGenerator) generateType(
return codegen.EmptyInterfaceType{}, nil

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for maintaining this great tool!
I have been trying to use it for a schema with anyOf and I have a comment on this PR.

When using anyOf, the type may not be set, and only be defined in the items which are part of the anyOf itself - see for instance https://json-schema.org/understanding-json-schema/reference/combining#anyOf.

Specifically, I tried running go-jsonschema from the lastest release as well as from this PR for this schema, and in both cases, I hit the EmptyInterfaceType.

type EmbeddedlinksarrayJson []interface{}

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! will look into it :)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@afrittoli so without a hinted type the library would have to use some heuristics to determine the type where possible. If that information is not available, or if the types are not compatible, the empty interface is all it can use to satisfy the type constraints. I could add that to make things a bit better, but it will only cover a handful of cases.

@joepjoosten
Copy link

It breaks with this testcase:

{
    "$schema": "http://json-schema.org/draft-07/schema#",
    "$defs": {
        "foo": {
            "type": "object",
            "properties": {
                "foo": {
                    "type": "string"
                }
            },
            "required": [
                "foo"
            ]
        }
    },
    "id": "https://example.com/objectAllOf",
    "type": "object",
    "properties": {
        "flags": {
            "anyOf": [
                {
                    "type": "string"
                },
                {
                    "type": "boolean"
                }
            ]
        },
        "configurations": {
            "type": "array",
            "items": {
                "anyOf": [
                    {
                        "type": "object",
                        "properties": {
                            "bar": {
                                "type": "number"
                            }
                        },
                        "required": [
                            "bar"
                        ]
                    },
                    {
                        "type": "object",
                        "properties": {
                            "baz": {
                                "type": "boolean"
                            }
                        }
                    },
                    {
                        "$ref": "#/$defs/foo"
                    }
                ]
            }
        }
    }
}

@omissis
Copy link
Owner Author

omissis commented May 24, 2024

@joepjoosten thanks for providing it! I will look at it as soon as I can.

@l0nedigit
Copy link

Any updates on the status of this PR? The fixes in this branch would be helpful to a project I am working on as well. Thanks

@omissis omissis removed this from the v0.17.0 milestone Sep 18, 2024
@omissis
Copy link
Owner Author

omissis commented Nov 17, 2024

A quick update for all those who are waiting: I have just finished bringing this one up to date with all the things I have merged on main, and fixed all the broken tests code that I could find. This puts me in a position where I can look at all the feedback above and try to fix the issues. This is now the top priority for this repo to merge, and save for very small and trivial fixes and improvements, I will not merge anything else until this is done. Thank you for your patience.

@ptodev
Copy link

ptodev commented Nov 20, 2024

Thank you, @omissis! If you are looking for more inspiration for testing, it might be worth looking into what other json schema tools use in their repos:

If I find time in the coming weeks, I could also take a look to see if there are any interesting test cases we could add.

@omissis
Copy link
Owner Author

omissis commented Dec 7, 2024

update: I have added support for the cases provided by @joepjoosten and @iiacoban42, now I am looking into the case @afrittoli highlighted. Once that is done, I still have a (private) set of schemas I want to test the library onto, and then if no further objections are raised, I will merge this PR and release a new version. Hopefully it'll see the light before end of the year :)

@strowk
Copy link

strowk commented Dec 22, 2024

Hey, I was using this from a branch and I have noticed the issue that wasn't present in published version - it generates duplicates now that were not there before.

I am running it like this:

go-jsonschema -p mcp https://raw.githubusercontent.com/modelcontextprotocol/specification/refs/heads/main/schema/schema.json -o ./schema.go

Go vet tells me "schema.go:2634:6: ImageContent redeclared in this block".
I can see the tool generating something like this:

type BlobResourceContents = BlobResourceContents

This creates an alias type, that has same name as original type, which creates conflict as golang does not like that.

I saw the connected issue about duplicate types, but it does not quite seem related, as I am not getting any numbers in those names, they are just full duplicates.

I can see four aliases generated for ImageContent in my schema.go file and I can see four different types in original schema that mention that type in their anyOf, but it does not seem to matter whether there is one or several. Any reference would result in that.

Here's quick example that is reproducing if run it from the root of go-jsonschema on this branch (this one for bash) :

cat << 'EOF' > schema.json
{
    "$schema": "http://json-schema.org/draft-07/schema#",
    "definitions": {
        "TextContent": {
            "description": "Text provided to or from an LLM.",
            "properties": {
                "text": {
                    "description": "The text content of the message.",
                    "type": "string"
                }
            },
            "required": [
                "text"
            ],
            "type": "object"
        },
        "CallToolResult": {
            "description": "The server's response to a tool call",
            "properties": {
                "content": {
                    "items": {
                        "anyOf": [
                            {
                                "$ref": "#/definitions/TextContent"
                            }
                        ]
                    },
                    "type": "array"
                }
            },
            "type": "object"
        }
    }
}

EOF

go run main.go -p mypackage schema.json -o schema.go
go vet ./schema.go

Gives me

vet.exe: .\schema.go:70:6: TextContent redeclared in this block

Essentially it seems that the tool does not deal well with the references inside of anyOf, as it generates an extra unneeded alias.

I tried to see how this works in code and I see where you generate these aliases, based on something called Dereferenced, but I don't understand the structure of this library enough to sort out through that. Just wanted to add something to your tests that makes sense to investigate before merging. Hope that helps!
Thanks for your library, BTW

@omissis
Copy link
Owner Author

omissis commented Dec 22, 2024

Hey @strowk thanks for your comment. yeah, I have noticed that, too, using my own private battery of tests where I also have that case. I think it might have to do with the fact I am trying to do deep merges of the underlying data structures. Unfortunately this is a blocking issue for merging this PR. I will try find a solution and report.

@omissis
Copy link
Owner Author

omissis commented Dec 23, 2024

@strowk can you check again using the latest commit from this branch please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants