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

MySQL Order By *_NULLS_(LAST|FIRST) incorrect syntax #5004

Closed
5 tasks done
kpervin opened this issue Dec 11, 2023 · 5 comments · Fixed by #5021
Closed
5 tasks done

MySQL Order By *_NULLS_(LAST|FIRST) incorrect syntax #5004

kpervin opened this issue Dec 11, 2023 · 5 comments · Fixed by #5021

Comments

@kpervin
Copy link
Contributor

kpervin commented Dec 11, 2023

Describe the bug

When using orderBy in a find operation, using ASC_NULLS_LAST or equivalent will insert the syntax order by ... asc nulls last. This is incorrect syntax.

If we have a table

CREATE TABLE foo
(
  id            INT UNSIGNED AUTO_INCREMENT
    PRIMARY KEY,
  text          VARCHAR(255) NOT NULL,
  optional_text VARCHAR(255) NULL,
);

Then to sort nulls on optional_text, we need to do the following:

Operation Syntax
ASC NULLS FIRST order by optional_text is not null, optional_text
ASC NULLS LAST order by optional_text is null, optional_text
DESC NULLS FIRST order by optional_text is not null, optional_text desc
DESC NULLS LAST order by optional_text is null, optional_text desc

Reproduction

https://github.com/kpervin/MikroORM-Issue-Example-Repo/blob/c446d1fd0b313da3c1ccb97915fc6a2fd09003ae/test/mysql-order-by-issue.spec.ts#L49-L73

What driver are you using?

@mikro-orm/mysql

MikroORM version

5.9.4

Node.js version

20.9.0

Operating system

Ubuntu 20.04 LTS

Validations

@B4nan
Copy link
Member

B4nan commented Dec 11, 2023

Looks like the support was added only for postgres (#677), it's indeed not working in mysql.

(and in a way, this is postgres only feature, what you want can be achieved explicitly)

@kpervin
Copy link
Contributor Author

kpervin commented Dec 11, 2023

(and in a way, this is postgres only feature, what you want can be achieved explicitly)

Is there a way to do this using EntityManager? Or can this only be done by the query builder?

If it's a PostGres only feature, could those values possibly be only exportable from the PostGres driver as a means to self-document their intent? If so, could other drivers have their syntax reflected in enums/functions exported from their respective package? Or would it make more sense to include an interpretation step in the driver for the shared core enum so it can be translated to driver-specific syntax?

@B4nan
Copy link
Member

B4nan commented Dec 11, 2023

You'd have to use a raw SQL fragment, but it should work.

I am sure we can map this automatically in other drivers, I am just saying it's more of a feature request to me (and as such will end up only in v6 probably).

@kpervin
Copy link
Contributor Author

kpervin commented Dec 11, 2023

I'm willing to put in a PR if need be. I know I've opened a few issues with no follow up. Been a bit busy at work and home to find time to put in the work.

@kpervin
Copy link
Contributor Author

kpervin commented Dec 12, 2023

@B4nan found that this works in the meantime, while raw sql fragments did not:

await orm.em.find(
      Foo,
      {},
      {
        orderBy: [{ optionalText: "IS NOT NULL" }, { optionalText: "asc" }],
      },
    );

B4nan pushed a commit that referenced this issue Dec 14, 2023
- Added `getOrderByExpression` to AbstractSqlPlatform.ts as an
overridable method to format `orderBy` queries. This uses syntax that is
recognized by both SQLite and PostGreSQL
- Added override of `getOrderByExpression` to MariaDbPlatform.ts and
MySqlPlatform.ts that adds functionality to handle `(ASC|DESC) NULLS
(FIRST|LAST)` syntax

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

Successfully merging a pull request may close this issue.

2 participants