Skip to content

Commit 71bd35a

Browse files
rjrudinrjrudin
committed
MLE-27078 One more fix for reading JSON as a binary
DocumentManagerImpl was needlessly using a class field (state = bad!) to track whether the user had modified the metadata categories to retrieve. So e.g. in the Spark connector, when we set "ALL" on the doc manager, the state variable was set to true - even though "ALL" is the default. That results in another way of triggering bug MLE-27191. The class field is now gone in favor of a simple private method to determine if the user has modified the metadata categories to something besides just "ALL" - and it's more accurate now. One of the new tests passes as a result - i.e. doing a bulk read after setting metadataCategories to ALL.
1 parent 5e00335 commit 71bd35a

File tree

2 files changed

+72
-56
lines changed

2 files changed

+72
-56
lines changed

‎marklogic-client-api/src/main/java/com/marklogic/client/impl/DocumentManagerImpl.java‎

Lines changed: 29 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -43,30 +43,24 @@
4343
abstract class DocumentManagerImpl<R extends AbstractReadHandle, W extends AbstractWriteHandle>
4444
extends AbstractLoggingManager implements DocumentManager<R, W>,
4545
TemporalDocumentManager<R, W> {
46+
4647
static final private long DEFAULT_PAGE_LENGTH = 50;
4748

4849
static final private Logger logger = LoggerFactory
4950
.getLogger(DocumentManagerImpl.class);
5051

51-
private boolean isProcessedMetadataModified = false;
52-
final private Set<Metadata> processedMetadata = new HashSet<Metadata>() {
53-
@Override
54-
public boolean add(Metadata e) {
55-
isProcessedMetadataModified = true;
56-
return super.add(e);
57-
}
52+
private boolean isMetadataCategoriesNotJustAll() {
53+
return metadataCategories == null ||
54+
metadataCategories.size() != 1 ||
55+
!Metadata.ALL.equals(metadataCategories.iterator().next());
56+
}
5857

59-
@Override
60-
public boolean addAll(Collection<? extends Metadata> c) {
61-
isProcessedMetadataModified = true;
62-
return super.addAll(c);
63-
}
64-
};
65-
{
66-
processedMetadata.add(Metadata.ALL);
67-
// we need to know if the user modifies after us
68-
isProcessedMetadataModified = false;
69-
}
58+
final private Set<Metadata> metadataCategories = new HashSet<>();
59+
60+
{
61+
// Initialize the default metadata to retrieve on a read.
62+
metadataCategories.add(Metadata.ALL);
63+
}
7064

7165
private RESTServices services;
7266
private Format contentFormat;
@@ -113,24 +107,25 @@ public void setContentFormat(Format format) {
113107
@Override
114108
public void setMetadataCategories(Set<Metadata> categories) {
115109
clearMetadataCategories();
116-
processedMetadata.addAll(categories);
110+
metadataCategories.addAll(categories);
117111
}
118112

119113
@Override
120114
public void setMetadataCategories(Metadata... categories) {
121115
clearMetadataCategories();
122-
for (Metadata category : categories)
123-
processedMetadata.add(category);
116+
for (Metadata category : categories) {
117+
metadataCategories.add(category);
118+
}
124119
}
125120

126121
@Override
127122
public Set<Metadata> getMetadataCategories() {
128-
return processedMetadata;
123+
return metadataCategories;
129124
}
130125

131126
@Override
132127
public void clearMetadataCategories() {
133-
processedMetadata.clear();
128+
metadataCategories.clear();
134129
}
135130

136131
@Override
@@ -374,7 +369,7 @@ public <T extends R> T read(DocumentDescriptor desc,
374369
requestLogger,
375370
desc,
376371
transaction,
377-
(metadataHandle != null) ? processedMetadata : null,
372+
(metadataHandle != null) ? metadataCategories : null,
378373
mergeTransformParameters((transform != null) ? transform
379374
: getReadTransform(), extraParams), metadataHandle, contentHandle);
380375

@@ -446,9 +441,8 @@ public DocumentPage read(long serverTimestamp, ServerTransform transform, Transa
446441
requestLogger,
447442
serverTimestamp,
448443
transaction,
449-
// the default for bulk is no metadata, which differs from the normal
450-
// default of ALL
451-
(isProcessedMetadataModified || !withContent) ? processedMetadata : null,
444+
// the default for bulk is no metadata, which differs from the normal default of ALL
445+
(isMetadataCategoriesNotJustAll() || !withContent) ? metadataCategories : null,
452446
nonDocumentFormat,
453447
mergeTransformParameters((transform != null) ? transform : getReadTransform(), extraParams),
454448
withContent,
@@ -542,10 +536,8 @@ private DocumentPage search(SearchQueryDefinition querydef, long start,
542536
}
543537
}
544538

545-
// the default for bulk is no metadata, which differs from the normal
546-
// default of ALL
547-
Set<Metadata> metadata = isProcessedMetadataModified ? processedMetadata
548-
: null;
539+
// the default for bulk is no metadata, which differs from the normal default of ALL
540+
Set<Metadata> metadata = isMetadataCategoriesNotJustAll() ? metadataCategories : null;
549541
return services.getBulkDocuments(requestLogger, serverTimestamp, querydef, start,
550542
getPageLength(), transaction, searchHandle, searchView, metadata,
551543
nonDocumentFormat, getReadTransform(), null, forestName);
@@ -943,7 +935,7 @@ protected TemporalDescriptor write(DocumentDescriptor desc, String temporalDocum
943935
requestLogger,
944936
desc,
945937
transaction,
946-
(metadataHandle != null) ? processedMetadata : null,
938+
(metadataHandle != null) ? metadataCategories : null,
947939
mergeTransformParameters((transform != null) ? transform
948940
: getWriteTransform(), extraParams), metadataHandle, contentHandle);
949941
}
@@ -1266,7 +1258,7 @@ protected DocumentDescriptorImpl create(DocumentUriTemplate template,
12661258
requestLogger,
12671259
template,
12681260
transaction,
1269-
(metadataHandle != null) ? processedMetadata : null,
1261+
(metadataHandle != null) ? metadataCategories : null,
12701262
mergeTransformParameters((transform != null) ? transform
12711263
: getWriteTransform(), extraParams), metadataHandle, contentHandle);
12721264
}
@@ -1326,7 +1318,7 @@ public void patch(DocumentDescriptor desc, DocumentPatchHandle patch,
13261318
DocumentPatchHandleImpl builtPatch = (patch instanceof DocumentPatchHandleImpl) ? (DocumentPatchHandleImpl) patch
13271319
: null;
13281320
services.patchDocument(requestLogger, desc, transaction,
1329-
(builtPatch != null) ? builtPatch.getMetadata() : processedMetadata,
1321+
(builtPatch != null) ? builtPatch.getMetadata() : metadataCategories,
13301322
(builtPatch != null) ? builtPatch.isOnContent() : true, patch);
13311323
}
13321324

@@ -1360,7 +1352,7 @@ public void patch(String uri, String temporalDocumentURI, String temporalCollec
13601352
extraParams = addTemporalParams(extraParams, temporalCollection, temporalDocumentURI, null);
13611353
DocumentPatchHandleImpl builtPatch = (patch instanceof DocumentPatchHandleImpl) ? (DocumentPatchHandleImpl) patch
13621354
: null;
1363-
services.patchDocument(requestLogger, new DocumentDescriptorImpl(uri, true), transaction, (builtPatch != null) ? builtPatch.getMetadata() : processedMetadata,
1355+
services.patchDocument(requestLogger, new DocumentDescriptorImpl(uri, true), transaction, (builtPatch != null) ? builtPatch.getMetadata() : metadataCategories,
13641356
(builtPatch != null) ? builtPatch.isOnContent() : true, extraParams, sourceDocumentURI, patch);
13651357
}
13661358

@@ -1416,7 +1408,7 @@ public void writeDefaultMetadata(String uri, Transaction transaction)
14161408
logger.info("Resetting metadata for {}", uri);
14171409

14181410
services.deleteDocument(requestLogger,
1419-
new DocumentDescriptorImpl(uri, true), transaction, processedMetadata,
1411+
new DocumentDescriptorImpl(uri, true), transaction, metadataCategories,
14201412
getWriteParams());
14211413
}
14221414

@@ -1433,7 +1425,7 @@ public void writeDefaultMetadata(Transaction transaction, String... uris)
14331425
if (uris.length == 0)
14341426
throw new IllegalArgumentException(
14351427
"Resetting document metadata with empty identifier list");
1436-
services.delete(requestLogger, transaction, processedMetadata, uris);
1428+
services.delete(requestLogger, transaction, metadataCategories, uris);
14371429
}
14381430

14391431
@Override

‎marklogic-client-api/src/test/java/com/marklogic/client/test/document/ReadJsonAsBinaryTest.java‎

Lines changed: 43 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -17,34 +17,39 @@
1717

1818
import static org.junit.jupiter.api.Assertions.assertEquals;
1919

20+
@Disabled("See MLE-27191 for description of the server bug")
2021
class ReadJsonAsBinaryTest extends AbstractClientTest {
2122

23+
private static final String URI = "/a.json";
24+
2225
@Test
23-
@Disabled("See MLE-27191 for description of the server bug")
24-
void test() {
26+
void genericDocumentManager() {
2527
try (DatabaseClient client = Common.newClient()) {
26-
final String uri = "/a.json";
27-
writeJsonAsBinary(uri, client);
28+
writeJsonAsBinary(URI, client);
2829

2930
GenericDocumentManager docManager = client.newDocumentManager();
31+
// A bulk read works...
32+
verifyBulkRead(docManager);
33+
// But a request for a single URI does not!
34+
verifySingleRead(docManager);
35+
}
36+
}
3037

31-
// A multipart request returns the correct document format.
32-
try (DocumentPage page = docManager.read(uri)) {
33-
DocumentRecord record = page.next();
34-
assertEquals(uri, record.getUri());
35-
assertEquals("application/json", record.getMimetype());
36-
assertEquals(Format.BINARY, record.getFormat());
37-
}
38+
@Test
39+
void managerWithModifiedMetadataCategories() {
40+
try (DatabaseClient client = Common.newClient()) {
41+
writeJsonAsBinary(URI, client);
3842

39-
// But a request for a single URI does not!
40-
try (InputStreamHandle handle = docManager.read(uri, new InputStreamHandle())) {
41-
assertEquals("application/json", handle.getMimetype());
42-
assertEquals(Format.BINARY, handle.getFormat(), "Unfortunately due to MLE-27191, the " +
43-
"format is JSON instead of binary. So this assertion will fail until that bug is fixed. " +
44-
"For a Java Client user, the workaround is to use the read method above and get a DocumentPage " +
45-
"back, as a multipart response seems to identify the correct headers.");
46-
}
43+
GenericDocumentManager docManager = client.newDocumentManager();
4744

45+
// Setting metadata categories to ALL should have no effect since that results in no metadata being
46+
// retrieved on a bulk read.
47+
docManager.setMetadataCategories(DocumentManager.Metadata.ALL);
48+
verifyBulkRead(docManager);
49+
50+
// Setting metadata categories to anything besides ALL will cause the bug
51+
docManager.setMetadataCategories(DocumentManager.Metadata.PERMISSIONS);
52+
verifyBulkRead(docManager);
4853
}
4954
}
5055

@@ -56,4 +61,23 @@ private void writeJsonAsBinary(String uri, DatabaseClient client) {
5661
jsonDocManager.setWriteTransform(new ServerTransform("toBinary"));
5762
jsonDocManager.write(uri, metadata, new JacksonHandle(new ObjectMapper().createObjectNode().put("a", 1)));
5863
}
64+
65+
private void verifySingleRead(GenericDocumentManager docManager) {
66+
try (InputStreamHandle handle = docManager.read(URI, new InputStreamHandle())) {
67+
assertEquals("application/json", handle.getMimetype());
68+
assertEquals(Format.BINARY, handle.getFormat(), "Unfortunately due to MLE-27191, the " +
69+
"format is JSON instead of binary. So this assertion will fail until that bug is fixed. " +
70+
"For a Java Client user, the workaround is to use the read method above and get a DocumentPage " +
71+
"back, as a multipart response seems to identify the correct headers.");
72+
}
73+
}
74+
75+
private void verifyBulkRead(GenericDocumentManager docManager) {
76+
try (DocumentPage page = docManager.read(URI)) {
77+
DocumentRecord record = page.next();
78+
assertEquals(URI, record.getUri());
79+
assertEquals("application/json", record.getMimetype());
80+
assertEquals(Format.BINARY, record.getFormat());
81+
}
82+
}
5983
}

0 commit comments

Comments
 (0)