spring-projects / spring-data-couchbase

Provides support to increase developer productivity in Java when using Couchbase. Uses familiar Spring concepts such as a template classes for core API usage and lightweight repository style data access.
https://spring.io/projects/spring-data-couchbase
Apache License 2.0
275 stars 191 forks source link

Update sub-document or attributes with query #1586

Closed rbleuse closed 1 year ago

rbleuse commented 1 year ago

Hi, I'm trying to update some attributes with a custom query, but I get the following exception :

exception ``` com.couchbase.client.core.error.InvalidArgumentException: Unsupported type for JSON value: class com.rbleuse.spring.reactive.couchbase.model.Person at com.couchbase.client.core.error.InvalidArgumentException.fromMessage(InvalidArgumentException.java:28) ~[core-io-2.3.4.jar:na] Suppressed: reactor.core.publisher.FluxOnAssembly$OnAssemblyException: Error has been observed at the following site(s): *__checkpoint ⇢ org.springframework.boot.actuate.metrics.web.reactive.server.MetricsWebFilter [DefaultWebFilterChain] *__checkpoint ⇢ HTTP PUT "/classroom" [ExceptionHandlingWebHandler] Original Stack Trace: at com.couchbase.client.core.error.InvalidArgumentException.fromMessage(InvalidArgumentException.java:28) ~[core-io-2.3.4.jar:na] at com.couchbase.client.java.json.JsonValue.coerce(JsonValue.java:94) ~[java-client-3.3.4.jar:na] at com.couchbase.client.java.json.JsonArray.add(JsonArray.java:178) ~[java-client-3.3.4.jar:na] at org.springframework.data.couchbase.repository.query.StringBasedN1qlQueryParser.putPositionalValue(StringBasedN1qlQueryParser.java:495) ~[spring-data-couchbase-4.4.3.jar:4.4.3] at org.springframework.data.couchbase.repository.query.StringBasedN1qlQueryParser.getPositionalPlaceholderValues(StringBasedN1qlQueryParser.java:437) ~[spring-data-couchbase-4.4.3.jar:4.4.3] at org.springframework.data.couchbase.repository.query.StringBasedN1qlQueryParser.getPlaceholderValues(StringBasedN1qlQueryParser.java:486) ~[spring-data-couchbase-4.4.3.jar:4.4.3] at org.springframework.data.couchbase.core.query.StringQuery.toN1qlSelectString(StringQuery.java:80) ~[spring-data-couchbase-4.4.3.jar:4.4.3] ```

Here is my document :

@Document
@Scope("dev")
@Collection("classroom")
data class Classroom(
    @field:Id
    val id: String,

    @field:Field
    val person: Person,

    @field:Field
    val roomNumber: String
)

Here is my Person :

data class Person(
    @field:Field
    val firstName: String,

    @field:Field
    val lastName: String
)

And here is my query :

@Query("""
        UPDATE #{#n1ql.bucket} c
        USE KEYS $1 SET
        c.person = $2
        WHERE c.#{#n1ql.filter}""")
    fun updateClassroom(id: String, person: Person)

It seems that the sdk requires a json object, so the only workaround I can think of is converting my Person with the MappingCouchbaseConverter.write(source, target) which will convert with correct field names and types. But this is an ugly solution, as the converter requires an id attribute else it'll throw An ID property is needed, but not found

mikereiche commented 1 year ago

Why not...

fun updateClassroom(id: String, person: Person) {
  Classroom classroom = classroomRepository.findById(id);
  classroom.person = person;
  classroomRepository.save(classroom);
}
rbleuse commented 1 year ago

Yes updating the whole document works but here my point is to update some parts of the document only (what we can do with an update query, cf https://docs.couchbase.com/server/current/n1ql/n1ql-language-reference/update.html)

My use case is, there are multiple processes that needs to update some attributes only on the same document, and updating the whole document will allow some race conditions and potentially data loss if 2 processes persist the document at the same time. That's why I need to update only some attributes, as it's possible to do with the java sdk

mikereiche commented 1 year ago

That's why I need to update only some attributes, as it's possible to do with the java sdk

Can you show me an example of this? Are you referring to sub-doc mutations? That's not supported in spring data couchbase.

and updating the whole document will allow some race conditions and potentially data loss

I'm not 100% sure that UPDATE on different parts of the document will avoid that. I'm checking with folks that would know.

You can certainly do all those three examples in https://docs.couchbase.com/server/current/n1ql/n1ql-language-reference/update.html, because they are just setting a simple attributes to a value. String, Long, long, integer etc. are all valid args to methods.

But in your case, you are setting a Person which would require a json arg. If you provide json for the arg, it should work. GsonBuilder can be used for creating Json from arbitrary java objects

    public String toString() throws RuntimeException {
        Gson gson = new GsonBuilder().create();
        return gson.toJson(this);
    }

updating the whole document will allow some race conditions and potentially data loss if 2 processes persist the document at the same time

If your entity has an @Version, spring-data-couchbase will use the CAS to do optimistic locking - it will throw an exception if the cas in the server does not match the cas of the incoming update.

Another solution is to use transactions, which will automatically retry in the event of a write-write conflict (it uses CAS)

rbleuse commented 1 year ago

Can you show me an example of this? Are you referring to sub-doc mutations? That's not supported in spring data couchbase.

yes I was referring to the sub-doc mutations

I'm not 100% sure that UPDATE on different parts of the document will avoid that. I'm checking with folks that would know.

Let's say I have an asynchronous process that needs to update only my person attribute, and another process that needs to update only the roomNumber. There is a chance that both processes run at same time, and if both processes replace the document, only the last version will be saved. Updating some parts of my document fixes this, and avoids me to do a read-before-write

GsonBuilder can be used for creating Json from arbitrary java objects

Indeed but if I rename some attributes with @Field or if I need specific converters, GsonBuilder won't help me. I think the only way is to use the provided MappingCouchbaseConverter

mikereiche commented 1 year ago

There is a chance that both processes run at same time, and if both processes replace the document, only the last version will be saved.

Like I said, if you have an @Version, you'd get a cas-mismatch on one of the save operations. And then your application could re-read, update and write.

Updating some parts of my document fixes this,

I don't think so. Couchbase database cannot lock part of a document for write (there is only one cas for the whole document). So to do an update of a single property, it still needs to (internally) read-update-write the whole document (the purpose of sub-doc operations is to avoid client-round-trip of the whole document, not lock part of a document). And I think internally it uses cas (at least I hope it does), and you'd get a cas-mismatch failure. I asked folks who should know, one person on the SDK team says there would be a cas-mismatch, but it's really a server behaviour, and am hoping for an answer from the server team. (if it does not use cas and does not check for cas-mismatch then there is no way to detect data loss from a write-write conflict).

[Edit: I have verification that UPDATE does use cas and will generate a cas-mismatch error if it occurs]

and avoids me to do a read-before-write Only the first read. On a cas-mismatch error - either internally by "update ... " or externally using the kv api, the application would still need to re-read, then update, then write.

So you have two choices - have your application leverage cas by having an @Version in your entity and by doing read-update-write. Or use transactions, which will do the read-before-write and check the cas for you. And retry on failure.

Even if couchbase n1ql "UPDATE ...." internally uses sub-docs this won't help since there is only one cas for the whole document, concurrent updates of different sub-docs will still have a cas-mismatch fail and the application will still need to reread-update-write.

p.s. even if "update" did lock-for-write a specific attribute on a document, your application would still need to handle the case of concurrent writes on the same attribute. (think bank-account-balance).

mikereiche commented 1 year ago

If allowing the MappingCouchbaseConverter to convert an entity to json will help, then we can expose an api that does that (without requiring the entity have an id). writeInternalRoot() will work - it just needs to be made exposed.

source // a repository object
CouchbaseDocument target = new CouchbaseDocument();
writeInternalRoot(source, target, ClassTypeInformation.from(source.getClass()), false, null);       
JsonObject jo=JsonObject.from(target.export())
rbleuse commented 1 year ago

[Edit: I have verification that UPDATE does use cas and will generate a cas-mismatch error if it occurs]

Oh I didn't know that part. So even if I'm using the update feature (mutateIn or query), internally it still read the document, update fields I specify, then save the whole document using cas-mismatch check ?

then we can expose an api that does that (without requiring the entity have an id).

For now I can bypass the mandatory id by setting a dummy value like this :

val target = CouchbaseDocument("id")
converter.write(source, target)

But exposing a direct api that doesn't require that id would be better. Furthermore in order to keep things immutable, it'd be even better to expose an api that returns directly the CouchbaseDocument (or JsonObject) instead of requiring it. Something like this :

public CouchbaseDocument convertToDocument(final Object source) {
    CouchbaseDocument target = new CouchbaseDocument();
    ...
    return target
}
mikereiche commented 1 year ago

So even if I'm using the update feature (mutateIn or query), internally it still read the document, update fields I specify, then save the whole document using cas-mismatch check ?

Yes. In their words: "UPDATE reads document with cas, then apply all expression's and construct new document and write with cas. If there is cas mismatch it will return error and user has to retry operation."

But this should have a much shorter window in between the get() and replace(), so a cas-mismatch should be much less likely.

rbleuse commented 1 year ago

Got it, thank you for the confirmation !

mikereiche commented 1 year ago

The enhancement would be to handle arbitrary (entity) objects as parameters, convert them to json using

Edit: this is actually an omission in parameter handling. It needs to handle not only simple types, but also collections and non-Simple types (like Person).

rbleuse commented 1 year ago

Hi @mikereiche, since this change, I'm not able to update a list with a query

Here is my query :

@Query(
        """UPDATE #{#n1ql.bucket} c
        USE KEYS ${'$'}clubId SET
        c.moderatorIds = ${'$'}moderatorIds,
        c.membershipCountMap = ${'$'}membershipCountMap
        WHERE c.#{#n1ql.filter}"""
    )
    fun updateMembership(
        @Param("clubId") clubId: UUID,
        @Param("moderatorIds") moderatorIds: List<String>,
        @Param("membershipCountMap") membershipCountMap: Map<String, Long>
    )

Here is the exception :

Unsupported type for JSON value: class org.springframework.data.couchbase.core.mapping.CouchbaseList
com.couchbase.client.core.error.InvalidArgumentException: Unsupported type for JSON value: class org.springframework.data.couchbase.core.mapping.CouchbaseList
    at app//com.couchbase.client.core.error.InvalidArgumentException.fromMessage(InvalidArgumentException.java:28)
    at app//com.couchbase.client.java.json.JsonValue.coerce(JsonValue.java:94)
    at app//com.couchbase.client.java.json.JsonObject.put(JsonObject.java:222)
    at app//org.springframework.data.couchbase.repository.query.StringBasedN1qlQueryParser.putNamedValue(StringBasedN1qlQueryParser.java:526)
    at app//org.springframework.data.couchbase.repository.query.StringBasedN1qlQueryParser.getNamedPlaceholderValues(StringBasedN1qlQueryParser.java:468)
    at app//org.springframework.data.couchbase.repository.query.StringBasedN1qlQueryParser.getPlaceholderValues(StringBasedN1qlQueryParser.java:494)
    at app//org.springframework.data.couchbase.core.query.StringQuery.toN1qlSelectString(StringQuery.java:80)

The List is converted into a CouchbaseList and then the update fails. Trying with an array produces the same exception

mikereiche commented 1 year ago

It looks like handling lists for named parameters was missing from the change. Work-around is to used ordered parameters instead $1, $2, $3 instead of $moderatorIds etc.

mikereiche commented 1 year ago

The change also includes a fix to handle lists of non-simple types (original would give NPE for non-simple lists for both named and ordered parameters).