Prisma-client-js: Runtime validation error on `orderBy: null` many query

[jasonkuhrt]

Originally raised https://github.com/prisma-labs/nexus-prisma/issues/457 but traced to a PhotonJS issue.

To repro simply try a many-query on any (?) Prisma 2 project:

await photon.posts.findMany({ orderBy: null })

TypeError: Cannot read property 'id' of null
    at Document.normalizePath (/Users/jasonkuhrt/projects/prisma/nexus-prisma/examples/blog/node_modules/@generated/photon/runtime/index.js:1786:34)
    at Document.validate (/Users/jasonkuhrt/projects/prisma/nexus-prisma/examples/blog/node_modules/@generated/photon/runtime/index.js:1690:31)
    at PostClient.get _document [as _document] (/Users/jasonkuhrt/projects/prisma/nexus-prisma/examples/blog/node_modules/@generated/photon/index.js:424:22)
    at PostClient.then (/Users/jasonkuhrt/projects/prisma/nexus-prisma/examples/blog/node_modules/@generated/photon/index.js:445:63)

[divyenduz]

I can confirm something similar (adjusted the title to suit me, let me know if you disagree @jasonkuhrt), on the default init schema + MySQL, this Photon call

const orderedUsers = await photon.users.findMany({
    orderBy: null,
  })

yields the following error.

(node:27910) UnhandledPromiseRejectionWarning: Error: 
Invalid `photon.users.findMany()` invocation in
/Users/divyendusingh/Documents/prisma/p2-277/index.ts:61:36

{
  orderBy: null
}

Argument orderBy of type UserOrderByInput needs at least one argument. Available args are listed in green.


    at Document.validate (/Users/divyendusingh/Documents/prisma/p2-277/node_modules/@prisma/photon/runtime/index.js:42536:23)
    at UserClient.get _document [as _document] (/Users/divyendusingh/Documents/prisma/p2-277/node_modules/@prisma/photon/index.js:212:22)
    at UserClient.then (/Users/divyendusingh/Documents/prisma/p2-277/node_modules/@prisma/photon/index.js:233:63)
    at processTicksAndRejections (internal/process/task_queues.js:85:5)
(node:27910) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:27910) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

If we don't want to allow null on orderBy, the types should not allow it, currently they do:

[jasonkuhrt]

Latest/Recent versions of Prisma seem to result in a different error. Here's what I'm seeing now.

❯ yarn -s prisma2 --version
[email protected], binary version: e7579bd35e0938dbf773f1706c098a0d14a5a038

Here is an example PSL (bigger than it needs to be, lazily taken from nexus-prisma examples/blog).

datasource db {
  provider = "sqlite"
  url      = "file:dev.db"
}

generator photon {
  provider = "photonjs"
}

model Blog {
  id        Int      @id @default(autoincrement())
  createdAt DateTime @default(now())
  updatedAt DateTime @updatedAt
  name      String
  viewCount Int      @default(0)
  posts     Post[]
  authors   User[]
}

model User {
  id        Int      @id @default(autoincrement())
  createdAt DateTime @default(now())
  updatedAt DateTime @updatedAt
  name      String?
  posts     Post[]
  blog      Blog
  rating    Float
  role      UserRole
}

enum UserRole {
  ADMIN
  AUTHOR
}

model Post {
  id     Int        @id @default(autoincrement())
  title  String
  tags   Tag[]
  blog   Blog
  status PostStatus
}

enum PostStatus {
  DRAFT
  PUBLISHED
}

model Tag {
  id    Int    @id @default(autoincrement())
  value String
}

Script to force repro:

import { Photon } from '@prisma/photon'

const photon = new Photon()

run().catch(e => {
  console.error(e)
  process.exit(1)
})

async function run() {
  await photon.posts({
    orderBy: { id: null }, // THIS TYPE CHECKS OK!
  })
}

```
❯ yarn -s ts-node src/repro.ts
TypeError: Cannot read property 'toString' of null
at transformOrderArg (/Users/jasonkuhrt/projects/prisma-labs/nexus-prisma/examples/blog/node_modules/@prisma/photon/runtime/index.js:40931:123)
at Object.enter (/Users/jasonkuhrt/projects/prisma-labs/nexus-prisma/examples/blog/node_modules/@prisma/photon/runtime/index.js:40944:32)
at visitArg (/Users/jasonkuhrt/projects/prisma-labs/nexus-prisma/examples/blog/node_modules/@prisma/photon/runtime/index.js:42588:32)
at /Users/jasonkuhrt/projects/prisma-labs/nexus-prisma/examples/blog/node_modules/@prisma/photon/runtime/index.js:42549:58
at Array.map ()
at visitField (/Users/jasonkuhrt/projects/prisma-labs/nexus-prisma/examples/blog/node_modules/@prisma/photon/runtime/index.js:42549:47)
at /Users/jasonkuhrt/projects/prisma-labs/nexus-prisma/examples/blog/node_modules/@prisma/photon/runtime/index.js:42539:53
at Array.map ()
at Object.visit (/Users/jasonkuhrt/projects/prisma-labs/nexus-prisma/examples/blog/node_modules/@prisma/photon/runtime/index.js:42539:40)
at Object.transformDocument (/Users/jasonkuhrt/projects/prisma-labs/nexus-prisma/examples/blog/node_modules/@prisma/photon/runtime/index.js:40935:20)

In an actual app (e.g. as reported https://github.com/prisma-labs/nexus-prisma/issues/457) the context is more complicated, with `t.crud.` etc. leading to ~conceptually something like:

```ts
export const Query = queryType({
  definition(t) {
    t.list.field('posts2', {
      type: 'Post',
      resolve(root, args, ctx) {
        return ctx.photon.posts({
          orderBy: { id: null },
        })
      },
    })
  }
})

export const Post = objectType({ // ...

[timsuchanek]

Thanks a lot for reporting 🙏
This issue is fixed in the latest alpha version of prisma2.
You can try it out with npm i -g prisma2@alpha.

In case it’s not fixed for you - please let us know and we’ll reopen this issue!

[timsuchanek]

To sum up the behavior: We're now treating null the same as undefined in any orderBy statement.

[jasonkuhrt]

@timsuchanek I just tried but it still does not work as expected as far as I can tell.

The same example above, now has runtime feedback as follows:

Error: 
Invalid `return ctx.prisma.posts()` invocation in
/Users/jasonkuhrt/projects/prisma-labs/nexus-prisma/examples/blog/src/schema/Query.ts:39:27

  35 
  36 t.list.field('posts2', {
  37   type: 'CustomPost',
  38   resolve(root,[object Object], args,[object Object], ctx) {
→ 39     return ctx.prisma.posts({
         orderBy: {
       ?   id?: asc | desc,
       ?   title?: asc | desc,
       ?   status?: asc | desc
         }
       })

Argument orderBy of type PostOrderByInput needs at least one argument. Available args are listed in green.

Note: Lines with ? are optional.

    at Document.validate (/Users/jasonkuhrt/projects/prisma-labs/nexus-prisma/examples/blog/node_modules/@prisma/client/runtime/index.js:42459:23)
    at PostClient.get _document [as _document] (/Users/jasonkuhrt/projects/prisma-labs/nexus-prisma/examples/blog/node_modules/@prisma/client/index.js:598:22)
    at PostClient.then (/Users/jasonkuhrt/projects/prisma-labs/nexus-prisma/examples/blog/node_modules/@prisma/client/index.js:623:63)
    at whenResultIsFinished (/Users/jasonkuhrt/projects/prisma-labs/nexus-prisma/examples/blog/node_modules/graphql-extensions/src/index.ts:171:12)
    at field.resolve (/Users/jasonkuhrt/projects/prisma-labs/nexus-prisma/examples/blog/node_modules/graphql-extensions/src/index.ts:156:7)
    at resolveFieldValueOrError (/Users/jasonkuhrt/projects/prisma-labs/nexus-prisma/examples/blog/node_modules/graphql/execution/execute.js:467:18)
    at resolveField (/Users/jasonkuhrt/projects/prisma-labs/nexus-prisma/examples/blog/node_modules/graphql/execution/execute.js:434:16)
    at executeFields (/Users/jasonkuhrt/projects/prisma-labs/nexus-prisma/examples/blog/node_modules/graphql/execution/execute.js:275:18)
    at executeOperation (/Users/jasonkuhrt/projects/prisma-labs/nexus-prisma/examples/blog/node_modules/graphql/execution/execute.js:219:122)
    at executeImpl (/Users/jasonkuhrt/projects/prisma-labs/nexus-prisma/examples/blog/node_modules/graphql/execution/execute.js:104:14)

But note that I get no static TS type error for code, repeated here for convenience:

    t.list.field('posts2', {
      type: 'Post',
      resolve(root, args, ctx) {
        return ctx.prisma.posts({
          orderBy: { id: null },
        })
      },
    })

Should we re-open?

[pantharshit00]

I can also able to reproduce this with latest alpha [email protected], binary version: 43dc93afaf4961d9a083741b3cdbd379748389d6 so I am going to reopen this.

[jasonkuhrt]

Thanks @pantharshit00

[timsuchanek]

Thanks for reporting back!
This behavior is expected. If you provide the orderBy type together with an empty object (or an object only containing null), we want to throw in runtime.

We were thinking of disallowing this on a type level, but the type errors for that situation are unreadable.

The reason to disallow this, is that if you provide the order by, you should also provide a valid value for it. Otherwise just don't provide the orderBy object or keep it null or undefined.

The rule we're enforcing here is "at least one" but at the same time "at most one". So just "equal one" for the field count. The reason is, that you as of now can't have multiple orderBy constraints active at the same time.

[jasonkuhrt]

Thanks for the info @timsuchanek. I can see how the type error could be unreadable be default. That seems like something which should be addressable with a TS plugin but not sure on level of effort (certainly not trivial).

Otherwise just don't provide the orderBy

For humans this makes sense, for machines or integrating two components its a bit more work? This is where having two APIs can make sense, one that is more raw AST-ish and one that is polished. Anyways, makes sense to me that we're focused on the human part now, re prisma client launch!

Re machine case, an example, makes mapping from GraphQL API input to photon more nuanced.

Query {
  things(orderBy:OrderBy)
}

```gql
query {
things(orderBy:null) # impossible to prevent user from doing this, per spec
}

```ts
{
  // ...
  resolve(_root, args, ctx) => {
    ctx.prisma.thing.getMany({
      orderBy: args.orderBy // bug right here
    })
  }
}

What's sad to me is there will be no static error. And it is a subtle integration bug that could easily not be caught until production.

If at all possible, I suggest to the core team to reconsider the decision of no static error, imo 🙂.

The way to fix the bug the above bug IIUC is:

{
  // ...
  resolve(_root, args, ctx) => {
    ctx.prisma.thing.getMany({
      orderBy: args.orderBy ?? undefined
    })
  }
}

or if not that then something brutish, like below, new DX problems (code is just gist, not exactly correct I'm pretty sure):

{
  // ...
  resolve(_root, args, ctx) => {
    // verbose
    // Param1 isn't available in TS out of box
    // reduces type safety
    const prismaArgs = {} as Param1<ctx.prisma.thing.getMany> 
    // verbose
    const { orderBy, ...argsRest } = args
    if (orderBy !== null) prismaArgs.orderBy = orderBy
    // type unsafe IIRC
    Object.assign(prismaArgs, argsRest)
    return ctx.prisma.thing.getMany(prismaArgs)
  }
}

For nexus-prisma we're fine, as we can encode the correct way to handle this.

My only real concerns here I think are 1) users doing custom resolvers and 2) the lack of static feedback.

I feel a user should never ever have to test for correct _type_ usage of their prisma client.

[timsuchanek]

Thanks @jasonkuhrt for the perspective! We'll take this into the next sprint and discuss what the best solution is.

[divyenduz]

Internal note: dropping from current sprint + product

[timsuchanek]

We are not allowing null in the TypeScript types anymore, therefore this is fixed.