-
Notifications
You must be signed in to change notification settings - Fork 2.8k
bump stackage to lts 13 and refer to hasura's pg-client-hs #1747
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
Conversation
|
Review app for commit f1d056a deployed to Heroku: https://hge-ci-pull-1747.herokuapp.com |
|
Review app for commit 957b64d deployed to Heroku: https://hge-ci-pull-1747.herokuapp.com |
shahidhk
left a comment
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.
@0x777 says is cool
|
Review app https://hge-ci-pull-1747.herokuapp.com is deleted |
## Description This PR removes as many constraints as possible from Backend without refactoring the code. Thanks to #1844, a few ToJSON functions can be removed from the IR, and several constraints were simply redundant. This PR borrows from similar work done as part of #1666. ## Note To remove constraints more aggressively, I have explored the possibility of _removing Representable altogether_, in a [separate commit](https://github.com/hasura/graphql-engine-mono/compare/nicuveo/remove-extension-constraints..nicuveo/tentative-remove-representable). I am not convinced it's a good idea in terms of readability of the code, but it's a possibility. Further work includes deciding what we want to do with `Show` and `ToTxt`; see #1747. hasura/graphql-engine-mono#1843 GitOrigin-RevId: 337324a
## Description When setting up a remote relationship to a remote schema, values coming from the left-hand side are given as _arguments_ to the targeted field of the remote schema. In turn, that means we need to adjust the arguments to that remote field; in the case of input objects, it means creating a brand new input object in which the relevant fields have been removed. To both avoid conflicts, and be explicit, we give a pretty verbose name to such an input object: its original name, followed by "remote_rel", followed by the full name of the field (table name + relationship name). The bug there was introduced when working on extending remote relationships to other backends: we changed the code that translates the table name to a graphql identifier to be generic, and use the table's `ToTxt` instance instead. However, when a table is not in the default schema, the character used by that instance is `.`, which is not a valid GraphQL name. This PR fixes it, by doing two things: - it defines a safe function to translate LHS identifiers to graphql names (by replacing all invalid characters by `_`) - it doesn't use `unsafeMkName` anymore, and checks at validation time that the type name is correct ## Further work On this PR: - [x] add a test - [x] write a Changelog entry Beyond this PR, we might want to: - prioritize #1747 - analyze all calls to `unsafeMkName` and remove as many as possible PR-URL: hasura/graphql-engine-mono#3363 GitOrigin-RevId: fe98eb1d34157b2c8323af453f5c369de616af38
<!-- The PR description should answer 2 important questions: --> ### What Fixes some issues with the operator lookup for built-in NDC operators (like `GreaterThan`, `LessThan` etc). - Make error a user error, rather than internal - Include more context in the error - Also lookup operator in the OpenDD operators list if a mapping cannot be found, as a missing mapping here means "the name is the same". V3_GIT_ORIGIN_REV_ID: fef0003cb6d59610b6ec411ac21620b36af5a10b
Layering the changes from #1685. The idea is to make it easier to merge #1685
Affected components