Skip to content

support pg-promise init options #3613

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

Merged
merged 12 commits into from
Apr 7, 2017
Merged

Conversation

rendongsc
Copy link
Contributor

@rendongsc rendongsc commented Mar 9, 2017

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
Contributor

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

@vitaly-t
Copy link
Contributor

vitaly-t commented Mar 11, 2017

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
Contributor

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

@vitaly-t
Copy link
Contributor

vitaly-t commented Mar 11, 2017

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

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

@flovilmart
Copy link
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
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
Contributor

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

@flovilmart flovilmart closed this Mar 11, 2017
@rendongsc
Copy link
Contributor Author

rendongsc commented Mar 11, 2017

@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

@rendongsc updated the pull request - view changes

@flovilmart
Copy link
Contributor

Could you add a test for that?

@rendongsc
Copy link
Contributor Author

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

@flovilmart
Copy link
Contributor

Perhaps a mock and expectations of call for example

@flovilmart
Copy link
Contributor

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

@rendongsc
Copy link
Contributor Author

@flovilmart I can do that.

@facebook-github-bot
Copy link

@rendongsc updated the pull request - view changes

@facebook-github-bot
Copy link

@rendongsc updated the pull request - view changes

@facebook-github-bot
Copy link

@rendongsc updated the pull request - view changes

@facebook-github-bot
Copy link

@rendongsc updated the pull request - view changes

@flovilmart
Copy link
Contributor

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

@facebook-github-bot
Copy link

@rendongsc updated the pull request - view changes

@facebook-github-bot
Copy link

@rendongsc updated the pull request - view changes

@facebook-github-bot
Copy link

@rendongsc updated the pull request - view changes

@facebook-github-bot
Copy link

@rendongsc updated the pull request - view changes

@facebook-github-bot
Copy link

@rendongsc updated the pull request - view changes

@flovilmart
Copy link
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