On Sun, Jul 20, 2008 at 11:01 PM, <[EMAIL PROTECTED]> wrote:
> +
> + /**
> + * Sets cache-control headers indicating the response is not cacheable.
> + */
> + public void setNoCache() {
> + this.headers.put("Cache-Control", Lists.newArrayList("no-cache"));
> + this.headers.put("Pragma", Lists.newArrayList("no-cache"));
> + }
This also needs to remove Expires headers.
>
>
> /**
> * @return consolidated cache expiration time or -1
> */
> public long getCacheExpiration() {
> - if (httpStatusCode != SC_OK) {
> - return getDate() + negativeCacheTtl;
> - }
> if (isStrictNoCache()) {
> return -1;
> }
> + if (httpStatusCode != SC_OK) {
> + return getDate() + negativeCacheTtl;
> + }
This breaks negative caching, since most servers send no cache directives on
error responses. The original reason for adding negative caching in the
first place is that a single popular gadget host having problems results in
a huge number of bad http requests, wasting resources and making an already
troubled site just get hit even harder, since now it has to endure 100% of
traffic. This is no different for OAuth than it is for any other request
type.
> +
> + /**
> + * HTTP cache.
> + */
This comment seems rather redundant to me.
> + private HttpCache cache;
>
> /**
> *
> @@ -151,13 +158,15 @@
> * @param authToken user's gadget security token
> * @param params OAuth fetch parameters sent from makeRequest
> * @param tokenStore storage for long lived tokens.
> + * @param cache cache to use for HTTP responses.
> */
> public OAuthFetcher(
> GadgetOAuthTokenStore tokenStore,
> BlobCrypter oauthCrypter,
> HttpFetcher nextFetcher,
> SecurityToken authToken,
> - OAuthRequestParams params) {
> + OAuthRequestParams params,
> + HttpCache cache) {
Now that we're up to 7 parameters on this constructor, can we accept that
making everything an HttpFetcher is not the best design choice we've ever
made? :) This is especially bad since only two of these parameters vary per
invocation. This is extremely confusing code to follow, and could be much
better written as a singleton with an inner class for request handling.
>
> private HttpResponse buildErrorResponse(GadgetException e) {
> if (error == null) {
> error = OAuthError.UNKNOWN_PROBLEM;
> @@ -566,6 +594,7 @@
> private HttpResponse buildNonDataResponse() {
> HttpResponse response = new HttpResponse(0, null, null);
> addResponseMetadata(response);
> + response.setNoCache();
Why add a new method for this instead of passing the headers in the ctor?
HttpResponse is already very problematic, as it should be immutable, but we
keep adding mutators to it. It's very hard to implement a cache of objects
when the data in the cache can be modified, and as a result most caches have
to be written more defensively than necessary, with copies being produced on
every read.
