Skip to content

Fix hard-coded "pkName" column in queries#347

Merged
dhmlau merged 2 commits intoloopbackio:masterfrom
ataft:patch-3
Nov 9, 2018
Merged

Fix hard-coded "pkName" column in queries#347
dhmlau merged 2 commits intoloopbackio:masterfrom
ataft:patch-3

Conversation

@ataft
Copy link
Copy Markdown
Contributor

@ataft ataft commented Oct 18, 2018

Description

Both the "buildQueryForeignKeys" and "buildQueryExportedForeignKeys" functions have the "pkName" column in their queries hard-coded to the following:
'PK' AS "pkName"

This fix populates the actual primary key name in both of these queries. This fixes issue #65, which should have never been closed.

Related issues

connect to #65

@slnode
Copy link
Copy Markdown

slnode commented Oct 18, 2018

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@dhmlau
Copy link
Copy Markdown
Member

dhmlau commented Oct 19, 2018

@ataft , thanks for another PR. :)
Could you please fix the commit message error?

**************************************************
**
**  Linting commit logs
**
**  2 problems found:
**    c72064f - Fix hard-coded "pkName" column in queries: line 3 longer than 72 characters.
**    c72064f - Fix hard-coded "pkName" column in queries: line 6 longer than 72 characters.
**
**************************************************

Again, see if you can follow this instruction to change the commit message https://help.github.com/articles/changing-a-commit-message/. If not, don't worry, I can modify it for you.

@dhmlau dhmlau force-pushed the patch-3 branch 2 times, most recently from 3b07529 to 522b62c Compare October 19, 2018 21:50
@dhmlau
Copy link
Copy Markdown
Member

dhmlau commented Oct 20, 2018

@slnode test please

@dhmlau
Copy link
Copy Markdown
Member

dhmlau commented Oct 20, 2018

@ataft, all the tests seem to be passed, however, there're some linting errors in lib/discovery.js. Could you please fix the following? Thanks!

266:1  error  Line 266 exceeds the maximum line length of 120  max-len
300:1  error  Line 300 exceeds the maximum line length of 120  max-len

Copy link
Copy Markdown
Contributor Author

@ataft ataft left a comment

Choose a reason for hiding this comment

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

Line length fix made, please review

@dhmlau
Copy link
Copy Markdown
Member

dhmlau commented Oct 22, 2018

@slnode test please

@dhmlau
Copy link
Copy Markdown
Member

dhmlau commented Oct 22, 2018

@zbarbuto @strongloop/sq-lb-apex, I'm not very familiar with postgresql, could you please help reviewing this PR? Thanks!

@zbarbuto
Copy link
Copy Markdown
Contributor

Mine's also not amazing and I've never used the discovery features in LB so I'm probably not the best person to review.

@bajtos
Copy link
Copy Markdown
Member

bajtos commented Oct 23, 2018

IMO, this pull request should include at least two new tests (one for buildQueryForeignKeys and another for buildQueryExportedForeignKeys) to verify the proposed changes.

I believe tests for auto-discovery are located here: https://github.com/strongloop/loopback-connector-postgresql/blob/master/test/postgresql.discover.test.js

I took a quick look and it seems to me that these tests assume the database already contains a set of tables. I am not sure if those tables are sufficient to test the fix made by this pull request. Personally, I would create new tables tailored to the specific needs of the new tests by calling db.connector.execute() to run CREATE TABLE statements. Just make sure to correctly handle the case where the tables were already created by a previous test run, in which case the setup code should drop those tables before (re)creating them again.

@ataft
Copy link
Copy Markdown
Contributor Author

ataft commented Oct 23, 2018

The current auto-discovery tests use the inventory table, which already has foreign keys, see here:
https://github.com/strongloop/loopback-connector-postgresql/blob/master/test/schema.sql#L1333

It seems like there's no need to add new tables and further complicate the testing process.

Lastly, this is a low-risk PR for forward and backward compatibility because 1) it replaces a non-working hard-coded column, and 2) it uses a subquery that returns a maximum of 1 value to create the column, so will never cause join issues or an error due to more than one value appearing.

@ataft
Copy link
Copy Markdown
Contributor Author

ataft commented Nov 8, 2018

Is this PR going to be merged?

@bajtos bajtos requested a review from raymondfeng November 9, 2018 08:27
@dhmlau dhmlau merged commit edaae68 into loopbackio:master Nov 9, 2018
@dhmlau
Copy link
Copy Markdown
Member

dhmlau commented Nov 9, 2018

Thanks @raymondfeng for the review.
@ataft , thanks for your contributions. Your PR has landed and I'll make a release of this connector shortly.

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.

6 participants