Skip to content

Conversation

rozza
Copy link
Member

@rozza rozza commented Sep 22, 2025

Avoid appending duplicate metadata

JAVA-5955

Avoid appending duplicate metadata

JAVA-5955
@rozza rozza requested a review from Copilot September 22, 2025 13:14
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the ClientMetadata implementation to avoid appending duplicate metadata to the MongoDB driver information. The changes introduce a new internal DriverInformation class and modify the metadata handling to check for duplicates before appending, addressing issue JAVA-5955.

Key changes:

  • Introduces new internal classes DriverInformation and DriverInformationHelper for managing driver information
  • Updates ClientMetadata to use a list-based approach with duplicate checking
  • Refactors tests to use the new infrastructure and adds comprehensive test coverage for duplicate metadata scenarios

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
AbstractClientMetadataProseTest.java Extensively refactored test methods to use new DriverInformation class and added multiple new test cases for duplicate metadata scenarios
MongoDriverInformationSpec.scala Updated exclusions list to include new synthetic method
MongoDriverInformationSpecification.groovy Updated test comments to reflect "append" vs "prepend" behavior
ClientMetadataTest.java Modified test expectations to align with new metadata building approach
ClientMetadata.java Core refactoring to use DriverInformation list with duplicate checking
package-info.java Added package documentation for internal client classes
DriverInformationHelper.java New utility class for extracting driver information fields
DriverInformation.java New data class representing driver information with proper equals/hashCode
Internal.java New annotation for marking internal APIs
MongoDriverInformation.java Refactored to use DriverInformation internally while maintaining public API

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@rozza rozza marked this pull request as ready for review September 22, 2025 14:13
@rozza rozza requested a review from a team as a code owner September 22, 2025 14:14
@rozza rozza requested review from nhachicha and removed request for a team September 22, 2025 14:14
ElementType.TYPE })
@Documented
@Alpha(Reason.CLIENT)
public @interface Internal {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to add an internal method MongoDriverInformation#getDriverInformationList to get the list of DriverInformation and added this annotation as a way to highlight things for internal use only.

If this ok, then I'll add a ticket to annotation each of the internal packages in their package-info.java

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have VisibleForTesting should we try to unify all these in a similar fashion to ApiStatus
(already supported in IntelliJ ~)

Image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didnt want to misuse that as it's not for testing. It's needed for the ClientMetaData API as well. This all stems from MongoDriverInformation being lossy, I need the combined information of name, version and platform.


import java.util.Objects;

public final class DriverInformation {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes required by the spec ensure that driver information as a whole can be checked for equality - so that duplicates aren't appended.

driverInformationList.add(driverInformation);
}
}
if (hasAddedNewData) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only recalculate the clientMetadataBsonDocument if there has been a real change.

tryWithLimit(client, d -> {
putAtPath(d, "driver.name", driverInformation.getInitialDriverName());
putAtPath(d, "driver.version", driverInformation.getInitialDriverVersion());
tryWithLimit(clientMetadata, d -> {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We add the initial driver information - if there is room. Then later we add the full driver information (eg the extended libraries that use the driver) if there is room.

This behavior is defined by the spec.

* @mongodb.server.release 3.4
*/
public final class MongoDriverInformation {
private final List<String> driverNames;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using lists of Strings, no longer works for the new test cases - as it meant we couldn't check for duplicates effectively.

}

def 'should not prepend data if none has been added'() {
def 'should not append data if none has been added'() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We never prepended the data - so these tests needed some tweaking to make sense.

options.getDriverPlatforms() == ['Java oracle64-1.8.0.31']
}

def 'should error if trying to set a version without setting a name'() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a specific new test from the spec that actually tests we can do this. So removed the check and this test.

}

@ParameterizedTest
@DisplayName("Test 1: Test that the driver updates metadata")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new prose tests. I renamed the existing ones to make it easier for future changes / adding new tests.

ElementType.TYPE })
@Documented
@Alpha(Reason.CLIENT)
public @interface Internal {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have VisibleForTesting should we try to unify all these in a similar fashion to ApiStatus
(already supported in IntelliJ ~)

Image

@rozza
Copy link
Member Author

rozza commented Sep 23, 2025

I used APIStatus in the kafka connector - but it hasn't been adopted by the Java driver. We seem to have our own inhouse annotations that do the same thing. Eg. VisibileForTesting seems to be inspired by their annotations library.

I think there is a ticket to move away from our own null annotations, to a more standardized one. Perhaps we should also look to adopt APIStatus as part of that work. JAVA-5938 thats not scheduled until 6.0.0

@rozza rozza requested a review from nhachicha September 23, 2025 12:25
Copy link
Contributor

@nhachicha nhachicha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's outside the scope of this PR, we can create a ticket to consider folding the @Internal annotation underAPIStatus 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants