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

Order of multiple nested orderBy params #2010

Closed
co-sic opened this issue Jul 6, 2021 · 4 comments · Fixed by #2211
Closed

Order of multiple nested orderBy params #2010

co-sic opened this issue Jul 6, 2021 · 4 comments · Fixed by #2211
Labels
enhancement New feature or request
Milestone

Comments

@co-sic
Copy link
Contributor

co-sic commented Jul 6, 2021

Is your feature request related to a problem? Please describe.
Lets say if have an entity
Address { city: string; street: string; }
and
Customer { name: string; address: Address; }

and i query the customers entity and want to order by address.city first, then name and then address.street, that is currently not possible as far as i am aware. The orderBy Param would look something like this:

{
  address: {
    city: 'ASC',
  },
  name: 'ASC',
  address: {
    street: 'ASC', 
  },
}

which is not possible because of the duplicate address key.

Describe the solution you'd like
Is there a solution for this which i am not aware of and if not, is there a chance to add something to make this possible? An idea without introducing a breaking change would be to automatically parse keys like "address.city".

{
  'address.city': 'ASC',
  name: 'ASC',
  'address.street': 'ASC', 
}

Or you could optionally allow for an array or object instead of the QueryOrder, where you could specify the order by a number. something like

{
  address: {
    city: ['ASC', 0],
    street: ['ASC', 2], 
  },
  name:  ['ASC', 1],
}
@co-sic co-sic added the enhancement New feature or request label Jul 6, 2021
@B4nan
Copy link
Member

B4nan commented Jul 6, 2021

I'd probably rather go with supporting array too instead of the dot notation, so you can do this:

orderBy: [
  { address: { city: 'ASC' } },
  { name: 'ASC' },
  { address: { street: 'ASC' } },
]

@co-sic
Copy link
Contributor Author

co-sic commented Jul 6, 2021

@B4nan can you estimate how big of a change that would be? If it's not to big and does not involve a lot of knowledge about the codebase, i could go ahead and open a PR. Maybe you could also point me in the direction where i would need to implement this change.

@B4nan
Copy link
Member

B4nan commented Jul 6, 2021

I guess it could be rather easy, the functional bit will be in QueryBuilder and QueryBuilderHelper, you would also have to adjust the typings a bit. All QB tests are in a single file and they test just the produced query, so they are fast and you can easily verify you did not break anything.

Mostly this method is what you have to change:

https://github.com/mikro-orm/mikro-orm/blob/master/packages/knex/src/query/QueryBuilderHelper.ts#L437

It might as well be as simple as looping over that array instead of using Object.keys() (in general what that method does need to happen for each item in that array).

QB tests are here:

https://github.com/mikro-orm/mikro-orm/blob/master/tests/QueryBuilder.test.ts

@B4nan B4nan added this to the 5.0 milestone Aug 26, 2021
@evantrimboli
Copy link
Contributor

Just to tack onto this, as discussed on Slack it would be really cool to support strict typing here too. Thanks!

B4nan added a commit that referenced this issue Sep 18, 2021
`FindOptions.orderBy` parameter is now strictly typed. It also allows passing an
array of objects instead of just a single object.

```ts
const books = await em.find(Book, {}, {
  orderBy: [
    { title: 1 },
    { author: { name: -1 } },
  ],
});
```

Closes #2010
B4nan added a commit that referenced this issue Sep 18, 2021
`FindOptions.orderBy` parameter is now strictly typed. It also allows passing an
array of objects instead of just a single object.

```ts
const books = await em.find(Book, {}, {
  orderBy: [
    { title: 1 },
    { author: { name: -1 } },
  ],
});
```

Closes #2010
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants