Skip to content

Fix bug cannot create foreign key #371

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 5, 2019
Merged

Fix bug cannot create foreign key #371

merged 1 commit into from
Apr 5, 2019

Conversation

danghung-dev
Copy link
Contributor

Description

If the foreign key is lower case, it can be created. But if it has any character uppercase. I cannot create.
I fix it by simple modify this.escapeName(fk.foreignKey)

image

Related issues

Checklist

  • [] New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide

@slnode
Copy link

slnode commented Mar 14, 2019

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

@elv1s
Copy link
Contributor

elv1s commented Mar 14, 2019

Thank you for the contribution! Please sign the CLA
https://cla.strongloop.com/agreements/strongloop/loopback-connector-postgresql

The change looks good, but I think you need to update the tests as well
https://github.com/strongloop/loopback-connector-postgresql/blob/master/test/postgresql.autoupdate.test.js#L630

@danghung-dev
Copy link
Contributor Author

Thank you @elv1s
I have already updated the tests and signed the CLA.

@elv1s
Copy link
Contributor

elv1s commented Mar 27, 2019

Thanks @danghung-dev, it looks like the cla check is still failing. Did you sign the loopback-connector-postgres cla? It is different from the loopback cla

@danghung-dev
Copy link
Contributor Author

image
Please check screenshot.
Maybe I did something wrong?

@dhmlau
Copy link
Member

dhmlau commented Mar 27, 2019

@slnode test please

@dhmlau
Copy link
Member

dhmlau commented Mar 27, 2019

@rmg, could you please help? Looks like @danghung-dev has signed the CLA but it's not showing the right status? Thanks!

@rmg
Copy link
Contributor

rmg commented Mar 27, 2019

This pull request includes a commit attributed to @smartlogvn, so they will also need to sign the CLA.

@danghung-dev
Copy link
Contributor Author

Thanks @rmg
Sorry, this is my mistake. I made commit but didn't check author.
I removed smartlogvn commit and force pushed again.

@elv1s
Copy link
Contributor

elv1s commented Mar 28, 2019

@slnode test please

elv1s
elv1s approved these changes Mar 28, 2019
View reviewed changes
Copy link
Contributor

@elv1s elv1s left a comment

Choose a reason for hiding this comment

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

LGTM

@dhmlau
Copy link
Member

dhmlau commented Mar 28, 2019

There is one test case failing:

1) autoupdate
13:41:50        foreign key constraint
13:41:50          should create, update, and delete foreign keys:
13:41:50 
13:41:50       Uncaught AssertionError [ERR_ASSERTION]: error: duplicate key value violates unique constraint "pg_namespace_nspname_index"
13:41:50       + expected - actual
13:41:50 
13:41:50       -false
13:41:50       +true
13:41:50       
13:41:50       at /home/jenkins/workspace/nb/loopback-connector-postgresql~master/3ed5ebba/test/postgresql.autoupdate.test.js:632:9
13:41:50       at /home/jenkins/workspace/nb/loopback-connector-postgresql~master/3ed5ebba/node_modules/loopback-connector/node_modules/async/dist/async.js:473:16
13:41:50       at iteratorCallback (node_modules/loopback-connector/node_modules/async/dist/async.js:1062:13)
13:41:50       at /home/jenkins/workspace/nb/loopback-connector-postgresql~master/3ed5ebba/node_modules/loopback-connector/node_modules/async/dist/async.js:969:16
13:41:50       at /home/jenkins/workspace/nb/loopback-connector-postgresql~master/3ed5ebba/lib/migration.js:321:24
13:41:50       at cbForWork (node_modules/loopback-datasource-juggler/lib/observer.js:234:34)
13:41:50       at /home/jenkins/workspace/nb/loopback-connector-postgresql~master/3ed5ebba/node_modules/loopback-connector/lib/sql.js:643:9
13:41:50       at Query.callback (lib/postgresql.js:152:7)
13:41:50       at Query.handleError (node_modules/pg/lib/query.js:142:17)
13:41:50       at Connection.connectedErrorMessageHandler (node_modules/pg/lib/client.js:183:17)
13:41:50       at Socket.<anonymous> (node_modules/pg/lib/connection.js:125:12)
13:41:50       at addChunk (_stream_readable.js:263:12)
13:41:50       at readableAddChunk (_stream_readable.js:250:11)
13:41:50       at Socket.Readable.push (_stream_readable.js:208:10)
13:41:50       at TCP.onread (net.js:601:20)
13:41:50

Since it's only failing for Node.js 8, let me kick off CI again to make sure.

@dhmlau
Copy link
Member

dhmlau commented Mar 28, 2019

@slnode test please

jannyHou
jannyHou reviewed Mar 28, 2019
View reviewed changes
Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

@danghung-dev Thank you for taking the investigation! I am glad you figured out the fix.

I left a comment about the test change.

test/postgresql.autoupdate.test.js
@@ -477,6 +477,9 @@ describe('autoupdate', function() {
'customerId': {
'type': 'String',
'length': 20,
'postgresql': {
Copy link
Contributor

Choose a reason for hiding this comment

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

If the columnName is the same as property name, why do we have to specify it again here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have to do that, if we don't the column name will be convert to lowercase.
Please take a look at this issue. #38

Copy link
Contributor

Choose a reason for hiding this comment

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

I see thanks for pointing to the issue.

@dhmlau dhmlau added the community-contribution Patches contributed by community label Mar 29, 2019
@dhmlau dhmlau requested a review from jannyHou March 30, 2019 02:24
jannyHou
jannyHou approved these changes Apr 5, 2019
View reviewed changes
Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

@danghung-dev Thank you for your contribution! LGTM 🚢

@jannyHou jannyHou merged commit 7369f7d into loopbackio:master Apr 5, 2019
dhmlau added a commit that referenced this pull request Apr 5, 2019
@dhmlau
3.6.1
39776a1 
 * Fix cannot create foreignkey (#371) (Hung)
 * Add regression test (jlawrencecfm)
 * Use canonical index name when dropping (jlawrencecfm)
 * add @elv1s as CODEOWNERS (Diana Lau)
ong-james pushed a commit to luminlife/loopback-connector-postgresql that referenced this pull request Oct 26, 2020
@Ongsby
Merge tag 'tags/v3.6.1' into lumin-release
d62a87e 
3.6.1

 * Fix cannot create foreignkey (loopbackio#371) (Hung)
 * Add regression test (jlawrencecfm)
 * Use canonical index name when dropping (jlawrencecfm)
 * add @elv1s as CODEOWNERS (Diana Lau)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elv1s elv1s elv1s approved these changes

@jannyHou jannyHou jannyHou approved these changes

@b-admike b-admike Awaiting requested review from b-admike

@dhmlau dhmlau Awaiting requested review from dhmlau dhmlau is a code owner

@zbarbuto zbarbuto Awaiting requested review from zbarbuto

Assignees
No one assigned
Labels
community-contribution Patches contributed by community
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@danghung-dev @slnode @elv1s @dhmlau @rmg @jannyHou