Skip to content
This repository has been archived by the owner on Jan 19, 2022. It is now read-only.

allow Pageable and Sort in PartTree methods #2394

Merged
merged 3 commits into from
Jun 11, 2020
Merged

allow Pageable and Sort in PartTree methods #2394

merged 3 commits into from
Jun 11, 2020

Conversation

dmitry-s
Copy link
Contributor

fixes #2390

@dmitry-s dmitry-s requested a review from meltsufin May 27, 2020 22:08
@meltsufin meltsufin requested a review from dzou May 28, 2020 13:57
@meltsufin
Copy link
Contributor

@dzou Can you please help review this? Thanks!

dzou
dzou suggested changes May 28, 2020
View reviewed changes
...g/springframework/cloud/gcp/data/spanner/repository/query/SpannerStatementQueryExecutor.java Outdated
@@ -419,8 +419,8 @@ private static String getChildrenSubquery(
return joiner.toString();
}

private static Pair<String, List<String>> buildPartTreeSqlString(PartTree tree,
SpannerMappingContext spannerMappingContext, Class type) {
private static SqlAndTags buildPartTreeSqlString(PartTree tree,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rename this method to buildPartTreeSqlQuery since it is returning a custom object rather than a string.

Also see below; suggestions for renaming SqlAndTags which I think would be more clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the name reflects what this method does.

...er/src/main/java/org/springframework/cloud/gcp/data/spanner/repository/query/SqlAndTags.java Outdated Show resolved Hide resolved
...er/src/main/java/org/springframework/cloud/gcp/data/spanner/repository/query/SqlAndTags.java Outdated Show resolved Hide resolved
...er/src/main/java/org/springframework/cloud/gcp/data/spanner/repository/query/SqlAndTags.java Outdated Show resolved Hide resolved
.../org/springframework/cloud/gcp/data/spanner/repository/query/SpannerStatementQueryTests.java Outdated Show resolved Hide resolved
.../org/springframework/cloud/gcp/data/spanner/repository/query/SpannerStatementQueryTests.java Outdated Show resolved Hide resolved
...g/springframework/cloud/gcp/data/spanner/repository/query/SpannerStatementQueryExecutor.java Outdated Show resolved Hide resolved
...g/springframework/cloud/gcp/data/spanner/repository/query/SpannerStatementQueryExecutor.java Outdated Show resolved Hide resolved
...g/springframework/cloud/gcp/data/spanner/repository/query/SpannerStatementQueryExecutor.java Outdated Show resolved Hide resolved
@dmitry-s dmitry-s requested review from dzou and elefeint May 28, 2020 22:00
@codecov
Copy link

codecov bot commented May 28, 2020 •
edited

Codecov Report

Merging #2394 into master will increase coverage by 0.01%.
The diff coverage is 90.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2394      +/-   ##
============================================
+ Coverage     73.92%   73.93%   +0.01%     
- Complexity     2097     2102       +5     
============================================
  Files           260      261       +1     
  Lines          7577     7589      +12     
  Branches        785      789       +4     
============================================
+ Hits           5601     5611      +10     
- Misses         1614     1617       +3     
+ Partials        362      361       -1
Flag Coverage Δ Complexity Δ
#integration ? ?
#unittests 73.93% <90.00%> (+0.01%) 2102.00 <10.00> (+5.00)
Impacted Files Coverage Δ Complexity Δ
...spanner/repository/query/PartTreeSpannerQuery.java 44.44% <60.00%> (+0.44%) 8.00 <2.00> (ø)
...epository/query/SpannerStatementQueryExecutor.java 85.33% <94.73%> (+0.71%) 70.00 <5.00> (+2.00)
...ner/repository/query/SqlStringAndPlaceholders.java 100.00% <100.00%> (ø) 3.00 <3.00> (?)
...tastore/GcpDatastoreEmulatorAutoConfiguration.java 65.51% <0.00%> (-6.90%) 6.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6bfbaf...2697ebd. Read the comment docs.

meltsufin
meltsufin suggested changes May 29, 2020
View reviewed changes
...g/springframework/cloud/gcp/data/spanner/repository/query/SpannerStatementQueryExecutor.java Outdated
if (tree.isExistsProjection()) {
stringBuilder.append(" LIMIT 1");
}
else if (tree.isLimiting()) {
stringBuilder.append(" LIMIT ").append(tree.getMaxResults());
}
else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can do else if

...va/org/springframework/cloud/gcp/data/spanner/repository/query/SqlStringAndPlaceholders.java Outdated
*
* @author Dmitry Solomakha
*
* @since 1.2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.2.4

...va/org/springframework/cloud/gcp/data/spanner/repository/query/SqlStringAndPlaceholders.java Outdated
*
* @since 1.2
*/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra space

.../springframework/cloud/gcp/data/spanner/repository/it/SpannerRepositoryIntegrationTests.java
@@ -181,6 +182,22 @@ public void queryMethodsTest() {
.findByTraderId("trader2");
assertThat(trader2TradesRetrieved).containsExactlyInAnyOrderElementsOf(trader2Trades);

assertThat(this.tradeRepository
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want, add test for limit in the method name and page request in the params. Which takes precedence?

@sonarcloud
Copy link

sonarcloud bot commented May 29, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 1 Code Smell

90.5% 90.5% Coverage
0.0% 0.0% Duplication

warning The version of Java (1.8.0_151) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more here

@dmitry-s dmitry-s merged commit 55f6a00 into master Jun 11, 2020
@dmitry-s dmitry-s deleted the spanner-pageable branch June 11, 2020 15:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dzou dzou dzou approved these changes

@meltsufin meltsufin meltsufin approved these changes

@elefeint elefeint Awaiting requested review from elefeint

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Pagination does not work with method-style (part-tree) queries
3 participants
@dmitry-s @meltsufin @dzou