Skip to content
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

fix: autosave being overwritten in BaseDataSource by setUrl #1309

Merged
merged 1 commit into from Oct 10, 2018

Conversation

Projects
None yet
4 participants
@davecramer
Copy link
Member

commented Oct 10, 2018

Fixes #1308

@codecov-io

This comment has been minimized.

Copy link

commented Oct 10, 2018

Codecov Report

Merging #1309 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #1309      +/-   ##
============================================
+ Coverage     68.54%   68.55%   +0.01%     
- Complexity     3879     3881       +2     
============================================
  Files           178      178              
  Lines         16198    16199       +1     
  Branches       2644     2645       +1     
============================================
+ Hits          11103    11106       +3     
+ Misses         3863     3862       -1     
+ Partials       1232     1231       -1
@@ -1110,7 +1110,9 @@ public void setUrl(String url) {
Properties p = org.postgresql.Driver.parseURL(url, null);

for (PGProperty property : PGProperty.values()) {
setProperty(property, property.get(p));
if (!this.properties.containsKey(property.getName())) {

This comment has been minimized.

Copy link
@rainoko

rainoko Oct 10, 2018

i do not know is it so simple. Because now I think that default values might be not initialized at all.
Maybe should check that property unset (== null) but even then there is cap with properties with primitive type values.

This comment has been minimized.

Copy link
@davecramer

davecramer Oct 10, 2018

Author Member

What this does is just make sure that if someone else has set the property it will not be overwritten in setURL.
Does this fix your problem ?

This comment has been minimized.

Copy link
@rainoko

rainoko Oct 10, 2018

yeah you are right of course. I'll check it out immediately

This comment has been minimized.

Copy link
@rainoko

rainoko Oct 10, 2018

Checked. This works like needed.

@davecramer davecramer merged commit 10201f6 into pgjdbc:master Oct 10, 2018

2 checks passed

codecov/project 68.55% (+0.01%) compared to d514ceb
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@davecramer davecramer deleted the davecramer:dsoverwrite branch Oct 10, 2018

public void testOverWriteDSProperties() throws Exception {
PGSimpleDataSource dataSource = new PGSimpleDataSource();
dataSource.setAutosave(AutoSave.CONSERVATIVE);
dataSource.setURL("jdbc:postgresql://localhost:5432/postgres");

This comment has been minimized.

Copy link
@vlsi

vlsi Oct 10, 2018

Member

@davecramer , is dataSource.setURL("jdbc:postgresql://localhost:5432/postgres?autoSave=never"); supported?

@davecramer

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2018

@vlsi define supported..

So if the user does:

dataSource.setAutosave(AutoSave.CONSERVATIVE);
dataSource.setURL("jdbc:postgresql://localhost:5432/postgres?autosave=never");

we would want it to set autosave to never ?

in which case

  if (p.containsKey(property.getName()) || !this.properties.containsKey(property.getName())) { 

set property would make more sense ?

@vlsi

This comment has been minimized.

Copy link
Member

commented Oct 10, 2018

I would expect that ?autosave=never should override existing value stored in dataSource.
In other words, I would expect dataSource.setURL("jdbc:postgresql://localhost:5432/postgres?autosave=never"); to be equivalent to dataSource.setURL("jdbc:postgresql://localhost:5432/postgres"); dataSource.setAutosave(..NEVER);

@davecramer

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2018

See #1309

@rainoko

This comment has been minimized.

Copy link

commented Oct 10, 2018

for example in HikariCP properties are feed with java.util.Properties, which is HashTable. so the property order is not specified.
So maybe if someone defines datasourceProperty explicitly and then with url should be exception.
I do not think that setting properties should be order specific.

@davecramer

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2018

@rainoko

This comment has been minimized.

Copy link

commented Oct 10, 2018

For me the rule seems to be that - you cant specify properties more than once.
But its not so easy to understand how many times user have specified property because after setting url all properties are set at least to default value - if there is one.

@davecramer

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2018

Well it seems to me that anything other than the last one wins is too complicated.

@rainoko

This comment has been minimized.

Copy link

commented Oct 10, 2018

I agree with that

@davecramer

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2018

@rainoko so you are fine with #1309 then?

@rainoko

This comment has been minimized.

Copy link

commented Oct 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.