fix: generate openAI-compatible json schemas for list types #272
Conversation
✅ Docs preview readyThe preview is ready to be viewed. View the preview File Changes 0 new, 2 changed, 0 removedBuild ID: d56bdcb80a30068675edf9d2 URL: https://www.apollographql.com/docs/deploy-preview/d56bdcb80a30068675edf9d2 |
|
Is there a utility we can include in our unit tests that does the same validation as https://jsonschema.dev/ ? |
|
Thanks for your input, @swcollard. With a customer waiting to evaluate the MCP server, merging this quick fix should be the immediate priority. I've created a backlog ticket for adding such unit tests, so we can address this improvement in a follow-up. |
| fn empty_file() { | ||
| let result = CustomScalarMap::from_str("").err().unwrap(); | ||
|
|
||
| insta::assert_debug_snapshot!(result, @r###" |
There was a problem hiding this comment.
This looks unrelated to the PR change? I'd say if we want to change the style here we should make a separate PR? though maybe Im misremembering what these # do :D
There was a problem hiding this comment.
I didn't really mean to change this. 😅 I just accepted the snapshot changes that Insta generated. This might be cause by the recent dependency updates.
There was a problem hiding this comment.
If these are just always updating, imo best to do a dedicated PR for updating the format, especially in unrelated files. For the ones in separate files it muddies the git history where this PR will show up in that files history, for the ones in the same file it makes reviewing more difficult.
I think you've got approval for the change so maybe not worth removing the ones in operations.rs, but I'd definitely encourage reverting the ones in this file to not muddy the history :)
There was a problem hiding this comment.
I tracked down the relevant Insta changeset, but you're right. I agree it can be confusing to include unrelated changes in this PR. I'll revert the changes in this file.
| ], | ||
| "items": Object { | ||
| "oneOf": Array [ | ||
| Object { |
There was a problem hiding this comment.
Looks like all our array test currently only have scalars, could we have one that s a list of input objects? (one for nullable and one for non nullable)
There was a problem hiding this comment.
I've added tests for input object lists.
| "###); | ||
| "#); |
There was a problem hiding this comment.
Not blocking, but what caused the change in all these snapshots to use the single #?
There was a problem hiding this comment.
This test code was written before Insta v1.40.0. It used to use ### for inline snapshots but now it uses # by default. See #272 (comment)
The MCP server is generating JSON schemas that don't match OpenAI's function calling specification. It puts
oneOfat the array level instead of usingitemsto define the JSON schemas for GraphQL list types. While some other LLMs are more flexible about this, it technically violates the JSON Schema specification that OpenAI strictly follows.This PR updates the list type handling logic to move
oneOfinsideitemsfor GraphQL list types.Before
Valid input fails validation
https://jsonschema.dev/s/kqwqR
After
Valid input passes validation
https://jsonschema.dev/s/3jAXW