Fix bug where settings for pg-pool were dropped #186

gregdingle · 2016-10-28T16:22:55Z

I'm not familiar with the issue

 // pg-pool doesn't handle string config correctly

but it seems like a bug to clear all other settings in this case.

I tested that an app would continue to connect with a connection url.

#185
@raymondfeng

slnode · 2016-10-28T16:22:57Z

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

slnode · 2016-10-28T16:22:57Z

Can one of the admins verify this patch?

slnode · 2016-10-28T16:22:57Z

Can one of the admins verify this patch?

superkhau · 2016-10-28T20:28:39Z

lib/postgresql.js

 this.constructor.super_.call(this, 'postgresql', settings);
+ this.clientConfig = settings;
 if (settings.url) {
 // pg-pool doesn't handle string config correctly


Can we delete this comment too? I assume pg pool is handling it correctly now?

Line 76? That is still accurate. Pg-pool expects a different key name than the pg module.

superkhau · 2016-11-10T07:58:12Z

Can you add a test to prevent regressions?

gregdingle · 2016-11-10T17:48:42Z

Sorry, it is not clear to me how to write such a test.

Is this the correct place to add the test?

https://github.com/strongloop/loopback-connector-postgresql/blob/master/test/postgresql.test.js https://github.com/strongloop/loopback-connector-postgresql/blob/master/test/postgresql.test.js

On Nov 10, 2016, at 8:58 AM, Simon Ho notifications@github.com wrote:

Can you add a test to prevent regressions?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub #186 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AABwfWojr5p5RVSzsYSbh-tFXGp_gDXFks5q8s6VgaJpZM4KjnW6.

superkhau · 2016-11-10T18:28:21Z

Feel free to create a new file in the test dir named postgresql.initialization.test.js, inside it should have a describe block like:

describe('initialization', function() { // start with failing test it('fails when'); it('passes when connection string is provided'); });

gregdingle · 2016-11-13T21:45:08Z

@superkhau I ran into an error trying to run the tests in the project.

Autocreate schema if not exists Connection fails: error: database "strongloop" does not exist It will be retried for the next request. 1) "before all" hook

The docs say:

The tests in this repository are mainly integration tests, meaning you will need to run them using our preconfigured test server. 1. Ask a core developer for instructions on how to set up test server credentials on your machine 2. `npm test`

Can you tell me how to set up the test environment? Or can you take care of postgresql.initialization.test.js?

superkhau · 2016-11-14T05:03:25Z

You'll have to set up your own posgresql instance on your machine and inject the credential (setup varies depending on your OS -- ie. mac vs windows etc, you can google that one). As for the credentials, see this list of environment variables: https://github.com/strongloop/loopback-connector-postgresql/blob/master/test/init.js#L12

gregdingle · 2016-11-15T21:57:05Z

I added a test as requested to prevent a regression. Although I created a new file, postgresql.initialization.test.js , it only covers the initialization logic related to the bug fix.

gregdingle · 2016-11-15T22:23:01Z

I'm not sure how to fix PR out of date, needs to be rebased . I rebased and re-pushed.

superkhau · 2016-11-16T06:16:27Z

@gregdingle Depending on how you forked, you can do something like: https://robots.thoughtbot.com/keeping-a-github-fork-updated. Basically your clone is not up to date with the latest master.

superkhau · 2016-11-16T06:18:17Z

Also, thanks for adding the test -- I will review it soon. So far it looks good with a few minor nitpicks to address after I do a more thorough review. 👍

gregdingle · 2016-11-16T08:27:07Z

I rebased and re-pushed with the latest master: https://github.com/strongloop/loopback-connector-postgresql/pull/186/commits .

superkhau

@gregdingle Reviewed, I left a bunch of comments. Please ping me again when you submit the fixups.

superkhau · 2016-11-16T21:47:21Z

test/postgresql.initialization.test.js

+
+var should = require('should');
+var DataSource = require('loopback-datasource-juggler').DataSource;
+var postgresql = require('../lib/postgresql.js');


nit -- move the should require statement under postgres to alpha sort requires.

superkhau · 2016-11-16T21:49:44Z

test/postgresql.initialization.test.js

+var postgresql = require('../lib/postgresql.js');
+
+describe('initialization', function() {
+ it('should take pg-pool setting', function(done) {


it('honours user-defined pg-pool settings'

superkhau · 2016-11-16T21:50:54Z

test/postgresql.initialization.test.js

+ it('should take pg-pool setting', function(done) {
+ // settings can be empty because we do not actually need to connect
+ var settings = {};
+ var dataSource = new DataSource(require('../'), settings);


Add var connector = require('..'); to the top of the file.

and change this line to:

var dataSource = new DataSource(connector, settings);

superkhau · 2016-11-16T21:53:12Z

test/postgresql.initialization.test.js

+ pool._factory.max.should.equal(999);
+ done();
+ });
+ }),


change , to ; and add a blank line between the it and the }); to separate out the tests.

Good catch. That was unintentional.

superkhau · 2016-11-16T21:57:08Z

test/postgresql.initialization.test.js

+ done();
+ });
+ }),
+ it('should take url setting and pg-pool setting', function(done) {


it('honours user-defined url settings'

superkhau · 2016-11-16T22:00:31Z

test/postgresql.initialization.test.js

+ // settings can be empty because we do not actually need to connect
+ var settings = {};
+ var dataSource = new DataSource(require('../'), settings);
+ dataSource.settings.max = 999; // non-default value


I'm curious as to whether the following works:

var settings = {max: 999}; var dataSource = new DataSource(connector, settings);

instead of:

// settings can be empty because we do not actually need to connect var settings = {}; var dataSource = new DataSource(connector, settings); dataSource.settings.max = 999; // non-default value

We should also verify the pool state before running the tests:

Something like:

pool._factory.max.should.not.equal(999); // or whatever the default is

Verifying the pool state before running the test will require initializing with defaults. I chose 999 as a non-default value that is very unlikely to ever become the default––currently 10.

superkhau · 2016-11-16T22:01:28Z

test/postgresql.initialization.test.js

+ });
+ }),
+ it('should take url setting and pg-pool setting', function(done) {
+ // settings can be empty because we do not actually need to connect


I think this comment was copy pasted from test above?

superkhau · 2016-11-16T22:02:02Z

test/postgresql.initialization.test.js

+ // settings can be empty because we do not actually need to connect
+ var settings = {url: 'postgres://'};
+ var dataSource = new DataSource(require('../'), settings);
+ dataSource.settings.max = 999; // non-default value


Do we need this setting for the test? We shouuld remove this setting since you're testing the url property specifically for this test.

I also think we should verify the state of the datasource before you run the tests:

Something like:

should.not.exist(datasource.connector.clientConfig) // or whatever the defaults are

Do we need this setting for the test? We shouuld remove this setting since you're testing the url property specifically for this test.

The original bug occurred only in the condition that both url and a user setting was set. I agree those should be independent but they were not.

superkhau · 2016-11-16T22:02:46Z

test/postgresql.initialization.test.js

+
+ var pool = dataSource.connector.pg.pool;
+ pool._factory.should.have.property('max');
+ pool._factory.max.should.equal(999);


I think the above 3 lines are irrelevant to this test since you already tested in the previous test.

See comment above.

The original bug occurred only in the condition that both url and a user setting was set. I agree those should be independent but they were not.

We should remove the 3 lines above and create another test for the case you described in this comment.

it('honours multiple user-defined pg-pool settings', ... do the tests for both conditions here

superkhau · 2016-11-16T22:03:12Z

test/postgresql.initialization.test.js

+ var dataSource = new DataSource(require('../'), settings);
+ dataSource.settings.max = 999; // non-default value
+ postgresql.initialize(dataSource, function() {
+ // dataSource is mutated


we can remove this comment too.

gregdingle · 2016-11-17T16:40:05Z

Update coming.

superkhau

Almost there! ;) Thanks for all the fixups.

superkhau · 2016-11-18T01:41:39Z

test/postgresql.initialization.test.js

+
+describe('initialization', function() {
+ it('honours user-defined pg-pool settings', function(done) {
+ var settings = {'max': 999}; // non-default value


max key does not need single quotes

{max: 999}

superkhau · 2016-11-18T01:45:41Z

test/postgresql.initialization.test.js

+ });
+
+ it('honours user-defined url settings', function(done) {
+ var settings = {url: 'postgres://', 'max': 999};


max key does not need single quotes here too

superkhau · 2016-11-18T01:50:29Z

test/postgresql.initialization.test.js

+
+ var pool = dataSource.connector.pg.pool;
+ pool._factory.should.have.property('max');
+ pool._factory.max.should.equal(999);


The original bug occurred only in the condition that both url and a user setting was set. I agree those should be independent but they were not.

We should remove the 3 lines above and create another test for the case you described in this comment.

it('honours multiple user-defined pg-pool settings', ... do the tests for both conditions here

superkhau · 2016-11-18T02:00:20Z

test/postgresql.initialization.test.js

+ var settings = {'max': 999}; // non-default value
+ var dataSource = new DataSource(connector, settings);
+ postgresql.initialize(dataSource, function() {
+ var pool = dataSource.connector.pg.pool;


Since we can't verify pool state, we can make sure pg was not set before your tests:

should.not.exist(dataSource.connector.pg);

to verify state before your pg pool tests.

slnode · 2016-11-18T22:06:56Z

Can one of the admins verify this patch?

gregdingle · 2016-11-18T22:11:44Z

@superkhau I fixed the tests as requested. But I noticed in the process that initialize was not necessary, so I simplified the code.

superkhau · 2016-11-18T23:05:22Z

@gregdingle Nice one on the simplification. 👍 LGTM -- gonna try to get CI to give us all greens and get it merged.

gregdingle · 2016-11-29T09:02:14Z

@superkhau Is there anything more I need to do here?

superkhau · 2016-11-30T05:36:21Z

@gregdingle Can you help rebase this PR? CI is failing and I'm trying to see if we can get green before landing.

gregdingle · 2016-11-30T10:12:35Z

@superkhau I rebased and repushed.

superkhau · 2016-11-30T21:33:08Z

@gregdingle The strange thing is your changes are failing on node 4.x and 6.x on windows, can you fix it?

ssh24 · 2017-02-14T16:40:08Z

README.md

 </thead>
- <tbody> 
+ <tbody>
 <tr>


We don't need this change to go with the PR.

ssh24 · 2017-02-14T16:49:50Z

test/postgresql.initialization.test.js

+ var clientConfig = dataSource.connector.clientConfig;
+ clientConfig.connectionString.should.equal(settings.url);
+ });
+});


Need a new line here. PR builder failing cause of that https://cis-jenkins.swg-devops.com/job/nb/job/loopback-connector-postgresql~master/label=x64%20&&%20linux%20&&%20nvm%20&&%20dbs,nodeVersion=6/100/consoleFull#-207526304206f8fc75-e0b9-479e-b471-e8fe633361ff

Fixed, thanks to @dhmlau's commit.

ssh24 · 2017-02-14T18:36:16Z

@gregdingle Could you please sign the CLA to proceed with the merge?

ssh24 · 2017-02-17T15:17:49Z

@gregdingle Could you please sign the CLA?

rmg · 2017-02-17T19:06:35Z

The failures on Windows are due to the new tests clobbering some state that is shared with the other tests. The reason it is only causing a failure on Windows is because the connection defaults to using the OS username and on Windows the database server does not have that user.

ssh24 · 2017-02-17T19:22:51Z

@rmg How is it passing on local windows with both node4 and node6 then?

rmg · 2017-02-17T19:47:04Z

@ssh24 because your local Windows environment has a DB user that matches your Windows username.

rmg · 2017-02-17T19:50:18Z

@ssh24 see #216 for the changes I had to make.

superkhau · 2017-02-17T20:12:55Z

@rmg TY for looking into it. @gregdingle Would you like to make your changes based on @rmg's suggested fixes at #216 and rebase this branch while you're at it so we can land?

ssh24 · 2017-02-27T15:19:15Z

@gregdingle Could you please rebase your code and fix your code according to this changes #216 ? Everything else looks good to me, only needs the test cases with those fixes.

gregdingle · 2017-02-27T23:41:57Z

@ssh24 @superkhau I merged in the fixes from #216. I did not squash the commits so you could see my work.

ssh24 · 2017-02-28T00:15:26Z

@slnode test please

ssh24

LGTM. Waiting for CI to pass and it can be landed.

gregdingle · 2017-02-28T00:41:10Z

🥇 😄 👍

superkhau · 2017-02-28T01:55:39Z

@gregdingle Thanks for the contribution. Great work and sorry for the delays. ;)

crandmck added the #community contribution label Oct 28, 2016

superkhau approved these changes Oct 28, 2016

View reviewed changes

superkhau self-assigned this Oct 28, 2016

superkhau suggested changes Oct 28, 2016

View reviewed changes

superkhau approved these changes Oct 28, 2016

View reviewed changes

superkhau suggested changes Nov 16, 2016

View reviewed changes

superkhau suggested changes Nov 18, 2016

View reviewed changes

superkhau approved these changes Nov 18, 2016

View reviewed changes

ssh24 reviewed Feb 14, 2017

View reviewed changes

README.md

</thead>

<tbody>

<tbody>

<tr>

Copy link

Contributor

ssh24 Feb 14, 2017

Choose a reason for hiding this comment

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

We don't need this change to go with the PR.

ssh24 reviewed Feb 14, 2017

View reviewed changes

rmg mentioned this pull request Feb 17, 2017

DO NOT MERGE: Rebased and cleaned up version of #186 #216

Closed

kjdelisle added the blocked label Feb 21, 2017

kjdelisle modified the milestones: Sprint 30, Sprint 29 Feb 23, 2017

kjdelisle removed the blocked label Feb 27, 2017

Amir-61 and others added 7 commits February 27, 2017 15:15

Update README with correct doc links, etc

87ec3fa

Fix bug where settings for pg-pool were dropped

6ff2537

Added test. Ran run-tests.

2ffb867

remove done() calls in test

7058807

Fix linting errors and unnesssary changes.

f7a9954

Fix unit tests

f6d8821

merge in #216

fbe3edf

kjdelisle added the #wip label Feb 27, 2017

ssh24 approved these changes Feb 28, 2017

View reviewed changes

ssh24 merged commit 6e33b93 into loopbackio:master Feb 28, 2017

ssh24 removed the #wip label Feb 28, 2017

Fix bug where settings for pg-pool were dropped #186

Fix bug where settings for pg-pool were dropped #186

Uh oh!

Conversation

gregdingle commented Oct 28, 2016

slnode commented Oct 28, 2016

slnode commented Oct 28, 2016

slnode commented Oct 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superkhau commented Nov 10, 2016

gregdingle commented Nov 10, 2016

superkhau commented Nov 10, 2016

gregdingle commented Nov 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

superkhau commented Nov 14, 2016

gregdingle commented Nov 15, 2016

gregdingle commented Nov 15, 2016

superkhau commented Nov 16, 2016

superkhau commented Nov 16, 2016

gregdingle commented Nov 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

superkhau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregdingle commented Nov 17, 2016

superkhau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superkhau Nov 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

slnode commented Nov 18, 2016

gregdingle commented Nov 18, 2016

superkhau commented Nov 18, 2016

gregdingle commented Nov 29, 2016

superkhau commented Nov 30, 2016

gregdingle commented Nov 30, 2016

superkhau commented Nov 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssh24 commented Feb 14, 2017

ssh24 commented Feb 17, 2017

rmg commented Feb 17, 2017

ssh24 commented Feb 17, 2017

rmg commented Feb 17, 2017

rmg commented Feb 17, 2017

superkhau commented Feb 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ssh24 commented Feb 27, 2017

gregdingle commented Feb 27, 2017

ssh24 commented Feb 28, 2017

ssh24 left a comment

Choose a reason for hiding this comment

gregdingle commented Feb 28, 2017

superkhau commented Feb 28, 2017

Labels

11 participants

gregdingle commented Nov 13, 2016 •

edited

Loading

gregdingle commented Nov 16, 2016 •

edited

Loading

superkhau Nov 18, 2016 •

edited

Loading

superkhau commented Nov 30, 2016 •

edited

Loading

superkhau commented Feb 17, 2017 •

edited

Loading