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

new graphql data representation #4942

Open
Geal opened this issue Apr 11, 2024 · 2 comments
Open

new graphql data representation #4942

Geal opened this issue Apr 11, 2024 · 2 comments
Labels
potentially-breaking Requires an incompatible change

Comments

@Geal
Copy link
Contributor

Geal commented Apr 11, 2024

The router is currently using serde_json_bytes::Value, a type derived from the similar one in serde_json, to represent JSON data.
It has been used well so far, and the optimization using the Bytes structure as underlying storage for strings helped a lot. But there's a few issues with this type, that we could address by changing it:

  • we reexport this type from another crate, while it is a fundamental part of the graphql response. We can't really evolve it (we have similar issues with the hyper crate). We can modify serde_json_bytes but it still has a lot of adherence to serde_json
  • we don't really use the data in the router. It is transformed a bit by inserting in the right array or object, and we validate enum values and number values, but we should not need to parse them
  • similarly, we use some complex and slow types like IndexMap that could be replaced with simpler vecs, because with some tweaks, we would not really require random access

Proposal

Instead of reexporting a type directly, we make an internal router type, with a similar API to serde_json_bytes::Value, but that hides the internal details. We make this API available from rhai too, to avoid large data transformations there. And once we have this API, we can look at improving the type freely in the future

@Geal Geal added the potentially-breaking Requires an incompatible change label Apr 11, 2024
@BrynCooke
Copy link
Contributor

Not sure about not needing random access. One of the things that has come up is the need to be able to select via json path into these structures. Agree with the rest of the proposal though.

@Geal
Copy link
Contributor Author

Geal commented Apr 11, 2024

it is useful some time, but the great majority of use cases (by number of times it is run in query execution) is iterating over the list of keys and values and filtering it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
potentially-breaking Requires an incompatible change
Projects
None yet
Development

No branches or pull requests

2 participants