hasura / graphql-engine

Blazing fast, instant realtime GraphQL APIs on your DB with fine grained access control, also trigger webhooks on database events.
https://hasura.io
Apache License 2.0
31.03k stars 2.76k forks source link

Invalid TypeScript Metadata SDK validation #6504

Open namoscato opened 3 years ago

namoscato commented 3 years ago

After working through https://github.com/hasura/graphql-engine/issues/6477 issues (applying https://github.com/hasura/graphql-engine/pull/6478 changes locally), I am experiencing some unexpected errors parsing what should be valid TableEntry metadata. I have isolated at least the first issue I am encountering to InsertPermissionEntry.

✅ For example, this works:

import {Convert, TableEntry} from '@hasura/metadata'

const table: TableEntry = {
  table: {schema: 'public', name: 'user'},
  insert_permissions: [
    {
      role: 'user',
      permission: {
        check: {
          company_id: {_eq: 'X-Hasura-Company-ID'}
        },
        columns: ['name']
      }
    }
  ]
}

Convert.toTableEntry(JSON.stringify(table))

❌ However, once insert_permissions[0].permission.check becomes a more complex array:

import {Convert, TableEntry} from '@hasura/metadata'

const table: TableEntry = {
  table: {schema: 'public', name: 'user'},
  insert_permissions: [
    {
      role: 'user',
      permission: {
        check: {
          _and: [
            {
              company_id: {_eq: 'X-Hasura-Company-ID'}
            }
          ]
        },
        columns: ['name']
      }
    }
  ]
}

Convert.toTableEntry(JSON.stringify(table))

I get an error:

/node_modules/@hasura/metadata/dist/generated/HasuraMetadataV2.js:531
    throw Error(`Invalid value ${JSON.stringify(val)} for type ${JSON.stringify(typ)}`);
          ^
Error: Invalid value [{"role":"user","permission":{"check":{"_and":[{"company_id":{"_eq":"X-Hasura-Company-ID"}}]},"columns":["name"]}}] for type [null,{"arrayItems":{"ref":"InsertPermissionEntry"}}]
    at invalidValue (/node_modules/@hasura/metadata/dist/generated/HasuraMetadataV2.js:531:11)

despite table being a valid TableEntry type in both scenarios.

/cc @GavinRay97

GavinRay97 commented 3 years ago

Hey @namoscato

So the Convert.toXX() methods don't actually do anything besides runtime validation. In general, I would recommend just using the TS type as type annotations on plain objects.

I have run into this issue too, Quicktype is really solid but trying to debug why it's auto-generated validators are failing for certain objects.

Based on the type definition, permission.check should be nearly equivalent to Record<string, any> (Quicktype cannot handle Record types at all, hence why all the types are { [key: string]: T }:

https://github.com/hasura/graphql-engine/blob/60613b4777f13c41d3d7965c3e8cfb4d2f3c8a25/contrib/metadata-types/src/types/HasuraMetadataV2.ts#L246-L251

Unless it's not interpreting object as { [key: string]: any }

I think that potentially changing the type to be:

check?: { [key: string]: any }

Might fix it (maybe it's the array breaking it), though wondering what the most-correct/least-loose type definition we could use here would be.

namoscato commented 3 years ago

Thanks for the reply, @GavinRay97!

So the Convert.toXX() methods don't actually do anything besides runtime validation. In general, I would recommend just using the TS type as type annotations on plain objects.

Makes sense. I'm actually toying with a GitHub Action idea that would validate metadata files in practice; the plain object above was just used to illustrate the issue.

Unless it's not interpreting object as { [key: string]: any }

Yeah, object definitely isn't any – or Array for that matter which probably explains it.

wondering what the most-correct/least-loose type definition we could use here would be

I'll give this some more thought as well.

GavinRay97 commented 3 years ago

Yeah, object definitely isn't any – or Array for that matter which probably explains it.

I would absolutely agree with you there, but, ya' know ¯_(ツ)_/¯ Javascript

image

I am trying to think back on this, and I recall the two major limitations of QuickType's TS type parsing abilities were:

export interface InsertPermission {
  /** This expression has to hold true for every new row that is inserted */
  check?: {
    [key: string]:
      | string
      | number
      | { [key: string]: string | number }
      | Array<{ [key: string]: string | number }>
  }
}

This is about as close as I can get, but it's still not entirely valid. It only supported a single level, I think we really need recursive types.

Maybe I can try to update QuickType or pop into their Slack to ask about this.

namoscato commented 3 years ago

I'm not really familiar with quicktype but was just playing around with it and see what you mean.

Maybe I can try to update QuickType or pop into their Slack to ask about this.

Sounds good!

In the meantime, I'd be in favor of just loosening things to:

check?: { [key: string]: any }