From fb6f6694bc41a0ca55c3f7569b0f5d730e5ffddf Mon Sep 17 00:00:00 2001 From: Rob Rudin Date: Tue, 6 Dec 2022 19:00:18 -0500 Subject: [PATCH] Improved test for HTTP chunking Also fixed a bug in the error handling where a `FailedRequest` was constructed and populated, but it was not actually passed into the `FailedRequestException`, thereby causing a null-pointer error when certain parts of the `FailedRequestException` were added. --- Jenkinsfile | 1 - .../marklogic/client/impl/OkHttpServices.java | 54 +++++---- .../client/test/rows/RowManagerTest.java | 105 +++++++++--------- 3 files changed, 86 insertions(+), 74 deletions(-) diff --git a/Jenkinsfile b/Jenkinsfile index fbf28575a..58b5c6bfa 100644 --- a/Jenkinsfile +++ b/Jenkinsfile @@ -83,7 +83,6 @@ pipeline{ export PATH=$GRADLE_USER_HOME:$JAVA_HOME/bin:$PATH cd java-client-api ./gradlew marklogic-client-api:test || true - ./gradlew marklogic-client-api-functionaltests:testFastFunctionalTests || true ''' sh label:'ml development tool test', script: '''#!/bin/bash export JAVA_HOME=$JAVA_HOME_DIR diff --git a/marklogic-client-api/src/main/java/com/marklogic/client/impl/OkHttpServices.java b/marklogic-client-api/src/main/java/com/marklogic/client/impl/OkHttpServices.java index f91042094..8457fbed5 100644 --- a/marklogic-client-api/src/main/java/com/marklogic/client/impl/OkHttpServices.java +++ b/marklogic-client-api/src/main/java/com/marklogic/client/impl/OkHttpServices.java @@ -3986,11 +3986,7 @@ public RESTServiceResultIterator postMultipartForm( requestBldr = setupRequest(requestBldr, multiBuilder, null); requestBldr = addTransactionScopedCookies(requestBldr, transaction); requestBldr = addTelemetryAgentId(requestBldr); - - if ("rows".equals(path)) { - requestBldr.addHeader("ML-Check-ML11-Headers", "true"); - requestBldr.addHeader("TE", "trailers"); - } + requestBldr = addTrailerHeadersIfNecessary(requestBldr, path); Function doPostFunction = funcBuilder -> doPost(reqlog, funcBuilder.header(HEADER_ACCEPT, multipartMixedWithBoundary()), multiBuilder.build()); @@ -4053,11 +4049,7 @@ private U postIteratedResourceImpl( requestBldr = setupRequest(requestBldr, inputMimetype, null); requestBldr = addTransactionScopedCookies(requestBldr, transaction); requestBldr = addTelemetryAgentId(requestBldr); - - if ("rows".equals(path)) { - requestBldr.addHeader("ML-Check-ML11-Headers", "true"); - requestBldr.addHeader("TE", "trailers"); - } + requestBldr = addTrailerHeadersIfNecessary(requestBldr, path); Consumer resendableConsumer = new Consumer() { public void accept(Boolean resendable) { @@ -4394,6 +4386,17 @@ private Request.Builder addTelemetryAgentId(Request.Builder requestBldr) { return requestBldr.header("ML-Agent-ID", "java"); } + private Request.Builder addTrailerHeadersIfNecessary(Request.Builder requestBldr, String path) { + if ("rows".equals(path)) { + // Standard header supported by ML 11 + requestBldr.addHeader("TE", "trailers"); + + // Proprietary header recognized by ML 10.0-9, per https://docs.marklogic.com/guide/relnotes/chap3#id_73268 + requestBldr.addHeader("ML-Check-ML11-Headers", "true"); + } + return requestBldr; + } + private boolean addParts( MultipartBody.Builder multiPart, RequestLogger reqlog, W[] input) { @@ -4594,22 +4597,27 @@ private U makeResults( } List partList = getPartList(entity); + String mlErrorCode = null; + String mlErrorMessage = null; try { - Headers trailer = response.trailers(); - String code = trailer.get("ml-error-code"); - String msg = trailer.get("ml-error-message"); - String sha = trailer.get("ml-content-sha256"); - - if (code != null && !"N/A".equals(code)) { - FailedRequest failure = new FailedRequest(); - failure.setMessageString(code); - failure.setStatusString(msg); - throw new FailedRequestException("failed to " + operation + " " - + entityType + " at rows" + ": " + code + ", " + msg); - } + Headers trailers = response.trailers(); + mlErrorCode = trailers.get("ml-error-code"); + mlErrorMessage = trailers.get("ml-error-message"); } catch (IOException e) { - throw new RuntimeException("No trailer header in repsonse"); + // This does not seem worthy of causing the entire operation to fail; we also don't expect this to occur, as it + // should only occur due to a programming error where the response body has already been consumed + logger.warn("Unexpected IO error while getting HTTP response trailers: " + e.getMessage()); + } + + if (mlErrorCode != null && !"N/A".equals(mlErrorCode)) { + FailedRequest failure = new FailedRequest(); + failure.setMessageString(mlErrorCode); + failure.setStatusString(mlErrorMessage); + failure.setStatusCode(500); + throw new FailedRequestException("failed to " + operation + " " + + entityType + " at rows" + ": " + mlErrorCode + ", " + mlErrorMessage, failure); } + Closeable closeable = response; return makeResults(constructor, reqlog, operation, entityType, partList, response, closeable); } diff --git a/marklogic-client-api/src/test/java/com/marklogic/client/test/rows/RowManagerTest.java b/marklogic-client-api/src/test/java/com/marklogic/client/test/rows/RowManagerTest.java index fa2dffe36..6fa1fe9a6 100644 --- a/marklogic-client-api/src/test/java/com/marklogic/client/test/rows/RowManagerTest.java +++ b/marklogic-client-api/src/test/java/com/marklogic/client/test/rows/RowManagerTest.java @@ -15,38 +15,23 @@ */ package com.marklogic.client.test.rows; -import static org.custommonkey.xmlunit.XMLAssert.assertXMLEqual; -import static org.junit.Assert.assertArrayEquals; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertTrue; - -import java.io.IOException; -import java.io.LineNumberReader; -import java.io.StringWriter; -import java.util.*; -import java.util.stream.Collectors; - -import javax.xml.namespace.QName; -import javax.xml.transform.OutputKeys; -import javax.xml.transform.Transformer; -import javax.xml.transform.TransformerConfigurationException; -import javax.xml.transform.TransformerException; -import javax.xml.transform.TransformerFactory; -import javax.xml.transform.TransformerFactoryConfigurationError; -import javax.xml.transform.dom.DOMSource; -import javax.xml.transform.stream.StreamResult; -import javax.xml.xpath.XPathExpressionException; - -import com.fasterxml.jackson.databind.node.ObjectNode; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.marklogic.client.FailedRequestException; import com.marklogic.client.datamovement.DataMovementManager; import com.marklogic.client.datamovement.WriteBatcher; +import com.marklogic.client.document.DocumentManager; +import com.marklogic.client.expression.PlanBuilder; import com.marklogic.client.io.*; import com.marklogic.client.query.DeleteQueryDefinition; import com.marklogic.client.query.QueryManager; import com.marklogic.client.row.*; +import com.marklogic.client.row.RowManager.RowSetPart; +import com.marklogic.client.row.RowManager.RowStructure; +import com.marklogic.client.row.RowRecord.ColumnKind; +import com.marklogic.client.test.Common; import com.marklogic.client.type.*; +import com.marklogic.client.util.EditableNamespaceContext; import org.junit.AfterClass; import org.junit.BeforeClass; import org.junit.Ignore; @@ -57,15 +42,19 @@ import org.w3c.dom.NodeList; import org.xml.sax.SAXException; -import com.fasterxml.jackson.databind.JsonNode; -import com.fasterxml.jackson.databind.ObjectMapper; -import com.marklogic.client.document.DocumentManager; -import com.marklogic.client.expression.PlanBuilder; -import com.marklogic.client.row.RowManager.RowSetPart; -import com.marklogic.client.row.RowManager.RowStructure; -import com.marklogic.client.row.RowRecord.ColumnKind; -import com.marklogic.client.test.Common; -import com.marklogic.client.util.EditableNamespaceContext; +import javax.xml.namespace.QName; +import javax.xml.transform.*; +import javax.xml.transform.dom.DOMSource; +import javax.xml.transform.stream.StreamResult; +import javax.xml.xpath.XPathExpressionException; +import java.io.IOException; +import java.io.LineNumberReader; +import java.io.StringWriter; +import java.util.*; +import java.util.stream.Collectors; + +import static org.custommonkey.xmlunit.XMLAssert.assertXMLEqual; +import static org.junit.Assert.*; public class RowManagerTest { private static String[] uris = null; @@ -528,26 +517,42 @@ public void testSQL0Result() { } @Test - public void testSQLException() { + public void testErrorWhileStreamingRows() { if (!Common.markLogicIsVersion11OrHigher()) { return; } - RowManager rowMgr = Common.client.newRowManager(); - PlanBuilder p = rowMgr.newPlanBuilder(); - PlanBuilder.ExportablePlan builtPlan = - p.fromSql("select case when lastName = 'Davis' then fn_error(fn_qname('', 'SQL-TABLENOTFOUND'), 'Internal Server Error') end, opticUnitTest.musician.* from (select * from opticUnitTest.musician order by lastName)"); - String exception = ""; - int rowNum = 0; - try { - for (RowRecord row: rowMgr.resultRows(builtPlan)) { - rowNum++; - } - } catch (Exception e) { - exception = e.toString(); - } - assertEquals(0, rowNum); - assertEquals("com.marklogic.client.FailedRequestException: failed to apply resource at rows: SQL-TABLENOTFOUND, Internal Server Error", exception); + final String validQueryThatEventuallyThrowsAnError = "select case " + + "when lastName = 'Davis' then fn_error(fn_qname('', 'SQL-TABLENOTFOUND'), 'Internal Server Error') end, " + + "opticUnitTest.musician.* from (select * from opticUnitTest.musician order by lastName)"; + + RowManager rowManager = Common.client.newRowManager(); + PlanBuilder.ModifyPlan plan = rowManager.newPlanBuilder().fromSql(validQueryThatEventuallyThrowsAnError); + + FailedRequestException ex = assertThrows( + "The SQL query is designed to not immediately fail - it will immediately return a 200 status code to the " + + "Java Client because the query itself can be executed - but will fail later as it streams rows; " + + "specifically, it will fail on the fourth row, which is the 'Davis' row. " + + "If chunking is configured correctly for the /v1/rows requests - i.e. if the " + + "'TE' header is present - then ML should return trailers in the HTTP response named 'ml-error-code' and " + + "'ml-error-message'. Those are intended to indicate that while a 200 was returned, an error occurred later " + + "while streaming data back. The Java Client is then expected to detect those trailers and throw a " + + "FailedRequestException. If the Java Client does not do that, then no exception will be thrown and this " + + "assertion will fail.", FailedRequestException.class, () -> rowManager.resultRows(plan)); + + assertEquals("A 500 is expected, even though ML immediately returned a 200 before it started streaming any data " + + "back; a 500 is used instead of a 400 here as we don't have a reliable way of knowing if the error " + + "occurred due to a bad request by the user, since the query was valid in the sense that it could be " + + "executed", 500, ex.getServerStatusCode()); + + assertEquals("The server error message is expected to be the value of the 'ml-error-message' trailer", + "SQL-TABLENOTFOUND", ex.getServerMessage()); + + assertEquals( + "The exception message is expected to be a formatted message containing the values of the 'ml-error-code' and " + + "'ml-error-message' trailers", + "Local message: failed to apply resource at rows: SQL-TABLENOTFOUND, Internal Server Error. Server Message: SQL-TABLENOTFOUND", + ex.getMessage()); } @Test