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

Scalar array query missing order clause #930

Closed
brett-nuske-alliancesoftware opened this issue Nov 12, 2019 · 7 comments · Fixed by prisma/prisma-engines#222
Closed

Scalar array query missing order clause #930

brett-nuske-alliancesoftware opened this issue Nov 12, 2019 · 7 comments · Fixed by prisma/prisma-engines#222
Assignees
Labels
bug/2-confirmed Bug has been reproduced and confirmed. kind/bug A reported bug.
Milestone

Comments

@brett-nuske-alliancesoftware

I have a model:

model skill {
  id            String   @default(cuid()) @id @unique
  node_id       String   @unique
  description   String
  importance    String   @default("Core")
  name          String[]
  references    String[]
  competencies  skill_competency[]
  active        Boolean  @default(true)
  order         Int
}

When executing:

photon.skills.findMany({
    where: {
        active: true,
    },
    orderBy: { order: 'asc' }
})

I notice in my PostgreSQL logs that the one of the queries that is executed is as follows:

SELECT "nodeId", "value" FROM "public"."skill_name" WHERE "nodeId" IN ('ck2v8nm78000045r0uadyfm8f', 'ck2v8nuom000145r0p7c0gti4')

Unfortunately the result of this query is not ordered and therefore the results for name are inconsistent. What I am experiencing is that if the results are as follows:

         nodeId                   value
 ck2v8nuom000145r0p7c0gti4          B
 ck2v8nm78000045r0uadyfm8f          A
 ck2v8nuom000145r0p7c0gti4          C

Then name for ck2v8nuom000145r0p7c0gti4 is ['C'] instead of ['B', 'C'].

My proposal is that the generated query should be:

SELECT "nodeId", "value" FROM "public"."skill_name" WHERE "nodeId" IN ('ck2v8nm78000045r0uadyfm8f', 'ck2v8nuom000145r0p7c0gti4') order by "nodeId", position asc
@pantharshit00 pantharshit00 added bug/2-confirmed Bug has been reproduced and confirmed. kind/bug A reported bug. bug/1-unconfirmed Bug should have enough information for reproduction, but confirmation has not happened yet. and removed bug/2-confirmed Bug has been reproduced and confirmed. labels Nov 14, 2019
@mavilein
Copy link
Member

@brett-nuske-alliancesoftware : Thanks for reaching out 🙏

I think that you found a bug there but the analysis and your suggestion is not entirely correct. Here a few observations:

  • The Photon query for the skill model specifies an order by. This only controls the the order by that is used to fetch the rows from the skill table. This does and should not influence the query for the scalar list field name.
  • The query for the table skill_name looks weird in my opinion. It should as well fetch the position column and use that to order it in memory in Photon.
  • You say the result for the nodeId ck2v8nuom000145r0p7c0gti4 is ['C']. This is really weird. If we have a ordering bug it should at least still contain the right values B and C.

@pantharshit00 pantharshit00 added bug/0-unknown Bug is new, does not have information for reproduction or reproduction could not be confirmed. and removed bug/1-unconfirmed Bug should have enough information for reproduction, but confirmation has not happened yet. labels Nov 14, 2019
@mavilein
Copy link
Member

I just compared the implementations of Prisma 1 and Prisma 2. We indeed added the suggested order by clause for nodeId and position for Prisma 1. That behavior got lost in the rewrite. Not sure if we should fix it though since we are considering to change the implementation of scalar lists soonish.

@pantharshit00
Copy link
Contributor

@brett-nuske-alliancesoftware

So for the ordering of Scalar lists, you should handle that on the application level. Scalar lists are returned on the order they are stored which is persisted by us. You can also store them as a relationship if you want for complex ordering directly via the database.

@pantharshit00 pantharshit00 added kind/question and removed bug/0-unknown Bug is new, does not have information for reproduction or reproduction could not be confirmed. kind/bug A reported bug. labels Nov 18, 2019
@brett-nuske-alliancesoftware
Copy link
Author

@pantharshit00 I was not looking for complex ordering. If I stored a String[] that contained ['A', 'B', 'C'] I would expect that to be returned in that order. This is not happening because the database is returning an arbitrary result set ordering as there is no order clause in the generated sql query.

@pantharshit00
Copy link
Contributor

Ok, I can confirm this bug now. We are not ordering by position.

So for following data, we should return ["A","B","C","D"]
image

But we are not returning that:
image

The SELECT statement does lack an order by clause:
image

@pantharshit00 pantharshit00 added bug/2-confirmed Bug has been reproduced and confirmed. kind/bug A reported bug. process/candidate and removed kind/question labels Nov 22, 2019
@janpio janpio added this to the Preview 18 milestone Nov 22, 2019
@pimeys
Copy link
Contributor

pimeys commented Nov 23, 2019

This is kind of obvious from the code where the order should be but is not. Although I still haven't been able to write a test where I could see the problem. That takes a bit more time than the fix which is two lines of code...

@pimeys
Copy link
Contributor

pimeys commented Nov 25, 2019

Ok, tested this with the following:

Model

model User {
  id     String  @default(cuid()) @id
  email  String  @unique
  name   String?
  skills String[]
}

Mutation

mutation {
  createOneUser(data: {
    email: "foo@bar.com"
    skills: { set: ["cats", "dogs", "mice", "elephants"] }
  }) { id }
}

First query

query {
  findOneUser(where: { id: "ck3eefbgd0000xhjiaepsz20k" }) {
    skills
  }
}

The values are in the right order. Now going to Postgres and deleting the dogs skill from the middle manually. Query is still in a right order. Now adding the same skill back in psql client with the same position to see that the dogs skill is actually in the end now.

The fix I merged now to master will fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/2-confirmed Bug has been reproduced and confirmed. kind/bug A reported bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants