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

misc: Add separate class to hold QueryExecutor.execute(...) options #1779

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sehrope
Copy link
Member

@sehrope sehrope commented May 9, 2020

Adds new internal ExecuteOptions and ExecuteManyOptions classes to hold all options for QueryExecutor.execute(...) so that future additions of new parameters can be more easily defaulted in a single location rather than modifying all call sites.

Also adds some legibility for repeated parameters of the same data type as things like maxRows, fetchSize, and even flags are all ints, usually defaulted to zero. Makes it easier to see which non-default value is being assigned.

This doesn't actually change execution, just cleans up the call sites to the QueryExecutor.execute(...) to use the new classes. Also, I don't think this would a breaking changed as I think the QueryExecutor and its interfaces are purely internal.

Passes tests locally though ran into a seemingly unrelated test error on travis (https://travis-ci.org/github/sehrope/pgjdbc/jobs/685079132#L718):

DatabaseMetaDataCacheTest > testGetTypeInfoUsesCache
    java.util.ConcurrentModificationException
        at java.util.ArrayList$Itr.checkForComodification(ArrayList.java:907)
        at java.util.ArrayList$Itr.next(ArrayList.java:857)
        at org.postgresql.util.TestLogHandler.getRecordsMatching(TestLogHandler.java:32)
        at org.postgresql.test.jdbc2.DatabaseMetaDataCacheTest.testGetTypeInfoUsesCache(DatabaseMetaDataCacheTest.java:78)

Let's see if that pops up again.

@davecramer
Copy link
Member

Curious what are you trying to solve here ?

@sehrope
Copy link
Member Author

sehrope commented May 11, 2020

There's a couple of open PRs to add features to the executor, e.g. #1718 or #1735. Each of them adds optional parameters to the QueryExecutor.execute(...) methods. Plus there's enough existing args with the same data type (int) that it's hard to follow what exactly is being invoked. Using a separate options class helps document the existing usage and limit conflicts for future additions.

So this PR doesn't actually add or change anything. It just refactors things a bit for future PRs.

@davecramer
Copy link
Member

Seems reasonable

draderaws and others added 3 commits August 9, 2020 06:23
)

* fix: don't ignore unquoted non-ascii whitespace

The postgresql backend only quotes ascii whitespace.
pgjdbc#1257

add comment to existing isSpace method
change from switch to chained ORs

* test: fix comment in ArrayTest.testDirectFieldString

* fix: revert spaces in array literal

* add changelog entry

add test showing inner white spaces not dropped on unquoted strings by
backend.

Co-authored-by: Vladimir Sitnikov <sitnikov.vladimir@gmail.com>
Co-authored-by: BO8979 <BO8979@W1971362.northamerica.cerner.net>
* remove postgresql-jre6 and postgresql-jre7 projects

* remove remnants of jre6 and jre7
@davecramer
Copy link
Member

@sehrope can you rebase please ?

@sehrope
Copy link
Member Author

sehrope commented Aug 11, 2020

Rebased. I think I might have made the checker happy too. If not I'll take another peek tonight.

Adds new ExecuteOptions and ExecuteManyOptions classes to hold all options for
QueryExecutor.execute(...) so that future additions of new parameters can be
more easily defaulted in a single location rather than modifying all call sites.
Also adds some legibility for repeated parameters of the same data type.
@codecov-commenter
Copy link

Codecov Report

Merging #1779 into master will increase coverage by 0.13%.
The diff coverage is 88.28%.

@@             Coverage Diff              @@
##             master    #1779      +/-   ##
============================================
+ Coverage     69.26%   69.39%   +0.13%     
- Complexity     4206     4222      +16     
============================================
  Files           197      199       +2     
  Lines         17981    18098     +117     
  Branches       2914     2916       +2     
============================================
+ Hits          12455    12560     +105     
- Misses         4181     4188       +7     
- Partials       1345     1350       +5     

@sehrope
Copy link
Member Author

sehrope commented Aug 13, 2020

@davecramer This should be good to go.

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.

None yet

5 participants