Redwood: Relation support

[gfpacheco]

Hi, I just tried Redwood for a new personal project.

At first I thought this could be the one boilerplate to rule them all, lots of great stuff, very organized.
I went through the entire tutorial, it's great btw.

But the illusion ended when I tried to create my first relation.

I wrote my prisma schema like this:

model Teacher {
  id Int @id @default(autoincrement())
  name String
  courses Course[]
}

model Course {
  id Int @id @default(autoincrement())
  name String
  teacher Teacher
}

And after running yarn rw db save I saw that Prisma generates the db field without the Id suffix, but that's just Prisma's fault, I guess I can live with that.

Then I ran yarn rw g scaffold for both teacher and course models and realized the generated GraphQL schema was like this:

  type Teacher {
    id: Int!
    name: String!
    courses: Course
  }

  type Course {
    id: Int!
    name: String!
    teacher: Teacher
  }

Not only teacher's courses field wasn't an array, both teacher's courses and course's teacher fields didn't have ! as expected since they didn't have ? on Prisma schema. So I went and fixed them (together with their inputs).

Then I went to the GraphQL Playground and created a teacher, so far so good, until I tried to create a course and got a really weird error, very hard to read (because it was coming from prisma), but I managed to identify: the createCourse function created by the scaffold was like this:

export const createCourse = ({ input }) => {
  return db.course.create({
    data: input,
  })
}

But needed to be like this:

export const createCourse = ({ input: { teacher, ...input } }) => {
  return db.course.create({
    data: { ...input, teacher: { connect: { id: teacher } } },
  })
}

My next disappointment was with the course query, of course querying the teacher field from course wouldn't work since the resolver only gets the course from the database and there's no resolver for the teacher field (the same happens with the courses field on Teacher).

PS: I still don't know how to create a field resolver, but I'll ask somewhere else.

Not to mention the web part, where teacher queries all have the courses fields but without any subfields (the same for Course's teacher field) and the forms have a Int field for each relation field.

I'm sorry it might seem I'm just saying bad things about a open source project, I really appreciate your efforts, I'm gonna keep using Redwood and try helping as much as I can. I just hoped there was anything in the docs saying relations are a work in progress or something.

PS: I have a few years of Node and GraphQL experience (not so much with tooling though), would be happy to tell how I (as a developer) would expect things to work, and maybe help with the implementation.

[cannikin]

We got Redwood to the point of being able to run through the tutorial as written, anything else (like automatically creating these relations) is being worked on by contributors like you! 😀 Want to help us out?

Although scaffolds will probably never be built out to the point of adding forms for relationships, the SDL and service generators should be able to introspect and build them when possible.

[gfpacheco]

I imagined that, but yes, I wanna help. I don't have much free time, but I'll try!

[weaversam8]

@gfpacheco @cannikin I've been stumbling through my own schema and dealing with issues with relations, and was planning on narrowing the problems down into a few issues. (Planning on posting some tonight.) Maybe this issue can serve as an "EPIC" for relation support as a whole.

[RobertBroersma]

I've found that exporting Model from a service file allows you to define field resolvers, since redwood will automatically pick them up just like Query and Mutation resolvers.

I was just playing a bit, defining Model resolvers in their matching service, resolving all relation fields. E.g:

// api/src/services/user.js
export const User = {
  posts: (_obj, { root }) =>
    db.user.findOne({ where: { id: root.id } }).posts(),
}

// api/src/services/post.js
export const Post = {
  owner: (_obj, { root }) =>
    db.post.findOne({ where: { id: root.id } }).owner(),
}

The pattern is simple! Something like this:

// api/src/services/<modelName>.js
export const <ModelName> = {
  <relationField>: db.<modelName>.findOne({ where: { id: root.id } }).<relationField>(),
}

What do you think?

[cannikin]

@RobertBroersma so you're saying with the syntax above this query would work?

query FIND_POST_BY_ID($id: Int!) {
  post: post(id: $id) {
    id
    title
    owner {
      name
    }
  }
}

@peterp Is this possible? These constants User and Post are being automatically imported and everything would magically work?

[weaversam8]

Nice find @RobertBroersma!

@cannikin - I haven't tested this locally, but it makes sense that this would work. I kinda like this as a preferred solution to #324 compared to what I proposed.

Edit: I prefer this as a solution because this should _actually_ lazy load relations, instead of ham-handedly fetching all relations when fetching a single model instance, like I proposed in this comment.

[RobertBroersma]

@cannikin Haven't tested that query, but yes I think that should work! I _have_ tested this query:

query {
  posts {
    title
    owner {
      name
    }
  }
}

Which works! Of course atm you'd need to manually adjust the geneated .sdl.js file to include the relations!

[thedavidprice]

Hey, all you geniuses! (..._maybe_ including @cannikin 😉)

When/if this is ironed out, it would be above and beyond helpful to have a topic about this (with an example) in the forum "Troubleshooting & How To".

Pretty please 🙏

[RobertBroersma]

I was playing around a bit more and had another thought. You can just re-use the service already defined:

// api/services/post/post.js
export const post = ({ id }, { context }) => {
  return db.post.findOne({
    where: { id },
  })
}

export const Post = {
  tags: (args, { root, ...rest}) => post(root, rest).tags(args),
  owner: (args, { root, ...rest}) => post(root, rest).owner(args),
}

I'm not sure on that arg forwarding syntax... but the added benefit is that you can do Authorization stuff in one place (post()). Does that make sense?

I'm quite new to Prisma; calling .findOne() multiple times doesn't cause any extra database work until it's resolved, either by itself or with a sub query, right?

[JohnMilazzo]

Link to forum topic:

https://community.redwoodjs.com/t/scaffold-generator-for-prisma-relations/207

[thedavidprice]

Thanks @JohnMilazzo I did link from that topic to here as you saw.

Once this discussion resolves to a few good solutions/patterns/examles, I’d like to create a forum Topic in the “How To” that leads with the examples as a starting place. Make sense?

[weaversam8]

@JohnMilazzo @thedavidprice making a "How To" topic seems wise, but I'd also really like to close #324 with a PR for this in the near term, depending on my bandwidth and what we end up deciding re @RobertBroersma's suggestions.

---

@RobertBroersma - I'm not sure if Prisma caches the calls to findOne(...) or not... that's an open question.

[cannikin]

Now, who wants to update the SDL/services generator to do this automatically? :)

[weaversam8]

@cannikin that what I meant when I said above that I'd like to close #324 with a PR for this :)

Assuming I have the bandwidth and can figure it out I'm happy to take this one

[cannikin]

@weaversam8 YES PLEASE!! 😀😀😀

[cannikin]

If anyone is still looking at this, we got a note from the Prisma team:

Hey folks 👋 another heads-up: A few more upcoming major breaking changes:

Renaming the prisma2 CLI command to prisma

The command that invokes the Prisma 2 CLI will be renamed to just prisma. The recommended installation and usage is:

npm install @prisma/cli --save-dev
npx prisma

New relation syntax

Previously relations could be declared with two relation fields where one was "virtual", the other one "represented the foreign key", e.g.:

model Post {
  id        Int     @id @default(autoincrement())
  title     String
  content   String?
  published Boolean @default(false)
  author    User?
}

model User {
  id    Int     @id @default(autoincrement())
  email String  @unique
  name  String?
  posts Post[]
}

After the next release, you will need to annotate author with the @relation attribute. In the @relation attribute you must specify a "link field" on the same model which references a key on another model (data modeling gets closer to SQL now):

model Post {
  id        Int     @id @default(autoincrement())
  title     String
  content   String?
  published Boolean @default(false)
  author    User?   @relation(fields:  [authorId], references: [id]) // physical relation field
  authorId  Int?    // link field
}

model User {
  id    Int     @id @default(autoincrement())
  email String  @unique
  name  String?
  posts Post[]  // virtual relation field
}

We already updated our new docs to incorporate these changes, you can take a look here: https://prisma2.netlify.com/reference/tools-and-interfaces/prisma-schema/relations

[weaversam8]

Thanks for the heads up @cannikin! Any idea if this beta version is available for testing anywhere?

[cannikin]

Yup it was released yesterday! https://github.com/prisma/prisma2/releases/tag/2.0.0-preview025

[thedavidprice]

@weaversam8 Just to clarify since there was some confusion on our end:

• Prisma2 preview025 was released last week and has been merged into Redwood per #350. However, we have not yet published a new version of Redwood including this change.
• Prisma2 beta will be coming next with the breaking changes that Rob mentioned above. We do know now about a release version available now for testing. (I'm assuming the Prisma2 master is what will be released as beta.)

On our end, we'll be taking our time with testing, fixes, and meeting requirements before merging into Redwood for release. As Rob mentioned, here are the Prisma2 docs for Relations in the beta:
https://prisma2.netlify.com/reference/tools-and-interfaces/prisma-schema/relations

[thedavidprice]

Prisma 2 Beta was released today #365

There’s a lot of specific updates and changes regarding Relations. @weaversam8 are you still interested in taking a look at helping with this? We’d appreciate it if possible!

[weaversam8]

I might be able to help out with some documentation / issue writing / testing, but I think my bandwidth for doing an actual implementation PR for the generation stuff has been shifted... I'm being pulled onto some COVID-related projects, and understandably that has to take priority right now. Apologies 😢

[gfpacheco]

I'll take a look at it.

I already upgraded prisma to beta.1 locally and now am trying to fix the generated sdl and service files.

Quick question though: why did you make all input fields optional while they're required on the types?

[thedavidprice]

@peterp See question above. I'm guessing this just has to do with _time_, specifically not enough of it. Any other suggestions and/or directions here to help get things going?

[cannikin]

Quick question though: why did you make all input fields optional while they're required on the types?

My thought was that you may have instances where you only allow someone to change a subset of all the available fields, and we shouldn't require you to include all of them if you only need to change some. Unless my understanding of GraphQL required types is wrong...but if you have two fields title and body in PostInput, wouldn't you then have to always pass along title and body to any and all mutations?

[gfpacheco]

Quick question though: why did you make all input fields optional while they're required on the types?

My thought was that you may have instances where you only allow someone to change a subset of all the available fields, and we shouldn't require you to include all of them if you only need to change some. Unless my understanding of GraphQL required types is wrong...but if you have two fields title and body in PostInput, wouldn't you then have to always pass along title and body to any and all mutations?

You're right, but that's assuming someone would use the same mutation to update different parts of an entity. And by doing so you're allowing the client to send null for fields that should be required (even to the create mutation that should get all required fields).

IMHO, if the user wants to update parts of an entity there should be a mutation just for that, and all the fields (that are required on the model) should be required on each of the inputs.

Redwood's generators seems to be the starting point from where people can build their own custom api. And I don't think making required fields optional on inputs by default is helping with that.

[weaversam8]

@gfpacheco I think having a single mutation to update individual fields on an entity probably makes sense. I think the real problem here is:

by [making input fields optional while they're required on the types] you're allowing the client to send null for the fields that should be required (even to the create mutation that should get all required fields).

(emphasis mine) I don't think that's acceptable...

@cannikin - is a good middle ground having a separate input for update vs create? It would make the generated SDL a bit more complicated, but I don't think that is too much of a tradeoff for the guaranteed correctness of the create mutation.

[gfpacheco]

I understand that's the initial thought for almost everyone, to try to bring the CRUD concepts for GraphQL. But my personal experience with it proves that idea wrong.

GraphQL is all about the client. And to make that experience better the client must be sure what the api expects just by reading the schema.

If a mutation's argument is optional, you expect to be able to not send it (or send null). And it's very frustrating to follow the schema and still get error because you didn't send an optional argument that is actually required by the database.

My 3 year experience with GraphQL tells me to create different mutations for different purposes. If I want to mutate a model in two different ways there should be two different mutations. Each one requiring all the necessary fields for it to succeed.

It's not only easier to maintain, but easier to use the api. I can give you lots of examples for each of my points.

OT: my experience also tells me that every mutation should return a different type, like MyMutationResponse that actually have the entities that were changed. That allows a single mutation to change more than one entity and return all of them (if the clients wants to). That is specially good on the long run, where you'll eventually change your code, do more stuff, etc.

[weaversam8]

If a mutation's argument is optional, you expect to be able to not send it (or send null). And it's very frustrating to follow the schema and still get error because you didn't send an optional argument that is actually required by the database.

@gfpacheco - I think you misinterpreted what I suggested above.

I don't think that having a schema that is inconsistent with reality is acceptable. I proposed creating a separate input for the create and update mutations to resolve this problem. I see fixing this issue to be important in order to satisfy the "contract" that the GraphQL SDL provides.

---

Separate from this is the debate about having a single multi-purpose mutation for update vs. multiple single-purpose mutations. Above, you suggest that we should create different mutations for each different property. I think that makes sense, but for the purposes of basic, generated SDL that is only supposed to serve as a starting point, I think it's acceptable to create a single update mutation, instead of one for each field.

Yes, it might be shoehorning CRUD concepts into GraphQL a bit, but I think that building a good developer experience for users who are starting out using the framework is really important, and bridging that gap through this sort of "pseudo-crud" concept is an acceptable way to do that, with the caveat that some (if not most) projects should eventually switch to a single-purpose mutation strategy. (In fact, if we took this approach, we could and should write some docs explaining this distinction, and why the scaffolded code is only a starting point and should eventually be modified.)

[gfpacheco]

@weaversam8 - Sorry, maybe I did misinterpreted your point there, but maybe I wasn't clear about mine either.

I suggest we make the Input fields just as required as the type fields.

If a GraphQL schema is type User { name: String! } I expect the name field to be required in any User input, not only on creation.

Making the input optional allows you to not send that field if you don't want it to be updated, and although I think that good it also allows the user to send null, and that will break even if it passes GraphQL validation.

I rather enforce the input requirement (for the sake of rule vs exception) unless the user really knows what he's doing (in that case he should do it himself).

[weaversam8]

Ah, I see your point now, having a field be non-optional on the input for the update mutation would permit someone to intentionally set an otherwise required field to null.

In the realm of complete correctness, your proposed solution would be necessary to ensure that the GraphQL is a complete and totally true representation of the constraints on the data.

---

That said, I'm leaning towards saying that the above scenario (setting a required field to null through an update) is enough of an edge-case that it's worth simplifying the SDL for the purpose of leaving it easy to understand for users who are getting introduced to GraphQL for the first time.

My reasoning behind this is: since the scaffolding generator is never going to be completely feature complete, we should optimize for correctness up until the point where it becomes more difficult for new users to understand at a quick read. At that point, we should stop, lest we make the framework less approachable.

I personally think that if we don't generate code that is _completely_ perfect with the scaffolding generator, it's acceptable as a finished product IFF we have good docs explaining why our code is _technically_ incorrect, where it can fail, and how people can build a more robust SDL + service.

Interested to get the perspective of the core team (@cannikin mostly, but @thedavidprice + others are welcome of course!) on this (the tradeoff specifically, and where we should draw that line.)

[gfpacheco]

I understand your point as well, leaving the fields option on update allows the user to break the form and only update what he wants at a time. I really like that, and if everyone else prefers that way I'll never talk about it again.

But I can't keep myself from explaining everything that made me choose the other way: I still think that fields (that are required in the model) should be required by default on input.

1. I think that dev wanting to break a model's form and update different parts of it in different pages is enough exception to the rule and they should know what they're doing and the tradeoffs of making input fields optionals.

2. Currently if I try to create a User without sending the name (using my schema above) this is the error message I get:

"\n\u001b[31mInvalid \u001b[1m`prisma.user.create()`\u001b[22m invocation in\u001b[39m\n\u001b[31m\u001b[4m/Users/guilhermepacheco/Workspace/test-app/api/src/services/users/users.js:34:22\u001b[24m\u001b[39m\n\n\n\u001b[31mInvalid \u001b[1m`prisma.user.create()`\u001b[22m invocation:\u001b[39m\n\n{\n  data: {\n\u001b[92m+   name: String,\u001b[39m\n\u001b[92m\u001b[39m\u001b[2m\u001b[32m\u001b[2m?   posts?: {\u001b[22m\u001b[2m\u001b[39m\u001b[22m\n\u001b[2m\u001b[32m\u001b[2m?     create?: PostCreateWithoutAuthorInput,\u001b[22m\u001b[2m\u001b[39m\u001b[22m\n\u001b[2m\u001b[32m\u001b[2m?     connect?: PostWhereUniqueInput\u001b[22m\u001b[2m\u001b[39m\u001b[22m\n\u001b[2m\u001b[32m\u001b[2m?   }\u001b[22m\u001b[2m\u001b[39m\u001b[22m\n\u001b[2m\u001b[32m\u001b[39m\u001b[22m  }\u001b[2m\u001b[22m\n\u001b[2m\u001b[22m}\n\nArgument \u001b[92mname\u001b[39m for \u001b[1mdata.name\u001b[22m is missing.\n\n\u001b[2mNote: Lines with \u001b[22m\u001b[0m\u001b[92m+\u001b[39m\u001b[0m \u001b[2mare required\u001b[22m\u001b[2m, lines with \u001b[32m?\u001b[39m are optional\u001b[22m\u001b[2m.\u001b[22m\n"

While this is the message I would get if the field were required on the input:

"Field UserInput.name of required type String! was not provided."

---

Currently we're making it easier for devs to do something that is not really standard stuff, but making the schema "incorrect" by doing so.

I propose we make sure the schema is correct for most people, and if users need to break down entity updates in different requests they can even choose between making fields optional or go for a multiple single-purpose mutation strategy.

[weaversam8]

@gfpacheco - just to clarify again, I'm not advocating we keep the incorrect schema on the create input, I'm suggesting we use two inputs, one for create and one for update, and have the update input be the incorrect one.

(The update input would be identical to the current single input, and the new create input would have the appropriate fields required.)

This would fix your above error on create (where I see the most potential for users leaving out required fields) and only leave potential for invalid data in update.