-
-
Notifications
You must be signed in to change notification settings - Fork 590
Add JSONPath scalar type and codec support #2571
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
base: main
Are you sure you want to change the base?
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Please:
- Add a column to the b.types table: https://github.com/graphile/crystal/blob/main/postgraphile/postgraphile/__tests__/kitchen-sink-schema.sql#L275
- Insert null into the column value for the first b.types row: https://github.com/graphile/crystal/blob/main/postgraphile/postgraphile/__tests__/kitchen-sink-data.sql#L158
- Insert a sensible expression into the second b.types row: https://github.com/graphile/crystal/blob/main/postgraphile/postgraphile/__tests__/kitchen-sink-data.sql#L208
- Add a column to the types mutation update and create operations:
ltreeArray: ["Bar.Baz.Qux", "Bar.Foo.Fah"] ltreeArray: ["Bar.Baz.Qux", "Bar.Foo.Fah"] - Ensure that the value is queried in the response to the mutation and to the types query:
ltreeArray - Run the full test suite with envvar
UPDATE_SNAPSHOTS=1
Thanks!
Great review thanks! 🙏 i was unable to get it to build and test (unrelated to this PR) but i’ll have another go at it later this week. |
All right, I managed to get the build and tests to run in an Ubuntu VM so it's looking a lot better now! A few whitespace changes snuck in, if you want me to revert those let me know! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Might make sense for us to implement default behaviors so that orderBy/condition on jsonPath is disabled by default. You can leave that with me if you want because I can't remember how to do it off-hand.
Description
I have been patching postgraphile to add support for
jsonpath
and thought it might make sense to add upstream.Performance impact
unknown, negligible I would presume since it just adds a codec
Security impact
Checklist
yarn lint:fix
passes.yarn test
passes.RELEASE_NOTES.md
file (if one exists).