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

Feature Request/Unexpected behaviour: Graphql handlers just return string ids instead of relationships #99

Open
phryneas opened this issue Jun 8, 2021 · 3 comments · May be fixed by #110

Comments

@phryneas
Copy link
Contributor

phryneas commented Jun 8, 2021

I just noticed that over in #96 while adding tests.

If I were to add a second model "Post" with a oneOf relationship "author" to "User", that would still be output as String instead of referencing the User type, which would be expected of a graphql api.

This is not a problem of my other PR, but a conceptual problem with toHandlers. It would probably have to be part of Factory instead of being part of Model, to handle these spanning relationships and quite some additional work on getGraphQLType, which is very simple right now:

export function getGraphQLType(value: any) {
const resolvedValue = typeof value === 'function' ? value() : value
switch (resolvedValue.constructor.name) {
case 'Number':
return GraphQLInt
case 'Boolean':
return GraphQLBoolean
default:
return GraphQLString
}
}

@kettanaito
Copy link
Member

Hey, @phryneas. You're right, the current GraphQL type of a relation would be reduced to a primitive, which doesn't produce a valid GraphQL schema in the end:

factory({
  user: {
    post: oneOf('post')
  },
  post: { ... }
})
type User {
-  post: String
+  post: Post
}

I can't say from the top of my head if the schema generation logic can determine a value being a relational node to infer its type, or we'd have to extend the logic to have access to relational nodes.

Please, would you be interested in tackling this? I'd be happy to help with code reviews in the process.

@phryneas
Copy link
Contributor Author

phryneas commented Jun 9, 2021

I don't really have the time for that as I have tons of open issues to work on myself :/

But if I find one or two hours I'll dig into the code - maybe it's less work than I'm imagining.

But otherwise, if someone else sees this issue and thinks "Hey, I can do this" - take it! :D

I don't really need this right now (although it would be nice in our examples), I just noticed it and wanted to put it here as a suggestion for the roadmap - with whichever priority you give it in the end :)

@kettanaito
Copy link
Member

kettanaito commented Jul 26, 2021

Since a model may reference other models (relational properties), it's no longer possible to generate a complete GraphQL schema from a single given model (it may depend on other models).

In other words, given a factory like this:

const db = factory({
  user: {
    id: primaryKey(datatype.uuid),
    role: oneOf('role'),
  },
  role: {
    id: primaryKey(datatype.uuid),
    name: String,
  },
})

Generating the GraphQL types via db.user.toGraphQLSchema() become a no-op in the current implementation, as in order to type the role field of the User type the library needs to generate the Role type of an unrelated model definition.

As I've suggested above, it looks like the library needs to have access to the entire dictionary to generate a complete GraphQL schema for any given model. At first, I thought about optimizing the generation process by analyzing model dependencies a single dependency has (what models it references, like user above references the role model), but I came to realize that'd be a bad idea. Models may depend on other models, and now we're compiling a model dependency tree and it's likely to be a mess that results in the entire dictionary being converted into a GraphQL schema anyway.

To summarize, it looks like this change implies the internal change:

-generateGraphQLSchema(model): GraphQLSchema
# We'd have to expose the dictionary and generate GraphQL types for all models.
+generateGraphQLSchema(model, dictionary): GraphQLSchema

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants