graphile / crystal

🔮 Graphile's Crystal Monorepo; home to Grafast, PostGraphile, pg-introspection, pg-sql2 and much more!
https://graphile.org/
Other
12.54k stars 569 forks source link

Aliasing undesirable field names #34

Closed calebmer closed 8 years ago

calebmer commented 8 years ago

It seems like what a lot of people find undesirable about PostGraphQL is how fields are named. How can we build a system (whether it be a JSON map or otherwise) which allows people to rename automatically named fields?

Any ideas are welcome, I'm have no ideas at the moment.

tobiasmuehl commented 8 years ago

How about implementing the alias by reflection over the GraphQL schema? Basically just copy everything but the name. Since the schema and the types can easily be queried using__schema and __types it's easy and doesn't impact performance as the resolving functions are identical. This feature could be included in the GraphQL-JS library seperate from PostGraphQL.

My idea is to implement a ES6 Weak Map so it's possible to work with the type/query-object directly without stringifying them. Probably a bad idea since Weak Maps can't easily be looped over. Probably better to use an Array.

So the dev needs to create empty Types/Queries (empty as in only name is set but no fields or resolvers), and then pass both source and target to the Weak Map that's argument of some function. This way it's possible to customize the new endpoint with virtual fields that don't require a trip to Postgres. The aliases can be kept in a different file from the generated schema as for easy SQL schema updates.

I'm unsure when this should happen. At Postgres reflection time, node.js server initalizing, sowhere in between?

Edit: I just noticed that it could also be possible to copy fields and methods from the referenced GraphQL objects directly within JavaScript without actually querying the GraphQL server.

ghost commented 8 years ago

Isn't this what you're looking for?

If so, then a simple JSON map passed in might be enough for us.

tobiasmuehl commented 8 years ago

@burkhardr No, that's not what where are trying to achieve. We want to alias the Query-definitions, because the Relay-compliant schemas aren't that human friendly. Example: Alias users[0].posts[0].name means usersNodes[0].edges[0].node.postsNodesByUserId.edges[0].name

ghost commented 8 years ago

Well, you could do that server-side, but the GraphQL connections / edges are named like that for a reason. The link I posted above allows you to do aliasing on the client-side, which - while not as convenient - is definitely the easier solution.

tobiasmuehl commented 8 years ago

This issue discusses a server side implementation. The thing with the alias in the query is that it's just working on one level.

With the built-in alias the only thing that actually happens is that the returned key has a different name, paths can't be simplified however. The primary function for alias is to be able to request and return multiple instances in one request.

By design this GraphQL query is impossible to fulfill:

{ 
  User(id: 1) {
    name,
    age
  },
  User(id: 2) {
    name,
    email
  }
}

Because it's impossible to return both objects on the same User key.

The aim of the discussed alias function is to make querying nicer, the builtin function just complicates the query as it's longer:

{ 
  User1: User(id: 1) {
    name,
    age
  },
  User2: User(id: 2) {
    name,
    email
  }
}
ghost commented 8 years ago

If you think of GraphQL as a straight REST replacement, then data structures like users[0].posts[0].name make perfect sense. Unfortunately, we're dealing with a graph here, so you won't be able to simplify things like edges and nodes away without losing some of the functionality they provide (e.g. pageInfo).

If you're willing to sacrifice that for added simplicity, then this feature should be strictly opt-in.

tobiasmuehl commented 8 years ago

I don't understand why the feature should be opt-in. The aliases have to be defined manually and they don't break the generated schema as they only offer a reference to the generated schema.

Can you elaborate?

calebmer commented 8 years ago

I finding a better name for fields like postNodesByAuthorId on the server is fine by me because I feel that's what a lot of people find issue with.

What do you think about using constraint names for these field names? So if the reference to person from the author_id column was named author our forward query name could be simply author and our reverse query name could be postNodesByAuthor.

Making connections optional is something I've discussed with @rattrayalex in #16. I don't really like the idea because it duplicates a lot of logic, but if enough people articulate a want/need I'll reconsider. I really think solving the postNodesByAuthorId naming solution is the best compromise.

ghost commented 8 years ago

Connections themselves shouldn't be optional. Inferring the name of the connection from the FK/column name makes sense to me, but you'll still not arrive at something like users[0].posts[0].name (you'll always have edges[], node, etc. for connections).

Sorry for the confusion. My sole argument was around not being able to boil a graph down to the same format you'd get from a classical REST endpoint without sacrificing some of its functionality.

calebmer commented 8 years ago

Closing in favor of a discussion in #42.