-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add Client Metadata Update Support. #1708
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
Changes from 18 commits
9753f28
889d5c8
4b3065c
2a684bf
28c1844
371ce0b
972fe9d
bcf9cc8
179b262
bc43ba0
3f5fc91
61192d8
e9f8dd6
95c1bb1
dd63a49
89d67bb
8a18d39
f296d0c
a8dc4fb
8ade58b
71350fa
9890aa1
28f7d88
246a040
8a58294
15ecef8
5c7a6e3
f466d08
3132541
d961447
74d8558
31ef18e
38ba5e6
edafc54
6aa0a1e
dab5451
1550122
b5c4c20
b6b4d67
d43972d
73bdbd8
66a5913
26246f0
843d890
850ebe7
d0cabe4
cb40b6a
c44200f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,49 @@ | ||||||
| /* | ||||||
| * Copyright 2008-present MongoDB, Inc. | ||||||
| * | ||||||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||||||
| * you may not use this file except in compliance with the License. | ||||||
| * You may obtain a copy of the License at | ||||||
| * | ||||||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||||||
| * | ||||||
| * Unless required by applicable law or agreed to in writing, software | ||||||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||||||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||||
| * See the License for the specific language governing permissions and | ||||||
| * limitations under the License. | ||||||
| */ | ||||||
|
|
||||||
| package com.mongodb.internal.connection; | ||||||
|
|
||||||
| import com.mongodb.MongoDriverInformation; | ||||||
| import com.mongodb.lang.Nullable; | ||||||
| import org.bson.BsonDocument; | ||||||
|
|
||||||
| import static com.mongodb.internal.connection.ClientMetadataHelper.createClientMetadataDocument; | ||||||
|
||||||
| import static com.mongodb.internal.connection.ClientMetadataHelper.updateClientMedataDocument; | ||||||
|
|
||||||
| /** | ||||||
| * Represents metadata of the current MongoClient. | ||||||
| * | ||||||
| * <p>This class is not part of the public API and may be removed or changed at any time</p> | ||||||
| */ | ||||||
| public class ClientMetadata { | ||||||
| private volatile BsonDocument clientMetadataBsonDocument; | ||||||
jyemin marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
|
|
||||||
| public ClientMetadata(@Nullable final String applicationName, final MongoDriverInformation mongoDriverInformation) { | ||||||
| this.clientMetadataBsonDocument = createClientMetadataDocument(applicationName, mongoDriverInformation); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Returns mutable BsonDocument that represents the client metadata. | ||||||
| */ | ||||||
| public BsonDocument getBsonDocument() { | ||||||
| return clientMetadataBsonDocument; | ||||||
|
||||||
| return clientMetadataBsonDocument; | |
| return clientMetadataBsonDocument.clone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We mostly pass mutable BsonDocuments through layers (except at the public API). To keep consistency in the codebase, I think it’s fine not to clone here. I’m open to defensive copy as well. Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine without defensive copy if it's consistent throughout the codebase.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -32,6 +32,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.io.File; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.nio.charset.StandardCharsets; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.nio.file.Files; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.util.ArrayList; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.util.List; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.util.function.Consumer; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -180,6 +181,38 @@ static boolean clientMetadataDocumentTooLarge(final BsonDocument document) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return buffer.getPosition() > MAXIMUM_CLIENT_METADATA_ENCODED_SIZE; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public static BsonDocument updateClientMedataDocument(final BsonDocument clientMetadataDocument, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public static BsonDocument updateClientMedataDocument(final BsonDocument clientMetadataDocument, | |
| public static BsonDocument updateClientMetadataDocument(final BsonDocument clientMetadataDocument, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Renamed, thanks!
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if the user doesn't supply the driverName when usingMongoDriverInformation.builder(). Should we handle the NPE?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean during initial metadata creation or subsequent updates?
For updates:
If the user-supplied MongoDriverInformation doesn’t include driverName, it’s ignored - null values aren’t added to the list returned by getDriverNames(), so we won’t receive null here:
mongo-java-driver/driver-core/src/main/com/mongodb/MongoDriverInformation.java
Lines 158 to 172 in d65def4
| private List<String> prependToList(final List<String> stringList, final String value) { | |
| if (value == null) { | |
| return stringList; | |
| } else { | |
| ArrayList<String> newList = new ArrayList<>(); | |
| newList.add(value); | |
| newList.addAll(stringList); | |
| return Collections.unmodifiableList(newList); | |
| } | |
| } | |
| private Builder() { | |
| List<String> immutableEmptyList = Collections.emptyList(); | |
| driverInformation = new MongoDriverInformation(immutableEmptyList, immutableEmptyList, immutableEmptyList); | |
| } |
For initial metadata creation:
If the user doesn’t specify driverName, we still include one by default to resulting client metadata BsonDocument. The driver name is initialized internally as it is a required field by the spec:
mongo-java-driver/driver-core/src/main/com/mongodb/internal/connection/ClientMetadataHelper.java
Lines 296 to 305 in 8ade58b
| static MongoDriverInformation getDriverInformation(@Nullable final MongoDriverInformation mongoDriverInformation) { | |
| MongoDriverInformation.Builder builder = mongoDriverInformation != null ? MongoDriverInformation.builder(mongoDriverInformation) | |
| : MongoDriverInformation.builder(); | |
| return builder | |
| .driverName(MongoDriverVersion.NAME) | |
| .driverVersion(MongoDriverVersion.VERSION) | |
| .driverPlatform(format("Java/%s/%s", getProperty("java.vendor", "unknown-vendor"), | |
| getProperty("java.runtime.version", "unknown-version"))) | |
| .build(); | |
| } |
This is also tested via prose tests:
Lines 75 to 82 in 71350fa
| public static Stream<Arguments> provideDriverInformation() { | |
| return Stream.of( | |
| Arguments.of("1.0", "Framework", "Framework Platform"), | |
| Arguments.of("1.0", "Framework", null), | |
| Arguments.of(null, "Framework", "Framework Platform"), | |
| Arguments.of(null, "Framework", null) | |
| ); | |
| } |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tryWithLimit invokes the Consumer<BsonDocument> lambda twice, we could build the appended string outside the tryWithLimit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a fair point. In this case, I’d choose to follow the existing pattern in ClientMetadataHelper to keep the code consistent and neat, as this pattern exists elsewhere in the class. Since updateMetadata is expected to be called most likely once per application lifecycle and isn’t on a hot path or GC-critical, I’d prefer to keep it as is for now. Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for consistency
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we try in a dedicated tryWithLimit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the spec, driver name and version must be updated atomically. If both can't fit, then both must be omitted - so splitting them in a tryWithLimit wouldn't comply with that requirement. We follow the same approach during initial metadata creation:
mongo-java-driver/driver-core/src/main/com/mongodb/internal/connection/ClientMetadataHelper.java
Lines 100 to 103 in 8ade58b
| tryWithLimit(client, d -> { | |
| putAtPath(d, "driver.name", listToString(fullDriverInfo.getDriverNames())); | |
| putAtPath(d, "driver.version", listToString(fullDriverInfo.getDriverVersions())); | |
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AssertJ has been used in this PR and added to test-base as it is a useful library that could be shared across all modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A decision to use a new assertion library seems like something that should be debated a bit more widely, rather than being brought in as part of a PR like this one. How important do you think it is to the assertions required for this PR?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that adopting a new assertion or testing approach should generally be a broader team decision. This isn’t a critical for this PR, so I can remove it from this PR. AssertJ has already been used in the Kotlin coroutine module (reference) and is present in the toml file, though it’s not widely used in the codebase. This PR made the library visible to all modules, based on its usefulness in the Hibernate repository, which was the motivation for extending its usage further.
For now, I will revert this change and we can revisit the discussion as a team.