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

Unhandled errors and wrong types in the context #29

Closed
alireza-ahmadi opened this issue Apr 21, 2021 · 2 comments · Fixed by #156
Closed

Unhandled errors and wrong types in the context #29

alireza-ahmadi opened this issue Apr 21, 2021 · 2 comments · Fixed by #156

Comments

@alireza-ahmadi
Copy link

With the current implementation, the demo server (and also any attempt for running this locally) returns An unexpected error has occurred..

I did a quick investigation and found out that this error is a result of a combination of issues on both the client and server.

Error Handling

const [{ fetching, data }] = useViewrQuery()

Here, we are ignoring the error object, therefore there can be cases where fetching === false and data is not an empty object, but data is not the object that it should be; As a result, React would try to render the Viewer component and would raise an unhandled exception because data.viewer is null.

To solve this issue, we can choose a more defensive approach and make sure that error is empty and data.viewer isn't null.

Auth (server-side)

If the user is not authenticated, the token variable in the line below would be null:

const token = (await getToken({ req, secret })) as JwtToken

The problem is viewer resolver (code below), assumes that id is always present; Therefore, the resolver tries to invoke findUnique method with the id variable from context, which would be undefined for unauthenticated users. In absence of the id, the server returns an error to the client, expressing the condition that passed to the findUnique method is wrong.

t.field('viewer', {
type: 'User',
resolve: async (_, {}, { prisma, id }) => {
return prisma.user.findUnique({ where: { id } })
},
})

A better implementation would be, again, choosing a more defensive approach and informing the client that an unauthenticated user cannot request for viewer's info, by raising a custom error.


I'd be glad if you comment on the mentioned problems. Also, I'm not aware of your plans for this boilerplate but, if you think that these problems should be fixed, I'd be happy to send a PR.

And BTW, thanks for the boilerplate.

@huv1k
Copy link
Owner

huv1k commented May 3, 2021

Hey @alireza-ahmadi,
thanks for the thorough feedback! Sorry for the late response, I was a little bit busy lately.

Error Handling

Yes, we should choose a more defensive approach, which you proposing to make more sense. I just took a shortcut for the first iteration. There should be handling for null.

Auth (server-side)

Yes, like you mention it's the wrong implementation, I wanted to go again with a short path and ignore handling these states. It makes 100% sense to have this in the production-ready application. This boilerplate was made from my side project and I want to keep it updated! We should have id as optional param and properly handle this state in resolvers.

If you have free time please contribute! I would like to keep it up to date, but it's hard to keep with all of my stuff going on right now.

@alireza-ahmadi
Copy link
Author

Hey @huv1k,
Thanks for the response.

I understand that this is a boilerplate hence, there is no need for handling every single state.

However, IMHO, the online demo and first attempt for running the boilerplate are somehow the first impressions of a developer from working with this codebase and, facing an unhandled exception right off the bat can prevent them from trusting this boilerplate, even though code quality and structure is great. So, while I explained the issue in detail to clarify it, my suggestion is to make the demo work without implementing a complex error handling solution or trying to handle every single state.

I will take a look at the codebase and, if time lets me, will send a PR in the next few days.

huv1k added a commit that referenced this issue Jul 31, 2022
👋 Hey strangers,

this PR updates this template with a couple of changes. 

## Schema builder
I decided to switch to [Pothos GraphQL](https://github.com/hayes/pothos) instead of Nexus, because there is [no future](graphql-nexus/nexus-plugin-prisma#1039 (comment)) for `nexus-prisma`, so it doesn't make sense to keep it inside this template. @hayes is doing a great job with Pothos and everything about it is good 🙌

## Move to JWT
I moved the session strategy to `JWT` so it's prepared for edge computing and Next.js middleware.

## New database
I decided to move to [PlanetScale](https://planetscale.com), they provide really generous free tier. They are a new cool kid in the block with database branching which is a perfect feature. If this doesn't suit, you can still you another database that Prisma supports. 

## Fixes
I fixed some problems with types and now it should work as expected. When `JWT` token is not presented, there is a fallback to `userId` and `userRole`. Fixes #29
realparadise pushed a commit to realparadise/next-auth-prisma that referenced this issue Jun 11, 2024
👋 Hey strangers,

this PR updates this template with a couple of changes. 

## Schema builder
I decided to switch to [Pothos GraphQL](https://github.com/hayes/pothos) instead of Nexus, because there is [no future](graphql-nexus/nexus-plugin-prisma#1039 (comment)) for `nexus-prisma`, so it doesn't make sense to keep it inside this template. @hayes is doing a great job with Pothos and everything about it is good 🙌

## Move to JWT
I moved the session strategy to `JWT` so it's prepared for edge computing and Next.js middleware.

## New database
I decided to move to [PlanetScale](https://planetscale.com), they provide really generous free tier. They are a new cool kid in the block with database branching which is a perfect feature. If this doesn't suit, you can still you another database that Prisma supports. 

## Fixes
I fixed some problems with types and now it should work as expected. When `JWT` token is not presented, there is a fallback to `userId` and `userRole`. Fixes huv1k/nextjs-auth-prisma#29
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 a pull request may close this issue.

2 participants