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

fix(cli): use field name for relations generation #813

Merged
merged 7 commits into from
Jul 15, 2020

Conversation

Rosenberg96
Copy link
Contributor

@Rosenberg96 Rosenberg96 commented Jul 9, 2020

Closes #786

When generating service files, unfortunately it has been missed that one can specify a field name that does not necessarily match the relation name. Tested with the following schema:

schema.prisma

model Profile {
  id          Int    @default(autoincrement()) @id
  name        String
  pictureTest Image  @relation(fields: [imageId], references: [id])
  imageId     String
}

model Image {
  id          String    @default(cuid()) @id
  url         String?
  ProfileTest Profile[]
}

Generated services:

export const Image = {
  ProfileTest: (_obj, { root }) =>
    db.image.findOne({ where: { id: root.id } }).ProfileTest(),
}

export const Profile = {
  pictureTest: (_obj, { root }) =>
    db.profile.findOne({ where: { id: root.id } }).pictureTest(),
}

Generated sdl

type Image {
    id: String!
    url: String
    ProfileTest: [Profile]!
}
type Profile {
    id: Int!
    name: String!
    pictureTest: Image!
    imageId: String!
}

Edit: Corrected the actual output to be correct after migration of DB.

@thedavidprice
Copy link
Contributor

Thanks for this @Rosenberg96! Looping in Rob for review now.

Copy link
Member

@cannikin cannikin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the generated code actually capitalizes the TEST part of the field names like that? That's definitely not intended!

But everything else looks correct to me...Profile -> pictureTest and Image -> ProfileTest. Wouldn't your code change the relations to Profile -> image and Image -> profiles?

@Rosenberg96
Copy link
Contributor Author

Rosenberg96 commented Jul 15, 2020

@cannikin Please ignore the TEST parts of the field names. I had not run db migrations, which made this incorrect (this was my previous field names). I have corrected this in the PR description.

In regards to your other comment I am not sure I follow. This does not change the actual relations between the two models. This is the SQLLite migration:

CREATE TABLE "quaint"."Profile" (
"id" INTEGER NOT NULL  PRIMARY KEY AUTOINCREMENT,"imageId" TEXT NOT NULL  ,"name" TEXT NOT NULL  ,FOREIGN KEY ("imageId") REFERENCES "Image"("id") ON DELETE CASCADE ON UPDATE CASCADE)

CREATE TABLE "quaint"."Image" (
"id" TEXT NOT NULL  ,"url" TEXT   ,
    PRIMARY KEY ("id"))

When re-reviewing this change I do run into the issue when a new Profile is to be created.

| Invalid `prisma.profile.create()` invocation in
api | /redwood-blog/api/src/services/profiles/profiles.js:14:21
api |
api | {
api |   data: {
api |     name: 'fdsfsd',
api |     imageId: 'ckcn8a7wq0000qot73aryy9o3',
api |     ~~~~~~~
api | +   pictureTest: {
api | +     create?: ImageCreateWithoutProfileTestInput,
api | +     connect?: ImageWhereUniqueInput
api | +   }
api |   }
api | }
api |
api | Unknown arg `imageId` in data.imageId for type ProfileCreateInput. Did you mean `name`?
api | Argument pictureTest for data.pictureTest is missing.

I am not too familiar with the web par of the generators, but it is clear that the imageId field should not be used, since this is a readonly property and not the actually relational key. I may suspect this is a known limitation, but for transparency I put this here.

PS: upstream/main is broken at the time of writing. Issue is in cli/commands/diagnostics.js and the @redwoodjs/structure package.

@cannikin
Copy link
Member

When re-reviewing this change I do run into the issue when a new Profile is to be created.

This is a known issue when generating scaffolds with foreign keys. I have an open issue with Prisma about letting us set this field instead of it being read-only.


We currently have an SDL test in place that has a relationship name profiles that's different than the model name UserProfiles: https://github.com/redwoodjs/redwood/blob/main/packages/cli/src/commands/generate/sdl/__tests__/fixtures/schema.prisma#L29

And you can see the text fixture that represents the generated SDL file has the correct type profiles: [UserProfile]!: https://github.com/redwoodjs/redwood/blob/main/packages/cli/src/commands/generate/sdl/__tests__/fixtures/singleWordSdl.js#L9

But shouldn't this be incorrect and your PR is what would fix it? Or is it the generated SERVICE names that are incorrect? I'm so confused! 😅

@Rosenberg96
Copy link
Contributor Author

Rosenberg96 commented Jul 15, 2020

@cannikin This PR fixes the generated service files. Your links point to test cases with the SDL's, which are in fact correct and are generated in the same as this now, since it not just uses the field.name see.

I should have left out the generated SDL from the PR description, but the mismatch was in the services <-> SDL before this change.

@cannikin
Copy link
Member

Ahhhh that makes more sense, thanks for being patient with me. :) Let's merge it!

@cannikin cannikin merged commit dfcb17b into redwoodjs:main Jul 15, 2020
@Rosenberg96 Rosenberg96 deleted the rsb-#786 branch July 15, 2020 18:16
@thedavidprice thedavidprice added this to the next release milestone Jul 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Service generator uses type as name instead of given name for relations
3 participants