Fix #158: support empty Projection clause; add full decoding of projection definition (but not yet processing) #286
Conversation
|
Right now this will only support empty "projection", fail on non-empty. I will create a follow-up for completing handling but want to get this merged so that we can reconcile work for sorting and keep PRs from getting huge. |
tatu-at-datastax
changed the title
|
On a related note @tatu-at-datastax , what do you think about adding support for empty |
Sounds like a good idea to me; if you can file a Github issue, cc Aaron it can be discussed. Uniformity is good I think, as my first impression. |
| return new FindOperation( | ||
| commandContext, | ||
| filters, | ||
| command.buildProjector(), |
There was a problem hiding this comment.
Not sure this is correct, the FindOperation inside FindOneAndUpdate should be passed identityProjector() because the document is merged with updates. Need to apply projection on top document that is returned [here] (https://github.com/stargate/jsonapi/blob/b6d4f73be517cf2e90fb50384e8e60f411754f99/src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/ReadAndUpdateOperation.java#L183). This needs Projection passed into ReadAndUpdateOperation
There was a problem hiding this comment.
Makes sense, will change.
There was a problem hiding this comment.
Changed in 2 commits.
| @@ -181,6 +182,10 @@ private Uni<UpdatedDocument> processUpdate( | |||
| if (returnDocumentInResponse) { | |||
| documentToReturn = | |||
| returnUpdatedDocument ? updatedDocument : originalDocument; | |||
| // In this case, we'll apply projection on before/after document (was | |||
| // not applied during find() phase | |||
| DocumentProjector projector = findOperation().projection(); | |||
There was a problem hiding this comment.
Think ReadAndUpdateOperation should get the projection in it's constructor? As stated in your comment is it part of different commit?
There was a problem hiding this comment.
Right, this will not work; it's the identity one.
There was a problem hiding this comment.
Discussed offline, yes, this would not work. Commented out and will resolve in follow-up PR; merging to avoid blocking.
|
Merged; had to re-run cancelled CI once. |
What this PR does:
Adds support for optional empty "projection" parameter, validation that if passed its JSON Object.
Serves as the base for actual projection implementation
Which issue(s) this PR fixes:
Fixes #158
Checklist