Support returning any deleted entities in ReactiveRemoveByIdOperation and ReactiveRemoveByQueryOperation

[aaronjwhiteside]

This should work alongside cas support in PR #1051

Idea for API:

MyEntity entity = template.removeById(MyEntity.class)
      .withCas(12345L)
      .one("abc123");

It would be good if this support was also extended to org.springframework.data.couchbase.repository.CouchbaseRepository

Right now we have a custom base repository exposing delete methods that return the deleted document.

Having this in the base implementation would reduce the surface area of our custom base repository and I suspect be helpful to others too.

[mikereiche]

I get it, but the couchbase API doesn't return the deleted document, there isn't even an option to return it. Probably because it would take as long as a get() and a remove(). Although N1QL delete has a "returning" clause, performance of the KV api is much better. And the spring CrudRepository interface delete methods all return void, and as far as I know, this is not supported on any other spring-data project.

[aaronjwhiteside]

My bad, in the example I used removeById, in reality we mostly delete entities using n1ql queries, for conditional deletes.

Assuming removeById() works withCas() for the KV store case (to avoid a race condition between GET'ing and DELETE'ing the document via KV store).

Agree on CrudRepository but there is nothing stopping you from adding these methods to the CouchbaseRepository.

For our use case, we return the deleted resource in our REST APIs. shrug

Perhaps something like this for the removeByQuery use case.

MyEntity entity = template.removeByQuery(MyEntity.class)
      .matching(...)
      .returning()
      .one();

In the meantime we workaround this by using a custom repository interface.

[mikereiche]

"we mostly delete entities using n1ql queries"

That helps.

"there is nothing stopping you from adding these methods to the CouchbaseRepository"

Not technically, but I've tried making additions to CouchbaseRepository in the past, and they were flat out rejected by spring data, as use of those extensions would break the principle of being able to substitute different spring-data-* implementations in an application. Pointing out that there was already one Couchbase-specific method, findAll(QueryScanConsistency), didn't help.

There is more freedom with the repository queries derived from method names. We can do something with that. I'll accept this as an enhancement.

[mikereiche]

There is a server issue describing why get-delete could be bad: https://issues.couchbase.com/browse/MB-31401

"Note that ARRAY_POP and similar have been considered in the past (they were suggested the original design). I can't find a link now, but the rationale why we decided against adding ARRAY_POP was this would introduce a destructive, non-idempotent scenario -

Consider if there was a transient error (e.g network) error during the ARRAY_POP, then the client wouldn't know if:

the operation completed successfully (but they never saw the result), or The operation never occurred (request was lost). As such, it would be difficult for applications to handle such errors correctly (this isn't the only non-idempotent example in subdoc, but it is one of the tricky ones as it is destructive).

By using the current two-stage approach:

(GET(array[-1]); on success perform a CAS DELETE(array[-1]) The dataloss is avoided - if the GET times out you simply retry; if the DELETE times out you can retry with the same CAS value until you either succeed or get a CAS mismatch (meaning someone else has since added an element)."

[mikereiche]

blocked on couchbase java sdk providing this feature.