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

Scaffold should not generate autogenerated fields #3783

Merged
merged 14 commits into from
Apr 20, 2022

Conversation

aggmoulik
Copy link
Contributor

@aggmoulik aggmoulik commented Nov 27, 2021

Fix #3682

While creating the smart way of filtering out autogenerated fields, I found there are certain functions that are used with the default function in Prisma schema that is autogenerated, and obviously we should not generate those fields as well.

Primsa Default Function

Signed-off-by: Moulik Aggarwal qwertymoulik@gmail.com

Signed-off-by: Moulik Aggarwal <qwertymoulik@gmail.com>
Signed-off-by: Moulik Aggarwal <qwertymoulik@gmail.com>
@thedavidprice thedavidprice added this to the next-release-priority milestone Dec 2, 2021
@redwoodjs-bot redwoodjs-bot bot moved this from New issues to In progress (priority) in Current-Release-Sprint Dec 2, 2021
@jtoar jtoar self-requested a review December 9, 2021 23:45
@thedavidprice
Copy link
Contributor

Ah, good catch @aggmoulik

Ping'ing @cannikin for review.

Thanks!

@thedavidprice thedavidprice moved this from In progress (priority) to v0.41 in Current-Release-Sprint Dec 17, 2021
@cannikin
Copy link
Member

Hmm, I'm not sure we always want to exclude all of these fields: just because the field is assigned a default value in the database doesn't mean you'll never provide a value to those fields. Just that if you do leave it NULL then it should automatically be given a value. For example, you may have a system that automatically posts to Twitter. There is a field named sendAt that you can customize if you want, for example you want to send a message an hour from now. But if you leave it blank, it should send right away (because the database will default to now()).

I could see something similar for cuid and uuid: they would act as unique identifiers for a record (say the slug for a blog post) unless you assign a unique identifier yourself.

Some folks may even set their own id value and not always rely on the database to generate one for them. We've kind of developed a standard in Redwood: every table should have an id column that is autogenerated, and optionally createdAt and updatedAt columns that are managed by the database (createdAt) and Prisma (updatedAt). (These are borrowed from Ruby on Rails). I feel like anything else should be fair game and should show in a form.

@jtoar jtoar removed their request for review December 17, 2021 17:26
@aggmoulik
Copy link
Contributor Author

Hey @cannikin, I understand your concerns here. I will remove the now(), cuid() and uuid() but I think we should exclude the following then:

  id Int @id @default(autoincrement()) // Already excluded in redwood
  uuid String @default(uuid()) // Will remove 
  cuid String @default(cuid()) // Will remove 
  autoincrementNotID Int @default(autoincrement()) // This case is not handled
  dbGeneratedField String @default(dbgenerated()) // this should be also excluded

I have tried to make it modular so that any change like this is automatically handled just by removing from constants.

@cannikin
Copy link
Member

I don't think we should exclude fields marked as dbgenerated(): it only gets a default value if you don't specify one. It's just that the default value is managed by the database because Prisma doesn't understand what the value is. Prisma gives an example in their docs of the database populating the field with a random value. But maybe I do want to give it a value sometimes, so it would need to be in a generated scaffold.

@cannikin
Copy link
Member

cannikin commented Jan 6, 2022

Howdy @aggmoulik hope you had a good holiday!

What did you think about my last comment?

@aggmoulik
Copy link
Contributor Author

Howdy @aggmoulik hope you had a good holiday!

What did you think about my last comment?

Hello @cannikin Sorry for the late reply, as was stuck in personal problems. Will fix the PR after looking at your comments.

@aggmoulik
Copy link
Contributor Author

@cannikin Updated the PR after your review.

@jtoar jtoar removed this from the next-release-priority milestone Jan 30, 2022
@thedavidprice thedavidprice added the release:fix This PR is a fix label Jan 30, 2022
@cannikin
Copy link
Member

Thanks! Are there any tests that can be updated to show that these fields are no longer generated?

@jtoar jtoar changed the title Scaffold should not generated autogenerated fields Scaffold should not generate autogenerated fields Feb 15, 2022
@cannikin
Copy link
Member

Hi @aggmoulik did you see my previous comment? This PR looks good but I'd love to get a test or two in there that confirms it's behaving as expected!

@aggmoulik
Copy link
Contributor Author

@cannikin Ok I will add tests for the same.

@netlify
Copy link

netlify bot commented Apr 6, 2022

Deploy Preview for redwoodjs-docs canceled.

Name Link
🔨 Latest commit 9544743
🔍 Latest deploy log https://app.netlify.com/sites/redwoodjs-docs/deploys/626055f90ab0c800085e63cb

@jtoar
Copy link
Contributor

jtoar commented Apr 15, 2022

Hey @aggmoulik, no worries if not, but still interested in this one? If not we'll get it to the finish line! Just wanted to give you the chance since you worked so hard on it 🦾

@cannikin cannikin merged commit 07c0b75 into redwoodjs:main Apr 20, 2022
@cannikin
Copy link
Member

Thanks for the work on this @aggmoulik, you get all the credit!

@jtoar jtoar added this to the next-release milestone Apr 20, 2022
@thedavidprice thedavidprice modified the milestones: next-release, v1.2.0 Apr 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Generate scaffolding should not include field using prisma's @updatedAt flag
4 participants