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 <[email protected]> Tested-by: Jenkins <[email protected]> Contrib: Jenkins <[email protected]> Integration-Tests: Jenkins <[email protected]> Reviewed-by: Ian Maxon <[email protected]> --- 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 <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Murtadha Hubail <[email protected]>
