Merge lp:~tjoneslo/akiban-server/fix-bug-1163967 into lp:~akiban-technologies/akiban-server/trunk

[Thomas Jones-Low]

Fix bug 1163967 - Update the delete processing for both the entity and the builder to return no body and a 204 (no content) status code.

Exposed the RestResponseBuilder#wrapException() method to allow the Delete processing to correctly and consistently return errors when they occur.

[Nathan Williams (nwilliams)]

RestResponseBuilder already has some handling for NO_CONTENT, but just not enough. Additionally, it will do jsonp automatically.

Perhaps just tweaking build() and/or createStreamingOutput() would be sufficient?

[Thomas Jones-Low (tjoneslo)]

Unfortunately, not quite that simple. The call Response.ResponseBuilder#entity() helpfully resets the response code from 204 to 200, even if the entity is an empty string. So we need to have a second code path for executing the rest response (i.e. the delete) if we expect the response to really read 204.

Update the RestServiceFilesIT to read through and compare header and response codes to verify the delete is returning correct response.

Fix the RestResponseBuilder to correctly handle the NO_CONTENT case, and handle jsonp settings for delete.

review: Needs Resubmitting

[Nathan Williams (nwilliams)]

How nice of it.

Was there a new .expected_header you added to one of the DELETE tests?

[Thomas Jones-Low (tjoneslo)]

Yes now adding new test (delete with jsonp) and header for existing test.

review: Needs Resubmitting

[Nathan Williams (nwilliams)]

Excellent. Thanks!

review: Approve

=== modified file 'src/main/java/com/akiban/rest/RestResponseBuilder.java'
--- src/main/java/com/akiban/rest/RestResponseBuilder.java	2013-03-22 20:05:57 +0000
+++ src/main/java/com/akiban/rest/RestResponseBuilder.java	2013-04-03 20:23:21 +0000
@@ -95,7 +95,16 @@
         if(outputBody == null && outputGenerator == null && jsonp == null) {
             status(Response.Status.NO_CONTENT);
         }
-        Response.ResponseBuilder builder = Response.status(status).entity(createStreamingOutput());
+        if (isJsonp) {
+            status(Response.Status.OK);
+        }
+        Response.ResponseBuilder builder;
+        
+        if (this.status == Response.Status.NO_CONTENT.getStatusCode()) {
+            builder = Response.status(status);
+        } else {
+                builder = Response.status(status).entity(createStreamingOutput());
+        }
         if(isJsonp) {
             builder.type(ResourceHelper.APPLICATION_JAVASCRIPT_TYPE);
         }
@@ -125,7 +134,7 @@
         return builder.toString();
     }
 
-    private WebApplicationException wrapException(Throwable e) {
+    public WebApplicationException wrapException(Throwable e) {
         final ErrorCode code;
         if(e instanceof InvalidOperationException) {
             code = ((InvalidOperationException)e).getCode();
@@ -180,7 +189,6 @@
         };
     }
 
-
     private static Map<Class, Response.Status> buildExceptionStatusMap() {
         Map<Class, Response.Status> map = new HashMap<>();
         map.put(NoSuchTableException.class, Response.Status.NOT_FOUND);
 
=== modified file 'src/main/java/com/akiban/rest/resources/BuilderResource.java'
--- src/main/java/com/akiban/rest/resources/BuilderResource.java	2013-03-26 20:55:25 +0000
+++ src/main/java/com/akiban/rest/resources/BuilderResource.java	2013-04-03 20:23:21 +0000
@@ -151,16 +151,16 @@
                            @Context final UriInfo uri) {
         final TableName tableName = parseTableName(request, entityName);
         checkTableAccessible(securityService, request, tableName);
-        return RestResponseBuilder
-                .forRequest(request)
-                .body(new RestResponseBuilder.BodyGenerator() {
-                    @Override
-                    public void write(PrintWriter writer) throws Exception {
-                        modelBuilder.create(tableName);
-                        restDMLService.delete(writer, tableName, getPKString(uri));
-                    }
-                })
-                .build();
+        try {
+            modelBuilder.create(tableName);
+            restDMLService.delete(tableName, getPKString(uri));
+            return RestResponseBuilder
+                    .forRequest(request)
+                    .status(Response.Status.NO_CONTENT)
+                    .build();
+        } catch (Exception e) {
+            throw RestResponseBuilder.forRequest(request).wrapException(e);
+        }
     }
 
     @GET
 
=== modified file 'src/main/java/com/akiban/rest/resources/EntityResource.java'
--- src/main/java/com/akiban/rest/resources/EntityResource.java	2013-03-26 18:08:04 +0000
+++ src/main/java/com/akiban/rest/resources/EntityResource.java	2013-04-03 20:23:21 +0000
@@ -182,14 +182,14 @@
                                  @Context final UriInfo uri) {
         final TableName tableName = parseTableName(request, entity);
         checkTableAccessible(reqs.securityService, request, tableName);
-        return RestResponseBuilder
-                .forRequest(request)
-                .body(new RestResponseBuilder.BodyGenerator() {
-                    @Override
-                    public void write(PrintWriter writer) throws Exception {
-                        reqs.restDMLService.delete(writer, tableName, getPKString(uri));
-                    }
-                })
-                .build();
+        try {
+            reqs.restDMLService.delete(tableName, getPKString(uri));
+            return RestResponseBuilder
+                    .forRequest(request)
+                    .status(Response.Status.NO_CONTENT)
+                    .build();
+        } catch (Exception e) {
+            throw RestResponseBuilder.forRequest(request).wrapException(e);
+        }
     }
 }
 
=== modified file 'src/main/java/com/akiban/server/service/restdml/RestDMLService.java'
--- src/main/java/com/akiban/server/service/restdml/RestDMLService.java	2013-03-25 22:52:39 +0000
+++ src/main/java/com/akiban/server/service/restdml/RestDMLService.java	2013-04-03 20:23:21 +0000
@@ -33,7 +33,7 @@
     public void getAllEntities(PrintWriter writer, TableName tableName, Integer depth);
     public void getEntities(PrintWriter writer, TableName tableName, Integer depth, String pks);
     public void insert(PrintWriter writer, TableName tableName, JsonNode node);
-    public void delete(PrintWriter writer, TableName tableName, String pks);
+    public void delete(TableName tableName, String pks);
     public void update(PrintWriter writer, TableName tableName, String values, JsonNode node);
 
     public void insertNoTxn(Session session, PrintWriter writer, TableName tableName, JsonNode node);
 
=== modified file 'src/main/java/com/akiban/server/service/restdml/RestDMLServiceImpl.java'
--- src/main/java/com/akiban/server/service/restdml/RestDMLServiceImpl.java	2013-03-25 22:52:39 +0000
+++ src/main/java/com/akiban/server/service/restdml/RestDMLServiceImpl.java	2013-04-03 20:23:21 +0000
@@ -182,7 +182,7 @@
     }
 
     @Override
-    public void delete(PrintWriter writer, TableName tableName, String identifier) {
+    public void delete(TableName tableName, String identifier) {
         try (Session session = sessionService.createSession();
              CloseableTransaction txn = transactionService.beginCloseableTransaction(session)) {
             AkibanInformationSchema ais = dxlService.ddlFunctions().getAIS(session);
 
=== modified file 'src/test/java/com/akiban/rest/RestServiceFilesIT.java'
--- src/test/java/com/akiban/rest/RestServiceFilesIT.java	2013-03-26 18:24:46 +0000
+++ src/test/java/com/akiban/rest/RestServiceFilesIT.java	2013-04-03 20:23:21 +0000
@@ -253,6 +253,9 @@
             if(!caseParams.expectedIgnore) {
                 compareExpected(caseParams.requestMethod + " response", caseParams.expectedResponse, actual);
             }
+            if (caseParams.expectedHeader != null) {
+                compareHeaders(httpConn, caseParams.expectedHeader);
+            }
         } finally {
             fullyDisconnect(httpConn);
         }
@@ -305,6 +308,19 @@
             assertEquals(assertMsg, expectedNode, actualNode);
         }
     }
+    
+    private void compareHeaders (HttpURLConnection httpConn, String checkHeaders) throws Exception {
+        String[] headerList = checkHeaders.split (Strings.NL);
+        for (String header : headerList) {
+            String[] nameValue = header.split(":", 2);
+            
+            if (nameValue[0].equals("responseCode")) {
+                assertEquals ("Headers Response", Integer.parseInt(nameValue[1].trim()), httpConn.getResponseCode());
+            } else {
+                assertEquals ("Headers check", nameValue[1].trim(), httpConn.getHeaderField(nameValue[0]));
+            }
+        }
+    }
 
     private void fullyDisconnect(HttpURLConnection httpConn) {
         // If there is a failure, leaving junk in any of the streams can cause cascading issues.
 
=== added file 'src/test/resources/com/akiban/rest/caoi/delete-c1-jsonp.delete'
--- src/test/resources/com/akiban/rest/caoi/delete-c1-jsonp.delete	1970-01-01 00:00:00 +0000
+++ src/test/resources/com/akiban/rest/caoi/delete-c1-jsonp.delete	2013-04-03 20:23:21 +0000
@@ -0,0 +1,1 @@
+/entity/test.customers/1?callback=delete
 \ No newline at end of file
 
=== added file 'src/test/resources/com/akiban/rest/caoi/delete-c1-jsonp.expected'
--- src/test/resources/com/akiban/rest/caoi/delete-c1-jsonp.expected	1970-01-01 00:00:00 +0000
+++ src/test/resources/com/akiban/rest/caoi/delete-c1-jsonp.expected	2013-04-03 20:23:21 +0000
@@ -0,0 +1,1 @@
+delete()
 \ No newline at end of file
 
=== added file 'src/test/resources/com/akiban/rest/caoi/delete-c1-jsonp.expected_header'
--- src/test/resources/com/akiban/rest/caoi/delete-c1-jsonp.expected_header	1970-01-01 00:00:00 +0000
+++ src/test/resources/com/akiban/rest/caoi/delete-c1-jsonp.expected_header	2013-04-03 20:23:21 +0000
@@ -0,0 +1,2 @@
+responseCode: 200
+Content-Type: application/javascript
 \ No newline at end of file
 
=== added file 'src/test/resources/com/akiban/rest/caoi/delete-c1.expected_header'
--- src/test/resources/com/akiban/rest/caoi/delete-c1.expected_header	1970-01-01 00:00:00 +0000
+++ src/test/resources/com/akiban/rest/caoi/delete-c1.expected_header	2013-04-03 20:23:21 +0000
@@ -0,0 +1,2 @@
+responseCode: 204
+Content-Type: application/json
 \ No newline at end of file