Skip to content
This repository has been archived by the owner on Apr 15, 2020. It is now read-only.

Transforms can lead to "empty" types breaking schema #37

Closed
AndKiel opened this issue Jan 15, 2020 · 14 comments
Closed

Transforms can lead to "empty" types breaking schema #37

AndKiel opened this issue Jan 15, 2020 · 14 comments

Comments

@AndKiel
Copy link

AndKiel commented Jan 15, 2020

Given:

  • ObjectType A
  • ObjectType B with a single Field of type A

When:

  • Schema transforms lead to ObjectType A being removed/hidden

Then:

  • ObjectType B becomes an empty type without any fields, making schema invalid

My current workaround is removing empty types through FilterTypes transform:

import { GraphQLObjectType } from "graphql";
import { FilterTypes } from "graphql-tools-fork";

new FilterTypes(type => {
  if (type instanceof GraphQLObjectType) {
    const fieldsMap = type.getFields();
    const fieldsCount = Object.keys(fieldsMap).length;
    return fieldsCount > 0;
  }
  return true;
})

The question is, should this be the responsibility of FilterToSchema transform to clean up empty types as an additional step? It's already hiding queries and mutations that use types which were hidden, I think (or something like that).

@yaacovCR
Copy link
Owner

This is supposed to be addressed within healSchema by pruneSchema. This may be an edge case or regression? If you can make a minimal reproduction repository, I will get on this.

@yaacovCR
Copy link
Owner

@AndKiel
Copy link
Author

AndKiel commented Jan 15, 2020

This edge-case was mentioned in #16 but maybe never fixed. I was trying out various versions, including v6.4 but wasn't working.

You can reproduce via:

import { FilterToSchema, transformSchema } from "graphql-tools-fork";
import { buildSchemaSync, ObjectType, Query, Resolver } from "type-graphql";

@ObjectType()
class EmptyObject {}

@Resolver()
class SomeResolver {
  @Query()
  public someQuery(): EmptyObject {
    return new EmptyObject();
  }
}

const schema = buildSchemaSync({
  resolvers: [SomeResolver]
});

it("removes types without any fields", async () => {
  const transformedSchema = transformSchema(schema, [
    new FilterToSchema(schema)
  ]);
  const objectTypes = transformedSchema.getTypeMap();
  expect(objectTypes).not.toHaveProperty("EmptyObject");
});

If you add workaround transform before FilterToSchema test will pass.

@yaacovCR
Copy link
Owner

If you are using FilterObjectFields or TransformObjectFields which use visitSchema under the hood, healSchema is called automatically. Otherwise, I think transforms should maybe be responsible for calling healSchema themselves.

@yaacovCR yaacovCR reopened this Jan 15, 2020
@yaacovCR
Copy link
Owner

yaacovCR commented Jan 15, 2020

Closed in error, waiting for reproduction. on another note because the schema is cloned prior to every transformation, and cloning includes healing, you only hit this with the last transformation, assuming there has not been a regression.

@yaacovCR
Copy link
Owner

Ah. This is because you are not using FilterObjectFields or TransformObjectFields. You can call healSchema directly prior to transformation if you want. I think that is a better solution, because the schema is invalid prior to transformation anyway, not really the role of transformSchema to fix schemas out of spec per se, that is what healSchema is for.

Why do you have an empty type like this?

@AndKiel
Copy link
Author

AndKiel commented Jan 15, 2020

I'm using (exactly in the specified order):

  • FilterRootFields
  • FilterObjectFields
  • FilterTypes
  • FilterRootFields
  • FilterToSchema

I think implementation details aren't important, hiding is based on custom decorators and metadata. I kinda half-expected FilterToSchema to take care of empty types and healing, probably wrongly.

@AndKiel
Copy link
Author

AndKiel commented Jan 15, 2020

Why do you have an empty type like this?

I don't. Type ends up being empty in the process of other transformations.

@yaacovCR
Copy link
Owner

FilterTypes uses visitSchema as well which should heal automatically. If you could make a minimal reproduction, I would be happy to take a look. It helps me tremendously, because then I can simply add to test suite. If you want to send a PR with failing test, that would also be great.

As an aside, FilterToSchema is a default transform that you usually should not have to call.

@yaacovCR
Copy link
Owner

FilterToSchema is a request transform and not a schema transform. I am not sure whether it removes from the request empty objects at that point. I am going to work on making sure delegateToSchema does not error on entirely empty queries, something helpful for type merging

@AndKiel
Copy link
Author

AndKiel commented Jan 15, 2020

Hmm, it's possible that FilterToSchema does literally nothing in my case because the transformed schema is also the target and the code has been wrong since the beginning in that place.

The thing is, a type ends up being empty in the process of other transformations.

I just did a quick check and calling healSchema on the schema after all my transformations does not fix the empty types.

So reproduction can become

import { healSchema } from "graphql-tools-fork";
import { buildSchemaSync, ObjectType, Query, Resolver } from "type-graphql";

@ObjectType()
class EmptyObject {}

@Resolver()
class SomeResolver {
  @Query()
  public someQuery(): EmptyObject {
    return new EmptyObject();
  }
}

const schema = buildSchemaSync({
  resolvers: [SomeResolver]
});

it("removes types without any fields", async () => {
  const healedSchema = healSchema(schema);
  const objectTypes = healedSchema.getTypeMap();
  expect(objectTypes).not.toHaveProperty("EmptyObject");
});

@yaacovCR
Copy link
Owner

Ahah. My guess/hope is interaction with type-graphql and healing code. Will take a look.

@yaacovCR
Copy link
Owner

@AndKiel I hope working for you?

@AndKiel
Copy link
Author

AndKiel commented Jan 17, 2020

Yes, everything seems to be working. I can call healSchema after all my transformations and I don't need that custom FilterTypes workaround anymore. Thanks.

yaacovCR added a commit that referenced this issue Jan 21, 2020
yaacovCR added a commit that referenced this issue Jan 21, 2020
yaacovCR added a commit that referenced this issue Feb 17, 2020
yaacovCR added a commit that referenced this issue Feb 27, 2020
yaacovCR added a commit that referenced this issue Mar 26, 2020
yaacovCR added a commit that referenced this issue Mar 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants