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

chore: Update jsonschema usage #6500

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

Stranger6667
Copy link

Hey!

This PR updates the jsonschema crate and applies its newer APIs.

Signed-off-by: Dmitry Dygalo <[email protected]>
@Stranger6667 Stranger6667 requested review from a team as code owners January 1, 2025 08:49
@apollo-cla
Copy link

@Stranger6667: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@svc-apollo-docs
Copy link
Collaborator

svc-apollo-docs commented Jan 1, 2025

✅ Docs Preview Ready

No new or changed pages found.

@goto-bus-stop goto-bus-stop self-requested a review January 2, 2025 08:19
@goto-bus-stop
Copy link
Member

goto-bus-stop commented Jan 3, 2025

That's awesome, thank you!

It looks like some of our generated definitions have naming that isn't allowed in a uri-reference anymore (things like #/definitions/apollo_router::Extendable<RouterStage>).

Does jsonschema support URL-encoded references? Should it work if we produced something like this instead?

{
  "definitions": { "apollo_router::Extendable<RouterStage>": { /* omitted */ } }
}
// elsewhere...
{
  "$ref": "#/definitions/apollo_router%3A%3AExtendable%3CRouterStage%3E"
}

@Stranger6667
Copy link
Author

@goto-bus-stop

I think it should support them, yep :) I haven't tried it myself, though

Is there a test failure because of this?

@goto-bus-stop
Copy link
Member

Yeah, there's a failure because the schema generated by schemars uses ,: <> characters in definitions and references. It seems that some of those characters were accepted in uri references by the old version of jsonschema we have in use at the moment, but not in the new version.
Don't worry if that's not something you want to look into, I've put it on my list so I'll take a look at patching it up on our end soon

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.

4 participants