Skip to content

support pg-promise init options#3613

Merged
flovilmart merged 12 commits into
parse-community:masterfrom
rendongsc:master
Apr 7, 2017
Merged

support pg-promise init options#3613
flovilmart merged 12 commits into
parse-community:masterfrom
rendongsc:master

Conversation

@rendongsc

@rendongsc rendongsc commented Mar 9, 2017

Copy link
Copy Markdown
Contributor

add database init options , hook pg connect.

var api = new ParseServer({
  databaseURI: databaseUri || 'postgres://Test:123@localhost:5432/Test',
  appId: process.env.APP_ID || 'myAppId',
  masterKey: process.env.MASTER_KEY || '',
  serverURL: process.env.SERVER_URL || 'http://localhost:1337/parse', 
  databaseOptions: {
    initOptions: {
      connect: function (client, dc, isFresh) {
        //do something here 
      }
    }
  }
});

@flovilmart

Copy link
Copy Markdown
Contributor

@rendongsc can you fix the lint issue and add tests to make sure we don't incur any regression?

@vitaly-t

vitaly-t commented Mar 11, 2017

Copy link
Copy Markdown
Contributor

As the author of pg-promise, I must say this looks wrong to me...

 client.pg.defaults[key] = dbOptions.pgOptions[key];

Variable client is the database object, which doesn't have property pg. Only the library's initialized object has it:

pgp.pg.defaults[key] = dbOptions.pgOptions[key];

So I presume this simply has never been tested, or else it would throw an error.

@flovilmart

Copy link
Copy Markdown
Contributor

@vitaly-t what do you recommend. Could you have a look if there's anything wrong there?

@vitaly-t

vitaly-t commented Mar 11, 2017

Copy link
Copy Markdown
Contributor

I have, and I published right there what it should be ;)

pgp.pg.defaults[key] = dbOptions.pgOptions[key];

@flovilmart

Copy link
Copy Markdown
Contributor

@vitaly-t got time for a PR? Who better than you? Should I close that one? Also, I believe with the amount of contributions you have, you should probably be granted commit access. Are you interested?

@vitaly-t

Copy link
Copy Markdown
Contributor

I'm not sure what this PR is really doing, but there issue that I described was there before.

There's only that one change is needed ;) I do not have rights to update the PR, you can do it ;)

@flovilmart

Copy link
Copy Markdown
Contributor

Just open a new one in that case, closing :)

@flovilmart flovilmart closed this Mar 11, 2017
@rendongsc

rendongsc commented Mar 11, 2017

Copy link
Copy Markdown
Contributor Author

@vitaly-t

pgp.pg.defaults[key] = dbOptions.pgOptions[key];

Not wrong, but does not work.

var api = new ParseServer({
  databaseURI: databaseUri || 'postgres://Test:123@localhost:5432/Test',
  cloud: process.env.CLOUD_CODE_MAIN || __dirname + '/cloud/main.js',
  appId: process.env.APP_ID || 'myAppId',
  masterKey: process.env.MASTER_KEY || '', 
  serverURL: process.env.SERVER_URL || 'http://localhost:1337/parse',  
  liveQuery: {
    classNames: ["Posts", "Comments"] 
  },
  databaseOptions: {
    pgOptions: {
      connect: function (client, dc, isFresh) {
        if (isFresh) {          
          client.query('SET search_path = cust');
        }
      }
    }
  }
});

@flovilmart flovilmart reopened this Mar 13, 2017
@facebook-github-bot

Copy link
Copy Markdown

@rendongsc updated the pull request - view changes

@flovilmart

Copy link
Copy Markdown
Contributor

Could you add a test for that?

@rendongsc

Copy link
Copy Markdown
Contributor Author

@flovilmart I test the way through the output log, and spec test specifications vary greatly, give me some time.

@flovilmart

Copy link
Copy Markdown
Contributor

Perhaps a mock and expectations of call for example

@flovilmart

Copy link
Copy Markdown
Contributor

@rendongsc did you get a chance to add some tests?

@rendongsc

Copy link
Copy Markdown
Contributor Author

@flovilmart I can do that.

@facebook-github-bot

Copy link
Copy Markdown

@rendongsc updated the pull request - view changes

@facebook-github-bot

Copy link
Copy Markdown

@rendongsc updated the pull request - view changes

@facebook-github-bot

Copy link
Copy Markdown

@rendongsc updated the pull request - view changes

@facebook-github-bot

Copy link
Copy Markdown

@rendongsc updated the pull request - view changes

@flovilmart

Copy link
Copy Markdown
Contributor

@rendongsc thanks for adding the tests, it seems that the linter is complaining about your spec files

@facebook-github-bot

Copy link
Copy Markdown

@rendongsc updated the pull request - view changes

@facebook-github-bot

Copy link
Copy Markdown

@rendongsc updated the pull request - view changes

@facebook-github-bot

Copy link
Copy Markdown

@rendongsc updated the pull request - view changes

@facebook-github-bot

Copy link
Copy Markdown

@rendongsc updated the pull request - view changes

@facebook-github-bot

Copy link
Copy Markdown

@rendongsc updated the pull request - view changes

@flovilmart

Copy link
Copy Markdown
Contributor

@rendongsc Thanks for the PR!

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.

4 participants