Adding Batch support for Google APIs

[rmistry, 2012-01-20 23:28:33]

Updated CL to include unit tests.
Also, updated CL to pass in transport and httpRequestInitializer instead of
requestFactory.

[yanivi, 2012-01-27 18:39:42]

First round of review comments.  Lots of design questions.

http://codereview.appspot.com/5539055/diff/10001/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchCallback.java
(right):

http://codereview.appspot.com/5539055/diff/10001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchCallback.java:47:
public void onSuccess(T t);
missing JavaDoc for onSuccess and onFailure

http://codereview.appspot.com/5539055/diff/10001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchCallback.java:47:
public void onSuccess(T t);
remove redundant public modifier on methods in interfaces

http://codereview.appspot.com/5539055/diff/10001/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java
(right):

http://codereview.appspot.com/5539055/diff/10001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:50:
BatchRequest batch = new BatchRequest(requestFactory, jsonFactory();
please update the code sample

http://codereview.appspot.com/5539055/diff/10001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:227:
public List<BatchUnparsedResponse<?>> executeUnparsed() throws IOException {
We should definitely have an executeUnparsed() method that returns the raw
HttpResponse, just like JsonHttpRequest.executeUnparsed.  So I feel strong that
if we have a method like what you propose here, it should be renamed.

http://codereview.appspot.com/5539055/diff/10001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:227:
public List<BatchUnparsedResponse<?>> executeUnparsed() throws IOException {
I don't understand the purpose of executeUnparsed.  What does it provide that
execute() doesn't already provide?  Is the idea to allow a developer to stop
parsing mid-way?  Or implement functionality between responses?

In my opinion a List of BatchUnparsedResponse is the wrong way to go.  That's
because it's not like you can just skip the first element and go straight to the
second element.  See my comment below about not wanting to parse the whole JSON
content into memory.

If so, I'd much rather have it return a class with a parseNextResponse(),
ignoreNextResponse(), cancel(), etc.

http://codereview.appspot.com/5539055/diff/10001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:242:
int outerHeadersEnd = part.indexOf(NEWLINE + NEWLINE);
We really ought to provide a facility for parsing multipart responses in
google-http-java-client.  Not a blocker though.  A feature request on the issue
tracker would suffice.

http://codereview.appspot.com/5539055/diff/10001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:279:
HttpRequest buildMultipartRequest() throws IOException {
Please make this method public and call it buildHttpRequest() to match the
naming of the method by that name in JsonHttpRequest.  I suspect these raw
HttpRequest objects will be needed for example to support async execution in App
Engine.

http://codereview.appspot.com/5539055/diff/10001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:301:
private String inputStreamToString(InputStream stream) throws IOException {
BAD!
This is a strong signal we are doing something wrong.  It defeats the primary
reason for unparsed responses which is to avoid loading the whole response into
memory.

http://codereview.appspot.com/5539055/diff/10001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:313:
String createMultipartBody(String batchBoundary) throws IOException {
As I understand it, multipart/mixed and multipart/related are basically the same
thing on the wire, except for the top-level Content-Type.  Therefore, I'd much
prefer instead implementing a MultipartContent class in google-http-java-client
that MultipartRelatedContent extends, and moving 99% of the logic currently in
MultipartRelatedContent to MultipartContent.

Normally I wouldn't want to make it a blocker, but I prefer not to see duplicate
code.

http://codereview.appspot.com/5539055/diff/10001/google-api-client/src/test/j...
File
google-api-client/src/test/java/com/google/api/client/googleapis/batch/BatchRequestTest.java
(right):

http://codereview.appspot.com/5539055/diff/10001/google-api-client/src/test/j...
google-api-client/src/test/java/com/google/api/client/googleapis/batch/BatchRequestTest.java:74:
@Override
Description	Resource	Path	Location	Type
The method onFailure(GoogleJsonError) of type BatchRequestTest.TestCallback1
must override a superclass
method	BatchRequestTest.java	/google-api-client/src/test/java/com/google/api/client/googleapis/batch	line
81	Java Problem
The method onFailure(GoogleJsonError) of type BatchRequestTest.TestCallback2
must override a superclass
method	BatchRequestTest.java	/google-api-client/src/test/java/com/google/api/client/googleapis/batch	line
98	Java Problem
The method onSuccess(BatchRequestTest.MockDataClass1) of type
BatchRequestTest.TestCallback1 must override a superclass
method	BatchRequestTest.java	/google-api-client/src/test/java/com/google/api/client/googleapis/batch	line
75	Java Problem
The method onSuccess(BatchRequestTest.MockDataClass2) of type
BatchRequestTest.TestCallback2 must override a superclass
method	BatchRequestTest.java	/google-api-client/src/test/java/com/google/api/client/googleapis/batch	line
92	Java Problem

[yanivi, 2012-01-27 18:54:48]

http://codereview.appspot.com/5539055/diff/10001/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchCallback.java
(right):

http://codereview.appspot.com/5539055/diff/10001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchCallback.java:45:
public interface BatchCallback<T> {
I think we should also provide a facility for providing the HTTP response
headers.  Perhaps just an extra GoogleHeaders parameter?

[rmistry, 2012-01-31 23:17:54]

http://codereview.appspot.com/5539055/diff/10001/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchCallback.java
(right):

http://codereview.appspot.com/5539055/diff/10001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchCallback.java:45:
public interface BatchCallback<T> {
On 2012/01/27 18:54:49, yanivi wrote:
> I think we should also provide a facility for providing the HTTP response
> headers.  Perhaps just an extra GoogleHeaders parameter?

Done.

http://codereview.appspot.com/5539055/diff/10001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchCallback.java:47:
public void onSuccess(T t);
On 2012/01/27 18:39:43, yanivi wrote:
> missing JavaDoc for onSuccess and onFailure

Done.

http://codereview.appspot.com/5539055/diff/10001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchCallback.java:47:
public void onSuccess(T t);
On 2012/01/27 18:39:43, yanivi wrote:
> remove redundant public modifier on methods in interfaces

Done.

http://codereview.appspot.com/5539055/diff/10001/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java
(right):

http://codereview.appspot.com/5539055/diff/10001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:50:
BatchRequest batch = new BatchRequest(requestFactory, jsonFactory();
On 2012/01/27 18:39:43, yanivi wrote:
> please update the code sample

Done.

http://codereview.appspot.com/5539055/diff/10001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:227:
public List<BatchUnparsedResponse<?>> executeUnparsed() throws IOException {
On 2012/01/27 18:39:43, yanivi wrote:
> We should definitely have an executeUnparsed() method that returns the raw
> HttpResponse, just like JsonHttpRequest.executeUnparsed.  So I feel strong
that
> if we have a method like what you propose here, it should be renamed.

Done.

http://codereview.appspot.com/5539055/diff/10001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:227:
public List<BatchUnparsedResponse<?>> executeUnparsed() throws IOException {
On 2012/01/27 18:39:43, yanivi wrote:
> I don't understand the purpose of executeUnparsed.  What does it provide that
> execute() doesn't already provide?  Is the idea to allow a developer to stop
> parsing mid-way?  Or implement functionality between responses?
> 
> In my opinion a List of BatchUnparsedResponse is the wrong way to go.  That's
> because it's not like you can just skip the first element and go straight to
the
> second element.  See my comment below about not wanting to parse the whole
JSON
> content into memory.
> 
> If so, I'd much rather have it return a class with a parseNextResponse(),
> ignoreNextResponse(), cancel(), etc.

Gave this a shot.

http://codereview.appspot.com/5539055/diff/10001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:242:
int outerHeadersEnd = part.indexOf(NEWLINE + NEWLINE);
On 2012/01/27 18:39:43, yanivi wrote:
> We really ought to provide a facility for parsing multipart responses in
> google-http-java-client.  Not a blocker though.  A feature request on the
issue
> tracker would suffice.

Filed http://code.google.com/p/google-http-java-client/issues/detail?id=63

http://codereview.appspot.com/5539055/diff/10001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:279:
HttpRequest buildMultipartRequest() throws IOException {
On 2012/01/27 18:39:43, yanivi wrote:
> Please make this method public and call it buildHttpRequest() to match the
> naming of the method by that name in JsonHttpRequest.  I suspect these raw
> HttpRequest objects will be needed for example to support async execution in
App
> Engine.

Done.

http://codereview.appspot.com/5539055/diff/10001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:301:
private String inputStreamToString(InputStream stream) throws IOException {
On 2012/01/27 18:39:43, yanivi wrote:
> BAD!
> This is a strong signal we are doing something wrong.  It defeats the primary
> reason for unparsed responses which is to avoid loading the whole response
into
> memory.

Done.

http://codereview.appspot.com/5539055/diff/10001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:313:
String createMultipartBody(String batchBoundary) throws IOException {
On 2012/01/27 18:39:43, yanivi wrote:
> As I understand it, multipart/mixed and multipart/related are basically the
same
> thing on the wire, except for the top-level Content-Type.  Therefore, I'd much
> prefer instead implementing a MultipartContent class in
google-http-java-client
> that MultipartRelatedContent extends, and moving 99% of the logic currently in
> MultipartRelatedContent to MultipartContent.
> 
> Normally I wouldn't want to make it a blocker, but I prefer not to see
duplicate
> code.

I think this would be a good feature request to file because it will take more
than moving most of the code from MultipartRelatedContent to MultipartContent,
we will also have to add support for specifying headers for each content part,
this is not obvious to me how to do right now but I can investigate later. Let
me know what you think. It would be great to focus on only the batch feature in
this CL and deal with the internal cleanup (to remove code duplication) in a
later CL.

http://codereview.appspot.com/5539055/diff/10001/google-api-client/src/test/j...
File
google-api-client/src/test/java/com/google/api/client/googleapis/batch/BatchRequestTest.java
(right):

http://codereview.appspot.com/5539055/diff/10001/google-api-client/src/test/j...
google-api-client/src/test/java/com/google/api/client/googleapis/batch/BatchRequestTest.java:74:
@Override
On 2012/01/27 18:39:43, yanivi wrote:
> Description	Resource	Path	Location	Type
> The method onFailure(GoogleJsonError) of type BatchRequestTest.TestCallback1
> must override a superclass
>
method	BatchRequestTest.java	/google-api-client/src/test/java/com/google/api/client/googleapis/batch	line
> 81	Java Problem
> The method onFailure(GoogleJsonError) of type BatchRequestTest.TestCallback2
> must override a superclass
>
method	BatchRequestTest.java	/google-api-client/src/test/java/com/google/api/client/googleapis/batch	line
> 98	Java Problem
> The method onSuccess(BatchRequestTest.MockDataClass1) of type
> BatchRequestTest.TestCallback1 must override a superclass
>
method	BatchRequestTest.java	/google-api-client/src/test/java/com/google/api/client/googleapis/batch	line
> 75	Java Problem
> The method onSuccess(BatchRequestTest.MockDataClass2) of type
> BatchRequestTest.TestCallback2 must override a superclass
>
method	BatchRequestTest.java	/google-api-client/src/test/java/com/google/api/client/googleapis/batch	line
> 92	Java Problem

Done.

[rmistry, 2012-03-13 13:52:01]

Ping.

[rmistry, 2012-03-26 19:21:13]

Synced with head.

[yanivi, 2012-03-27 14:07:47]

Sorry I haven't had time to look at it yet.  I think we need to push this
feature to 1.9.  We need to release 1.8 ASAP.

[rmistry, 2012-04-05 13:55:25]

Added @since 1.9

[aalbert, 2012-04-10 20:16:54]

It seems redundant to have to construct a BatchRequest with an HttpTransport,
HttpRequestInitializer & JsonFactory since all these have already been given to
the GoogleClient constructor.

I'd really like to see a GoogleClient.batch() method that creates a BatchRequest
for me so I can do this:

        Calendar client = Calendar.builder(httpTransport, jsonFactory)
                .setHttpRequestInitializer(...)
                .build();
        client.batch()
                .queue(...)
                .queue(...)
                .executeAndParse();

[aalbert, 2012-04-10 22:33:20]

http://codereview.appspot.com/5539055/diff/32001/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java
(right):

http://codereview.appspot.com/5539055/diff/32001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:300:
multipartBody.append(data);
This is wrong. data.toString() returns something like:

"com.google.api.client.http.json.JsonHttpContent@1b273cc"

You should be using data.writeTo() which suggests that you shopuld be using
stream instead of a StringBuilder to build the string.

I suggest something like:
    final ByteArrayOutputStream byteArrayOutputStream = new
ByteArrayOutputStream();
    final PrintStream printStream = new PrintStream(byteArrayOutputStream);
Instead of:
    StringBuilder multipartBody = new StringBuilder();

Here's an entire method that works for me:
  String createMultipartBody(String batchBoundary) throws IOException {
    final ByteArrayOutputStream byteArrayOutputStream = new
ByteArrayOutputStream();
    final PrintStream printStream = new PrintStream(byteArrayOutputStream);

    final int contentId = 1;
    for (HttpRequest request : requests) {
      // Write batch separator.
      printStream.append("--").append(batchBoundary).append(NEWLINE);

      // Write multipart headers.
      printStream.append("Content-Type: application/http").append(NEWLINE);
      printStream.append("Content-Transfer-Encoding: binary").append(NEWLINE);
      printStream.append("Content-ID: ").print(contentId++);
      printStream.append(NEWLINE).append(NEWLINE);

      // Write the batch method and path.
      printStream.append(request.getMethod().toString())
              .append(" ")
              .append(request.getUrl().toString())
              .append(NEWLINE);

      // Write the batch headers.
      final HttpHeaders headers = request.getHeaders();
      if (headers != null) {
        headers.setUserAgent(null);
        headers.setAcceptEncoding(null);
        for (Map.Entry<String, Object> header : headers.entrySet()) {
          writeHeader(printStream, header.getKey(), header.getValue());
        }
      }

      // Write the data to the body.
      final JsonHttpContent data = (JsonHttpContent) request.getContent();
      if (data != null) {
        writeHeader(printStream, "Content-Type", "" + data.getType());
        writeHeader(printStream, "Content-Length", "" + data.getLength());
        printStream.append(NEWLINE);
        data.writeTo(printStream);
      }
      printStream.append(NEWLINE);
    }

    // Write the end of the batch separator.
    printStream.append("--").append(batchBoundary).append("--");
    printStream.append(NEWLINE);
    printStream.flush();
    return byteArrayOutputStream.toString();
  }

[aalbert, 2012-04-10 22:42:12]

BatchRequest.execute* methods do not handle Auth Token expiration.

It seems that the batch request succeeds but the underlying queued individual
events fail if the auth token has expired.

The BatchRequest went through with a 200 but the callbacks for each of the
inserts it contained came back through onFailure() and contained an
Authentication error.

If I call some other non-batch method before calling batch, for example,
calendar.events().list().execute(), the auth token get's renewed and the list
request succeeds as wel ass any subsequent BatchRequests I make (including the
queued requests).

It seems that the batch request don't require a token and succeeds but when the
backend tries to make the queued reuqests, it's using the expired token and
fails.

[rmistry, 2012-04-16 14:44:19]

On 2012/04/10 20:16:54, aalbert wrote:
> It seems redundant to have to construct a BatchRequest with an HttpTransport,
> HttpRequestInitializer & JsonFactory since all these have already been given
to
> the GoogleClient constructor.
> 
> I'd really like to see a GoogleClient.batch() method that creates a
BatchRequest
> for me so I can do this:
> 
>         Calendar client = Calendar.builder(httpTransport, jsonFactory)
>                 .setHttpRequestInitializer(...)
>                 .build();
>         client.batch()
>                 .queue(...)
>                 .queue(...)
>                 .executeAndParse();

Done.

[rmistry, 2012-04-16 15:28:04]

http://codereview.appspot.com/5539055/diff/32001/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java
(right):

http://codereview.appspot.com/5539055/diff/32001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:300:
multipartBody.append(data);
On 2012/04/10 22:33:20, aalbert wrote:
> This is wrong. data.toString() returns something like:
> 
> "com.google.api.client.http.json.JsonHttpContent@1b273cc"
> 
> You should be using data.writeTo() which suggests that you shopuld be using
> stream instead of a StringBuilder to build the string.
> 
> I suggest something like:
>     final ByteArrayOutputStream byteArrayOutputStream = new
> ByteArrayOutputStream();
>     final PrintStream printStream = new PrintStream(byteArrayOutputStream);
> Instead of:
>     StringBuilder multipartBody = new StringBuilder();
> 
> Here's an entire method that works for me:
>   String createMultipartBody(String batchBoundary) throws IOException {
>     final ByteArrayOutputStream byteArrayOutputStream = new
> ByteArrayOutputStream();
>     final PrintStream printStream = new PrintStream(byteArrayOutputStream);
> 
>     final int contentId = 1;
>     for (HttpRequest request : requests) {
>       // Write batch separator.
>       printStream.append("--").append(batchBoundary).append(NEWLINE);
> 
>       // Write multipart headers.
>       printStream.append("Content-Type: application/http").append(NEWLINE);
>       printStream.append("Content-Transfer-Encoding: binary").append(NEWLINE);
>       printStream.append("Content-ID: ").print(contentId++);
>       printStream.append(NEWLINE).append(NEWLINE);
> 
>       // Write the batch method and path.
>       printStream.append(request.getMethod().toString())
>               .append(" ")
>               .append(request.getUrl().toString())
>               .append(NEWLINE);
> 
>       // Write the batch headers.
>       final HttpHeaders headers = request.getHeaders();
>       if (headers != null) {
>         headers.setUserAgent(null);
>         headers.setAcceptEncoding(null);
>         for (Map.Entry<String, Object> header : headers.entrySet()) {
>           writeHeader(printStream, header.getKey(), header.getValue());
>         }
>       }
> 
>       // Write the data to the body.
>       final JsonHttpContent data = (JsonHttpContent) request.getContent();
>       if (data != null) {
>         writeHeader(printStream, "Content-Type", "" + data.getType());
>         writeHeader(printStream, "Content-Length", "" + data.getLength());
>         printStream.append(NEWLINE);
>         data.writeTo(printStream);
>       }
>       printStream.append(NEWLINE);
>     }
> 
>     // Write the end of the batch separator.
>     printStream.append("--").append(batchBoundary).append("--");
>     printStream.append(NEWLINE);
>     printStream.flush();
>     return byteArrayOutputStream.toString();
>   }

This was a huge bug, thank you for catching it!
Done.

[rmistry, 2012-04-16 19:09:50]

On 2012/04/10 22:42:12, aalbert wrote:
> BatchRequest.execute* methods do not handle Auth Token expiration.
> 
> It seems that the batch request succeeds but the underlying queued individual
> events fail if the auth token has expired.
> 
> The BatchRequest went through with a 200 but the callbacks for each of the
> inserts it contained came back through onFailure() and contained an
> Authentication error.
> 
> If I call some other non-batch method before calling batch, for example,
> calendar.events().list().execute(), the auth token get's renewed and the list
> request succeeds as wel ass any subsequent BatchRequests I make (including the
> queued requests).
> 
> It seems that the batch request don't require a token and succeeds but when
the
> backend tries to make the queued reuqests, it's using the expired token and
> fails.

This actually turned out to be very difficult to handle with the current design.
I added Javadoc in BatchRequest on how users can get around the problem. We will
continue to brainstorm to see if there is a better solution.

[aalbert, 2012-04-16 19:46:40]

Can you expand a bit on why this is hard?


On 2012/04/16 19:09:50, rmistry wrote:
> On 2012/04/10 22:42:12, aalbert wrote:
> > BatchRequest.execute* methods do not handle Auth Token expiration.
> > 
> > It seems that the batch request succeeds but the underlying queued
individual
> > events fail if the auth token has expired.
> > 
> > The BatchRequest went through with a 200 but the callbacks for each of the
> > inserts it contained came back through onFailure() and contained an
> > Authentication error.
> > 
> > If I call some other non-batch method before calling batch, for example,
> > calendar.events().list().execute(), the auth token get's renewed and the
list
> > request succeeds as wel ass any subsequent BatchRequests I make (including
the
> > queued requests).
> > 
> > It seems that the batch request don't require a token and succeeds but when
> the
> > backend tries to make the queued reuqests, it's using the expired token and
> > fails.
> 
> This actually turned out to be very difficult to handle with the current
design.
> I added Javadoc in BatchRequest on how users can get around the problem. We
will
> continue to brainstorm to see if there is a better solution.

[rmistry, 2012-04-16 20:13:40]

On 2012/04/16 19:46:40, aalbert wrote:
> Can you expand a bit on why this is hard?
> 

In the normal flow, when an access token expires a 401 is returned, this 401 is
handled by
http://javadoc.google-http-java-client.googlecode.com/hg/1.8.3-beta/com/googl...
which is normally
http://javadoc.google-api-java-client.googlecode.com/hg/1.8.0-beta/com/google...

GoogleCredential refreshes the access token and the request is retried. However
in the case of batch requests the main request is a 200 and the individual ones
are 401. The main request succeeds and GoogleCredential is never called to check
for 401s.

The way the python client handles this is to put the whole response in memory,
parse it and retry the 401s. We however did not want to put the whole response
in memory for java and instead use an InputStreamReader. This is why it is
difficult, I have tried to document a workaround to this in BatchRequest
javadoc.

> 
> On 2012/04/16 19:09:50, rmistry wrote:
> > On 2012/04/10 22:42:12, aalbert wrote:
> > > BatchRequest.execute* methods do not handle Auth Token expiration.
> > > 
> > > It seems that the batch request succeeds but the underlying queued
> individual
> > > events fail if the auth token has expired.
> > > 
> > > The BatchRequest went through with a 200 but the callbacks for each of the
> > > inserts it contained came back through onFailure() and contained an
> > > Authentication error.
> > > 
> > > If I call some other non-batch method before calling batch, for example,
> > > calendar.events().list().execute(), the auth token get's renewed and the
> list
> > > request succeeds as wel ass any subsequent BatchRequests I make (including
> the
> > > queued requests).
> > > 
> > > It seems that the batch request don't require a token and succeeds but
when
> > the
> > > backend tries to make the queued reuqests, it's using the expired token
and
> > > fails.
> > 
> > This actually turned out to be very difficult to handle with the current
> design.
> > I added Javadoc in BatchRequest on how users can get around the problem. We
> will
> > continue to brainstorm to see if there is a better solution.

[aalbert, 2012-04-16 20:49:05]

Can't the server side side check check the batched requests before executing
them and see if any of them to be authenticated. If so, it would try to
authenticate first before executing any of them. If it fails, it will return a
401 on the main request.

If none of the individual requests require auth, then the batch would be
executed normally...


On 2012/04/16 20:13:40, rmistry wrote:
> On 2012/04/16 19:46:40, aalbert wrote:
> > Can you expand a bit on why this is hard?
> > 
> 
> In the normal flow, when an access token expires a 401 is returned, this 401
is
> handled by
>
http://javadoc.google-http-java-client.googlecode.com/hg/1.8.3-beta/com/googl...)
> which is normally
>
http://javadoc.google-api-java-client.googlecode.com/hg/1.8.0-beta/com/google...
> 
> GoogleCredential refreshes the access token and the request is retried.
However
> in the case of batch requests the main request is a 200 and the individual
ones
> are 401. The main request succeeds and GoogleCredential is never called to
check
> for 401s.
> 
> The way the python client handles this is to put the whole response in memory,
> parse it and retry the 401s. We however did not want to put the whole response
> in memory for java and instead use an InputStreamReader. This is why it is
> difficult, I have tried to document a workaround to this in BatchRequest
> javadoc.
> 
> > 
> > On 2012/04/16 19:09:50, rmistry wrote:
> > > On 2012/04/10 22:42:12, aalbert wrote:
> > > > BatchRequest.execute* methods do not handle Auth Token expiration.
> > > > 
> > > > It seems that the batch request succeeds but the underlying queued
> > individual
> > > > events fail if the auth token has expired.
> > > > 
> > > > The BatchRequest went through with a 200 but the callbacks for each of
the
> > > > inserts it contained came back through onFailure() and contained an
> > > > Authentication error.
> > > > 
> > > > If I call some other non-batch method before calling batch, for example,
> > > > calendar.events().list().execute(), the auth token get's renewed and the
> > list
> > > > request succeeds as wel ass any subsequent BatchRequests I make
(including
> > the
> > > > queued requests).
> > > > 
> > > > It seems that the batch request don't require a token and succeeds but
> when
> > > the
> > > > backend tries to make the queued reuqests, it's using the expired token
> and
> > > > fails.
> > > 
> > > This actually turned out to be very difficult to handle with the current
> > design.
> > > I added Javadoc in BatchRequest on how users can get around the problem.
We
> > will
> > > continue to brainstorm to see if there is a better solution.

[yanivi, 2012-04-20 21:12:34]

Sorry I haven't had time to review this deeply yet, but I've gotten a chance to
play with it a little bit, and I have a few comments already that might be
helpful.

One thing I noticed that really annoyed me is that we don't log HTTP response
and request content of type "multipart/mixed" because it isn't in the whitelist
in LogContent.isTextBasedContentType.  As far as I can tell, there appears to be
no work-around.  I had to manually add this for debugging:

|| contentType.startsWith("multipart/mixed")

I wonder if we should change the logic that we use to decide whether to log
responses.

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchCallback.java
(right):

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchCallback.java:30:
batch.queue(volumesList, Volumes.class, new BatchCallback<Volumes>() {
"<"  --> "&lt;"
">" --> "&gt;"

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchCallback.java:34:
volumes.list();
volumes.list() doesn't make sense.  You're probably looking at the wrong Volumes
:)

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java
(right):

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:251:
public void executeAndParse() throws IOException {
[optional]

When I first played with this code, I assumed that execute() would do all of
this iterative parsing, and I was surprised that it wasn't the case.

I would prefer that execute() be renamed something like executeByParts or
executeStreaming, and that executeAndParse() be renamed execute().  It would
just be more intuitive to me.

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:303:
public HttpRequest buildHttpRequest() throws IOException {
should we throw an IllegalArgumentException if requests.isEmpty()?

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java
(right):

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:127:
contentId = (line.substring("Content-ID: ".length()));
Based on the latest discussion with the server team, it sounds like this will
soon change to a "Content-ID: response-" prefix.

Note that at the time I am writing this comment, the server is actually
returning an "X-Original-Content-ID: " prefix, but this is supposedly a mistake.

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/test/j...
File
google-api-client/src/test/java/com/google/api/client/googleapis/batch/BatchRequestTest.java
(right):

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/test/j...
google-api-client/src/test/java/com/google/api/client/googleapis/batch/BatchRequestTest.java:31:
* Tests {@link BatchRequestTest}.
BatchRequest

my brain just went into an infinite loop ;)

[yanivi, 2012-04-20 22:17:27]

More comments...

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java
(right):

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:66:
public void onFailure(GoogleJsonError e, GoogleHeaders responseHeaders) {
I don't like making the assumption that this is a JSON feed.  Ideally we should
be able to use Google batch for protobuf or XML content types.  So please work
on generalizing this.  I'm serious about the protobuf support in particular, and
we should file a feature request for protobuf batch, but at this point I'm
satisfied simply to ensure that a protobuf implementation would be possible, but
not actually implement it yet.  

That said, certainly okay with providing JSON-specific layer on top of a generic
Google batch layer.

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:74:
* {@link #executeAndParse()} parses all responses and invokes their callbacks.
Low memory devices
I don't get it.  Why would you want to use execute() in low memory devices?  I
have a hard time coming up with any use case for execute().  Seems like you'd
always want to call executeAndParse().

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:90:
response.skipNextResponse();
I have a hard time coming up with a use case for skipNextResponse too.

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:95:
* Note: If an access token expires then the individual responses may contain
401s but the overall
I know this is complicated, but it would be really awesome if we could automate
all of this.  After all, everyone writing a real client application will have to
go through the effort of handling access token expiration, so it makes sense
that it would be implemented in the client library.

By the way -- and I ask this without having look at the implementation in detail
yet -- why can't we call HttpRequest.getUnsuccessfulResponseHandler?

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:130:
*/
add a comment about the implementation not being thread safe

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:134:
private String batchUrl = "https://www.googleapis.com/batch";
GenericUrl, not String

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:136:
private static final String NEWLINE = "\n";
Instead use StringUtils.LINE_SEPARATOR

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:178:
public BatchRequest(HttpTransport transport, HttpRequestInitializer
httpRequestInitializer,
Doesn't it make sense that we'd theoretically want to support a different
HttpRequestInitializer for each request part?  In fact, when queue is called, we
can get the HttpRequestFactory from the
JsonHttpRequest.getClient().getRequestFactory(), which also gives us the
transport to use.  So really we should need to pass anything to the BatchRequest
constructor.

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:225:
JsonHttpRequest jsonHttpRequest, Class<T> dataClass, BatchCallback<T> callback)
I see the problem:  We have an assumption of JSON already in JsonHttpRequest :)

Can we fix that?  As far as I can tell there is nothing JSON specific about
JsonHttpRequest.  JsonHttpClient uses JSON only in serialization/parsing of HTTP
content.  Can we implement a super class of each of those that was not
JSON-specific and pass that one into here?

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:313:
final ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream();
Nope, can't do that because you risk OutOfMemoryError's on low-memory devices. 
We should be writing directly to the HTTP request stream.  See the
implementation of MultipartRelatedContent.

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:336:
for (Map.Entry<String, Object> header : headers.entrySet()) {
how naive :)

The actual implementation of header serialization is unfortunately a bit more
complicated.  please take a look at HttpRequest.execute().  I wonder if we can
extract the logic from there into a reusable method?

[aalbert, 2012-04-20 22:24:11]

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java
(right):

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:95:
* Note: If an access token expires then the individual responses may contain
401s but the overall
I still don't understand why on the server side, executing a batch request can't
be made to require authentication itself if one or more of it's embedded
requests requires authentication?
This way, the batch request itself will fail with a 401.


On 2012/04/20 22:17:27, yanivi wrote:
> I know this is complicated, but it would be really awesome if we could
automate
> all of this.  After all, everyone writing a real client application will have
to
> go through the effort of handling access token expiration, so it makes sense
> that it would be implemented in the client library.
> 
> By the way -- and I ask this without having look at the implementation in
detail
> yet -- why can't we call HttpRequest.getUnsuccessfulResponseHandler?

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:178:
public BatchRequest(HttpTransport transport, HttpRequestInitializer
httpRequestInitializer,
I think in most cases, all requests will use the same transport etc' so I'd
appreciate a way to not have to require them with every call to queue()

An optional queue() method that allows to override it seems like a good idea
though.

On 2012/04/20 22:17:27, yanivi wrote:
> Doesn't it make sense that we'd theoretically want to support a different
> HttpRequestInitializer for each request part?  In fact, when queue is called,
we
> can get the HttpRequestFactory from the
> JsonHttpRequest.getClient().getRequestFactory(), which also gives us the
> transport to use.  So really we should need to pass anything to the
BatchRequest
> constructor.

[rmistry, 2012-04-24 13:51:08]

Addressed all comments except the last two in BatchRequest, I will work on them
next. Wanted to send what I had so far.

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchCallback.java
(right):

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchCallback.java:30:
batch.queue(volumesList, Volumes.class, new BatchCallback<Volumes>() {
On 2012/04/20 21:12:34, yanivi wrote:
> "<"  --> "&lt;"
> ">" --> "&gt;"

Done.

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchCallback.java:34:
volumes.list();
On 2012/04/20 21:12:34, yanivi wrote:
> volumes.list() doesn't make sense.  You're probably looking at the wrong
Volumes
> :)

Done.

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java
(right):

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:66:
public void onFailure(GoogleJsonError e, GoogleHeaders responseHeaders) {
On 2012/04/20 22:17:27, yanivi wrote:
> I don't like making the assumption that this is a JSON feed.  Ideally we
should
> be able to use Google batch for protobuf or XML content types.  So please work
> on generalizing this.  I'm serious about the protobuf support in particular,
and
> we should file a feature request for protobuf batch, but at this point I'm
> satisfied simply to ensure that a protobuf implementation would be possible,
but
> not actually implement it yet.  
> 
> That said, certainly okay with providing JSON-specific layer on top of a
generic
> Google batch layer.

This will actually be a lot of work. I would like to push back on this. We have
an immediate need for JSON batch, I do not see why we need to delay that for a
generic implementation specifically due to protobuf batch.

Once we have JSON batch in, I can then work on generalizing JsonHttpClient,
JsonHttpRequest, JsonHttpRequestInitializer and create a JsonBatchCallback,
JsonBatchRequest, JsonBatchUnparsedResponse. If I attempt to do this now I can
see it taking a very long time to be submitted, I would rather create multiple
feature requests for google-http-java-client and google-api-java-client to
accomplish this.

Actually as a compromise, I can rename and move these classes to
com.google.api.client.googleapis.batch.json
and then later I can generalize the implementation in
com.google.api.client.googleapis.batch. What do you think.

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:74:
* {@link #executeAndParse()} parses all responses and invokes their callbacks.
Low memory devices
On 2012/04/20 22:17:27, yanivi wrote:
> I don't get it.  Why would you want to use execute() in low memory devices?  I
> have a hard time coming up with any use case for execute().  Seems like you'd
> always want to call executeAndParse().

I suppose both methods will end up doing exactly the same thing. I wanted
executeAsParts() because users will then be able to parse each response when
they were ready and skip a response if required (I cannot come up with a usecase
yet for skipping see comment below).
Are you recommending that we remove executeAsParts, remove ignoreNextResponse
and not expose parseNextResponse?

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:90:
response.skipNextResponse();
On 2012/04/20 22:17:27, yanivi wrote:
> I have a hard time coming up with a use case for skipNextResponse too.

I am not sure either, but should we still go ahead and provide this
functionality in-case users find a need for it? 
There may be some use cases relating to memory conservation.

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:95:
* Note: If an access token expires then the individual responses may contain
401s but the overall
On 2012/04/20 22:17:27, yanivi wrote:
> I know this is complicated, but it would be really awesome if we could
automate
> all of this.  After all, everyone writing a real client application will have
to
> go through the effort of handling access token expiration, so it makes sense
> that it would be implemented in the client library.

I am not sure how to automate this especially since my solution relies on the
call backs to figure out what is unauthenticated.

> 
> By the way -- and I ask this without having look at the implementation in
detail
> yet -- why can't we call HttpRequest.getUnsuccessfulResponseHandler?

UnsucessfulResponseHandler will not be invoked because the Batch Request is a
200 regardless of the status codes of the individual requests.

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:95:
* Note: If an access token expires then the individual responses may contain
401s but the overall
On 2012/04/20 22:24:11, aalbert wrote:
> I still don't understand why on the server side, executing a batch request
can't
> be made to require authentication itself if one or more of it's embedded
> requests requires authentication?
> This way, the batch request itself will fail with a 401.

This would be a non-backwards compatible change and the stack team has to buy
off on it and implement it. Also the current behavior does make sense for some
of the other client library implementations, for eg: Python stores all responses
in memory, figures out which ones have 401s and retries them.

I will leave it upto Yaniv to decide if we would like to request this of the
stack team.

> 
> 
> On 2012/04/20 22:17:27, yanivi wrote:
> > I know this is complicated, but it would be really awesome if we could
> automate
> > all of this.  After all, everyone writing a real client application will
have
> to
> > go through the effort of handling access token expiration, so it makes sense
> > that it would be implemented in the client library.
> > 
> > By the way -- and I ask this without having look at the implementation in
> detail
> > yet -- why can't we call HttpRequest.getUnsuccessfulResponseHandler?
>

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:130:
*/
On 2012/04/20 22:17:27, yanivi wrote:
> add a comment about the implementation not being thread safe

Done.

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:134:
private String batchUrl = "https://www.googleapis.com/batch";
On 2012/04/20 22:17:27, yanivi wrote:
> GenericUrl, not String

Done.

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:136:
private static final String NEWLINE = "\n";
On 2012/04/20 22:17:27, yanivi wrote:
> Instead use StringUtils.LINE_SEPARATOR

Done.

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:178:
public BatchRequest(HttpTransport transport, HttpRequestInitializer
httpRequestInitializer,
On 2012/04/20 22:17:27, yanivi wrote:
> Doesn't it make sense that we'd theoretically want to support a different
> HttpRequestInitializer for each request part?  In fact, when queue is called,
we
> can get the HttpRequestFactory from the
> JsonHttpRequest.getClient().getRequestFactory(), which also gives us the
> transport to use.  So really we should need to pass anything to the
BatchRequest
> constructor.

Yes, like Alon mentioned, in most cases all requests will use the same
transport. The batch request is a single request with a single
HttpRequestInitializer, I do not know how to support multiple request
initializers I also think that this is a similar problem as the 401s.

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:225:
JsonHttpRequest jsonHttpRequest, Class<T> dataClass, BatchCallback<T> callback)
On 2012/04/20 22:17:27, yanivi wrote:
> I see the problem:  We have an assumption of JSON already in JsonHttpRequest
:)
> 
> Can we fix that?  As far as I can tell there is nothing JSON specific about
> JsonHttpRequest.  JsonHttpClient uses JSON only in serialization/parsing of
HTTP
> content.  Can we implement a super class of each of those that was not
> JSON-specific and pass that one into here?

Please see my first comment in this class.

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:251:
public void executeAndParse() throws IOException {
On 2012/04/20 21:12:34, yanivi wrote:
> [optional]
> 
> When I first played with this code, I assumed that execute() would do all of
> this iterative parsing, and I was surprised that it wasn't the case.
> 
> I would prefer that execute() be renamed something like executeByParts or
> executeStreaming, and that executeAndParse() be renamed execute().  It would
> just be more intuitive to me.

sgtm. Done.

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:303:
public HttpRequest buildHttpRequest() throws IOException {
On 2012/04/20 21:12:34, yanivi wrote:
> should we throw an IllegalArgumentException if requests.isEmpty()?

Done with unit test.

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java
(right):

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:127:
contentId = (line.substring("Content-ID: ".length()));
On 2012/04/20 21:12:34, yanivi wrote:
> Based on the latest discussion with the server team, it sounds like this will
> soon change to a "Content-ID: response-" prefix.
> 
> Note that at the time I am writing this comment, the server is actually
> returning an "X-Original-Content-ID: " prefix, but this is supposedly a
mistake.

Production is currently returning "Content:ID: response-" prefix. Fixed it.

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/test/j...
File
google-api-client/src/test/java/com/google/api/client/googleapis/batch/BatchRequestTest.java
(right):

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/test/j...
google-api-client/src/test/java/com/google/api/client/googleapis/batch/BatchRequestTest.java:31:
* Tests {@link BatchRequestTest}.
On 2012/04/20 21:12:34, yanivi wrote:
> BatchRequest
> 
> my brain just went into an infinite loop ;)

Done.

[yanivi, 2012-04-24 20:47:43]

Just a few quick comments, but still not a thorough review...

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java
(right):

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:66:
public void onFailure(GoogleJsonError e, GoogleHeaders responseHeaders) {
On 2012/04/24 13:51:08, rmistry wrote:
> On 2012/04/20 22:17:27, yanivi wrote:
> > I don't like making the assumption that this is a JSON feed.  Ideally we
> should
> > be able to use Google batch for protobuf or XML content types.  So please
work
> > on generalizing this.  I'm serious about the protobuf support in particular,
> and
> > we should file a feature request for protobuf batch, but at this point I'm
> > satisfied simply to ensure that a protobuf implementation would be possible,
> but
> > not actually implement it yet.  
> > 
> > That said, certainly okay with providing JSON-specific layer on top of a
> generic
> > Google batch layer.
> 
> This will actually be a lot of work. I would like to push back on this. We
have
> an immediate need for JSON batch, I do not see why we need to delay that for a
> generic implementation specifically due to protobuf batch.

> 
> Once we have JSON batch in, I can then work on generalizing JsonHttpClient,
> JsonHttpRequest, JsonHttpRequestInitializer and create a JsonBatchCallback,
> JsonBatchRequest, JsonBatchUnparsedResponse. If I attempt to do this now I can
> see it taking a very long time to be submitted, I would rather create multiple
> feature requests for google-http-java-client and google-api-java-client to
> accomplish this.
> 
> Actually as a compromise, I can rename and move these classes to
> com.google.api.client.googleapis.batch.json
> and then later I can generalize the implementation in
> com.google.api.client.googleapis.batch. What do you think.

Seems like a reasonable compromise.  Move it to
com.google.apis.client.googleapis.json.batch and rename classes to
BatchJsonCallback, BatchJsonRequest, BatchJsonUnparsedResponse.

But let's not drop the ball on this.  Internally at Google we use protobufs a
lot so there is a lot of pressure on us to provide better support for them if we
want our library to be taken more seriously.

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:74:
* {@link #executeAndParse()} parses all responses and invokes their callbacks.
Low memory devices
On 2012/04/24 13:51:08, rmistry wrote:
> On 2012/04/20 22:17:27, yanivi wrote:
> > I don't get it.  Why would you want to use execute() in low memory devices? 
I
> > have a hard time coming up with any use case for execute().  Seems like
you'd
> > always want to call executeAndParse().
> 
> I suppose both methods will end up doing exactly the same thing. I wanted
> executeAsParts() because users will then be able to parse each response when
> they were ready and skip a response if required (I cannot come up with a
usecase
> yet for skipping see comment below).
> Are you recommending that we remove executeAsParts, remove ignoreNextResponse
> and not expose parseNextResponse?

Yes.  Start simple.  Listen to feedback from developers if they actually need
it.

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:90:
response.skipNextResponse();
On 2012/04/24 13:51:08, rmistry wrote:
> On 2012/04/20 22:17:27, yanivi wrote:
> > I have a hard time coming up with a use case for skipNextResponse too.
> 
> I am not sure either, but should we still go ahead and provide this
> functionality in-case users find a need for it? 
> There may be some use cases relating to memory conservation. 

I'm not sure about the memory conservation claim.  The only case I can think of
where there may be a memory saving is if you don't want to parse a successful
response.  In that case, I think it would be better to allow the user to specify
that when building the request or when calling the queue method.  But let's keep
it simple for the first release.

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:95:
* Note: If an access token expires then the individual responses may contain
401s but the overall
On 2012/04/24 13:51:08, rmistry wrote:
> On 2012/04/20 22:24:11, aalbert wrote:
> > I still don't understand why on the server side, executing a batch request
> can't
> > be made to require authentication itself if one or more of it's embedded
> > requests requires authentication?
> > This way, the batch request itself will fail with a 401.
> 
> This would be a non-backwards compatible change and the stack team has to buy
> off on it and implement it. Also the current behavior does make sense for some
> of the other client library implementations, for eg: Python stores all
responses
> in memory, figures out which ones have 401s and retries them.
> 
> I will leave it upto Yaniv to decide if we would like to request this of the
> stack team.
> 
> > 
> > 
> > On 2012/04/20 22:17:27, yanivi wrote:
> > > I know this is complicated, but it would be really awesome if we could
> > automate
> > > all of this.  After all, everyone writing a real client application will
> have
> > to
> > > go through the effort of handling access token expiration, so it makes
sense
> > > that it would be implemented in the client library.
> > > 
> > > By the way -- and I ask this without having look at the implementation in
> > detail
> > > yet -- why can't we call HttpRequest.getUnsuccessfulResponseHandler?
> > 
> 

I filed a feature request to the server team asking the whole batch request to
fail if the access token expired.  We'll see if it gets implemented.

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:95:
* Note: If an access token expires then the individual responses may contain
401s but the overall
On 2012/04/24 13:51:08, rmistry wrote:
> On 2012/04/20 22:17:27, yanivi wrote:
> > I know this is complicated, but it would be really awesome if we could
> automate
> > all of this.  After all, everyone writing a real client application will
have
> to
> > go through the effort of handling access token expiration, so it makes sense
> > that it would be implemented in the client library.
> 
> I am not sure how to automate this especially since my solution relies on the
> call backs to figure out what is unauthenticated.
> 
> > 
> > By the way -- and I ask this without having look at the implementation in
> detail
> > yet -- why can't we call HttpRequest.getUnsuccessfulResponseHandler?
> 
> UnsucessfulResponseHandler will not be invoked because the Batch Request is a
> 200 regardless of the status codes of the individual requests.

I now see what the problem is with UnsuccessfulResponseHandler: we would need an
instance of HttpResponse to pass to UnsuccessfulResponseHandler.handleResponse. 
We may want to invent something specific for handling auth errors for batch,
maybe something like:

interface BatchErrorHandler {
  boolean handleResponse(int statusCode) throws IOException;
}

Presumably you'd have it tied to a Credential, and you'd call
Credential.refreshToken() if needed.

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:178:
public BatchRequest(HttpTransport transport, HttpRequestInitializer
httpRequestInitializer,
On 2012/04/24 13:51:08, rmistry wrote:
> On 2012/04/20 22:17:27, yanivi wrote:
> > Doesn't it make sense that we'd theoretically want to support a different
> > HttpRequestInitializer for each request part?  In fact, when queue is
called,
> we
> > can get the HttpRequestFactory from the
> > JsonHttpRequest.getClient().getRequestFactory(), which also gives us the
> > transport to use.  So really we should need to pass anything to the
> BatchRequest
> > constructor.
> 
> Yes, like Alon mentioned, in most cases all requests will use the same
> transport. The batch request is a single request with a single
> HttpRequestInitializer, I do not know how to support multiple request
> initializers I also think that this is a similar problem as the 401s.

I completely agree that the transport is the same.  The question is whether the
HttpRequestInitializer is the same.

In retrospect I'm now considering that HttpRequestInitializer shouldn't be the
same.  Rather, it should only be applied to the top-level HTTP request, and not
at all to the parts.

But I'll need to think about this more...

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:313:
final ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream();
On 2012/04/20 22:17:27, yanivi wrote:
> Nope, can't do that because you risk OutOfMemoryError's on low-memory devices.

> We should be writing directly to the HTTP request stream.  See the
> implementation of MultipartRelatedContent.

Ping.

(I know you already mentioned you haven't done it yet, but it's easier for me to
track this way)

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:336:
for (Map.Entry<String, Object> header : headers.entrySet()) {
On 2012/04/20 22:17:27, yanivi wrote:
> how naive :)
> 
> The actual implementation of header serialization is unfortunately a bit more
> complicated.  please take a look at HttpRequest.execute().  I wonder if we can
> extract the logic from there into a reusable method?

Ping.

http://codereview.appspot.com/5539055/diff/64001/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java
(right):

http://codereview.appspot.com/5539055/diff/64001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:142:
private static final String NEWLINE = StringUtils.LINE_SEPARATOR;
[optional] inline NEWLINE, i.e. just use StringUtil.LINE_SEPARATOR directly

[rmistry, 2012-04-25 12:50:17]

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java
(right):

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:66:
public void onFailure(GoogleJsonError e, GoogleHeaders responseHeaders) {
On 2012/04/24 20:47:43, yanivi wrote:
> On 2012/04/24 13:51:08, rmistry wrote:
> > On 2012/04/20 22:17:27, yanivi wrote:
> > > I don't like making the assumption that this is a JSON feed.  Ideally we
> > should
> > > be able to use Google batch for protobuf or XML content types.  So please
> work
> > > on generalizing this.  I'm serious about the protobuf support in
particular,
> > and
> > > we should file a feature request for protobuf batch, but at this point I'm
> > > satisfied simply to ensure that a protobuf implementation would be
possible,
> > but
> > > not actually implement it yet.  
> > > 
> > > That said, certainly okay with providing JSON-specific layer on top of a
> > generic
> > > Google batch layer.
> > 
> > This will actually be a lot of work. I would like to push back on this. We
> have
> > an immediate need for JSON batch, I do not see why we need to delay that for
a
> > generic implementation specifically due to protobuf batch.
> 
> > 
> > Once we have JSON batch in, I can then work on generalizing JsonHttpClient,
> > JsonHttpRequest, JsonHttpRequestInitializer and create a JsonBatchCallback,
> > JsonBatchRequest, JsonBatchUnparsedResponse. If I attempt to do this now I
can
> > see it taking a very long time to be submitted, I would rather create
multiple
> > feature requests for google-http-java-client and google-api-java-client to
> > accomplish this.
> > 
> > Actually as a compromise, I can rename and move these classes to
> > com.google.api.client.googleapis.batch.json
> > and then later I can generalize the implementation in
> > com.google.api.client.googleapis.batch. What do you think.
> 
> Seems like a reasonable compromise.  Move it to
> com.google.apis.client.googleapis.json.batch and rename classes to
> BatchJsonCallback, BatchJsonRequest, BatchJsonUnparsedResponse.

Done.

> 
> But let's not drop the ball on this.  Internally at Google we use protobufs a
> lot so there is a lot of pressure on us to provide better support for them if
we
> want our library to be taken more seriously.

Filed http://code.google.com/p/google-api-java-client/issues/detail?id=468.

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:74:
* {@link #executeAndParse()} parses all responses and invokes their callbacks.
Low memory devices
On 2012/04/24 20:47:43, yanivi wrote:
> On 2012/04/24 13:51:08, rmistry wrote:
> > On 2012/04/20 22:17:27, yanivi wrote:
> > > I don't get it.  Why would you want to use execute() in low memory
devices? 
> I
> > > have a hard time coming up with any use case for execute().  Seems like
> you'd
> > > always want to call executeAndParse().
> > 
> > I suppose both methods will end up doing exactly the same thing. I wanted
> > executeAsParts() because users will then be able to parse each response when
> > they were ready and skip a response if required (I cannot come up with a
> usecase
> > yet for skipping see comment below).
> > Are you recommending that we remove executeAsParts, remove
ignoreNextResponse
> > and not expose parseNextResponse?
> 
> Yes.  Start simple.  Listen to feedback from developers if they actually need
> it.

Done.

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:90:
response.skipNextResponse();
On 2012/04/24 20:47:43, yanivi wrote:
> On 2012/04/24 13:51:08, rmistry wrote:
> > On 2012/04/20 22:17:27, yanivi wrote:
> > > I have a hard time coming up with a use case for skipNextResponse too.
> > 
> > I am not sure either, but should we still go ahead and provide this
> > functionality in-case users find a need for it? 
> > There may be some use cases relating to memory conservation. 
> 
> I'm not sure about the memory conservation claim.  The only case I can think
of
> where there may be a memory saving is if you don't want to parse a successful
> response.  In that case, I think it would be better to allow the user to
specify
> that when building the request or when calling the queue method.  But let's
keep
> it simple for the first release.

Removed the methods.

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:95:
* Note: If an access token expires then the individual responses may contain
401s but the overall
On 2012/04/24 20:47:43, yanivi wrote:
> On 2012/04/24 13:51:08, rmistry wrote:
> > On 2012/04/20 22:17:27, yanivi wrote:
> > > I know this is complicated, but it would be really awesome if we could
> > automate
> > > all of this.  After all, everyone writing a real client application will
> have
> > to
> > > go through the effort of handling access token expiration, so it makes
sense
> > > that it would be implemented in the client library.
> > 
> > I am not sure how to automate this especially since my solution relies on
the
> > call backs to figure out what is unauthenticated.
> > 
> > > 
> > > By the way -- and I ask this without having look at the implementation in
> > detail
> > > yet -- why can't we call HttpRequest.getUnsuccessfulResponseHandler?
> > 
> > UnsucessfulResponseHandler will not be invoked because the Batch Request is
a
> > 200 regardless of the status codes of the individual requests.
> 
> I now see what the problem is with UnsuccessfulResponseHandler: we would need
an
> instance of HttpResponse to pass to
UnsuccessfulResponseHandler.handleResponse. 
> We may want to invent something specific for handling auth errors for batch,
> maybe something like:
> 
> interface BatchErrorHandler {
>   boolean handleResponse(int statusCode) throws IOException;
> }
> 
> Presumably you'd have it tied to a Credential, and you'd call
> Credential.refreshToken() if needed.

Not sure I understand. We will know there is a 401 only when we start parsing
individual responses, at that point refreshing tokens will not help because we
already have responses. It would only work if we refresh tokens and try requests
again.
Though all this will be made infinitely simpler if the feature request you filed
to the server team is implemented.

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:178:
public BatchRequest(HttpTransport transport, HttpRequestInitializer
httpRequestInitializer,
On 2012/04/24 20:47:43, yanivi wrote:
> On 2012/04/24 13:51:08, rmistry wrote:
> > On 2012/04/20 22:17:27, yanivi wrote:
> > > Doesn't it make sense that we'd theoretically want to support a different
> > > HttpRequestInitializer for each request part?  In fact, when queue is
> called,
> > we
> > > can get the HttpRequestFactory from the
> > > JsonHttpRequest.getClient().getRequestFactory(), which also gives us the
> > > transport to use.  So really we should need to pass anything to the
> > BatchRequest
> > > constructor.
> > 
> > Yes, like Alon mentioned, in most cases all requests will use the same
> > transport. The batch request is a single request with a single
> > HttpRequestInitializer, I do not know how to support multiple request
> > initializers I also think that this is a similar problem as the 401s.
> 
> I completely agree that the transport is the same.  The question is whether
the
> HttpRequestInitializer is the same.
> 
> In retrospect I'm now considering that HttpRequestInitializer shouldn't be the
> same.  Rather, it should only be applied to the top-level HTTP request, and
not
> at all to the parts.
> 
> But I'll need to think about this more...

Let me know what you conclude.

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:313:
final ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream();
On 2012/04/24 20:47:43, yanivi wrote:
> On 2012/04/20 22:17:27, yanivi wrote:
> > Nope, can't do that because you risk OutOfMemoryError's on low-memory
devices.
> 
> > We should be writing directly to the HTTP request stream.  See the
> > implementation of MultipartRelatedContent.
> 
> Ping.
> 
> (I know you already mentioned you haven't done it yet, but it's easier for me
to
> track this way)

Done in http://codereview.appspot.com/6121048/

http://codereview.appspot.com/5539055/diff/51002/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:336:
for (Map.Entry<String, Object> header : headers.entrySet()) {
On 2012/04/24 20:47:43, yanivi wrote:
> On 2012/04/20 22:17:27, yanivi wrote:
> > how naive :)
> > 
> > The actual implementation of header serialization is unfortunately a bit
more
> > complicated.  please take a look at HttpRequest.execute().  I wonder if we
can
> > extract the logic from there into a reusable method?
> 
> Ping.

Done in http://codereview.appspot.com/6121048/.

http://codereview.appspot.com/5539055/diff/64001/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java
(right):

http://codereview.appspot.com/5539055/diff/64001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:142:
private static final String NEWLINE = StringUtils.LINE_SEPARATOR;
On 2012/04/24 20:47:43, yanivi wrote:
> [optional] inline NEWLINE, i.e. just use StringUtil.LINE_SEPARATOR directly

NEWLINE makes more sense now in http://codereview.appspot.com/6121048/.

[rmistry, 2012-04-26 12:14:10]

I implemented a proposal to solve the unauthenticated requests problem.
If the design looks ok, please let me know and I will clean up javadocs and add
unit tests.

[rmistry, 2012-04-30 17:08:10]

Brought in MultipartMixedContent based on our IM conversation

[yanivi, 2012-04-30 20:10:39]

At this point, I'm still reviewing it at a high level.  Some really interesting
comments below...

http://codereview.appspot.com/5539055/diff/78001/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/json/BatchJsonRequest.java
(right):

http://codereview.appspot.com/5539055/diff/78001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/json/BatchJsonRequest.java:157:
* @param credential The credential to use when creating an {@link HttpRequest}
or {@code null}
The coupling to Credential concerns me.  It means that other forms of
authentication won't be supported.  What we really ideally want is to use is
HttpUnsuccessfulResponseHandler directly.  The only catch is that you have to
pass it an HttpResponse.  See my other comment regarding my thoughts about how
to construct a fake HttpResponse.

http://codereview.appspot.com/5539055/diff/78001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/json/BatchJsonRequest.java:201:
* @param jsonHttpRequest JSON HTTP Request
Why couple batch to JsonHttpRequest?  Makes no sense to me.  Why not just pass
in HttpRequest as a parameter?  It would be really useful to be able to use
batch with any Google request.

http://codereview.appspot.com/5539055/diff/78001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/json/BatchJsonRequest.java:203:
* @param callback Batch Callback or {@code null} for none
I'm not comfortable with the idea of allowing there to be no callback.  What
happens if there is an error?  If your intention is to provide the user with a
way of saying that a successful response be ignored, that seems like a more
reasonable option, and can be accomplished via an additional parameter to the
queue method.

http://codereview.appspot.com/5539055/diff/78001/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/json/BatchJsonUnparsedResponse.java
(right):

http://codereview.appspot.com/5539055/diff/78001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/json/BatchJsonUnparsedResponse.java:118:
StringBuilder partContent = new StringBuilder();
Unfortunately, OutOfMemoryError is likely to result from this.  We need to
figure out a way to pass the stream directly to the parser.

I have a crazy idea that just might work: use BufferedReader to read the
headers, but use the InputStream directly to parse the response.

http://codereview.appspot.com/5539055/diff/78001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/json/BatchJsonUnparsedResponse.java:123:
JsonParser jsonParser = jsonFactory.createJsonParser(partContent.toString());
ok so this is where the actual tie-in to JSON happens.  If we can figure out a
way to make this generic, then we will be able to completely decouple our batch
implementation from the JSON.  That would allow it to be reused for example for
protobuf or XML.

There is another use case though: Latitude uses an old format in which the JSON
response is wrapped inside a top-level "data" key.  We use JsonCParser for that
case.

What we *really* need is to use HttpParser.  That would solve all of the above
problems.

The only catch: we would need to pass an HttpResponse to HttpParser.  But I
think it should be theoretically possible using a technique similar to what
MockHttpTransport does.  i.e. we need to provide an implementation of
HttpTransport that doesn't actually make a real request, but whose returned
LowLevelHttpResponse.getContent() returns the inputStream.

[rmistry, 2012-05-03 12:17:08]

http://codereview.appspot.com/5539055/diff/78001/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/json/BatchJsonRequest.java
(right):

http://codereview.appspot.com/5539055/diff/78001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/json/BatchJsonRequest.java:157:
* @param credential The credential to use when creating an {@link HttpRequest}
or {@code null}
On 2012/04/30 20:10:39, yanivi wrote:
> The coupling to Credential concerns me.  It means that other forms of
> authentication won't be supported.  What we really ideally want is to use is
> HttpUnsuccessfulResponseHandler directly.  The only catch is that you have to
> pass it an HttpResponse.  See my other comment regarding my thoughts about how
> to construct a fake HttpResponse.

Done.

http://codereview.appspot.com/5539055/diff/78001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/json/BatchJsonRequest.java:201:
* @param jsonHttpRequest JSON HTTP Request
On 2012/04/30 20:10:39, yanivi wrote:
> Why couple batch to JsonHttpRequest?  Makes no sense to me.  Why not just pass
> in HttpRequest as a parameter?  It would be really useful to be able to use
> batch with any Google request.

Done.

http://codereview.appspot.com/5539055/diff/78001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/json/BatchJsonRequest.java:203:
* @param callback Batch Callback or {@code null} for none
On 2012/04/30 20:10:39, yanivi wrote:
> I'm not comfortable with the idea of allowing there to be no callback.  What
> happens if there is an error?  If your intention is to provide the user with a
> way of saying that a successful response be ignored, that seems like a more
> reasonable option, and can be accomplished via an additional parameter to the
> queue method.

Made the callback compulsory. Done.

http://codereview.appspot.com/5539055/diff/78001/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/json/BatchJsonUnparsedResponse.java
(right):

http://codereview.appspot.com/5539055/diff/78001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/json/BatchJsonUnparsedResponse.java:118:
StringBuilder partContent = new StringBuilder();
On 2012/04/30 20:10:39, yanivi wrote:
> Unfortunately, OutOfMemoryError is likely to result from this.  We need to
> figure out a way to pass the stream directly to the parser.
> 
> I have a crazy idea that just might work: use BufferedReader to read the
> headers, but use the InputStream directly to parse the response.

I spent a lot of time trying to get this to work and we had a long discussion
yesterday about this. This was the summary of our discussion (as compiled by
Yaniv):

1. ideally we should use a stream parser approach
2. jackson parser appears to support this, but it's a bit "messy"
3. gson parser appears to not support this
4. we expect each individual batch part to not be too large to store in memory
5. for now, we decide to store whole part in memory, and wait for feedback to
hear if this becomes a problem in practice
6. we can specify this limitation in the javadoc

http://codereview.appspot.com/5539055/diff/78001/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/json/BatchJsonUnparsedResponse.java:123:
JsonParser jsonParser = jsonFactory.createJsonParser(partContent.toString());
On 2012/04/30 20:10:39, yanivi wrote:
> ok so this is where the actual tie-in to JSON happens.  If we can figure out a
> way to make this generic, then we will be able to completely decouple our
batch
> implementation from the JSON.  That would allow it to be reused for example
for
> protobuf or XML.
> 
> There is another use case though: Latitude uses an old format in which the
JSON
> response is wrapped inside a top-level "data" key.  We use JsonCParser for
that
> case.
> 
> What we *really* need is to use HttpParser.  That would solve all of the above
> problems.
> 
> The only catch: we would need to pass an HttpResponse to HttpParser.  But I
> think it should be theoretically possible using a technique similar to what
> MockHttpTransport does.  i.e. we need to provide an implementation of
> HttpTransport that doesn't actually make a real request, but whose returned
> LowLevelHttpResponse.getContent() returns the inputStream.

Done.
Though there is a strange logging issue that comes up due to these fake requests
and responses, I see this logged:


May 3, 2012 7:38:25 AM com.google.api.client.http.HttpRequest execute
CONFIG: -------------- REQUEST  --------------
POST http://google.com/
Accept-Encoding: gzip
User-Agent: Google-HTTP-Java-Client/1.9.0-beta-SNAPSHOT (gzip)

May 3, 2012 7:38:25 AM com.google.api.client.http.HttpResponse <init>
CONFIG: -------------- RESPONSE --------------


Everytime the fake response is created. Do you know how to stop this from
logging? I tried request.setContentLoggingLimit(0); but it still gets logged.

http://codereview.appspot.com/5539055/diff/96002/google-api-client/src/test/j...
File
google-api-client/src/test/java/com/google/api/client/googleapis/batch/BatchRequestTest.java
(right):

http://codereview.appspot.com/5539055/diff/96002/google-api-client/src/test/j...
google-api-client/src/test/java/com/google/api/client/googleapis/batch/BatchRequestTest.java:248:
//  public void testExecuteWithError() throws Exception {
This test is commented out because:
Previously in BatchUnparsedResponse I was doing-
jsonParser.skipToKey("error");
if (jsonParser.getCurrentToken() != JsonToken.END_OBJECT) {
 callback.onFailure(jsonParser.parser(GoogleJsonError.class, ...
}
 
Now I am doing:
callback.onFailure(parser.parse(response, errorClass), responseHeaders);
  
I can no longer skip to the error key and parse properly as GoogleJsonError. Any
thoughts on what to do in BatchUnparsedResponse or the Callback with HttpParser?

[yanivi, 2012-05-04 18:58:38]

Next round of comments, this time starting to get into some of the details...

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchCallback.java
(right):

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchCallback.java:29:
batch.queue(volumesList, credential, Volumes.class, GoogleJsonError.class,
I see some error messages with this sample code.  Please recheck this sample
code and the one in BatchRequest.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java
(right):

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:36:
* An instance of this class represents a single Batch of requests.
"Batch" -> "batch"

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:116:
static class CallbackAndDataClass<T, E> {
as far as I can tell the generic type parameters don't make sense here

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:117:
private BatchCallback<T, E> callback;
[optional] just make these package private and remove the getter methods

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:185:
HttpRequest batchRequest;
move field declaration to before constructors

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:254:
execute();
I'm really worried about an infinite loop.  Can we please set an upper limit for
the number of retries?

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/MultipartMixedContent.java
(right):

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/MultipartMixedContent.java:64:
Preconditions.checkArgument(!requests.isEmpty());
    Preconditions.checkArgument(requests.size() < Integer.MAX_VALUE);

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/MultipartMixedContent.java:75:
String twoDashes = "--";
private static final String TWO_DASHES (or DASH_DASH)

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/MultipartMixedContent.java:97:
writer.write(request.getUrl().toString());
.build() instead of .toString()

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/MultipartMixedContent.java:106:
writeHeader(writer, "Content-Type", "" + data.getType());
remove the "" +

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/MultipartMixedContent.java:107:
writeHeader(writer, "Content-Length", "" + data.getLength());
String.valueOf(data.getLength())

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/MultipartMixedContent.java:127:
public String getBoundary() {
remove unused getBoundary() and getRequests()

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/MultipartMixedContent.java:139:
private void writeHeader(Writer writer, String name, Object value) throws
IOException {
simpler to make value String and then remove the .toString() below

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java
(right):

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java:118:
return new BatchRequest(getRequestFactory().getTransport(),
you need to call BatchRequest.setBatchUrl(getRootUrl() + "batch")

there is no way to avoid this: we need a getRootUrl()

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java:119:
getRequestFactory().getInitializer(), new JsonHttpParser(getJsonFactory()));
initializer is a tricky concept.  The question is whether initializer applies to
each individual request part or to the batch part.

For example, imagine Credential as the initializer. Furthermore, imagine that I
use a Credential for Calendar in one request part and Credential for Tasks in
another request part.  I'd want no Authorization header at the batch request
level, but rather an Authorization header at each individual part.

So I'd argue that the initializer should be applied to the individual request
parts, not at the batch level.  That said, I do think there is value in allowing
a developer to specify an HttpRequestInitializer on the batch request itself. 
That could potentially be done as an optional parameter to the batch method. 
Another possible option is to specify a batch request initializer in
GoogleClient.Builder.  There may be another way to do this.

What do you think?

[yanivi, 2012-05-04 20:47:35]

More comments...

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchCallback.java
(right):

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchCallback.java:48:
public interface BatchCallback<T, E> {
[optional]
BatchCallback is really nice for parsed responses.

But ideally I'd like to have an alternative like:

public interface BatchUnparsedCallback {
  void onResponse(InputStream content, GoogleHeaders);
}
to provide an ability to get the content stream directly.  Two possible use
cases I can think of:

* Media download

* For efficiency, to use a streaming parser and avoid allocating any objects

Could be done in a future changeset though.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java
(right):

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:105:
private Map<String, CallbackAndDataClass<?, ?>> idToCallbackAndDataClass =
Can we even merge idToCallbackAndDataClass with requests?  After all, it is just
a flat list of requests with their associated information.  This whole idea of a
map is sort of unnecessary here since the keys are essentially equivalent to the
index into the requests array.

I do worry about unnecessary complexity in our implementation.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:109:
private Map<String, HttpUnsuccessfulResponseHandler>
idToUnsuccessfulResponseHandler =
why not just merge this into idToCallbackAndDataClass?

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:113:
private boolean retryUnsuccessfulRequests = true;
this should really be a local variable inside execute()

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:116:
static class CallbackAndDataClass<T, E> {
[optional] call it RequestInfo?

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:150:
HttpParser httpParser) {
httpParser should ideally be a parameter for queue

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:195:
* @param dataClass Data class the response will be parsed into or {@code null}
for none
Can we please provide more detail on what null means for data class?  The fact
that this causes the response to be ignored and onSuccess to not be called was
non-obvious to me.  Another option is we could still call onSuccess but with
null for the content, which at least notifies that a response as been received
and gives option to parse headers.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:235:
String boundary = "--" + contentType.substring(boundaryStartIndex +
"boundary=".length());
Seems kind of hacky.  I filed this:

http://code.google.com/p/google-http-java-client/issues/detail?id=100

for now, can we please split on the ";"'s and then find the part that starts
with "boundary="?

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:267:
HttpContent content = new MultipartMixedContent(requests, "END_OF_PART");
[optional] I worry slightly about this boundary clashing with actual data.  What
do you think of trying to make this even more obscure?

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java
(right):

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:28:
import com.google.api.client.testing.http.HttpTesting;
Please don't introduce any dependency on the com.google.api.client.testing.http
package from here.  We don't want to accidentally introduce any testing-only
code into production code.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:69:
private List<String> unsuccessfulContentIDsForRetry = new ArrayList<String>();
I would just make this field package private and allow it to be accessed
directly.

Or better yet: why not just build the List<HttpRequest> unsuccessfulRequests
directly here and avoid the unsuccessfulContentIDsForRetry field?

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:84:
public BatchUnparsedResponse(InputStream inputStream, String boundary,
HttpParser httpParser,
avoid the public modifier on a package private class

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:132:
responseHeaders.set(headerParts[0], headerParts[1]);
oh oh.  I think we need HttpHeaders.parseHeadersForMultipartResponses

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:135:
// Extract the response part content.
can we please add a TODO to investigate a way to use the stream directly?  It
would reduce the chance of an OutOfMemoryError and likely make the parsing
noticeably more efficient.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:137:
while ((line = bufferedReader.readLine()) != null && !line.equals("")) {
line.equals("") is the boundary or should we check instead for
line.equals(boundary)?

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:172:
if (statusCode == HttpStatusCodes.STATUS_CODE_OK) {
shouldn't we use HttpStatusCodes.isSuccess?

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:265:
new ByteArrayInputStream(partContent.getBytes()), statusCode);
first of all don't call String.getBytes() and instead call
StringUtils.getBytesUtf8(String).

But this is also inefficient because we parse the stream as bytes, UTF-8 encode
into a String, UTF-8 decode into bytes, and then UTF-8 encode it again when
parsing the JSON.  It would be more efficiently to only do the UTF-8 encoding
once.  Ideally we wouldn't read the the partContent as String but instead as
bytes -- let's call it partBytes -- and then just pass that directly to new
ByteArrayInputStream().

[rmistry, 2012-05-07 19:37:36]

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchCallback.java
(right):

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchCallback.java:29:
batch.queue(volumesList, credential, Volumes.class, GoogleJsonError.class,
On 2012/05/04 18:58:38, yanivi wrote:
> I see some error messages with this sample code.  Please recheck this sample
> code and the one in BatchRequest.

Done.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchCallback.java:48:
public interface BatchCallback<T, E> {
On 2012/05/04 20:47:35, yanivi wrote:
> [optional]
> BatchCallback is really nice for parsed responses.
> 
> But ideally I'd like to have an alternative like:
> 
> public interface BatchUnparsedCallback {
>   void onResponse(InputStream content, GoogleHeaders);
> }
> to provide an ability to get the content stream directly.  Two possible use
> cases I can think of:
> 
> * Media download
> 
> * For efficiency, to use a streaming parser and avoid allocating any objects
> 
> Could be done in a future changeset though.

Added TODO.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java
(right):

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:36:
* An instance of this class represents a single Batch of requests.
On 2012/05/04 18:58:38, yanivi wrote:
> "Batch" -> "batch"

Done.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:105:
private Map<String, CallbackAndDataClass<?, ?>> idToCallbackAndDataClass =
On 2012/05/04 20:47:35, yanivi wrote:
> Can we even merge idToCallbackAndDataClass with requests?  After all, it is
just
> a flat list of requests with their associated information.  This whole idea of
a
> map is sort of unnecessary here since the keys are essentially equivalent to
the
> index into the requests array.
> 
> I do worry about unnecessary complexity in our implementation.

You are right the Map is unnecessary here. Changed to a List. However, I am
still keeping requests separate from RequestInfo because it makes the below code
a little cleaner by having a separate List for requests. Namely for lines 252
and 267 below.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:109:
private Map<String, HttpUnsuccessfulResponseHandler>
idToUnsuccessfulResponseHandler =
On 2012/05/04 20:47:35, yanivi wrote:
> why not just merge this into idToCallbackAndDataClass?

Done.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:113:
private boolean retryUnsuccessfulRequests = true;
On 2012/05/04 20:47:35, yanivi wrote:
> this should really be a local variable inside execute()

I am not sure this would work, execute() is called from execute() for
unsuccessful requests, making it local in execute() would override the intended
value.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:116:
static class CallbackAndDataClass<T, E> {
On 2012/05/04 20:47:35, yanivi wrote:
> [optional] call it RequestInfo?

sgtm. Done.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:116:
static class CallbackAndDataClass<T, E> {
On 2012/05/04 18:58:38, yanivi wrote:
> as far as I can tell the generic type parameters don't make sense here

Maybe I am missing something, but how else can we establish the type of
callback, dataClass and errorClass?

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:117:
private BatchCallback<T, E> callback;
On 2012/05/04 18:58:38, yanivi wrote:
> [optional] just make these package private and remove the getter methods

Done.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:150:
HttpParser httpParser) {
On 2012/05/04 20:47:35, yanivi wrote:
> httpParser should ideally be a parameter for queue

Done.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:185:
HttpRequest batchRequest;
On 2012/05/04 18:58:38, yanivi wrote:
> move field declaration to before constructors

Done.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:195:
* @param dataClass Data class the response will be parsed into or {@code null}
for none
On 2012/05/04 20:47:35, yanivi wrote:
> Can we please provide more detail on what null means for data class?  The fact
> that this causes the response to be ignored and onSuccess to not be called was
> non-obvious to me.  Another option is we could still call onSuccess but with
> null for the content, which at least notifies that a response as been received
> and gives option to parse headers.

Added javadoc about how it will call onSuccess with null content.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:235:
String boundary = "--" + contentType.substring(boundaryStartIndex +
"boundary=".length());
On 2012/05/04 20:47:35, yanivi wrote:
> Seems kind of hacky.  I filed this:
> 
> http://code.google.com/p/google-http-java-client/issues/detail?id=100
> 
> for now, can we please split on the ";"'s and then find the part that starts
> with "boundary="?

Done.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:254:
execute();
On 2012/05/04 18:58:38, yanivi wrote:
> I'm really worried about an infinite loop.  Can we please set an upper limit
for
> the number of retries?

The number of retries will always be only 1 because retryUnsuccesfulRequests
will be false the 2nd time execute() is called. I do not think an infinite loop
is possible here.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:267:
HttpContent content = new MultipartMixedContent(requests, "END_OF_PART");
On 2012/05/04 20:47:35, yanivi wrote:
> [optional] I worry slightly about this boundary clashing with actual data. 
What
> do you think of trying to make this even more obscure?

Yes I agree. What do you think of: __END_OF_PART__

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java
(right):

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:28:
import com.google.api.client.testing.http.HttpTesting;
On 2012/05/04 20:47:35, yanivi wrote:
> Please don't introduce any dependency on the
com.google.api.client.testing.http
> package from here.  We don't want to accidentally introduce any testing-only
> code into production code.

Done.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:69:
private List<String> unsuccessfulContentIDsForRetry = new ArrayList<String>();
On 2012/05/04 20:47:35, yanivi wrote:
> I would just make this field package private and allow it to be accessed
> directly.
> 
> Or better yet: why not just build the List<HttpRequest> unsuccessfulRequests
> directly here and avoid the unsuccessfulContentIDsForRetry field?

Made it package private. I think it is more useful for BatchRequest to be able
to access list of contentIDs because then it can easily find the requests that
should be retried.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:84:
public BatchUnparsedResponse(InputStream inputStream, String boundary,
HttpParser httpParser,
On 2012/05/04 20:47:35, yanivi wrote:
> avoid the public modifier on a package private class

Done.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:132:
responseHeaders.set(headerParts[0], headerParts[1]);
On 2012/05/04 20:47:35, yanivi wrote:
> oh oh.  I think we need HttpHeaders.parseHeadersForMultipartResponses

Do we really need this? Shouldn't 131 and 132 work as expected? are you worried
about arrays etc?

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:135:
// Extract the response part content.
On 2012/05/04 20:47:35, yanivi wrote:
> can we please add a TODO to investigate a way to use the stream directly?  It
> would reduce the chance of an OutOfMemoryError and likely make the parsing
> noticeably more efficient.

Done.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:137:
while ((line = bufferedReader.readLine()) != null && !line.equals("")) {
On 2012/05/04 20:47:35, yanivi wrote:
> line.equals("") is the boundary or should we check instead for
> line.equals(boundary)?

This should work, Done.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:172:
if (statusCode == HttpStatusCodes.STATUS_CODE_OK) {
On 2012/05/04 20:47:35, yanivi wrote:
> shouldn't we use HttpStatusCodes.isSuccess?

Yes right. Done.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:265:
new ByteArrayInputStream(partContent.getBytes()), statusCode);
On 2012/05/04 20:47:35, yanivi wrote:
> first of all don't call String.getBytes() and instead call
> StringUtils.getBytesUtf8(String).
> 
> But this is also inefficient because we parse the stream as bytes, UTF-8
encode
> into a String, UTF-8 decode into bytes, and then UTF-8 encode it again when
> parsing the JSON.  It would be more efficiently to only do the UTF-8 encoding
> once.  Ideally we wouldn't read the the partContent as String but instead as
> bytes -- let's call it partBytes -- and then just pass that directly to new
> ByteArrayInputStream().

I see what you mean. But I do not know how to read the partContent as bytes, we
are using a BufferedReader here and if we attempted to get bytes directly from
the inputstream we would run into the same problem as before where some data is
buffered and some is not. Did I misunderstand you?

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/MultipartMixedContent.java
(right):

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/MultipartMixedContent.java:64:
Preconditions.checkArgument(!requests.isEmpty());
On 2012/05/04 18:58:38, yanivi wrote:
>     Preconditions.checkArgument(requests.size() < Integer.MAX_VALUE);

Done.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/MultipartMixedContent.java:75:
String twoDashes = "--";
On 2012/05/04 18:58:38, yanivi wrote:
> private static final String TWO_DASHES (or DASH_DASH)

Done.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/MultipartMixedContent.java:97:
writer.write(request.getUrl().toString());
On 2012/05/04 18:58:38, yanivi wrote:
> .build() instead of .toString()

Done.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/MultipartMixedContent.java:106:
writeHeader(writer, "Content-Type", "" + data.getType());
On 2012/05/04 18:58:38, yanivi wrote:
> remove the "" +

I did not realize when eclipse added this.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/MultipartMixedContent.java:107:
writeHeader(writer, "Content-Length", "" + data.getLength());
On 2012/05/04 18:58:38, yanivi wrote:
> String.valueOf(data.getLength())

Done.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/MultipartMixedContent.java:127:
public String getBoundary() {
On 2012/05/04 18:58:38, yanivi wrote:
> remove unused getBoundary() and getRequests()

Done.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/MultipartMixedContent.java:139:
private void writeHeader(Writer writer, String name, Object value) throws
IOException {
On 2012/05/04 18:58:38, yanivi wrote:
> simpler to make value String and then remove the .toString() below

Done.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java
(right):

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java:118:
return new BatchRequest(getRequestFactory().getTransport(),
On 2012/05/04 18:58:38, yanivi wrote:
> you need to call BatchRequest.setBatchUrl(getRootUrl() + "batch")
> 
> there is no way to avoid this: we need a getRootUrl()

Yes I will fix this for batch and mediaUpload in 1.10 with
http://code.google.com/p/google-http-java-client/issues/detail?id=97.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java:119:
getRequestFactory().getInitializer(), new JsonHttpParser(getJsonFactory()));
On 2012/05/04 18:58:38, yanivi wrote:
> initializer is a tricky concept.  The question is whether initializer applies
to
> each individual request part or to the batch part.
> 
> For example, imagine Credential as the initializer. Furthermore, imagine that
I
> use a Credential for Calendar in one request part and Credential for Tasks in
> another request part.  I'd want no Authorization header at the batch request
> level, but rather an Authorization header at each individual part.
> 
> So I'd argue that the initializer should be applied to the individual request
> parts, not at the batch level.  That said, I do think there is value in
allowing
> a developer to specify an HttpRequestInitializer on the batch request itself. 
> That could potentially be done as an optional parameter to the batch method. 
> Another possible option is to specify a batch request initializer in
> GoogleClient.Builder.  There may be another way to do this.
> 
> What do you think?

I had thought about the initializer for individual requests as well but
BatchRequest.queue takes in an HttpRequest which has already been built by the
RequestFactory and already been initialized. Thus, I saw no need to attempt to
further initiailize the given HttpRequest. I believe we should assume that the
HttpRequest given to batch.queue has been initialized and setup without any
further work needed on it.

About the batch level initializer, BatchRequest already takes in an initializer
which is used to initialize the top level batch request.
I guess the question here is if you call googleClient.batch then should the
initializer given to googleClient be automatically used for batch as well?
I am not sure, we could add an optional parameter here and if that is null then
to use getRequestFactory().getInitializer(). Thoughts?

[yanivi, 2012-05-09 16:27:54]

next batch of comments...

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java
(right):

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:113:
private boolean retryUnsuccessfulRequests = true;
On 2012/05/07 19:37:36, rmistry wrote:
> On 2012/05/04 20:47:35, yanivi wrote:
> > this should really be a local variable inside execute()
> 
> I am not sure this would work, execute() is called from execute() for
> unsuccessful requests, making it local in execute() would override the
intended
> value.

this wouldn't happen if you just make an execute() method with a boolean
retryUnsuccessfulRequests parameter.

I think extra fields add extra unnecessary complexity

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:254:
execute();
On 2012/05/07 19:37:36, rmistry wrote:
> On 2012/05/04 18:58:38, yanivi wrote:
> > I'm really worried about an infinite loop.  Can we please set an upper limit
> for
> > the number of retries?
> 
> The number of retries will always be only 1 because retryUnsuccesfulRequests
> will be false the 2nd time execute() is called. I do not think an infinite
loop
> is possible here.

this would be easier to understand if you made retryUncessarulRequests a
parameter rather than a field

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:267:
HttpContent content = new MultipartMixedContent(requests, "END_OF_PART");
On 2012/05/07 19:37:36, rmistry wrote:
> On 2012/05/04 20:47:35, yanivi wrote:
> > [optional] I worry slightly about this boundary clashing with actual data. 
> What
> > do you think of trying to make this even more obscure?
> 
> Yes I agree. What do you think of: __END_OF_PART__

SGTM :)

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java
(right):

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:69:
private List<String> unsuccessfulContentIDsForRetry = new ArrayList<String>();
On 2012/05/07 19:37:36, rmistry wrote:
> On 2012/05/04 20:47:35, yanivi wrote:
> > I would just make this field package private and allow it to be accessed
> > directly.
> > 
> > Or better yet: why not just build the List<HttpRequest> unsuccessfulRequests
> > directly here and avoid the unsuccessfulContentIDsForRetry field?
> 
> Made it package private. I think it is more useful for BatchRequest to be able
> to access list of contentIDs because then it can easily find the requests that
> should be retried.

I have to push back on this.  I really like my original "List<HttpRequest>
unsuccessfulRequests" idea.  Please take a look again based on my other comment
that we have to pass in the original requests anyway.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:132:
responseHeaders.set(headerParts[0], headerParts[1]);
On 2012/05/07 19:37:36, rmistry wrote:
> On 2012/05/04 20:47:35, yanivi wrote:
> > oh oh.  I think we need HttpHeaders.parseHeadersForMultipartResponses
> 
> Do we really need this? Shouldn't 131 and 132 work as expected? are you
worried
> about arrays etc?

Yes, we really need this.  HttpResponse handles repeated headers, primitive
types like Integer, complex Java field types like generics, arrays, and wildcard
types, and so on.  I don't want to duplicate the complex logic here.  Granted,
we are using GoogleHeaders so only a subset of that is actually needed, but
still I'd rather eliminate duplicate complex logic when possible.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:265:
new ByteArrayInputStream(partContent.getBytes()), statusCode);
On 2012/05/07 19:37:36, rmistry wrote:
> On 2012/05/04 20:47:35, yanivi wrote:
> > first of all don't call String.getBytes() and instead call
> > StringUtils.getBytesUtf8(String).

Ping: this is important for functional correctness.

> > 
> > But this is also inefficient because we parse the stream as bytes, UTF-8
> encode
> > into a String, UTF-8 decode into bytes, and then UTF-8 encode it again when
> > parsing the JSON.  It would be more efficiently to only do the UTF-8
encoding
> > once.  Ideally we wouldn't read the the partContent as String but instead as
> > bytes -- let's call it partBytes -- and then just pass that directly to new
> > ByteArrayInputStream().
> 
> I see what you mean. But I do not know how to read the partContent as bytes,
we
> are using a BufferedReader here and if we attempted to get bytes directly from
> the inputstream we would run into the same problem as before where some data
is
> buffered and some is not. Did I misunderstand you?

I think the fix may be to not use BufferedReader and write your own line-parsing
algorithm based on InputStream.  Just add a TODO for now if you think that's too
complicated for this release.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/MultipartMixedContent.java
(right):

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/MultipartMixedContent.java:64:
Preconditions.checkArgument(!requests.isEmpty());
On 2012/05/07 19:37:36, rmistry wrote:
> On 2012/05/04 18:58:38, yanivi wrote:
> >     Preconditions.checkArgument(requests.size() < Integer.MAX_VALUE);
> 
> Done.

ok that was a stupid comment from me: List.size() is an int, so guaranteed to be
< Integer.MAX_VALUE

please revert this check

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java
(right):

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java:118:
return new BatchRequest(getRequestFactory().getTransport(),
On 2012/05/07 19:37:36, rmistry wrote:
> On 2012/05/04 18:58:38, yanivi wrote:
> > you need to call BatchRequest.setBatchUrl(getRootUrl() + "batch")
> > 
> > there is no way to avoid this: we need a getRootUrl()
> 
> Yes I will fix this for batch and mediaUpload in 1.10 with
> http://code.google.com/p/google-http-java-client/issues/detail?id=97.

I have to push back on this for 1.9.  Internally at Google we sometimes use test
servers and we want those used for batch properly.  Please at least handle that
case by assuming rootUrl is new GenericUrl(getBaseUrl()).setPathParts({"/"}).

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java:119:
getRequestFactory().getInitializer(), new JsonHttpParser(getJsonFactory()));
On 2012/05/07 19:37:36, rmistry wrote:
> On 2012/05/04 18:58:38, yanivi wrote:
> > initializer is a tricky concept.  The question is whether initializer
applies
> to
> > each individual request part or to the batch part.
> > 
> > For example, imagine Credential as the initializer. Furthermore, imagine
that
> I
> > use a Credential for Calendar in one request part and Credential for Tasks
in
> > another request part.  I'd want no Authorization header at the batch request
> > level, but rather an Authorization header at each individual part.
> > 
> > So I'd argue that the initializer should be applied to the individual
request
> > parts, not at the batch level.  That said, I do think there is value in
> allowing
> > a developer to specify an HttpRequestInitializer on the batch request
itself. 
> > That could potentially be done as an optional parameter to the batch method.

> > Another possible option is to specify a batch request initializer in
> > GoogleClient.Builder.  There may be another way to do this.
> > 
> > What do you think?
> 
> I had thought about the initializer for individual requests as well but
> BatchRequest.queue takes in an HttpRequest which has already been built by the
> RequestFactory and already been initialized. Thus, I saw no need to attempt to
> further initiailize the given HttpRequest. I believe we should assume that the
> HttpRequest given to batch.queue has been initialized and setup without any
> further work needed on it.
> 
> About the batch level initializer, BatchRequest already takes in an
initializer
> which is used to initialize the top level batch request.
> I guess the question here is if you call googleClient.batch then should the
> initializer given to googleClient be automatically used for batch as well?
> I am not sure, we could add an optional parameter here and if that is null
then
> to use getRequestFactory().getInitializer(). Thoughts?
> 

My recommendation:

batch() should pass null to the httpRequestInitializer parameter of the
BatchRequest constructor.

Add an additional batch(HttpRequestInitializer) method that allows specifying
the httpRequestInitializer parameter of the BatchRequest constructor.  But I'm
okay with delaying  batch(HttpRequestInitializer) until 1.10.

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchCallback.java
(right):

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchCallback.java:54:
* @param t Instance of the parsed data model class
or null if null was passed to the dataClass parameter

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java
(right):

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:97:
private List<HttpRequest> requests = new ArrayList<HttpRequest>();
final

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:100:
private List<RequestInfo<?, ?>> requestInfos = new ArrayList<RequestInfo<?,
?>>();
final

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:106:
HttpRequest batchRequest;
let's not add unnecessary fields: batchRequest should be a local variable in
execute() and executeUnparsed()

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:157:
List<HttpRequest> getRequests() {
[optional] I wouldn't bother with a package private getter method; I would just
make the field package private

similarly for the other package private getters here and in
BatchUnparsedResponse

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:170:
* @param <T> destination class type
@param <E>

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:186:
Class<E> errorClass,
missing @param in JavaDoc for errorClass

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:189:
Preconditions.checkNotNull(callback);
checkNotNull on parser and errorClass

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:210:
// Find the boundary.
something more like "Find the boundary parameter from the Content-Type header"

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:222:
InputStream contentStream = response.getContent();
add a comment like "Parse the content stream"

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java
(right):

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:42:
* @since 1.9
remove @since on package private classes

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:75:
List<RequestInfo<?, ?>> requestInfos,
[optional] instead of passing in requestInfos, just pass in BatchRequest
instance and access its fields directly.

Alternatively, can just make this an inner class of BatchRequest

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:76:
HttpRequest batchRequest)
doesn't make sense to have a batchRequest parameter

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:103:
while ((line = bufferedReader.readLine()) != null && !line.equals("")) {
this logic to parse the Content-ID is unnecessary

just replace it with contentId++ and make contentId a field

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:117:
while ((line = bufferedReader.readLine()) != null && !line.equals("")) {
Is it true that headers are required to be contained within a single line? 
Can't headers expand over multiple lines?  I don't actually know the answer to
this question.  Can you please check on this?

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:126:
while ((line = bufferedReader.readLine()) != null && !line.startsWith(boundary))
{
is it startsWith or equals?  I don't remember what the protocol says exactly

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:132:
parseAndCallback(requestInfos.get(Integer.parseInt(contentId) - 1),
I think this might be incorrect in the case of a retry since the content ID's
are re-normalized in MultipartMixedContent

we need to make sure that the requestInfos and request arrays match exactly
one-for-one (yet another reason to merge the two)

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:159:
&& requestInfo.unsuccessfulResponseHandler.handleResponse(batchRequest,
response, true)) {
don't pass batchRequest to handleResponse; instead it needs to be the individual
request (which need to be passed in the constructor of this class)

[yanivi, 2012-05-09 20:29:45]

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java
(right):

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:184:
HttpParser httpParser,
httpParser and unsuccessfulResponseHandler shouldn't be parameters to queue:

1. use HttpResponse.parseAs(dataClass) instead of using an httpParser parameter

2. use HttpRequest.getUnsuccessfulResponseHandler() instead of using an
unsuccessfulResponseHandler parameter

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:249:
public HttpRequest buildHttpRequest() throws IOException {
Design question: should we even provide a public buildHttpRequest()?  My primary
concern is that it does not handle retry appropriately in case of an expired
access token, so perhaps we don't want developers to use it directly.

This is similar to the argument why we don't provide a buildHttpRequest() method
for MediaHttpUploader.

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:252:
HttpRequest request = requestFactory.buildPostRequest(this.batchUrl, content);
As we discussed, there is currently a bug that we are not calling the individual
HTTP request interceptors.  For example, for OAuth 2 Credential, we are not
setting the Authorization header.

I propose that we add a interceptor to the built HTTP request for batch that
iterates over the request parts and calls their interceptors.  Do you think that
is technically feasible?

[yanivi, 2012-05-09 21:05:03]

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java
(right):

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:176:
*        {@code null} is specified here then if the request is successful
something to investigate: instead of null, might it be more elegant to use
Void.class here for this case?  It is basically a way to say ignore this
parameter.

[rmistry, 2012-05-10 12:12:40]

Addressed all comments except one (HttpHeaders)

http://codereview.appspot.com/5539055/diff/96002/google-api-client/src/test/j...
File
google-api-client/src/test/java/com/google/api/client/googleapis/batch/BatchRequestTest.java
(right):

http://codereview.appspot.com/5539055/diff/96002/google-api-client/src/test/j...
google-api-client/src/test/java/com/google/api/client/googleapis/batch/BatchRequestTest.java:248:
//  public void testExecuteWithError() throws Exception {
On 2012/05/03 12:17:08, rmistry wrote:
> This test is commented out because:
> Previously in BatchUnparsedResponse I was doing-
> jsonParser.skipToKey("error");
> if (jsonParser.getCurrentToken() != JsonToken.END_OBJECT) {
>  callback.onFailure(jsonParser.parser(GoogleJsonError.class, ...
> }
>  
> Now I am doing:
> callback.onFailure(parser.parse(response, errorClass), responseHeaders);
>   
> I can no longer skip to the error key and parse properly as GoogleJsonError.
Any
> thoughts on what to do in BatchUnparsedResponse or the Callback with
HttpParser?

Ping.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java
(right):

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:113:
private boolean retryUnsuccessfulRequests = true;
On 2012/05/09 16:27:54, yanivi wrote:
> On 2012/05/07 19:37:36, rmistry wrote:
> > On 2012/05/04 20:47:35, yanivi wrote:
> > > this should really be a local variable inside execute()
> > 
> > I am not sure this would work, execute() is called from execute() for
> > unsuccessful requests, making it local in execute() would override the
> intended
> > value.
> 
> this wouldn't happen if you just make an execute() method with a boolean
> retryUnsuccessfulRequests parameter.
> 
> I think extra fields add extra unnecessary complexity

Done.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:254:
execute();
On 2012/05/09 16:27:54, yanivi wrote:
> On 2012/05/07 19:37:36, rmistry wrote:
> > On 2012/05/04 18:58:38, yanivi wrote:
> > > I'm really worried about an infinite loop.  Can we please set an upper
limit
> > for
> > > the number of retries?
> > 
> > The number of retries will always be only 1 because retryUnsuccesfulRequests
> > will be false the 2nd time execute() is called. I do not think an infinite
> loop
> > is possible here.
> 
> this would be easier to understand if you made retryUncessarulRequests a
> parameter rather than a field

Done.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:267:
HttpContent content = new MultipartMixedContent(requests, "END_OF_PART");
On 2012/05/09 16:27:54, yanivi wrote:
> On 2012/05/07 19:37:36, rmistry wrote:
> > On 2012/05/04 20:47:35, yanivi wrote:
> > > [optional] I worry slightly about this boundary clashing with actual data.

> > What
> > > do you think of trying to make this even more obscure?
> > 
> > Yes I agree. What do you think of: __END_OF_PART__
> 
> SGTM :)

Done.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java
(right):

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:69:
private List<String> unsuccessfulContentIDsForRetry = new ArrayList<String>();
On 2012/05/09 16:27:54, yanivi wrote:
> On 2012/05/07 19:37:36, rmistry wrote:
> > On 2012/05/04 20:47:35, yanivi wrote:
> > > I would just make this field package private and allow it to be accessed
> > > directly.
> > > 
> > > Or better yet: why not just build the List<HttpRequest>
unsuccessfulRequests
> > > directly here and avoid the unsuccessfulContentIDsForRetry field?
> > 
> > Made it package private. I think it is more useful for BatchRequest to be
able
> > to access list of contentIDs because then it can easily find the requests
that
> > should be retried.
> 
> I have to push back on this.  I really like my original "List<HttpRequest>
> unsuccessfulRequests" idea.  Please take a look again based on my other
comment
> that we have to pass in the original requests anyway.

Done.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:132:
responseHeaders.set(headerParts[0], headerParts[1]);
On 2012/05/09 16:27:54, yanivi wrote:
> On 2012/05/07 19:37:36, rmistry wrote:
> > On 2012/05/04 20:47:35, yanivi wrote:
> > > oh oh.  I think we need HttpHeaders.parseHeadersForMultipartResponses
> > 
> > Do we really need this? Shouldn't 131 and 132 work as expected? are you
> worried
> > about arrays etc?
> 
> Yes, we really need this.  HttpResponse handles repeated headers, primitive
> types like Integer, complex Java field types like generics, arrays, and
wildcard
> types, and so on.  I don't want to duplicate the complex logic here.  Granted,
> we are using GoogleHeaders so only a subset of that is actually needed, but
> still I'd rather eliminate duplicate complex logic when possible.

Have not started on this yet, wanted to send what I had so far.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:265:
new ByteArrayInputStream(partContent.getBytes()), statusCode);
On 2012/05/09 16:27:54, yanivi wrote:
> On 2012/05/07 19:37:36, rmistry wrote:
> > On 2012/05/04 20:47:35, yanivi wrote:
> > > first of all don't call String.getBytes() and instead call
> > > StringUtils.getBytesUtf8(String).
> 
> Ping: this is important for functional correctness.
> 
> > > 
> > > But this is also inefficient because we parse the stream as bytes, UTF-8
> > encode
> > > into a String, UTF-8 decode into bytes, and then UTF-8 encode it again
when
> > > parsing the JSON.  It would be more efficiently to only do the UTF-8
> encoding
> > > once.  Ideally we wouldn't read the the partContent as String but instead
as
> > > bytes -- let's call it partBytes -- and then just pass that directly to
new
> > > ByteArrayInputStream().
> > 
> > I see what you mean. But I do not know how to read the partContent as bytes,
> we
> > are using a BufferedReader here and if we attempted to get bytes directly
from
> > the inputstream we would run into the same problem as before where some data
> is
> > buffered and some is not. Did I misunderstand you?
> 
> I think the fix may be to not use BufferedReader and write your own
line-parsing
> algorithm based on InputStream.  Just add a TODO for now if you think that's
too
> complicated for this release.

Done.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/MultipartMixedContent.java
(right):

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/MultipartMixedContent.java:64:
Preconditions.checkArgument(!requests.isEmpty());
On 2012/05/09 16:27:54, yanivi wrote:
> On 2012/05/07 19:37:36, rmistry wrote:
> > On 2012/05/04 18:58:38, yanivi wrote:
> > >     Preconditions.checkArgument(requests.size() < Integer.MAX_VALUE);
> > 
> > Done.
> 
> ok that was a stupid comment from me: List.size() is an int, so guaranteed to
be
> < Integer.MAX_VALUE
> 
> please revert this check

Done.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java
(right):

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java:118:
return new BatchRequest(getRequestFactory().getTransport(),
On 2012/05/09 16:27:54, yanivi wrote:
> On 2012/05/07 19:37:36, rmistry wrote:
> > On 2012/05/04 18:58:38, yanivi wrote:
> > > you need to call BatchRequest.setBatchUrl(getRootUrl() + "batch")
> > > 
> > > there is no way to avoid this: we need a getRootUrl()
> > 
> > Yes I will fix this for batch and mediaUpload in 1.10 with
> > http://code.google.com/p/google-http-java-client/issues/detail?id=97.
> 
> I have to push back on this for 1.9.  Internally at Google we sometimes use
test
> servers and we want those used for batch properly.  Please at least handle
that
> case by assuming rootUrl is new GenericUrl(getBaseUrl()).setPathParts({"/"}).

Done.

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java:119:
getRequestFactory().getInitializer(), new JsonHttpParser(getJsonFactory()));
On 2012/05/09 16:27:54, yanivi wrote:
> On 2012/05/07 19:37:36, rmistry wrote:
> > On 2012/05/04 18:58:38, yanivi wrote:
> > > initializer is a tricky concept.  The question is whether initializer
> applies
> > to
> > > each individual request part or to the batch part.
> > > 
> > > For example, imagine Credential as the initializer. Furthermore, imagine
> that
> > I
> > > use a Credential for Calendar in one request part and Credential for Tasks
> in
> > > another request part.  I'd want no Authorization header at the batch
request
> > > level, but rather an Authorization header at each individual part.
> > > 
> > > So I'd argue that the initializer should be applied to the individual
> request
> > > parts, not at the batch level.  That said, I do think there is value in
> > allowing
> > > a developer to specify an HttpRequestInitializer on the batch request
> itself. 
> > > That could potentially be done as an optional parameter to the batch
method.
> 
> > > Another possible option is to specify a batch request initializer in
> > > GoogleClient.Builder.  There may be another way to do this.
> > > 
> > > What do you think?
> > 
> > I had thought about the initializer for individual requests as well but
> > BatchRequest.queue takes in an HttpRequest which has already been built by
the
> > RequestFactory and already been initialized. Thus, I saw no need to attempt
to
> > further initiailize the given HttpRequest. I believe we should assume that
the
> > HttpRequest given to batch.queue has been initialized and setup without any
> > further work needed on it.
> > 
> > About the batch level initializer, BatchRequest already takes in an
> initializer
> > which is used to initialize the top level batch request.
> > I guess the question here is if you call googleClient.batch then should the
> > initializer given to googleClient be automatically used for batch as well?
> > I am not sure, we could add an optional parameter here and if that is null
> then
> > to use getRequestFactory().getInitializer(). Thoughts?
> > 
> 
> My recommendation:
> 
> batch() should pass null to the httpRequestInitializer parameter of the
> BatchRequest constructor.
> 
> Add an additional batch(HttpRequestInitializer) method that allows specifying
> the httpRequestInitializer parameter of the BatchRequest constructor.  But I'm
> okay with delaying  batch(HttpRequestInitializer) until 1.10.

Created batch(HttpRequestInitializer). Done.

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchCallback.java
(right):

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchCallback.java:54:
* @param t Instance of the parsed data model class
On 2012/05/09 16:27:55, yanivi wrote:
> or null if null was passed to the dataClass parameter

Not adding this since we do not allow nulls anymore due to Void.

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java
(right):

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:97:
private List<HttpRequest> requests = new ArrayList<HttpRequest>();
On 2012/05/09 16:27:55, yanivi wrote:
> final

Done.

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:100:
private List<RequestInfo<?, ?>> requestInfos = new ArrayList<RequestInfo<?,
?>>();
On 2012/05/09 16:27:55, yanivi wrote:
> final

Done.

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:106:
HttpRequest batchRequest;
On 2012/05/09 16:27:55, yanivi wrote:
> let's not add unnecessary fields: batchRequest should be a local variable in
> execute() and executeUnparsed()

Done.

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:157:
List<HttpRequest> getRequests() {
On 2012/05/09 16:27:55, yanivi wrote:
> [optional] I wouldn't bother with a package private getter method; I would
just
> make the field package private
> 
> similarly for the other package private getters here and in
> BatchUnparsedResponse

Done.

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:170:
* @param <T> destination class type
On 2012/05/09 16:27:55, yanivi wrote:
> @param <E>

Done.

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:176:
*        {@code null} is specified here then if the request is successful
On 2012/05/09 21:05:03, yanivi wrote:
> something to investigate: instead of null, might it be more elegant to use
> Void.class here for this case?  It is basically a way to say ignore this
> parameter.

If I try to parse Void.class I get:
java.lang.IllegalArgumentException: unable to create new instance of class
java.lang.Void because it is void

So I am instead passing null into the callback when it is Void.class

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:184:
HttpParser httpParser,
On 2012/05/09 20:29:45, yanivi wrote:
> httpParser and unsuccessfulResponseHandler shouldn't be parameters to queue:
> 
> 1. use HttpResponse.parseAs(dataClass) instead of using an httpParser
parameter
Since this is a fake response I am instead getting the HttpParser from the
request by specifying the content type from the headers.

> 
> 2. use HttpRequest.getUnsuccessfulResponseHandler() instead of using an
> unsuccessfulResponseHandler parameter
Done.

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:186:
Class<E> errorClass,
On 2012/05/09 16:27:55, yanivi wrote:
> missing @param in JavaDoc for errorClass

Done.

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:189:
Preconditions.checkNotNull(callback);
On 2012/05/09 16:27:55, yanivi wrote:
> checkNotNull on parser and errorClass

Done.

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:210:
// Find the boundary.
On 2012/05/09 16:27:55, yanivi wrote:
> something more like "Find the boundary parameter from the Content-Type header"

Done.

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:222:
InputStream contentStream = response.getContent();
On 2012/05/09 16:27:55, yanivi wrote:
> add a comment like "Parse the content stream"

Done.

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:249:
public HttpRequest buildHttpRequest() throws IOException {
On 2012/05/09 20:29:45, yanivi wrote:
> Design question: should we even provide a public buildHttpRequest()?  My
primary
> concern is that it does not handle retry appropriately in case of an expired
> access token, so perhaps we don't want developers to use it directly.
> 
> This is similar to the argument why we don't provide a buildHttpRequest()
method
> for MediaHttpUploader.

Made it package private (for unit tests). Done.

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:252:
HttpRequest request = requestFactory.buildPostRequest(this.batchUrl, content);
On 2012/05/09 20:29:45, yanivi wrote:
> As we discussed, there is currently a bug that we are not calling the
individual
> HTTP request interceptors.  For example, for OAuth 2 Credential, we are not
> setting the Authorization header.
> 
> I propose that we add a interceptor to the built HTTP request for batch that
> iterates over the request parts and calls their interceptors.  Do you think
that
> is technically feasible?

This was a good idea and looks like it is working. Done.

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java
(right):

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:42:
* @since 1.9
On 2012/05/09 16:27:55, yanivi wrote:
> remove @since on package private classes

Done.

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:75:
List<RequestInfo<?, ?>> requestInfos,
On 2012/05/09 16:27:55, yanivi wrote:
> [optional] instead of passing in requestInfos, just pass in BatchRequest
> instance and access its fields directly.
> 
> Alternatively, can just make this an inner class of BatchRequest

I slightly prefer having it separate from BatchRequest and I think the current
parameters work instead of passing the whole object around.

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:76:
HttpRequest batchRequest)
On 2012/05/09 16:27:55, yanivi wrote:
> doesn't make sense to have a batchRequest parameter

Done.

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:103:
while ((line = bufferedReader.readLine()) != null && !line.equals("")) {
On 2012/05/09 16:27:55, yanivi wrote:
> this logic to parse the Content-ID is unnecessary
> 
> just replace it with contentId++ and make contentId a field

Done.

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:117:
while ((line = bufferedReader.readLine()) != null && !line.equals("")) {
On 2012/05/09 16:27:55, yanivi wrote:
> Is it true that headers are required to be contained within a single line? 
> Can't headers expand over multiple lines?  I don't actually know the answer to
> this question.  Can you please check on this?

Looks like headers can expand over multiple lines:
http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:126:
while ((line = bufferedReader.readLine()) != null && !line.startsWith(boundary))
{
On 2012/05/09 16:27:55, yanivi wrote:
> is it startsWith or equals?  I don't remember what the protocol says exactly

I am using startsWith here because I do not know if it is going to be a boundary
separating requests or the final boundary.

A boundary separating requests is --boundary
and the final boundary is --boundary--

The check for final boundary I am doing below.

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:132:
parseAndCallback(requestInfos.get(Integer.parseInt(contentId) - 1),
On 2012/05/09 16:27:55, yanivi wrote:
> I think this might be incorrect in the case of a retry since the content ID's
> are re-normalized in MultipartMixedContent
> 
> we need to make sure that the requestInfos and request arrays match exactly
> one-for-one (yet another reason to merge the two)

I think it works now. Done.

http://codereview.appspot.com/5539055/diff/111009/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:159:
&& requestInfo.unsuccessfulResponseHandler.handleResponse(batchRequest,
response, true)) {
On 2012/05/09 16:27:55, yanivi wrote:
> don't pass batchRequest to handleResponse; instead it needs to be the
individual
> request (which need to be passed in the constructor of this class)

Done.

[rmistry, 2012-05-10 13:24:44]

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java
(right):

http://codereview.appspot.com/5539055/diff/96003/google-api-client/src/main/j...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:132:
responseHeaders.set(headerParts[0], headerParts[1]);
On 2012/05/10 12:12:40, rmistry wrote:
> On 2012/05/09 16:27:54, yanivi wrote:
> > On 2012/05/07 19:37:36, rmistry wrote:
> > > On 2012/05/04 20:47:35, yanivi wrote:
> > > > oh oh.  I think we need HttpHeaders.parseHeadersForMultipartResponses
> > > 
> > > Do we really need this? Shouldn't 131 and 132 work as expected? are you
> > worried
> > > about arrays etc?
> > 
> > Yes, we really need this.  HttpResponse handles repeated headers, primitive
> > types like Integer, complex Java field types like generics, arrays, and
> wildcard
> > types, and so on.  I don't want to duplicate the complex logic here. 
Granted,
> > we are using GoogleHeaders so only a subset of that is actually needed, but
> > still I'd rather eliminate duplicate complex logic when possible.
> 
> Have not started on this yet, wanted to send what I had so far.

Done.
Instead of adding HttpHeaders.parseHeadersForMultipartResponses I added
headerNames and headerValues to FakeLowLevelHttpResponse and this seems to work.

[yanivi, 2012-05-10 21:13:29]

almost there...

http://codereview.appspot.com/5539055/diff/96002/google-api-client/src/test/j...
File
google-api-client/src/test/java/com/google/api/client/googleapis/batch/BatchRequestTest.java
(right):

http://codereview.appspot.com/5539055/diff/96002/google-api-client/src/test/j...
google-api-client/src/test/java/com/google/api/client/googleapis/batch/BatchRequestTest.java:248:
//  public void testExecuteWithError() throws Exception {
On 2012/05/10 12:12:40, rmistry wrote:
> On 2012/05/03 12:17:08, rmistry wrote:
> > This test is commented out because:
> > Previously in BatchUnparsedResponse I was doing-
> > jsonParser.skipToKey("error");
> > if (jsonParser.getCurrentToken() != JsonToken.END_OBJECT) {
> >  callback.onFailure(jsonParser.parser(GoogleJsonError.class, ...
> > }
> >  
> > Now I am doing:
> > callback.onFailure(parser.parse(response, errorClass), responseHeaders);
> >   
> > I can no longer skip to the error key and parse properly as GoogleJsonError.
> Any
> > thoughts on what to do in BatchUnparsedResponse or the Callback with
> HttpParser?
> 
> Ping.

Good question.  One ugly option is to add a container class that wraps
GoogleJsonError, e.g.

public class GoogleJsonErrorContainer {
  private @Key GoogleJsonError error;
}

another ugly option is to provide the ability to specify an alternative parser
for parsing the error response, i.e. using JsonCParser for error but
JsonHttpParser for success.

another ugly option is to override JsonHttpParser with a parser that works just
like JsonHttpParser for success responses and JsonCParser for error responses.

sorry I cannot come up with a more elegant solution right now.

of course the real question is why are Google APIs using an "error" wrapper in
the first place :)  but that's not something that can be fixed now...

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchCallback.java
(right):

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchCallback.java:48:
// TODO(rmistry): Add BatchUnparsedCallback with onResponse(InputStream content,
GoogleHeaders).
can you please put the TODO elsewhere?  I'd like to keep the interface clean of
implementation TODOs.

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java
(right):

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:149:
*        none. If {@code null} is specified here then if the request is
unsuccessful
please update JavaDoc for errorClass

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:159:
Preconditions.checkNotNull(httpRequest);
[optional] move the null checks into the RequestInfo constructor and fix tests
that call it with null parameters

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:210:
}
probably good practice to call requestInfos.clear() at the end of execute()

it would also allow that BatchRequest object to be reused.  should probably
specify that in the JavaDoc for execute()

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:214:
public HttpResponse executeUnparsed() throws IOException {
I have the same concern about executeUnparsed() being public as I do for
buildHttpRequest(), since it doesn't handle unsuccessful responses.  Can we put
a TODO for later for both of them?

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:232:
private class BatchInterceptor implements HttpExecuteInterceptor {
[optional] remove the "private" keyword and remove the unnecessary constructor

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java
(right):

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:78:
this.bufferedReader = new BufferedReader(new InputStreamReader(inputStream));
this uses the default charset, but instead you need to use UTF-8, e.g.

new InputStreamReader(inputStream, CharEncoding.UTF_8)

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:106:
// TODO(rmistry): Handle inner headers that span multiple lines.
Please add a link to for more details about header folding:
http://tools.ietf.org/html/rfc2616#section-2.2

Sven says that the Google API server shouldn't return headers spanning multiple
lines, so we're probably OK.

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:148:
Preconditions.checkNotNull(parser);
don't need to checkNotNull parser since we don't need parser to be != null for
Void.class, and calling parser.parse will already throw null

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:152:
callback.onSuccess(parser.parse(response, requestInfo.dataClass),
responseHeaders);
[optional]
simpler to write it like this:

      T parsed = requestInfo.dataClass == Void.class ? null :
        parser.parse(response, requestInfo.dataClass);
      callback.onSuccess(parsed, responseHeaders);

Oh!  That gives me an idea:

http://code.google.com/p/google-http-java-client/issues/detail?id=110

:)

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java
(right):

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java:136:
* @param httpRequestInitializer The initializer to use when creating batch HTTP
requests or
please make it more clear that it is the http request initializer for the
top-level request, not the individual contained requests

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java:143:
GenericUrl baseUrl = new GenericUrl(getBaseUrl());
TODO(rmistry): batch.setBatchUrl(new GenericUrl(getRootUrl() + "batch"));

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java:147:
baseUrl.setPathParts(pathParts);
[optional] fun Java trick:

    baseUrl.setPathParts(Arrays.asList("", "batch"));

[rmistry, 2012-05-11 12:57:57]

http://codereview.appspot.com/5539055/diff/96002/google-api-client/src/test/j...
File
google-api-client/src/test/java/com/google/api/client/googleapis/batch/BatchRequestTest.java
(right):

http://codereview.appspot.com/5539055/diff/96002/google-api-client/src/test/j...
google-api-client/src/test/java/com/google/api/client/googleapis/batch/BatchRequestTest.java:248:
//  public void testExecuteWithError() throws Exception {
On 2012/05/10 21:13:29, yanivi wrote:
> On 2012/05/10 12:12:40, rmistry wrote:
> > On 2012/05/03 12:17:08, rmistry wrote:
> > > This test is commented out because:
> > > Previously in BatchUnparsedResponse I was doing-
> > > jsonParser.skipToKey("error");
> > > if (jsonParser.getCurrentToken() != JsonToken.END_OBJECT) {
> > >  callback.onFailure(jsonParser.parser(GoogleJsonError.class, ...
> > > }
> > >  
> > > Now I am doing:
> > > callback.onFailure(parser.parse(response, errorClass), responseHeaders);
> > >   
> > > I can no longer skip to the error key and parse properly as
GoogleJsonError.
> > Any
> > > thoughts on what to do in BatchUnparsedResponse or the Callback with
> > HttpParser?
> > 
> > Ping.
> 
> Good question.  One ugly option is to add a container class that wraps
> GoogleJsonError, e.g.
> 
> public class GoogleJsonErrorContainer {
>   private @Key GoogleJsonError error;
> }
> 
> another ugly option is to provide the ability to specify an alternative parser
> for parsing the error response, i.e. using JsonCParser for error but
> JsonHttpParser for success.
> 
> another ugly option is to override JsonHttpParser with a parser that works
just
> like JsonHttpParser for success responses and JsonCParser for error responses.
> 
> sorry I cannot come up with a more elegant solution right now.
> 
> of course the real question is why are Google APIs using an "error" wrapper in
> the first place :)  but that's not something that can be fixed now...

I think the Container class would be useful to have available. Done.

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchCallback.java
(right):

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchCallback.java:48:
// TODO(rmistry): Add BatchUnparsedCallback with onResponse(InputStream content,
GoogleHeaders).
On 2012/05/10 21:13:29, yanivi wrote:
> can you please put the TODO elsewhere?  I'd like to keep the interface clean
of
> implementation TODOs.

Done.

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java
(right):

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:149:
*        none. If {@code null} is specified here then if the request is
unsuccessful
On 2012/05/10 21:13:29, yanivi wrote:
> please update JavaDoc for errorClass

Done.

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:159:
Preconditions.checkNotNull(httpRequest);
On 2012/05/10 21:13:29, yanivi wrote:
> [optional] move the null checks into the RequestInfo constructor and fix tests
> that call it with null parameters

I think I prefer having the null checks in the queue method.

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:210:
}
On 2012/05/10 21:13:29, yanivi wrote:
> probably good practice to call requestInfos.clear() at the end of execute()
> 
> it would also allow that BatchRequest object to be reused.  should probably
> specify that in the JavaDoc for execute()

Done.

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:214:
public HttpResponse executeUnparsed() throws IOException {
On 2012/05/10 21:13:29, yanivi wrote:
> I have the same concern about executeUnparsed() being public as I do for
> buildHttpRequest(), since it doesn't handle unsuccessful responses.  Can we
put
> a TODO for later for both of them?

Done.

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:232:
private class BatchInterceptor implements HttpExecuteInterceptor {
On 2012/05/10 21:13:29, yanivi wrote:
> [optional] remove the "private" keyword and remove the unnecessary constructor

Done.

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java
(right):

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:78:
this.bufferedReader = new BufferedReader(new InputStreamReader(inputStream));
On 2012/05/10 21:13:29, yanivi wrote:
> this uses the default charset, but instead you need to use UTF-8, e.g.
> 
> new InputStreamReader(inputStream, CharEncoding.UTF_8)

Done.

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:106:
// TODO(rmistry): Handle inner headers that span multiple lines.
On 2012/05/10 21:13:29, yanivi wrote:
> Please add a link to for more details about header folding:
> http://tools.ietf.org/html/rfc2616#section-2.2
> 
> Sven says that the Google API server shouldn't return headers spanning
multiple
> lines, so we're probably OK.

Done.

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:148:
Preconditions.checkNotNull(parser);
On 2012/05/10 21:13:29, yanivi wrote:
> don't need to checkNotNull parser since we don't need parser to be != null for
> Void.class, and calling parser.parse will already throw null

Done.

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:152:
callback.onSuccess(parser.parse(response, requestInfo.dataClass),
responseHeaders);
On 2012/05/10 21:13:29, yanivi wrote:
> [optional]
> simpler to write it like this:
> 
>       T parsed = requestInfo.dataClass == Void.class ? null :
>         parser.parse(response, requestInfo.dataClass);
>       callback.onSuccess(parsed, responseHeaders);
> 
> Oh!  That gives me an idea:
> 
> http://code.google.com/p/google-http-java-client/issues/detail?id=110
> 
> :)

Done.

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java
(right):

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java:136:
* @param httpRequestInitializer The initializer to use when creating batch HTTP
requests or
On 2012/05/10 21:13:29, yanivi wrote:
> please make it more clear that it is the http request initializer for the
> top-level request, not the individual contained requests

Done.

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java:143:
GenericUrl baseUrl = new GenericUrl(getBaseUrl());
On 2012/05/10 21:13:29, yanivi wrote:
> TODO(rmistry): batch.setBatchUrl(new GenericUrl(getRootUrl() + "batch"));

Done.

http://codereview.appspot.com/5539055/diff/114012/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java:147:
baseUrl.setPathParts(pathParts);
On 2012/05/10 21:13:29, yanivi wrote:
> [optional] fun Java trick:
> 
>     baseUrl.setPathParts(Arrays.asList("", "batch"));

Done.

[rmistry, 2012-05-11 14:35:29]

Added unit tests for 401s and Void callbacks.

[yanivi, 2012-05-11 15:37:27]

http://codereview.appspot.com/5539055/diff/113025/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java
(right):

http://codereview.appspot.com/5539055/diff/113025/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:222:
public HttpResponse executeUnparsed() throws IOException {
package private?

http://codereview.appspot.com/5539055/diff/113025/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java
(right):

http://codereview.appspot.com/5539055/diff/113025/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:81:
new BufferedReader(new InputStreamReader(inputStream, CharEncoding.UTF_8));
"UTF-8"

we don't want to add a dependence on org.apache.commons.codec :)

http://codereview.appspot.com/5539055/diff/113025/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/json/GoogleJsonErrorContainer.java
(right):

http://codereview.appspot.com/5539055/diff/113025/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/json/GoogleJsonErrorContainer.java:26:
public class GoogleJsonErrorContainer extends GenericJson {
please add a TestCase for this class, see e.g. GoogleJsonErrorTest

http://codereview.appspot.com/5539055/diff/113025/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/json/GoogleJsonErrorContainer.java:32:
public GoogleJsonError getError() {
make these two accessors final

[rmistry, 2012-05-11 16:33:17]

http://codereview.appspot.com/5539055/diff/113025/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java
(right):

http://codereview.appspot.com/5539055/diff/113025/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchRequest.java:222:
public HttpResponse executeUnparsed() throws IOException {
On 2012/05/11 15:37:27, yanivi wrote:
> package private?

Done.

http://codereview.appspot.com/5539055/diff/113025/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java
(right):

http://codereview.appspot.com/5539055/diff/113025/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/batch/BatchUnparsedResponse.java:81:
new BufferedReader(new InputStreamReader(inputStream, CharEncoding.UTF_8));
On 2012/05/11 15:37:27, yanivi wrote:
> "UTF-8"
> 
> we don't want to add a dependence on org.apache.commons.codec :)

Oops. Done.

http://codereview.appspot.com/5539055/diff/113025/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/json/GoogleJsonErrorContainer.java
(right):

http://codereview.appspot.com/5539055/diff/113025/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/json/GoogleJsonErrorContainer.java:26:
public class GoogleJsonErrorContainer extends GenericJson {
On 2012/05/11 15:37:27, yanivi wrote:
> please add a TestCase for this class, see e.g. GoogleJsonErrorTest

Done.

http://codereview.appspot.com/5539055/diff/113025/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/json/GoogleJsonErrorContainer.java:32:
public GoogleJsonError getError() {
On 2012/05/11 15:37:27, yanivi wrote:
> make these two accessors final

Done.

[yanivi, 2012-05-11 17:43:04]

LGTM