nhost / hasura-auth

Authentication for Hasura.
https://nhost.io
MIT License
377 stars 114 forks source link

support hasura naming conventions #375

Closed joshuadutton closed 4 months ago

joshuadutton commented 1 year ago

Most of the apis from both naming conventions have compatible types, thanks to graphql names being explicitly set for fields and operations. There were a few that had mismatched __typename fields, and since that field wasn't used anywhere in the code, I had the code generator omit typenames for all graphql types.

Tests:

changeset-bot[bot] commented 1 year ago

⚠️ No Changeset found

Latest commit: f7ef8b83ac45064b4957be7b2babdf86d3088b4d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

joshuadutton commented 1 year ago

The latest commit is to preemptively help prepare for when Nhost uses Hasura v2.21.0 or greater since more of the graphql-default naming conventions were changed in that release.

The commit uses the alias affectedRows: affected_rows.

szilarddoro commented 1 year ago

I checked the test you mentioned, and it's expected in that case that the test fails as the snapshot is different. I think we can leave it as is and assume that the hasura-default will be used by default. If we change this in the future, then we can just simply update the snapshot and everything will be fine.

An issue I noticed: The newly introduced HASURA_GRAPHQL_NAMING_CONVENTION environment variable doesn't have a fallback value in the env.ts file and it causes errors when Hasura Auth tries to apply the metadata. Could you please add hasura-default as the second argument to the castStringEnv function there?

joshuadutton commented 1 year ago

Thanks for the feedback. I'll make the changes and update the PR tomorrow. FYI, I'm in MST timezone.

joshuadutton commented 1 year ago

It's taking me longer than expected because some enum types were added, and that has cascading effects to some of the input types. I've addressed all the feedback and I've also pushed a WIP commit with some transforms created and the rest stubbed out. I'll finish the rest soon.

xmlking commented 10 months ago

waiting for this feature :(

dbarrosop commented 4 months ago

With the migration to go this is no longer needed as we are talking to postgres directly.

Thanks a lot for setting this in motion though.

xmlking commented 4 months ago

after enabling hasura naming convention, auth server is not starting. is there any workaround ?

      HASURA_GRAPHQL_EXPERIMENTAL_FEATURES: naming_convention
      HASURA_GRAPHQL_DEFAULT_NAMING_CONVENTION: graphql-default
auth  | {"level":"info","message":"Log level: info"}
auth  | {"level":"info","message":"Waiting for Hasura to be ready..."}
auth  | {"level":"info","message":"Hasura is ready"}
auth  | {"level":"info","message":"Applying SQL migrations..."}
auth  | {"level":"info","message":"SQL migrations applied"}
auth  | {"level":"info","message":"Applying metadata..."}
auth  | {"level":"info","message":"Metadata applied"}
auth  | /nix/store/pzr5969fyqqii70qgmfnmfdi499s12az-node_modules-prod/node_modules/.pnpm/graphql-request@3.7.0_graphql@16.2.0/node_modules/graphql-request/dist/index.js:340
auth  |                         throw new types_1.ClientError(__assign(__assign({}, errorResult), { status: response.status, headers: response.headers }), { query: query, variables: variables });
auth  |                               ^
auth  |
auth  | ClientError: 'insertAuthRoles' has no argument named 'on_conflict': {"response":{"errors":[{"message":"'insertAuthRoles' has no argument named 'on_conflict'","extensions":{"path":"$.selectionSet.insertAuthRoles","code":"validation-failed"}}],"status":200,"headers":{}},"request":{"query":"mutation upsertRoles($roles: [authRoles_insert_input!]!) {\n  insertAuthRoles(\n    objects: $roles\n    on_conflict: {constraint: roles_pkey, update_columns: []}\n  ) {\n    affected_rows\n    returning {\n      role\n    }\n  }\n}","variables":{"roles":[{"role":"me"},{"role":"user"}]}}}
auth  |     at /nix/store/pzr5969fyqqii70qgmfnmfdi499s12az-node_modules-prod/node_modules/.pnpm/graphql-request@3.7.0_graphql@16.2.0/node_modules/graphql-request/dist/index.js:340:31
auth  |     at step (/nix/store/pzr5969fyqqii70qgmfnmfdi499s12az-node_modules-prod/node_modules/.pnpm/graphql-request@3.7.0_graphql@16.2.0/node_modules/graphql-request/dist/index.js:63:23)
auth  |     at Object.next (/nix/store/pzr5969fyqqii70qgmfnmfdi499s12az-node_modules-prod/node_modules/.pnpm/graphql-request@3.7.0_graphql@16.2.0/node_modules/graphql-request/dist/index.js:44:53)
auth  |     at fulfilled (/nix/store/pzr5969fyqqii70qgmfnmfdi499s12az-node_modules-prod/node_modules/.pnpm/graphql-request@3.7.0_graphql@16.2.0/node_modules/graphql-request/dist/index.js:35:58)
auth  |     at process.processTicksAndRejections (node:internal/process/task_queues:95:5) {
auth  |   response: {
auth  |     errors: [
auth  |       {
auth  |         message: "'insertAuthRoles' has no argument named 'on_conflict'",
auth  |         extensions: {
auth  |           path: '$.selectionSet.insertAuthRoles',
auth  |           code: 'validation-failed'
auth  |         }
auth  |       }
auth  |     ],
auth  |     status: 200,
auth  |     headers: Headers {
auth  |       [Symbol(map)]: [Object: null prototype] {
auth  |         date: [ 'Thu, 02 May 2024 22:39:35 GMT' ],
auth  |         'content-type': [ 'application/json; charset=utf-8' ],
auth  |         'content-length': [ '162' ]
auth  |       }
auth  |     }
auth  |   },
auth  |   request: {
auth  |     query: 'mutation upsertRoles($roles: [authRoles_insert_input!]!) {\n' +
auth  |       '  insertAuthRoles(\n' +
auth  |       '    objects: $roles\n' +
auth  |       '    on_conflict: {constraint: roles_pkey, update_columns: []}\n' +
auth  |       '  ) {\n' +
auth  |       '    affected_rows\n' +
auth  |       '    returning {\n' +
auth  |       '      role\n' +
auth  |       '    }\n' +
auth  |       '  }\n' +
auth  |       '}',
auth  |     variables: { roles: [ { role: 'me' }, { role: 'user' } ] }
auth  |   }
auth  | }
auth  |
auth  | Node.js v18.19.1
auth  | {"time":"2024-05-02T22:39:36.162444086Z","level":"ERROR","msg":"node server failed","error":"exit status 1"}
auth  | {"time":"2024-05-02T22:39:36.162501336Z","level":"INFO","msg":"shutting down server"}
auth  | {"time":"2024-05-02T22:39:36.162541044Z","level":"ERROR","msg":"server failed","error":"http: Server closed"}
auth  | {"time":"2024-05-02T22:40:36.290321169Z","level":"INFO","msg":"auth v0.29.3"}
dbarrosop commented 4 months ago

you will need to wait until the migration to go is complete.