asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Murtadha Hubail (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: [ASTERIXDB-2122][API] Ensure Valid JSON on Result Printing
Date Thu, 05 Oct 2017 21:29:56 GMT
Murtadha Hubail has submitted this change and it was merged.

Change subject: [ASTERIXDB-2122][API] Ensure Valid JSON on Result Printing
......................................................................


[ASTERIXDB-2122][API] Ensure Valid JSON on Result Printing

- user model changes: no
- storage format changes: no
- interface changes: no

Details:
- Ensure results JSON field is closed when an exception is
  encountered.
- Always return errors field if it exists in the API response.
- Add test case.

Change-Id: Ic16e9d234c5e127ea24e382c49f3c7cfa5bce9c7
Reviewed-on: https://asterix-gerrit.ics.uci.edu/2057
Sonar-Qube: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Tested-by: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Contrib: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Integration-Tests: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Reviewed-by: Ian Maxon <imaxon@apache.org>
---
M asterixdb/asterix-app/src/main/java/org/apache/asterix/app/result/ResultPrinter.java
A asterixdb/asterix-app/src/test/java/org/apache/asterix/app/result/ResultPrinterTest.java
M asterixdb/asterix-app/src/test/java/org/apache/asterix/test/common/ResultExtractor.java
3 files changed, 146 insertions(+), 32 deletions(-)

Approvals:
  Jenkins: Verified; No violations found; ; Verified
  Ian Maxon: Looks good to me, approved

Objections:
  Anon. E. Moose #1000171: 



diff --git a/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/result/ResultPrinter.java
b/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/result/ResultPrinter.java
index 04ac0b3..c8d25f1 100644
--- a/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/result/ResultPrinter.java
+++ b/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/result/ResultPrinter.java
@@ -200,32 +200,33 @@
 
     public void print(ResultReader resultReader) throws HyracksDataException {
         printPrefix();
+        try {
+            final IFrameTupleAccessor fta = resultReader.getFrameTupleAccessor();
+            final IFrame frame = new VSizeFrame(resultDisplayFrameMgr);
 
-        final IFrameTupleAccessor fta = resultReader.getFrameTupleAccessor();
-        final IFrame frame = new VSizeFrame(resultDisplayFrameMgr);
-
-        while (resultReader.read(frame) > 0) {
-            final ByteBuffer frameBuffer = frame.getBuffer();
-            final byte[] frameBytes = frameBuffer.array();
-            fta.reset(frameBuffer);
-            final int last = fta.getTupleCount();
-            for (int tIndex = 0; tIndex < last; tIndex++) {
-                final int start = fta.getTupleStartOffset(tIndex);
-                int length = fta.getTupleEndOffset(tIndex) - start;
-                if (conf.fmt() == SessionConfig.OutputFormat.CSV
-                        && ((length > 0) && (frameBytes[start + length
- 1] == '\n'))) {
-                    length--;
+            while (resultReader.read(frame) > 0) {
+                final ByteBuffer frameBuffer = frame.getBuffer();
+                final byte[] frameBytes = frameBuffer.array();
+                fta.reset(frameBuffer);
+                final int last = fta.getTupleCount();
+                for (int tIndex = 0; tIndex < last; tIndex++) {
+                    final int start = fta.getTupleStartOffset(tIndex);
+                    int length = fta.getTupleEndOffset(tIndex) - start;
+                    if (conf.fmt() == SessionConfig.OutputFormat.CSV
+                            && ((length > 0) && (frameBytes[start + length
- 1] == '\n'))) {
+                        length--;
+                    }
+                    String result = new String(frameBytes, start, length, UTF_8);
+                    if (wrapArray && notFirst) {
+                        output.out().print(", ");
+                    }
+                    notFirst = true;
+                    displayRecord(result);
                 }
-                String result = new String(frameBytes, start, length, UTF_8);
-                if (wrapArray && notFirst) {
-                    output.out().print(", ");
-                }
-                notFirst = true;
-                displayRecord(result);
+                frameBuffer.clear();
             }
-            frameBuffer.clear();
+        } finally {
+            printPostfix();
         }
-
-        printPostfix();
     }
 }
diff --git a/asterixdb/asterix-app/src/test/java/org/apache/asterix/app/result/ResultPrinterTest.java
b/asterixdb/asterix-app/src/test/java/org/apache/asterix/app/result/ResultPrinterTest.java
new file mode 100644
index 0000000..6810e19
--- /dev/null
+++ b/asterixdb/asterix-app/src/test/java/org/apache/asterix/app/result/ResultPrinterTest.java
@@ -0,0 +1,107 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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 org.apache.asterix.app.result;
+
+import static org.apache.hyracks.util.StorageUtil.StorageUnit.KILOBYTE;
+
+import java.io.ByteArrayOutputStream;
+import java.io.PrintWriter;
+import java.nio.charset.StandardCharsets;
+
+import org.apache.asterix.api.http.server.AbstractQueryApiServlet;
+import org.apache.asterix.api.http.server.ResultUtil;
+import org.apache.asterix.common.api.IApplicationContext;
+import org.apache.asterix.common.config.CompilerProperties;
+import org.apache.asterix.common.exceptions.AsterixException;
+import org.apache.asterix.test.common.ResultExtractor;
+import org.apache.asterix.translator.IStatementExecutor;
+import org.apache.asterix.translator.SessionConfig;
+import org.apache.asterix.translator.SessionOutput;
+import org.apache.commons.io.IOUtils;
+import org.apache.hyracks.util.StorageUtil;
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+public class ResultPrinterTest {
+
+    /**
+     * Ensures that a valid JSON is returned when an exception is encountered
+     * while printing results and that the errors field is returned in the
+     * presence of results fields.
+     *
+     * @throws Exception
+     */
+    @Test
+    public void exceptionWhilePrinting() throws Exception {
+        final RuntimeException expectedException = new RuntimeException("Error reading result");
+        final IApplicationContext appCtx = Mockito.mock(IApplicationContext.class);
+        final CompilerProperties compilerProperties = Mockito.mock(CompilerProperties.class);
+        Mockito.when(appCtx.getCompilerProperties()).thenReturn(compilerProperties);
+        Mockito.when(compilerProperties.getFrameSize()).thenReturn(StorageUtil.getIntSizeInBytes(32,
KILOBYTE));
+        final ByteArrayOutputStream baos = new ByteArrayOutputStream();
+        final PrintWriter out = new PrintWriter(baos, true);
+        final SessionOutput sessionOutput = createSessionOutput(out);
+        final ResultPrinter rs = new ResultPrinter(appCtx, sessionOutput, new IStatementExecutor.Stats(),
null);
+        final ResultReader resultReader = Mockito.mock(ResultReader.class);
+        // throw exception after the resultPrefix is written
+        Mockito.when(resultReader.getFrameTupleAccessor()).thenThrow(new RuntimeException(expectedException));
+        out.print("{");
+        try {
+            rs.print(resultReader);
+        } catch (RuntimeException e) {
+            ResultUtil.printError(out, e, true);
+            printMetrics(out, 1);
+        }
+        out.print("}");
+        out.flush();
+        final String resultStr = new String(baos.toByteArray(), StandardCharsets.UTF_8);
+        boolean exceptionThrown = false;
+        try {
+            // ensure result is valid json and error will be returned and not results.
+            ResultExtractor.extract(IOUtils.toInputStream(resultStr, StandardCharsets.UTF_8));
+        } catch (AsterixException e) {
+            exceptionThrown = true;
+            Assert.assertTrue(e.getMessage().contains(expectedException.getMessage()));
+        }
+        Assert.assertTrue(exceptionThrown);
+    }
+
+    private static SessionOutput createSessionOutput(PrintWriter resultWriter) {
+        SessionOutput.ResultDecorator resultPrefix = ResultUtil.createPreResultDecorator();
+        SessionOutput.ResultDecorator resultPostfix = ResultUtil.createPostResultDecorator();
+        SessionOutput.ResultAppender appendHandle = ResultUtil.createResultHandleAppender(null);
+        SessionOutput.ResultAppender appendStatus = ResultUtil.createResultStatusAppender();
+        SessionConfig.OutputFormat format = SessionConfig.OutputFormat.CLEAN_JSON;
+        SessionConfig sessionConfig = new SessionConfig(format);
+        sessionConfig.set(SessionConfig.FORMAT_WRAPPER_ARRAY, true);
+        sessionConfig.set(SessionConfig.FORMAT_INDENT_JSON, true);
+        sessionConfig.set(SessionConfig.FORMAT_QUOTE_RECORD,
+                format != SessionConfig.OutputFormat.CLEAN_JSON && format != SessionConfig.OutputFormat.LOSSLESS_JSON);
+        return new SessionOutput(sessionConfig, resultWriter, resultPrefix, resultPostfix,
appendHandle, appendStatus);
+    }
+
+    private static void printMetrics(PrintWriter pw, long errorCount) {
+        pw.print("\t\"");
+        pw.print(AbstractQueryApiServlet.ResultFields.METRICS.str());
+        pw.print("\": {\n");
+        ResultUtil.printField(pw, "errorCount", errorCount, false);
+        pw.print("\t}\n");
+    }
+}
\ No newline at end of file
diff --git a/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/common/ResultExtractor.java
b/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/common/ResultExtractor.java
index 834c104..b95a283 100644
--- a/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/common/ResultExtractor.java
+++ b/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/common/ResultExtractor.java
@@ -81,11 +81,11 @@
     private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
 
     public static InputStream extract(InputStream resultStream) throws Exception {
-        return extract(resultStream, EnumSet.of(ResultField.RESULTS, ResultField.ERRORS));
+        return extract(resultStream, EnumSet.of(ResultField.RESULTS));
     }
 
     public static InputStream extractMetrics(InputStream resultStream) throws Exception {
-        return extract(resultStream, EnumSet.of(ResultField.METRICS, ResultField.ERRORS));
+        return extract(resultStream, EnumSet.of(ResultField.METRICS));
     }
 
     public static String extractHandle(InputStream resultStream) throws Exception {
@@ -110,7 +110,8 @@
         final ObjectNode result = OBJECT_MAPPER.readValue(resultStr, ObjectNode.class);
 
         LOGGER.fine("+++++++\n" + result + "\n+++++++\n");
-
+        // if we have errors field in the results, we will always return it
+        checkForErrors(result);
         final StringBuilder resultBuilder = new StringBuilder();
         for (Iterator<String> fieldNameIter = result.fieldNames(); fieldNameIter.hasNext();)
{
             final String fieldName = fieldNameIter.next();
@@ -154,12 +155,6 @@
 
                     }
                     break;
-                case ERRORS:
-                    final JsonNode errors = fieldValue.get(0).get("msg");
-                    if (!result.get(ResultField.METRICS.getFieldName()).has("errorCount"))
{
-                        throw new AsterixException("Request reported error but not an errorCount");
-                    }
-                    throw new AsterixException(errors.asText());
                 case REQUEST_ID:
                 case METRICS:
                 case CLIENT_CONTEXT_ID:
@@ -174,4 +169,15 @@
         }
         return IOUtils.toInputStream(resultBuilder.toString(), StandardCharsets.UTF_8);
     }
+
+    private static void checkForErrors(ObjectNode result) throws AsterixException {
+        final JsonNode errorsField = result.get(ResultField.ERRORS.getFieldName());
+        if (errorsField != null) {
+            final JsonNode errors = errorsField.get(0).get("msg");
+            if (!result.get(ResultField.METRICS.getFieldName()).has("errorCount")) {
+                throw new AsterixException("Request reported error but not an errorCount");
+            }
+            throw new AsterixException(errors.asText());
+        }
+    }
 }
\ No newline at end of file

-- 
To view, visit https://asterix-gerrit.ics.uci.edu/2057
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ic16e9d234c5e127ea24e382c49f3c7cfa5bce9c7
Gerrit-PatchSet: 3
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Murtadha Hubail <mhubail@apache.org>
Gerrit-Reviewer: Anon. E. Moose #1000171
Gerrit-Reviewer: Ian Maxon <imaxon@apache.org>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Murtadha Hubail <mhubail@apache.org>

Mime
View raw message