Fix: do not use QuerySpec when SqlContractDefinitionStore#findAll()
#1539
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1539 +/- ##
==========================================
- Coverage 67.16% 67.12% -0.04%
==========================================
Files 736 736
Lines 16139 16144 +5
Branches 1048 1050 +2
==========================================
- Hits 10839 10836 -3
- Misses 4823 4831 +8
Partials 477 477
Continue to review full report at Codecov.
|
paullatzelsperger
force-pushed
the
bugfix/1515_pass_INTEGER_MAXVALUE_in_sql_contractdefstore
branch
from
fd422b6
to
bd98be1
Compare
| @@ -69,7 +69,15 @@ ContractDefinition mapResultSet(ResultSet resultSet) throws Exception { | |||
|
|
|||
| @Override | |||
| public @NotNull Collection<ContractDefinition> findAll() { | |||
| return findAll(QuerySpec.none()).collect(Collectors.toList()); | |||
| return transactionContext.execute(() -> { | |||
There was a problem hiding this comment.
I would make this more explicit as it was done in the SqlAssetIndex at line 132, passing to the other findAll method a QuerySpec instance with an insanely high limit value.
In this way the thing will be explicit (as I would expect this to change in the future), there will be no duplication and no SQL syntax will overflow in the store implementation
There was a problem hiding this comment.
There isn't really any duplication, there is just two (slightly) different SQL statements. In reality those two approaches would have the same effect, and once we have true pagination in IDS, we can remove the parameterless findAll() method completely. So I'd either leave it as is, because after all, it's a "true" findAll or remove the findAll altogether :)
What do you mean by "SQL syntax overflow"?
There was a problem hiding this comment.
having a string with SELECT something FROM somewhere in the Store/Index, we never did that anywhere else, all the SQL stuff is in the Statements class
There was a problem hiding this comment.
ah got it what you mean. How about we remove findAll() altogether then, and provide a static method QuerySpec.max() that creates a QuerySpec with limit = Integer.MAX_VALUE?
There was a problem hiding this comment.
this commit removes the parameterless findAll(). WDYT?
paullatzelsperger
requested review from
ronjaquensel and
juliapampus
as code owners
paullatzelsperger
force-pushed
the
bugfix/1515_pass_INTEGER_MAXVALUE_in_sql_contractdefstore
branch
from
ec62ff0
to
a73fa53
Compare
What this PR changes/adds
Does not use a
QuerySpecwhen executingSqlContractDefinitionStore#findAll, instead execute a plainSELECT * FROM....Why it does that
all = no limits!
Linked Issue(s)
Closes #1515
Checklist
no-changelog)