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

Support for auth() in @default attribute #958

Merged
merged 14 commits into from Jan 26, 2024

Conversation

Azzerty23
Copy link
Contributor

Hi, I'm trying to implement this feature: #310

It was an excellent opportunity to get hands-on experience with the codebase.

I believe I managed to get something working. But when I run the last 2 tests of auth.test.ts in isolation, they pass. However, if I run all the tests in the file, they do not. Could you help me figure out what I missed, @ymc9, @jiashengguo, please?

Also, I did not succeed in having the @default(auth()) comments in the generated Prisma schema (I simply omitted them).

Here is the approach I followed:

  • Retrieving and inserting values from the user context in the new withDefaultAuth() proxy. I did not fetch user data from the Prisma client. Is it worth a separated proxy?
  • I worked around the existing @default attribute. While creating a new one might have been simpler, I believe this aligns more naturally in a API design perspective.
  • Omitted the generation of @default(auth()) in the Prisma schema, as it is not Prisma compatible.
  • I am considering adding validation for the fields in the provided context (like in withPolicy proxy).

I'd appreciate going further after a quick review to ensure I'm heading in the right direction :)

@Azzerty23 Azzerty23 changed the base branch from main to v2 January 23, 2024 10:01
@ymc9
Copy link
Member

ymc9 commented Jan 23, 2024

Hi @Azzerty23 , you are amazing! Thanks for working on this feature. I've been dreaming of having it for quite a while and was thinking about it yesterday 😄. It'll definitely make the code of all ZenStack users cleaner.

I've skimmed through the change, and the structure looks brilliant. You must have spent quite some time to figure things out , as many pieces of code are not yet thoroughly documented. Really appreciate it!

I'm not sure what happened with the test cases. Let me debug and figure out why. I'll also do a thorough review pass and attach my comments.

@Azzerty23
Copy link
Contributor Author

Thank you so much @ymc9! I must say I'm learning a lot from you :) Let me reply to your comments.

@Azzerty23 Azzerty23 force-pushed the feature/default-auth-attributes branch from 6a2531c to 3311530 Compare January 24, 2024 16:54
ymc9 and others added 5 commits January 26, 2024 15:08
- Generate a function to provide value for fields using `auth()` in `@default` so we don't need to evaluate at runtime
- Correct the way of visiting nested create payload
@ymc9 ymc9 merged commit 2b3cf70 into zenstackhq:v2 Jan 26, 2024
2 checks passed
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.

None yet

2 participants