http://codereview.appspot.com/1855044/diff/45001/46004 File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java (left):
http://codereview.appspot.com/1855044/diff/45001/46004#oldcode60 java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java:60: "vary", "expires", "date", "pragma", "cache-control", "transfer-encoding", "www-authenticate" On 2010/07/26 18:56:17, gagan.goku wrote:
On 2010/07/26 18:40:22, zhoresh wrote: > Please make sure you don't loose this values as part of the change.
We made sure that we covered all of these headers. Anupama had added
"etag" and
"last-modified" headers to CACHING_DIRECTIVES. I think her latest
patch got out
of sync somehow. Resync would fix this.
Right. Sorry about the confusion. Added these back into UriUtils.java. http://codereview.appspot.com/1855044/diff/45001/46005 File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java (right): http://codereview.appspot.com/1855044/diff/45001/46005#newcode146 java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:146: UriUtils.DisallowedHeaders.CLIENT_STATE_DIRECTIVES); On 2010/07/26 21:25:02, johnfargo wrote:
These look to me like the right header groups.
The full delta, as of this patch revision, is:
Previously allowed, now disallowed: "server" (from
OUTPUT_TRANSFER_DIRECTIVES)
Previously disallowed, now passed through: "etag", "last-modified".
Presumably
these are the ones you mention are coming in an updated patch#? (CLIENT_STATE_DIRECTIVES additions)
Right. Could you confirm that is it ok to have "server" in the disallowed header list - or was there a specific reason why it was not included in ProxyBase earlier? http://codereview.appspot.com/1855044/diff/45001/46005#newcode154 java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:154: IOUtils.copy(results.getResponse(), response.getOutputStream()); On 2010/07/26 21:25:02, johnfargo wrote:
Alas, not so. Per:
http://java.sun.com/products/servlet/2.3/javadoc/javax/servlet/http/HttpServletResponse.html#sendError%28int)
"After using this method, the response should be considered to be
committed and
should not be written to."
What we really want is: response.sendError(code, results.getResponseAsString());
I have changed this flow to not use sendError at all and use the copy-to-output-stream approach always. Does this sound fine? http://codereview.appspot.com/1855044/diff/45001/46006 File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java (right): http://codereview.appspot.com/1855044/diff/45001/46006#newcode144 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:144: resp.sendError(HttpResponse.SC_BAD_GATEWAY); On 2010/07/26 21:58:21, johnfargo wrote:
do you intend to merge:
http://codereview.appspot.com/1811042/diff/54001/55005?context=50&column_width=100
(setStatus vs. sendError) in this CL as well? That would accommodate
the
sendError(...) concern to a large degree.
Per
(http://blogs.atlassian.com/developer/2007/07/responsesenderror_vs_responses.html)
[first resource I was able to find on the topic]
sendError differs from setStatus only in that sendError sends a
preconfigured
error page. setStatus presumes you're going to provide the error
context itself.
So setStatus seems reasonable for most if not all Shindig use cases
I've found. Yes. Synced now to get the setStatus change made by Gagan. http://codereview.appspot.com/1855044/diff/45001/46006#newcode216 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:216: public static void rewriteContentType(HttpRequest req, HttpServletResponse response) { On 2010/07/26 21:25:02, johnfargo wrote:
nit: I'd prefer this to be maybeRewriteContentType, to reflect the
if-condition. Done. http://codereview.appspot.com/1855044/show
