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

'orderBy' conversion is wrong in case I also include a non encrypted value #65

Closed
martijn-dev opened this issue Jun 21, 2023 · 7 comments
Labels

Comments

@martijn-dev
Copy link
Contributor

martijn-dev commented Jun 21, 2023

The situation is the following:

The prisma schema:

model Visitor {
  id             String            @id @default(uuid())
  key            String            @unique /// @encrypted
  keyHash        String?           @unique /// @encryption:hash(key)
  created_at     DateTime          @default(now())

  @@map("visitors")
}

I would like to do a findMany with a orderBy on the key and on the created_at.

I understand it is quite useless to sort on a encrypted value and I kinda understand the desision made in issue #43. If I only sort on a encrypted field I get the silent error: Error: Running orderBy on encrypted field Visitor.key is not supported (results won't be sorted).

But I will do the following:

this.prisma.visitor.findMany({
           take: 5,
           skip: 0,
           cursor: undefined,
           orderBy: [
             {
               key: 'asc',
             },
             {
               created_at: 'asc'
             }
           ],
         })

which does not result in a silent error but in the following:

this.prisma.visitor.findMany({
           take: 5,
           skip: 0,
           cursor: undefined,
           orderBy: [
             {
               key: 'v1.aesgcm256.xxxxxxx.8xxxxxxxxxxxxxxxxxxWhfw==',
             keyHash: 'xxxxxxxxx-hash-xxxxxxxxxxxx'
             },
             {
               created_at: 'asc'
             }
           ] 
         })

Which results in a Primsa error Argument orderBy: Got invalid value since you should only provide 1 property per object.

I would expect it to convert to:

this.prisma.visitor.findMany({
           take: 5,
           skip: 0,
           cursor: undefined,
           orderBy: [
             {
             keyHash: 'asc'
             },
             {
               created_at: 'asc'
             }
           ] 
         })

or in case you want to prevent sorting on encrypted fields

this.prisma.visitor.findMany({
           take: 5,
           skip: 0,
           cursor: undefined,
           orderBy: [
             {
               created_at: 'asc'
             }
           ] 
         })

Please let me know if you need more information!

@franky47
Copy link
Member

franky47 commented Jun 21, 2023

I'm not familiar with the array syntax for orderBy, what is this supposed to do?

orderBy: [
 {
   keyHash: 'xxxxxxxxx-hash-xxxxxxxxxxxx'
 },
 {
   created_at: 'asc'
 }
] 

@martijn-dev
Copy link
Contributor Author

First of all, sorry, I see I made a typo. My expected result would be:

orderBy: [
 {
   keyHash: 'asc'
 },
 {
   created_at: 'asc'
 }
]

I changed it in the original issue as well.

With an array syntax in orderBy you can order your result by multiple properties. The above example is a bit weird for this usecase.

But let's say the Visitors have a name and a created_at. And you would like to have your visitors sorted by name and after that by created_at. A query like this would work then:

orderBy: [
 {
   name: 'asc'
 },
 {
   created_at: 'desc'
 }
] 

If there are multiple visitors with the same name these will then be ordered by created_at.

https://www.prisma.io/docs/concepts/components/prisma-client/filtering-and-sorting#sorting

@franky47
Copy link
Member

I see, it makes more sense with the correction.

That being said, ordering on the hash will not give you the result you expect. While you would indeed have multiple visitors with the same name being neighbours in the resulting array, ordered by created_at, the actual names would not be ordered correctly.

@github-actions
Copy link

🎉 This issue has been resolved in version 1.4.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@franky47
Copy link
Member

Version 1.4.5 includes a "fix" only for the misdetection of the orderBy clause with an array syntax. I still recommend doing sorting at runtime post-query if the application requires a particular order.

@martijn-dev
Copy link
Contributor Author

martijn-dev commented Jun 21, 2023

Yeah you're right. Sorting on the hash never results in a usefull outcome.

But should in this case the encrypted field then be taken out by the array? So your're left with just:

orderBy: [
             {
               created_at: 'asc'
             }
           ] 

or would you throw the same error as on a single orderBy query?

EDIT: Ah you're faster than light :) It became the last option. Thanks for your fast responses!

@franky47
Copy link
Member

Well I tried to get this:

orderBy: [
 {
   name: 'asc'
 },
 {
   created_at: 'desc'
 }
] 

to become this:

orderBy: [
 {
   created_at: 'desc'
 }
] 

but the overwriting mechanism is not (yet) smart enough to know how to prune an empty object or array, so I end up with this:

orderBy: [
 {
 },
 {
   created_at: 'desc'
 }
] 

which causes the query to throw, after displaying the error that orderBy is not supported on encrypted fields. I figure it's clear enough a message to not fiddle with the query further, and let the developer remove the unsupported orderBy clause.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants