graphql-zeus icon indicating copy to clipboard operation
graphql-zeus copied to clipboard

Types not checked. Apollo typed query hook with Hasura endpoint allows anything.

Open seanonthenet opened this issue 3 years ago • 10 comments

Is this expected behaviour?

Autocomplete works but the query essentially allows any object without error:

Screenshot 2021-11-11 at 13 12 03@2x

Love Zeus. ❤️⚡️ Thanks.

seanonthenet avatar Nov 11 '21 19:11 seanonthenet

I think we can repair it thanks!!

aexol avatar Nov 12 '21 10:11 aexol

Discovered this issue yesterday while testing zeus. It seems like it is only possible to add non-existing fields if there are at least 1 existing field declared in the query.

So as an example:

=== Schema ===

type MalSeasonIdentifier  {
  season: MalYearSeason! 
  year: Int! 
}

type Query {
  animeSeasonList: [MalSeasonIdentifier!]! 
}

=== TS === this does not give any errors as 'year' is a valid field ->

const result = await chain("query")({
	animeSeasonList: {
		year: true,
		fieldThatDoesNotExist: "f"
	}
})

this does give error ->

const result = await chain("query")({
	animeSeasonList: {
		fieldThatDoesNotExist: "f"
	}
})

image

Having a safety mechanism to alert us of incorrect queries is important, especially as system evolves and fields may get deprecated and disappear.

Keep up the good work, i hope someone will be able to look into this soon.

IceBlizz6 avatar Dec 19 '21 01:12 IceBlizz6

Any ideas how this may be fixed? Pretty fundamental for error-avoiding

omaiboroda avatar Apr 20 '22 18:04 omaiboroda

play with types and try to find a solution. Maybe with latest version of TS you can do smth, before it was impossible for me to do

aexol avatar Apr 21 '22 10:04 aexol

Also wondering if it's possible to fix this ?

karibertils avatar Jul 10 '22 14:07 karibertils

@karibertils https://zeus.graphqleditor.com/page/plugins/typedDocumentNode.html we are in the process of transitioning to this. Try using this way!

aexol avatar Jul 10 '22 15:07 aexol

@aexol

This looks like a nice way to do things, and I see the types are now working for the variables which is really cool. But typescript still shows no error when adding random fields which don't exist in the schema. See below.

image

karibertils avatar Jul 10 '22 17:07 karibertils

Yes, I still see this problem locally. Pretty significant problem if the whole point is to avoid writing invalid queries. I see this without using hooks -- just the raw client:

const client = (() => {
  const chain = Chain('...');
  return {
    query: chain('query'),
    mutation: chain('mutation')
  }
})();

const test = async () => {
  // Typescript is also happy with this destructuring
  const { lots: [{ someFakeField }] } = await client.query({ 
    lots: [
      {}, 
      {
        id: true,
        // this field doesn't exist but doesn't trigger error since 'id' does exist
        someFakeField: true,
      }
    ] 
  });
}

I will say that it seems to handle this well (i.e. error appropriately) when I write it instead as:

  const fields: ValueTypes["Lot"] = {
    id: true,
    someFakeField: true,
  };
  
  const { lots: [{someFakeField}] } = await client.query({ 
    lots: [
      {
        where: {
          category: { equals: Category.STEEL } 
        }
      }, 
      fields
    ] 
  });

which seems to indicate the type is not narrow enough

mattkindy avatar Jul 21 '22 01:07 mattkindy

I dug a bit into the generated code and noticed this section:

export const Thunder =
  (fn: FetchFunction) =>
  <O extends keyof typeof Ops, SCLR extends ScalarDefinition, R extends keyof ValueTypes = GenericOperation<O>>(
    operation: O,
    graphqlOptions?: ThunderGraphQLOptions<SCLR>,
  ) =>
  <Z extends ValueTypes[R]>(o: Z | ValueTypes[R], ops?: OperationOptions & { variables?: Record<string, unknown> }) =>
    fn(
      Zeus(operation, o, {
        operationOptions: ops,
        scalars: graphqlOptions?.scalars,
      }),
      ops?.variables,
    ).then((data) => {
      if (graphqlOptions?.scalars) {
        return decodeScalarsInResponse({
          response: data,
          initialOp: operation,
          initialZeusQuery: o as VType,
          returns: ReturnTypes,
          scalars: graphqlOptions.scalars,
          ops: Ops,
        });
      }
      return data;
    }) as Promise<InputType<GraphQLTypes[R], Z, SCLR>>;

It seemed to widen the acceptance input type a bit much... I could be wrong, but when I made the change to remove Z and simply continue using ValueTypes[R], it seemed to work appropriately on input. However, removing 'Z' or otherwise typing the fields as ValueTypes["Lot"] made the response type wider than it should be.

export const Thunder =
  (fn: FetchFunction) =>
  <O extends keyof typeof Ops, SCLR extends ScalarDefinition, R extends keyof ValueTypes = GenericOperation<O>>(
    operation: O,
    graphqlOptions?: ThunderGraphQLOptions<SCLR>,
  ) =>
  (o: ValueTypes[R], ops?: OperationOptions & { variables?: Record<string, unknown> }) =>
    fn(
      Zeus(operation, o, {
        operationOptions: ops,
        scalars: graphqlOptions?.scalars,
      }),
      ops?.variables,
    ).then((data) => {
      if (graphqlOptions?.scalars) {
        return decodeScalarsInResponse({
          response: data,
          initialOp: operation,
          initialZeusQuery: o as VType,
          returns: ReturnTypes,
          scalars: graphqlOptions.scalars,
          ops: Ops,
        });
      }
      return data;
    }) as Promise<InputType<GraphQLTypes[R], ValueTypes[R], SCLR>>;

mattkindy avatar Jul 21 '22 01:07 mattkindy

Any updates on this? If mattkindy's proposed changes works well then maybe look into making a pull request. If not then would be nice if this could be prioritized. As people have noted above, this is currently a big issue and the only blocker for my team to use it.

IceBlizz6 avatar Sep 11 '22 14:09 IceBlizz6

I will definitely be looking at this again. Have been slammed with other things -- am interested in helping resolving this.

mattkindy avatar Oct 24 '22 23:10 mattkindy

This is a TS problem I will take a look soon. With latest versions of TS it should be possible to resolve it

aexol avatar Oct 25 '22 08:10 aexol

@aexol @mattkindy Did any of you get further on this? My team needs to migrate soon, would be really great if this could get some love :)

IceBlizz6 avatar Jul 04 '23 08:07 IceBlizz6

In case it helps someone or the maintainers of this project, we fixed all the typings issues we had with zeus in our fork. Here are 2 PRs that could be useful to someone else:

  • https://github.com/elba-security/graphql-zeus/pull/7
  • https://github.com/elba-security/graphql-zeus/pull/8

Disclaimer: this is using graphql-zeus v4 as we never upgraded to v5. But I think that some type fixes could be applied to v5 as well.

ValentinH avatar Jul 04 '23 09:07 ValentinH

That looks interesting, in case i want to try that fork then is it possible to get that from a package manager or do i need to compile it myself?

IceBlizz6 avatar Jul 04 '23 09:07 IceBlizz6

Yes it's available on npm: @elba-security/codegen

See the other commits for small changes as there is no change log. Most changes are bug fixes, others are small adaptations to better fit with Hasura types (uuid or json for example).

ValentinH avatar Jul 04 '23 10:07 ValentinH

@aexol @mattkindy Did any of you get further on this? My team needs to migrate soon, would be really great if this could get some love :)

Afraid not. After @aexol dropped their comment, it coincided with this taking a backseat for us.

mattkindy avatar Jul 04 '23 12:07 mattkindy

@aexol Have you looked further into this issue?

IceBlizz6 avatar Jul 09 '23 10:07 IceBlizz6

Is this fix on 5.3.2 ? Cause i am testing it and it still seems to accept anything.

IceBlizz6 avatar Oct 26 '23 10:10 IceBlizz6