Akiban Server

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

  1. fix-bug-1163967
  2. Merge into trunk
Proposed by Thomas Jones-Low
Status: Merged
Approved by: Nathan Williams
Approved revision: 2610
Merged at revision: 2609
Proposed branch: lp:~tjoneslo/akiban-server/fix-bug-1163967
Merge into: lp:~akiban-technologies/akiban-server/trunk
Diff against target: 187 lines (+54/-24)
10 files modified
src/main/java/com/akiban/rest/RestResponseBuilder.java (+11/-3)
src/main/java/com/akiban/rest/resources/BuilderResource.java (+10/-10)
src/main/java/com/akiban/rest/resources/EntityResource.java (+9/-9)
src/main/java/com/akiban/server/service/restdml/RestDMLService.java (+1/-1)
src/main/java/com/akiban/server/service/restdml/RestDMLServiceImpl.java (+1/-1)
src/test/java/com/akiban/rest/RestServiceFilesIT.java (+16/-0)
src/test/resources/com/akiban/rest/caoi/delete-c1-jsonp.delete (+1/-0)
src/test/resources/com/akiban/rest/caoi/delete-c1-jsonp.expected (+1/-0)
src/test/resources/com/akiban/rest/caoi/delete-c1-jsonp.expected_header (+2/-0)
src/test/resources/com/akiban/rest/caoi/delete-c1.expected_header (+2/-0)
To merge this branch: bzr merge lp:~tjoneslo/akiban-server/fix-bug-1163967
Reviewer Review Type Date Requested Status
Nathan Williams Approve
Thomas Jones-Low Needs Resubmitting
Review via email: mp+156904@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Nathan Williams (nwilliams) wrote :

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?

Revision history for this message
Thomas Jones-Low (tjoneslo) wrote :

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
Revision history for this message
Nathan Williams (nwilliams) wrote :

How nice of it.

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

Revision history for this message
Thomas Jones-Low (tjoneslo) wrote :

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

review: Needs Resubmitting
Revision history for this message
Nathan Williams (nwilliams) wrote :

Excellent. Thanks!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/main/java/com/akiban/rest/RestResponseBuilder.java'
2--- src/main/java/com/akiban/rest/RestResponseBuilder.java 2013-03-22 20:05:57 +0000
3+++ src/main/java/com/akiban/rest/RestResponseBuilder.java 2013-04-03 20:23:21 +0000
4@@ -95,7 +95,16 @@
5 if(outputBody == null && outputGenerator == null && jsonp == null) {
6 status(Response.Status.NO_CONTENT);
7 }
8- Response.ResponseBuilder builder = Response.status(status).entity(createStreamingOutput());
9+ if (isJsonp) {
10+ status(Response.Status.OK);
11+ }
12+ Response.ResponseBuilder builder;
13+
14+ if (this.status == Response.Status.NO_CONTENT.getStatusCode()) {
15+ builder = Response.status(status);
16+ } else {
17+ builder = Response.status(status).entity(createStreamingOutput());
18+ }
19 if(isJsonp) {
20 builder.type(ResourceHelper.APPLICATION_JAVASCRIPT_TYPE);
21 }
22@@ -125,7 +134,7 @@
23 return builder.toString();
24 }
25
26- private WebApplicationException wrapException(Throwable e) {
27+ public WebApplicationException wrapException(Throwable e) {
28 final ErrorCode code;
29 if(e instanceof InvalidOperationException) {
30 code = ((InvalidOperationException)e).getCode();
31@@ -180,7 +189,6 @@
32 };
33 }
34
35-
36 private static Map<Class, Response.Status> buildExceptionStatusMap() {
37 Map<Class, Response.Status> map = new HashMap<>();
38 map.put(NoSuchTableException.class, Response.Status.NOT_FOUND);
39
40=== modified file 'src/main/java/com/akiban/rest/resources/BuilderResource.java'
41--- src/main/java/com/akiban/rest/resources/BuilderResource.java 2013-03-26 20:55:25 +0000
42+++ src/main/java/com/akiban/rest/resources/BuilderResource.java 2013-04-03 20:23:21 +0000
43@@ -151,16 +151,16 @@
44 @Context final UriInfo uri) {
45 final TableName tableName = parseTableName(request, entityName);
46 checkTableAccessible(securityService, request, tableName);
47- return RestResponseBuilder
48- .forRequest(request)
49- .body(new RestResponseBuilder.BodyGenerator() {
50- @Override
51- public void write(PrintWriter writer) throws Exception {
52- modelBuilder.create(tableName);
53- restDMLService.delete(writer, tableName, getPKString(uri));
54- }
55- })
56- .build();
57+ try {
58+ modelBuilder.create(tableName);
59+ restDMLService.delete(tableName, getPKString(uri));
60+ return RestResponseBuilder
61+ .forRequest(request)
62+ .status(Response.Status.NO_CONTENT)
63+ .build();
64+ } catch (Exception e) {
65+ throw RestResponseBuilder.forRequest(request).wrapException(e);
66+ }
67 }
68
69 @GET
70
71=== modified file 'src/main/java/com/akiban/rest/resources/EntityResource.java'
72--- src/main/java/com/akiban/rest/resources/EntityResource.java 2013-03-26 18:08:04 +0000
73+++ src/main/java/com/akiban/rest/resources/EntityResource.java 2013-04-03 20:23:21 +0000
74@@ -182,14 +182,14 @@
75 @Context final UriInfo uri) {
76 final TableName tableName = parseTableName(request, entity);
77 checkTableAccessible(reqs.securityService, request, tableName);
78- return RestResponseBuilder
79- .forRequest(request)
80- .body(new RestResponseBuilder.BodyGenerator() {
81- @Override
82- public void write(PrintWriter writer) throws Exception {
83- reqs.restDMLService.delete(writer, tableName, getPKString(uri));
84- }
85- })
86- .build();
87+ try {
88+ reqs.restDMLService.delete(tableName, getPKString(uri));
89+ return RestResponseBuilder
90+ .forRequest(request)
91+ .status(Response.Status.NO_CONTENT)
92+ .build();
93+ } catch (Exception e) {
94+ throw RestResponseBuilder.forRequest(request).wrapException(e);
95+ }
96 }
97 }
98
99=== modified file 'src/main/java/com/akiban/server/service/restdml/RestDMLService.java'
100--- src/main/java/com/akiban/server/service/restdml/RestDMLService.java 2013-03-25 22:52:39 +0000
101+++ src/main/java/com/akiban/server/service/restdml/RestDMLService.java 2013-04-03 20:23:21 +0000
102@@ -33,7 +33,7 @@
103 public void getAllEntities(PrintWriter writer, TableName tableName, Integer depth);
104 public void getEntities(PrintWriter writer, TableName tableName, Integer depth, String pks);
105 public void insert(PrintWriter writer, TableName tableName, JsonNode node);
106- public void delete(PrintWriter writer, TableName tableName, String pks);
107+ public void delete(TableName tableName, String pks);
108 public void update(PrintWriter writer, TableName tableName, String values, JsonNode node);
109
110 public void insertNoTxn(Session session, PrintWriter writer, TableName tableName, JsonNode node);
111
112=== modified file 'src/main/java/com/akiban/server/service/restdml/RestDMLServiceImpl.java'
113--- src/main/java/com/akiban/server/service/restdml/RestDMLServiceImpl.java 2013-03-25 22:52:39 +0000
114+++ src/main/java/com/akiban/server/service/restdml/RestDMLServiceImpl.java 2013-04-03 20:23:21 +0000
115@@ -182,7 +182,7 @@
116 }
117
118 @Override
119- public void delete(PrintWriter writer, TableName tableName, String identifier) {
120+ public void delete(TableName tableName, String identifier) {
121 try (Session session = sessionService.createSession();
122 CloseableTransaction txn = transactionService.beginCloseableTransaction(session)) {
123 AkibanInformationSchema ais = dxlService.ddlFunctions().getAIS(session);
124
125=== modified file 'src/test/java/com/akiban/rest/RestServiceFilesIT.java'
126--- src/test/java/com/akiban/rest/RestServiceFilesIT.java 2013-03-26 18:24:46 +0000
127+++ src/test/java/com/akiban/rest/RestServiceFilesIT.java 2013-04-03 20:23:21 +0000
128@@ -253,6 +253,9 @@
129 if(!caseParams.expectedIgnore) {
130 compareExpected(caseParams.requestMethod + " response", caseParams.expectedResponse, actual);
131 }
132+ if (caseParams.expectedHeader != null) {
133+ compareHeaders(httpConn, caseParams.expectedHeader);
134+ }
135 } finally {
136 fullyDisconnect(httpConn);
137 }
138@@ -305,6 +308,19 @@
139 assertEquals(assertMsg, expectedNode, actualNode);
140 }
141 }
142+
143+ private void compareHeaders (HttpURLConnection httpConn, String checkHeaders) throws Exception {
144+ String[] headerList = checkHeaders.split (Strings.NL);
145+ for (String header : headerList) {
146+ String[] nameValue = header.split(":", 2);
147+
148+ if (nameValue[0].equals("responseCode")) {
149+ assertEquals ("Headers Response", Integer.parseInt(nameValue[1].trim()), httpConn.getResponseCode());
150+ } else {
151+ assertEquals ("Headers check", nameValue[1].trim(), httpConn.getHeaderField(nameValue[0]));
152+ }
153+ }
154+ }
155
156 private void fullyDisconnect(HttpURLConnection httpConn) {
157 // If there is a failure, leaving junk in any of the streams can cause cascading issues.
158
159=== added file 'src/test/resources/com/akiban/rest/caoi/delete-c1-jsonp.delete'
160--- src/test/resources/com/akiban/rest/caoi/delete-c1-jsonp.delete 1970-01-01 00:00:00 +0000
161+++ src/test/resources/com/akiban/rest/caoi/delete-c1-jsonp.delete 2013-04-03 20:23:21 +0000
162@@ -0,0 +1,1 @@
163+/entity/test.customers/1?callback=delete
164\ No newline at end of file
165
166=== added file 'src/test/resources/com/akiban/rest/caoi/delete-c1-jsonp.expected'
167--- src/test/resources/com/akiban/rest/caoi/delete-c1-jsonp.expected 1970-01-01 00:00:00 +0000
168+++ src/test/resources/com/akiban/rest/caoi/delete-c1-jsonp.expected 2013-04-03 20:23:21 +0000
169@@ -0,0 +1,1 @@
170+delete()
171\ No newline at end of file
172
173=== added file 'src/test/resources/com/akiban/rest/caoi/delete-c1-jsonp.expected_header'
174--- src/test/resources/com/akiban/rest/caoi/delete-c1-jsonp.expected_header 1970-01-01 00:00:00 +0000
175+++ src/test/resources/com/akiban/rest/caoi/delete-c1-jsonp.expected_header 2013-04-03 20:23:21 +0000
176@@ -0,0 +1,2 @@
177+responseCode: 200
178+Content-Type: application/javascript
179\ No newline at end of file
180
181=== added file 'src/test/resources/com/akiban/rest/caoi/delete-c1.expected_header'
182--- src/test/resources/com/akiban/rest/caoi/delete-c1.expected_header 1970-01-01 00:00:00 +0000
183+++ src/test/resources/com/akiban/rest/caoi/delete-c1.expected_header 2013-04-03 20:23:21 +0000
184@@ -0,0 +1,2 @@
185+responseCode: 204
186+Content-Type: application/json
187\ No newline at end of file

Subscribers

People subscribed via source and target branches