MLE-29883 (GH #1938) Include document version in bulk reads

[rjdew-progress]

Impact

Users who rely on MarkLogic optimistic locking cannot use DocumentManager.read(String uris...) to get a usable document version number. This can prevent correct version checks before writes and affect concurrent update handling.

Expected behavior

DocumentManager.read(String uris...) should return DocumentRecords with DocumentDescriptors that include the correct document version number, like DocumentManager.exists(String uri) does.

Actual behavior

DocumentManager.read(String uris...) returns a DocumentPage with DocumentRecords whose DocumentDescriptors always have the version number set to -1.

Steps to reproduce

Use DocumentManager.exists(String uri).

Observe that it returns a DocumentDescriptor with the proper version number set.

Use DocumentManager.read(String uris...).

Observe that it returns a DocumentPage with DocumentRecords whose DocumentDescriptors have the version number set to -1.

GitHub issue #1938

[Copilot]

Pull request overview

This PR updates the Java MarkLogic client to include a usable document version number (for optimistic locking) in DocumentDescriptor objects returned from bulk multi-document reads/searches, aligning DocumentManager.read(String...) behavior with DocumentManager.exists(String).

Changes:

• Extracts document version from multipart Content-Disposition (via versionId) for bulk reads and propagates it into DocumentDescriptor.
• Updates the conditional documents test to assert that bulk read/search descriptor versions are present and match exists().

Reviewed changes

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

File	Description
marklogic-client-api/src/main/java/com/marklogic/client/impl/OkHttpServices.java	Parse versionId from multipart headers and apply it to DocumentDescriptor for bulk document records.
marklogic-client-api/src/test/java/com/marklogic/client/test/ConditionalDocumentTest.java	Strengthen assertions to verify bulk read/search descriptors carry the same version as exists().

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[rjrudin]

One thought on where to put this - I tried to avoid adding anything to OkHttpServices because it's so large already. The trick is where to put it instead, as there's not a good pattern for that.

Nonetheless, I would still lean towards a separate class in the impl.okhttp subpackage. It could be as simple as ContentDispositionUtil with a public static String getVersion. One small bonus would be that it provides a nice home for declaring the version as a private static, e.g. private static final VERSION_PATTERN = "...".

Could also add the ETag fallback, which seems useful since that was the existing behavior - I can't say for sure, but maybe an older version of MarkLogic returned it as an ETag?

[rjdew-progress]

Moved to OkHttpUtil and now fallback to the ETAG header.

[rjrudin]

Yeah, seeing this existing code here makes me think that Copilot's suggestion is a good one about using the ETag as a fallback.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

marklogic-client-api/src/main/java/com/marklogic/client/impl/okhttp/OkHttpUtil.java:277

• getHeaderVersion() only matches a Content-Disposition parameter spelled exactly as " versionId=" (note the required leading space and no quotes). Per header grammar, optional whitespace is allowed and servers may emit ;versionId=... or versionId="...", which would cause version parsing to fail and fall back to UNKNOWN_VERSION (or ETag), reintroducing the original issue.

	public static long getHeaderVersion(BodyPart part) {
		String contentDisposition = getHeader(part, HEADER_CONTENT_DISPOSITION);
		String versionRegex = ".* versionId=([0-9]+).*";
		if (contentDisposition != null && contentDisposition.matches(versionRegex)) {
			String version = contentDisposition.replaceFirst("^.*" + versionRegex + ".*$", "$1");

[rjrudin]

Sorry, this is the only part that I meant moving. I actually would keep the rest in OkHttpServices for now. It could all use a huge refactor to simplify it. My point before was only that I try not to add to it if possible. So I would keep all the other methods in there for now and just put the new code somewhere else.

I actually wouldn't put it in this class, which at least has a small scope for now of creating a client builder object. Maybe a HeaderUtil instead - something with a scope of processing headers from MarkLogic?

[rjdew-progress]

Moved the functions to a separate HeaderUtil class. I still moved other header related functions since they were private, so there isn't a risk of a user being dependent on the them and it'll clean up the OkHttpServices a bit.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (4)

marklogic-client-api/src/main/java/com/marklogic/client/impl/okhttp/OkHttpUtil.java:43

• These static RESTServices header constant imports are unused in OkHttpUtil (this class only builds/configures OkHttpClient instances). Removing them keeps the import block accurate and avoids unused-import lint issues.

import static com.marklogic.client.impl.RESTServices.HEADER_CONTENT_DISPOSITION;
import static com.marklogic.client.impl.RESTServices.HEADER_CONTENT_LENGTH;
import static com.marklogic.client.impl.RESTServices.HEADER_CONTENT_TYPE;
import static com.marklogic.client.impl.RESTServices.HEADER_ETAG;
import static com.marklogic.client.impl.RESTServices.HEADER_ML_EFFECTIVE_TIMESTAMP;
import static com.marklogic.client.impl.RESTServices.HEADER_VND_MARKLOGIC_DOCUMENT_FORMAT;

marklogic-client-api/src/main/java/com/marklogic/client/impl/okhttp/HeaderUtil.java:193

• Use the existing HEADER_CONTENT_DISPOSITION constant instead of a string literal to avoid typos/case mismatches and keep header handling consistent across the codebase.

				// Jakarta Mail's parser failed due to malformed Content-Disposition header.
				// Check if MarkLogic sent a malformed "format=" parameter at the end, which violates RFC 2183.
				String contentDisposition = getHeader(part, "Content-Disposition");
				if (contentDisposition != null && contentDisposition.matches(".*;\\s*format\\s*=\\s*$")) {
					// Remove the trailing "; format=" to fix the malformed header

marklogic-client-api/src/main/java/com/marklogic/client/impl/okhttp/HeaderUtil.java:105

• The versionId regex requires a leading space before "versionId=", so it will fail to extract versions from valid Content-Disposition headers like "...;versionId=123" (no whitespace) or quoted values. This can cause bulk reads to still return UNKNOWN_VERSION.

		String contentDisposition = getHeader(part, HEADER_CONTENT_DISPOSITION);
		String versionRegex = ".* versionId=([0-9]+).*";
		if (contentDisposition != null && contentDisposition.matches(versionRegex)) {
			String version = contentDisposition.replaceFirst("^.*" + versionRegex + ".*$", "$1");
			return Utilities.parseLong(version, DocumentDescriptor.UNKNOWN_VERSION);

marklogic-client-api/src/main/java/com/marklogic/client/impl/okhttp/OkHttpUtil.java:23

• The additional imports added here (MarkLogicIOException, DocumentDescriptor, jakarta.mail types, slf4j, etc.) are not used anywhere in this file, which adds noise and can trigger failures if -Xlint:unused is enabled alongside -Werror.

This issue also appears on line 38 of the same file.

import com.marklogic.client.MarkLogicIOException;
import com.marklogic.client.MarkLogicInternalException;
import com.marklogic.client.document.ContentDescriptor;
import com.marklogic.client.document.DocumentDescriptor;
import com.marklogic.client.extra.okhttpclient.OkHttpClientConfigurator;