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

enable order by ordinal position for query columns #520

Merged
merged 1 commit into from
Jan 21, 2024

Conversation

aaqilniz
Copy link
Contributor

@aaqilniz aaqilniz commented Oct 13, 2023

lb4 discover analyses the table structures for MySQL using INFORMATION_SCHEMA.COLUMNS
It does not seem to use the ordinal when reading the columns.

This PR adds an option to enable users to get table properties/columns in ordinal_position.

To enable this option, the user can pass an option, orderBy through connectorDiscoveryOptions while discovering the models. e.g. --connectorDiscoveryOptions = '{"orderBy":"ordinal_position"}'

Checklist

  • DCO (Developer Certificate of Origin) signed in all commits
  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • Commit messages are following our guidelines

@aaqilniz aaqilniz force-pushed the fix/ordinal_position branch 6 times, most recently from f2d3251 to cdf35c5 Compare October 15, 2023 11:28
@aaqilniz aaqilniz marked this pull request as ready for review October 15, 2023 11:30
@aaqilniz
Copy link
Contributor Author

Hi, @dhmlau. Can you please have a look at the PR?

The tests are failing due to the access being denied for the given username & password.

@dhmlau
Copy link
Member

dhmlau commented Oct 16, 2023

@aaqilniz, i just kicked off CI again. Other PRs (e.g. #521) seems to be running fine. Let's see if it's just some hiccups in CI.

@aaqilniz
Copy link
Contributor Author

Hi, @dhmlau. Thanks for the response. This is strange. The tests are failing for another PR of mine but are working fine for the PR you mentioned.

@aaqilniz aaqilniz mentioned this pull request Nov 4, 2023
5 tasks
@samarpanB
Copy link
Collaborator

CI check should pass after merging #539

@samarpanB
Copy link
Collaborator

@aaqilniz can you please rebase this PR ?

@aaqilniz aaqilniz force-pushed the fix/ordinal_position branch 2 times, most recently from be17483 to efa09f5 Compare November 16, 2023 17:45
@aaqilniz
Copy link
Contributor Author

@samarpanB. Tests are finally passing with this PR too.

lib/discovery.js Outdated Show resolved Hide resolved
@aaqilniz
Copy link
Contributor Author

aaqilniz commented Nov 27, 2023

Hi, @samarpanB. I've updated the PR. Please have a look at it now.
To make this orderBy as optional, we need to make an update to loopback-connector too. Here is the PR for it too.

The tests are failing due to this change required in loopback-connector.

@aaqilniz aaqilniz requested a review from samarpanB December 3, 2023 08:15
@samarpanB
Copy link
Collaborator

@aaqilniz tests seem to be failing. Can you rebase and check please ?

@aaqilniz aaqilniz force-pushed the fix/ordinal_position branch 2 times, most recently from 0051b18 to 8d87890 Compare December 29, 2023 15:28
@aaqilniz aaqilniz force-pushed the fix/ordinal_position branch 3 times, most recently from 0d5c807 to 5275beb Compare January 21, 2024 02:05
@aaqilniz aaqilniz force-pushed the fix/ordinal_position branch from 5275beb to c3777eb Compare January 21, 2024 02:17
@aaqilniz aaqilniz merged commit 8b94368 into loopbackio:master Jan 21, 2024
5 checks passed
@aaqilniz aaqilniz changed the title add order by ordinal position for query columns enable order by ordinal position for query columns Jan 21, 2024
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.

3 participants