Skip to content
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

"Import specific types" works as "Import all types" #235

Closed
dortamiguel opened this issue Oct 4, 2018 · 28 comments
Closed

"Import specific types" works as "Import all types" #235

dortamiguel opened this issue Oct 4, 2018 · 28 comments

Comments

@dortamiguel
Copy link

dortamiguel commented Oct 4, 2018

On the documentation says that I can import certain types from a .graphql file like this:

Import specific types: # import A from 'schema.graphql'

But when I do that it, works like this other one:

Import all types: # import * from 'schema.graphql'

I know this because if I reference a type that is not imported using the "Import specific types" I don't get an error, which is the behaviour I expect from reading the docs.

This are the dependencies I'm using:

{
  "dependencies": {
    "apollo-server": "^2.1.0",
    "graphql": "^14.0.2",
    "graphql-import": "^0.7.1",
    "graphql-middleware": "^1.7.4",
    "graphql-tools": "^4.0.0"
  }
}

I'm using apollo server, and this is how I pass the schema to the server.

import { ApolloServer, gql } from "apollo-server";
import { importSchema } from "graphql-import";
import resolvers from "@root/resolvers";

const schema = importSchema("./src/schema.graphql");

const server = new ApolloServer({
  typeDefs: gql(schema),
  resolvers,
});
@dogma0
Copy link

dogma0 commented Oct 16, 2018

It happens to me too

@ryanquinn3
Copy link

I'm experiencing the same thing but the problem is made much more apparent when trying to use graphqlgen.

Let's say I have 3 types in my prisma schema and I only want to expose (and write resolvers for) 2 of them in my public API. That is currently not possible with this bug. Grapqlgen is receiving every type from prisma.graphql and then is trying to make me implement resolvers for each.

@Elfayer
Copy link

Elfayer commented Nov 14, 2018

Any fix/work around to this?

@divyenduz
Copy link
Contributor

@ellipticaldoor : Can you provide a minimal reproduction? Import specific types would also import the types that are used by that type (like inputs for its where etc).

I will take a look at this today.

@Elfayer
Copy link

Elfayer commented Nov 14, 2018

I'm not sure how much of a reproduction you want (github repo?), but the following schema.graphql is not working as expected:

Problem

# import Node from './generated/prisma.graphql'

type Query {
  profile: UserProfile
}

type UserProfile implements Node {
  id: ID!
  name: String!
  role: Role!
  posts: [Post!]!
  location: Location!
}

What happens?

It imports more than expected. I didn't declare or imported the types Role, Post nor Location. It is actually going to be taken from the prisma.graphql which has those types. (as well as the types in those from the cascade import which is fine)
Note: Obviously the type imported Node won't generate any cascade import.

What is expected?

graphql-import should not import types that are not defined as specific. We should get an error later on for not having a complete schema (maybe it's no more the responsibility of graphql-import)

@divyenduz
Copy link
Contributor

@Elfayer : Thanks for the details, Github repo would be the best (if you have the time).. maybe a repo that just prints the discrepancy using importSchema function from graphql-import directly.

If there is no Github repo, I will try to create one and validate/fix this soon anyways.

@Elfayer
Copy link

Elfayer commented Nov 14, 2018

@divyenduz Reproduction here: https://codesandbox.io/s/k9w3wx4k1r

@divyenduz
Copy link
Contributor

@Elfayer : It is indeed importing only types that are used, albeit "magically" since they are not explicitly imported. However, it is not importing everything. Like in this case: https://codesandbox.io/s/0xj69o9qmw

I added an unused enum to prisma.graphql which is not used/imported in schema.graphql and it does not get imported.

@Elfayer
Copy link

Elfayer commented Nov 15, 2018

@divyenduz Are you saying it is a normal behavior? I do not expect that behavior from a "specific" import.
Moreover, here is an updated example: https://codesandbox.io/s/5x39k14w4k
I import my Post schema: # import Post from "Post.graphql"
While it is importing prisma's Post instead.

Also, if you try to replace # import Node from "prisma.graphql" to # import Unused from "prisma.graphql", you'll realize it does import the same thing as before without the Unused type.

@divyenduz
Copy link
Contributor

@Elfayer : No, I am not stating that this is normal behavior, I just noticed that the bug (or feature) is different from original specification.

Your last comment also points out some confusion regarding collisions and also points out that specific import does not work at all. Maybe these are three separate issues.

To my understanding @maticzav will pick this up sometime soon and help us navigate through it 🙂

Thanks for reporting it with all the details and the reproductions, helps a lot 🙌

@Elfayer
Copy link

Elfayer commented Nov 15, 2018

Happy to help 😉
Also note it's not just specific imports, if you try # import * from "prisma.graphql", it won't import the type Unused either. So it really needs a deep review.

@frandiox
Copy link

frandiox commented Dec 4, 2018

I have a User type in the DB and another User type (with less fields) in the API. I'm not importing User from the DB. However, in the server playground the type I see is the one from the DB. The one from the API only works if I change its name.

I guess it's being overridden by the one in the DB because of this "import all" issue? I'm only doing # import Address from "../generated/prisma.graphql", which shouldn't leak User.

@chanlito
Copy link

chanlito commented Jan 8, 2019

Hey Prisma team, it'd be nice if somebody look into this issue since Oct 2018. Because I'm doing selective import, but instead I'm getting everything.

@amille14
Copy link

Experiencing the same issues as above and it makes it very difficult to reuse anything from a generated prisma schema without exposing far too much in your API. Would really appreciate if someone from the Prisma team could look into fixing this soon.

@dortamiguel
Copy link
Author

If it's not going to be fixed I think will be good to at least mention it on the Readme, just to avoid surprises

@maticzav
Copy link
Collaborator

maticzav commented Jan 28, 2019

Hey @ellipticaldoor 👋,

Could you check out the reproduction test that I made here? I tried to follow the CodeSandbox reproductions above but I cannot reproduce the failing behaviour.

https://github.com/prisma/graphql-import/blob/import-specific-fix/fixtures/specific/a.graphql

https://github.com/prisma/graphql-import/blob/35e7f41cb0bf56a11176c515c39e8d630b69afe1/src/index.test.ts#L811

@amille14
Copy link

amille14 commented Jan 29, 2019

@maticzav hey, thanks for your response. Maybe this gist will help clarify the issue that I and others are seeing: https://gist.github.com/amille14/b063f5cabfd4838c27bec5abe83ee544

In my example, importing only User from file A also imports Post from file A (since it's referenced by User), even though Post is being imported later from file B. I would expect only the specific User type to have been imported from file A, and not everything it references.

The reason you are unable to repro the issue in your tests is because the UnusedType and UnusedEnum are not referenced by the B interface being imported. It seems that only types referenced by other imported types are pulled in during a specific type import.

@maticzav
Copy link
Collaborator

Hey @amille14, thank you for the follow-up! After commenting, I figured out how to reproduce it myself as well. You can already see new test cases and code changes in the above branch.

Interestingly, I discovered that such functionality isn't a bug in the current code but a feature. I believe @schickling and @kbrandwijk must have thought that would come in handy.

@amille14
Copy link

Awesome! Appreciate you taking a look at this as it has been preventing me from reusing pieces of my generated Prisma schema in my API (due to security concerns, exposing too much info about the database), resulting in a ton of extra work on the API layer.

I could see the current behavior being useful as an import option, but at least in my case it was not the expected default behavior. Thank you, hope this fix gets merged soon!

@maticzav
Copy link
Collaborator

Hey @schickling @timsuchanek @amille14 @ellipticaldoor 👋,

Let me first give a brief overview of how graphql-import currently works, and what I believe this issue is trying to propose. Before I begin, it's worth mentioning that this issue might be the melting pot of the multiple problems which seemingly stem from the same problem but have different origins.

From the reproduction cases published above, I was able to figure the pattern which, as mentioned, is not an issue but rather a feature turned issue. (Very Facebook-ish.) Anyhow, currently graphql-middleware imports all the fields and directives connected with the imported type. What this means is that if we are importing type B which depends on types C, E and D, we will import all these types alongside B.

I believe this is a very nice feature and wouldn't call it an issue at all. As an example compare the two schemas below.

# import B from "b.graphql"

type A {
  first: String
  second: Float
  b: B
}

B depends on C.

# import B, C from "b.graphql"

type A {
  first: String
  second: Float
  b: B
}

The first one is, in my opinion, far cleaner than the second one. What's C import doing there anyway? Sadly, I believe that particular case might be the origin of this issue.

I think it would make sense to take a step backwards here and rethink the intention of grpahql-import. Should we make it very descriptive and trade in the magic and clarity, or push to the bare minimum and have every import explicitly specified?

@frandiox
Copy link

@maticzav Thanks for debugging this. About this "hidden" feature, it was mentioned in other issue if I don't remember wrong, and I personally think it's fine.

However, I think there's another related issue since I've seen leaked types that were not a dependency of other types. I believe it was due to nested imports (not sure if they are supported): A-file imports some types from B-file, B-file imports some types from C-file. As a result, all types from C-file were leaking and overwriting other types. Perhaps this is what happened to others.

@amille14
Copy link

amille14 commented Jan 30, 2019

OK! @maticzav after reading your last comment I went back and took another look at this and ran some test cases. I think I figured out what's causing all the confusion, at least on my end. I have some examples that will hopefully clarify all of this for everyone. This post might get a little long so bare with me.

The problem seems to stem from the fact that importing a single specific type does not import that type, it only imports its dependencies.

Example 1:

a.graphql:

type A {
  something: String
}

index.graphql:

# import A from './a.graphql'

Expected output from importSchema('./index.graphql'):

type A {
  something: String
}

Actual output from importSchema('./index.graphql'):

Yes, the output is empty.

Example 2:

a.graphql:

type A {
  something: String
  b: B
}

type B {
  something: String
}

index.graphql:

# import A from './a.graphql'

Expected output from importSchema('./index.graphql'):

type A {
  something: String
  b: B
}

type B {
  something: String
}

Actual output from importSchema('./index.graphql'):

type B {
  something: String
}

Only the dependencies (B) are imported, not the specified type (A).


Now, this really starts causing problems when you have types in different files that have the same name as the specific type you are trying to import (but which does not actually get imported, as seen in the example above). For some reason, the type with a matching name from a different file will get pulled in instead of the one from the file you wanted.

Example 3:

a.graphql

type A {
  b: B
  correct: String
}

type B {
  c: C
  incorrect: String
}

type C {
  incorrect: String
}

b.graphql

type B {
  a: A
  c: C
  correct: String
}

c.graphql

type C {
  correct: String
}

type A {
  incorrect: String
}

index.graphql

# import A from './a.graphql'
# import B from './b.graphql'
# import C from './c.graphql'

Expected output from importSchema('./index.graphql'):

type A {
  b: B
  correct: String
}

type B {
  a: A
  c: C
  correct: String
}

type C {
  correct: String
}

Actual output from importSchema('./index.graphql'):

type B {
  a: A
  c: C
  correct: String
}

type A {
  incorrect: String
}

type C {
  correct: String
}

Notice that the incorrect type A is imported in the final schema. This is definitely a bug, not a "feature" in my book. Otherwise, I think the general behavior of automagically pulling in dependencies is ok, as long as it pulls in the correct dependencies. It's possible that just fixing examples 1 and 2 will also fix example 3.

@dortamiguel
Copy link
Author

Hey @ellipticaldoor 👋,

Could you check out the reproduction test that I made here? I tried to follow the CodeSandbox reproductions above but I cannot reproduce the failing behaviour.

https://github.com/prisma/graphql-import/blob/import-specific-fix/fixtures/specific/a.graphql

graphql-import/src/index.test.ts

Line 811 in 35e7f41

test.only('import specific types', t => {

hey! tomorrow I can check it!

@maticzav
Copy link
Collaborator

I'm experiencing the same thing but the problem is made much more apparent when trying to use graphqlgen.

Let's say I have 3 types in my prisma schema and I only want to expose (and write resolvers for) 2 of them in my public API. That is currently not possible with this bug. Grapqlgen is receiving every type from prisma.graphql and then is trying to make me implement resolvers for each.

@ryanquinn3 could you provide a reproduction to the issue you are experiencing? I am not sure I understand your problem. Isn't graphqlgen supposed to scaffold all the types for you? To my understanding, if you imported models from Prisma you wouldn't experience the bugs anymore. Could you verify that?

@maticzav
Copy link
Collaborator

maticzav commented Feb 4, 2019

Hey @amille14 👋,

Thank you for providing such a great overlook of the issue. I am having difficulties reproducing your results. Could you provide a reproduction repository for your cases? What's particularly interesting to me is that there are test cases with almost exactly the same inputs as you provided but resolve in different outcomes.

Example 1

https://github.com/prisma/graphql-import/blob/680f2dde31fb62e0b7ff929d82aa8ebad2067654/src/index.test.ts#L109

https://github.com/prisma/graphql-import/blob/master/fixtures/imports-only/all.graphql

Example 2

https://github.com/prisma/graphql-import/blob/680f2dde31fb62e0b7ff929d82aa8ebad2067654/src/index.test.ts#L142

https://github.com/prisma/graphql-import/tree/master/fixtures/field-types

Example 3

Example 3 occurs due to an unordered definition pool.


All in all, I believe this issue isn't as trivial as it might appear at first sight. My suggestion at this point is to reform the way graphql-import works; in particular, how dependencies are imported. Currently, if a dependency depends on a type with the same name as the one defined in the root schema, bundle resolves the collision very severely. We could fix this by introducing "import-scoped" definitions - instead of trying to merge types with the same name we differentiate between dependencies and defined types.

input

# schema.graphql

# import type Car from './a.graphql'

type Query {
  cars: [Car!]!
}

type Wheel {
  id: ID!
  name: String!
  description: String!
  price: Int!
}

# a.graphql

type Car {
  id: ID!
  model: String!
  wheels: [Wheel!]!
}

type Wheel {
  id: ID!
  color: String!
}

output

type Query {
  cars: [Car!]!
}

type Wheel {
  id: ID!
  name: String!
  description: String!
  price: Int!
}

type Car {
  id: ID!
  model: String!
  wheels: [AWheel!]!
}

# AWheel specific to imported Car type
type AWheel {
  id: ID!
  color: String!
}

@amille14
Copy link

amille14 commented Feb 5, 2019

Hey @maticzav. I just created this repro repository with tests https://github.com/amille14/graphql-import-tests (run yarn test)

As you can see, the first 3 tests are failing. These correspond to the 3 example cases I gave in my previous post. I added a 4th example case to show that # import Query.* from ... is working properly. It's only specific imports like # import User from ... that are not.

@ardatan
Copy link
Owner

ardatan commented Jan 1, 2020

Hi @ellipticaldoor and @amille14 !
In 1.0.0 beta release, we introduced a lot of changes including this enhancement;
Could you install graphql-import@beta to try new changes? Don't forget to modify your code regarding to the migration notes in README.
https://github.com/ardatan/graphql-import#updating-from-07x

@ardatan
Copy link
Owner

ardatan commented Mar 17, 2020

Available in 1.0.0!

@ardatan ardatan closed this as completed Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants