Find command implementation #37
Conversation
|
Oh shoot. Did not mean to merge yet... sorry. WIll complete review in the meantime should be fine. |
| type = SchemaType.STRING, | ||
| implementation = String.class) | ||
| String pagingState, | ||
| @Valid |
There was a problem hiding this comment.
How is defaulting handled here, given it's int and not Integer? (compared to limit for example)
Should this actually be Integer?
There was a problem hiding this comment.
The default value will be 0. This is handled in FindCommandResolver. Instead of removing Optional its better to pass as Integer. Will make this change
| boolean readDocument, | ||
| ObjectMapper objectMapper) { | ||
| return queryExecutor | ||
| .executeRead(query, Optional.ofNullable(pagingState), Optional.empty()) | ||
| .executeRead(query, Optional.ofNullable(pagingState), Optional.of(pageSize)) |
There was a problem hiding this comment.
Since it's passed as int, it isn't really optional ever, it can never be absent?
There was a problem hiding this comment.
Will change int to Integer so can handle for null
| int limit = | ||
| command.options() != null && command.options().limit() != 0 | ||
| ? command.options().limit() | ||
| : documentConfig.maxLimit(); |
There was a problem hiding this comment.
Do we not have something like "default limit"? Max limit would imply upper bound for explicit limit, not default?
There was a problem hiding this comment.
This is the maximum documents we will return for a query. Default and maximum are same here.
There was a problem hiding this comment.
Ok I think I get it: if user does not specify limit, we'll indicate "as many as you can get", which is max.
Makes sense.
src/main/java/io/stargate/sgv3/docsapi/service/resolver/model/impl/FindCommandResolver.java
Show resolved
Hide resolved
...in/java/io/stargate/sgv3/docsapi/service/resolver/model/impl/matcher/FilterableResolver.java
Show resolved
Hide resolved
src/main/java/io/stargate/sgv3/docsapi/service/resolver/model/impl/FindCommandResolver.java
Show resolved
Hide resolved
|
Ok, makes sense: I have some questions on defaulting/max limits, but otherwise looks good as initial merge. |
No description provided.