-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Passes x-hasura-* headers as part of the user object in the body #1459
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
…d removes it from reqHeader as analyze is admin only endpoint
|
Review app for commit 1b13ff7 deployed to Heroku: https://hge-ci-pull-1459.herokuapp.com |
|
|
||
| // Check if x-hasura-role is available in some form in the headers | ||
| const totalHeaders = Object.keys(reqHeaders); | ||
| for (let i = 0; i < totalHeaders.length; i += 1) { |
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.
Can we use totalHeaders.map instead of for syntax?
| for (let i = 0; i < totalHeaders.length; i += 1) { | ||
| // If header has x-hasura-* | ||
| const lHead = totalHeaders[i].toLowerCase(); | ||
| if (lHead.indexOf('x-hasura-') !== -1) { |
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.
use slice?
|
The functionality looks good to me. We'll need to improve the explain UI though, showing the role for which it was analyzed for. Maybe we can open another issue. |
0x777
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.
LGTM.
|
Review app for commit 86b907c deployed to Heroku: https://hge-ci-pull-1459.herokuapp.com |
|
Review app for commit 8950545 deployed to Heroku: https://hge-ci-pull-1459.herokuapp.com |
|
Review app https://hge-ci-pull-1459.herokuapp.com is deleted |
### What
Consider the following query:
```graphql
query {
institution_in_a_country_with_a_city_named_stockholm: InstitutionMany(
where: { location: { country: { cities: { name: { _eq: "Stockholm" } } } } }
) {
id
location {
country {
cities {
name
}
}
}
}
}
```
We are filtering by the object field `name` inside the nested array
`cities` field, that itself is nested inside `location.countries`. This
previously did not work because the nested field pathing sent to the
connector was incorrect.
### How
The code did not correctly set the `column` vs the `field_path` on
`Expression::LocalNestedArray`. This has been fixed.
The test postgres DB schema has been updated to add the `cities` array
in order to facilitate testing this.
The justfile script that is used to update the postgres schema in test
DataConnectorLinks got modifies as the schema started to exceed the size
limit that can be sent via command line args. It now writes the schema
and capabilities to temporary files and feeds them into `jq`, instead of
using command line args to pass in schema and capabilities.
V3_GIT_ORIGIN_REV_ID: aca7a0b8deac91355a21be6bdab4edcf617ff2e1
Passes x-hasura-* headers as part of the user object in the body and removes it from reqHeader as analyze is admin only endpoint
#1457
Description
Previously all the analyze requests were sent with user: { 'x-hasura-role': 'admin'}. This allows passing arbitrary x-hasura-* as part of the user object
@0x777 It removes the x-hasura-* values from the request headers and puts it inside the user object inside body.
What component does this PR affect?
Requires changes from other components? If yes, please mark the components:
Related Issue
Solution and Design
Type
Checklist: