blitzjs.com icon indicating copy to clipboard operation
blitzjs.com copied to clipboard

docs(multitenancy): fix example update mutation

Open xkrsz opened this issue 2 years ago • 9 comments

In the current example, the code snippet is invalid. update() is limited to unique model fields, so the only way to execute this call is to add a unique constraint on id and organizationId. There's also updateMany(), but I wouldn't use that as an example as it will scan all table rows and can hurt performance. To my knowledge Prisma doesn't yet support limiting updateMany() to the first updated record.

Do let me know if explanation can be improved and if we should add more examples/info here.

xkrsz avatar Oct 20 '21 18:10 xkrsz

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/blitzjs/blitzjs-com/52BgfyKnrtwrPftEL93BN4ARf47Y
✅ Preview: https://blitzjs-com-git-fork-krszwsk-patch-1-blitzjs.vercel.app

vercel[bot] avatar Oct 20 '21 18:10 vercel[bot]

Looks nice. One question: in this case, there could be multiple Projects with the same id?

JuanM04 avatar Oct 20 '21 19:10 JuanM04

@JuanM04 No, what the query is aiming for is to verify that user can only update a project from current organisation.

~~We could also findFirst and then evaluate just the returned data, but I'd generally prefer not returning anything from the DB query unless it's actually relevant. Might be overcomplicating though, open to simplify.~~

EDIT: The above would be valid for fetching data, not for updating. Ignore it. I don't think there's any other option to perform the update with one query and not sift through the entire table of records, so my first explanation is still valid.

xkrsz avatar Oct 20 '21 21:10 xkrsz

Wouldn't it be better to do two queries: a findUnique and then an update? in that way, you can check the organization id manually and throw an error like "This user cannot update this project because they doesn't belong to this organization". I understand the efficiency of only one update query, but the other way is more verbose (and we are doing documentation for getting started) and doesn't need an extra index

Example:

import { resolver } from "blitz"
import db from "db"
import * as z from "zod"

const UpdateProject = z
  .object({
    id: z.number(),
    name: z.string(),
  })
  .nonstrict()

export default resolver.pipe(
  resolver.zod(UpdateProject),
  resolver.authorize(),
  async ({ id, ...data }, ctx) => {
    if (!ctx.session.orgId) throw new Error("Missing session.orgId")
    
    const project = await db.project.findUnique({
      where: { id },
      rejectOnNotFound: true,
    })
    
    if (project.organizationId !== ctx.session.orgId) {
      throw new Error("This user cannot update this project because they doesn't belong to this organization")
    }
    
    return db.project.update({
      where: { id },
      data,
    })
  }
)

JuanM04 avatar Oct 21 '21 10:10 JuanM04

I think we should document both ways, the unique constraint that @krszwsk has and @JuanM04's approach.

@krszwsk what happens in your case if the id doesn't exist, the org id doesn't exist, or the combo doesn't exist? Does it just return null? If so, we should check that and throw NotFoundError() as is our pattern for getItem().

And for @JuanM04's snippet, we should use that same pattern, so don't use rejectOnNotFound but instead manually throw NotFoundError() (as a side note, we should add support so that use of rejectOnNotFound will display as NotFoundError on the frontend).

Lastly on Juan's snippet, I think it would be slightly better to use findMany and then you can filter on both id and orgId.

flybayer avatar Oct 21 '21 20:10 flybayer

@JuanM04 update will return if it has changed a record, so we can detect in case we haven't found anything and return a verbose error.

@flybayer findMany will cycle through the table, won't stop on the first found record AFAIK, findFirst would be best there I guess.

xkrsz avatar Oct 25 '21 12:10 xkrsz

As for other suggestions, agreed, let's use both, I'll update the PR soon

xkrsz avatar Oct 25 '21 12:10 xkrsz

@flybayer also, @JuanM04 's snippet above already checks if user is allowed to modify the org's project manually, so no need to query that in the query I think 🤔

xkrsz avatar Oct 25 '21 12:10 xkrsz

Ah yes, I meant findFirst 🤦‍♂️

flybayer avatar Oct 30 '21 20:10 flybayer