Skip to content

Update sub-document or attributes with query #1586

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
rbleuse opened this issue Oct 11, 2022 · 13 comments · Fixed by #1595 or #1606
Closed

Update sub-document or attributes with query #1586

rbleuse opened this issue Oct 11, 2022 · 13 comments · Fixed by #1595 or #1606
Assignees
Labels
type: enhancement A general enhancement

Comments

@rbleuse
Copy link
Contributor

rbleuse commented Oct 11, 2022

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

  • Is that possible to update some of my document attributes without manually convert my parameters ?
  • Is there any other elegant solution ?
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 11, 2022
@mikereiche
Copy link
Collaborator

Why not...

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

@mikereiche mikereiche added status: feedback-provided Feedback has been provided and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 11, 2022
@mikereiche mikereiche self-assigned this Oct 11, 2022
@rbleuse
Copy link
Contributor Author

rbleuse commented Oct 11, 2022

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
Copy link
Collaborator

mikereiche commented Oct 11, 2022

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
Copy link
Contributor Author

rbleuse commented Oct 12, 2022

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
Copy link
Collaborator

mikereiche commented Oct 12, 2022

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
Copy link
Collaborator

mikereiche commented Oct 12, 2022

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
Copy link
Contributor Author

rbleuse commented Oct 12, 2022

[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
Copy link
Collaborator

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.

@mikereiche mikereiche added type: enhancement A general enhancement and removed status: feedback-provided Feedback has been provided labels Oct 13, 2022
@rbleuse
Copy link
Contributor Author

rbleuse commented Oct 13, 2022

Got it, thank you for the confirmation !

@mikereiche
Copy link
Collaborator

mikereiche commented Oct 13, 2022

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
Copy link
Contributor Author

rbleuse commented Nov 2, 2022

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 mikereiche reopened this Nov 2, 2022
@mikereiche
Copy link
Collaborator

mikereiche commented Nov 2, 2022

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 added a commit that referenced this issue Nov 2, 2022
mikereiche added a commit that referenced this issue Nov 2, 2022
@mikereiche
Copy link
Collaborator

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
3 participants