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

feat(mysql): support order by nulls first/last #5021

Merged
merged 5 commits into from
Dec 14, 2023

Conversation

kpervin
Copy link
Contributor

@kpervin kpervin commented Dec 14, 2023

Fixes #5004

  • 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

Copy link

codecov bot commented Dec 14, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d8facbb) 99.66% compared to head (2f97327) 99.66%.
Report is 24 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5021   +/-   ##
=======================================
  Coverage   99.66%   99.66%           
=======================================
  Files         221      221           
  Lines       16212    16252   +40     
  Branches     3894     3907   +13     
=======================================
+ Hits        16158    16198   +40     
  Misses         54       54           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@B4nan B4nan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you check how sqlite behaves? we might need the same there

This uses syntax that is recognized by both SQLite and PostGreSQL

edit: i should read the whole thing first :]

packages/mariadb/src/MariaDbPlatform.ts Show resolved Hide resolved
Co-authored-by: Martin Adámek <banan23@gmail.com>
@kpervin
Copy link
Contributor Author

kpervin commented Dec 14, 2023

can you check how sqlite behaves? we might need the same there

This uses syntax that is recognized by both SQLite and PostGreSQL

edit: i should read the whole thing first :]

It probably would make sense to create test suites for each language. Currently the QueryBuilder.test.ts file is only testing against MySQL from what I could see.

Copy link
Member

@B4nan B4nan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just two nits, please apply those suggestions to both drivers

otherwise ready to merge, thanks!

It probably would make sense to create test suites for each language. Currently the QueryBuilder.test.ts file is only testing against MySQL from what I could see.

part of that file is also about postgres, but agreed this would deserve a split and other drivers, probably not a complete copy of what we have there but at least for the differences

@@ -74,4 +74,22 @@ export class MariaDbPlatform extends AbstractSqlPlatform {
return `alter table ${quotedTableName} add fulltext index ${quotedIndexName}(${quotedColumnNames.join(',')})`;
}

private readonly NULLS_TRANSLATE = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe lets remove those to ORDER_BY_NULLS_TRANSLATE?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, did you mean rename (not remove)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

packages/mariadb/src/MariaDbPlatform.ts Outdated Show resolved Hide resolved
@kpervin kpervin changed the title fix(core): fixing order-by enum handling by MySQL and MariaDB driver fix(sql): fixing order-by enum handling by MySQL and MariaDB driver Dec 14, 2023
@B4nan B4nan changed the title fix(sql): fixing order-by enum handling by MySQL and MariaDB driver feat(mysql): support order by nulls first/last Dec 14, 2023
@B4nan B4nan merged commit df75b24 into mikro-orm:master Dec 14, 2023
10 of 11 checks passed
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 this pull request may close these issues.

MySQL Order By *_NULLS_(LAST|FIRST) incorrect syntax
2 participants