* Added a closeInputStream getter/setter for AbstractInputStreamContent.writeTo.
* Added a closeInputStream parameter to AbstractInputStreamContent.copy
* Made AbstractInputStreamContent.getInputStream public.
* Flushing the output stream at the end of HttpContent.writeTo implementations.
* Added allowEmptyContent to HttpRequest (defaulted to true) with a unit test.
http://codereview.appspot.com/5482047/diff/1/google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java File google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java (right): http://codereview.appspot.com/5482047/diff/1/google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java#newcode60 google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:60: public abstract InputStream getInputStream() throws IOException; while we are ...
12 years, 2 months ago
(2012-01-13 16:27:31 UTC)
#3
http://codereview.appspot.com/5482047/diff/1/google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java File google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java (right): http://codereview.appspot.com/5482047/diff/1/google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java#newcode60 google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:60: public abstract InputStream getInputStream() throws IOException; By the way, ...
12 years, 2 months ago
(2012-01-13 16:40:30 UTC)
#4
http://codereview.appspot.com/5482047/diff/1/google-http-client/src/main/java...
File
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java
(right):
http://codereview.appspot.com/5482047/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:60:
public abstract InputStream getInputStream() throws IOException;
By the way, changing an overridable protected method to public is a
backwards-incompatible change. Note the build errors:
Description Resource Path Location Type
Cannot reduce the visibility of the inherited method from
AbstractInputStreamContent ByteArrayContent.java /google-http-client/src/main/java/com/google/api/client/http line
118 Java Problem
Cannot reduce the visibility of the inherited method from
AbstractInputStreamContent FileContent.java /google-http-client/src/main/java/com/google/api/client/http line
70 Java Problem
http://codereview.appspot.com/5482047/diff/1/google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java File google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java (right): http://codereview.appspot.com/5482047/diff/1/google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java#newcode60 google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:60: public abstract InputStream getInputStream() throws IOException; On 2012/01/13 16:27:31, ...
12 years, 2 months ago
(2012-01-17 18:27:13 UTC)
#5
http://codereview.appspot.com/5482047/diff/1/google-http-client/src/main/java...
File
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java
(right):
http://codereview.appspot.com/5482047/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:60:
public abstract InputStream getInputStream() throws IOException;
On 2012/01/13 16:27:31, yanivi wrote:
> while we are changing this, can we rename it to newInputStream or
> createInputStream or something? getter methods shouldn't be creating new
> instances.
>
> The idea would be to deprecate the protected method, and add something like:
>
> /** @since 1.7 */
> public InputStream newInputStream() throws IOException {
> return getInputStream();
> }
Done.
http://codereview.appspot.com/5482047/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:60:
public abstract InputStream getInputStream() throws IOException;
On 2012/01/13 16:40:31, yanivi wrote:
> By the way, changing an overridable protected method to public is a
> backwards-incompatible change. Note the build errors:
>
> Description Resource Path Location Type
> Cannot reduce the visibility of the inherited method from
>
AbstractInputStreamContent ByteArrayContent.java /google-http-client/src/main/java/com/google/api/client/http line
> 118 Java Problem
> Cannot reduce the visibility of the inherited method from
>
AbstractInputStreamContent FileContent.java /google-http-client/src/main/java/com/google/api/client/http line
> 70 Java Problem
Hmm, I think I am doing something wrong in my process. When I make changes in my
mercurial client and do 'mvn compile' it passes. But when I did 'rm -rf target;
mvn compile' I saw the error you pasted above.
Any pointers on this?
http://codereview.appspot.com/5482047/diff/1/google-http-client/src/main/java...
File
google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java
(right):
http://codereview.appspot.com/5482047/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java:85:
private String uploadContentType;
On 2012/01/13 16:27:31, yanivi wrote:
> These headers should really go in GoogleHeaders instead. Let's keep this
class
> based on IETF standards.
Moved to GoogleHeaders in http://codereview.appspot.com/5450097/http://codereview.appspot.com/5482047/diff/1/google-http-client/src/main/java...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):
http://codereview.appspot.com/5482047/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:161:
private BackOffPolicy backOffPolicy = new ExponentialBackOffPolicy();
On 2012/01/13 16:27:31, yanivi wrote:
> I thought we wanted backoff policy to be off by default?
We do, I think I had messed up the sync with head when I was working from India.
Fixed.
http://codereview.appspot.com/5482047/diff/1/google-http-client/src/main/java...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:734:
if (!isAllowEmptyContent()
On 2012/01/13 16:27:31, yanivi wrote:
> unit test please
Done.
http://codereview.appspot.com/5482047/diff/7001/google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java File google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java (right): http://codereview.appspot.com/5482047/diff/7001/google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java#newcode3 google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:3: * [optional] I would be nice if you could ...
12 years, 2 months ago
(2012-01-20 20:32:17 UTC)
#6
http://codereview.appspot.com/5482047/diff/7001/google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java File google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java (right): http://codereview.appspot.com/5482047/diff/7001/google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java#newcode3 google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:3: * On 2012/01/20 20:32:17, yanivi wrote: > [optional] I ...
12 years, 2 months ago
(2012-01-23 16:11:54 UTC)
#8
http://codereview.appspot.com/5482047/diff/7001/google-http-client/src/main/j...
File
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java
(right):
http://codereview.appspot.com/5482047/diff/7001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:3:
*
On 2012/01/20 20:32:17, yanivi wrote:
> [optional] I would be nice if you could avoid these whitespace changes.
I apologize for them, they can get distracting. Eclipse made the changes
automatically.
http://codereview.appspot.com/5482047/diff/7001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:62:
@Deprecated
On 2012/01/20 20:32:17, yanivi wrote:
> need to also deprecate the methods that override this, e.g.
>
>
> Description Resource Path Location Type
> The method ByteArrayContent.getInputStream() overrides a deprecated method
from
>
AbstractInputStreamContent ByteArrayContent.java /google-http-client/src/main/java/com/google/api/client/http line
> 118 Java Problem
How did you get those warnings? I ran mvn findbugs:gui and did not get anything.
mvn install also did not give me any warnings.
Fixed overriden methods.
http://codereview.appspot.com/5482047/diff/7001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:69:
* {@link InputStream} from the source data whenever invoked.
On 2012/01/20 20:32:17, yanivi wrote:
> something like "for backwards compatibility this calls getInputStream, but
this
> will change in 1.8"
Done.
http://codereview.appspot.com/5482047/diff/7001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:77:
@Override
On 2012/01/20 20:32:17, yanivi wrote:
> no @Override because it won't compile in Java 5
>
>
> Description Resource Path Location Type
> The method writeTo(OutputStream) of type AbstractInputStreamContent must
> override a superclass
>
method AbstractInputStreamContent.java /google-http-client/src/main/java/com/google/api/client/http line
> 78 Java Problem
>
> similarly below and in HttpRequestTest
>
> Make sure you set up your Eclipse project properly.
Ah cannot @Override when implementing interfaces in Java 5. This too should have
been caught by maven. I am not sure what is wrong with my setup.
http://codereview.appspot.com/5482047/diff/7001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:79:
InputStream inputStream = getInputStream();
On 2012/01/20 20:32:17, yanivi wrote:
> should call newInputStream() instead
Done.
http://codereview.appspot.com/5482047/diff/7001/google-http-client/src/main/j...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):
http://codereview.appspot.com/5482047/diff/7001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:86:
* empty contents. Defaults to {@code false}.
On 2012/01/20 21:45:38, yanivi wrote:
> Design issue: should we make the default true? I hesitate to assume that this
> is the right behavior for all servers. Changing the content on HTTP requests
> might be an unexpected unintuitive behavior.
Yes that is a good point. We should turn it off in
GoogleClient.buildHttpRequest.
I will do this in: http://codereview.appspot.com/5450097/http://codereview.appspot.com/5482047/diff/7001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:158:
private BackOffPolicy backOffPolicy = null;
On 2012/01/20 20:32:17, yanivi wrote:
> remove "= null"
Done.
http://codereview.appspot.com/5482047/diff/7001/google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java File google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java (right): http://codereview.appspot.com/5482047/diff/7001/google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java#newcode62 google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:62: @Deprecated On 2012/01/23 16:11:54, rmistry wrote: > On 2012/01/20 ...
12 years, 2 months ago
(2012-01-23 18:30:56 UTC)
#9
http://codereview.appspot.com/5482047/diff/7001/google-http-client/src/main/j...
File
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java
(right):
http://codereview.appspot.com/5482047/diff/7001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:62:
@Deprecated
On 2012/01/23 16:11:54, rmistry wrote:
> On 2012/01/20 20:32:17, yanivi wrote:
> > need to also deprecate the methods that override this, e.g.
> >
> >
> > Description Resource Path Location Type
> > The method ByteArrayContent.getInputStream() overrides a deprecated method
> from
> >
>
AbstractInputStreamContent ByteArrayContent.java /google-http-client/src/main/java/com/google/api/client/http line
> > 118 Java Problem
>
> How did you get those warnings? I ran mvn findbugs:gui and did not get
anything.
> mvn install also did not give me any warnings.
>
> Fixed overriden methods.
These are problems in Eclipse. I just look in the Problems tab :)
http://codereview.appspot.com/5482047/diff/7001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:69:
* {@link InputStream} from the source data whenever invoked.
On 2012/01/23 16:11:54, rmistry wrote:
> On 2012/01/20 20:32:17, yanivi wrote:
> > something like "for backwards compatibility this calls getInputStream, but
> this
> > will change in 1.8"
>
> Done.
How will it change in 1.8? I assume it will be made abstract? You should be
more specific.
http://codereview.appspot.com/5482047/diff/7001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:77:
@Override
On 2012/01/23 16:11:54, rmistry wrote:
> On 2012/01/20 20:32:17, yanivi wrote:
> > no @Override because it won't compile in Java 5
> >
> >
> > Description Resource Path Location Type
> > The method writeTo(OutputStream) of type AbstractInputStreamContent must
> > override a superclass
> >
>
method AbstractInputStreamContent.java /google-http-client/src/main/java/com/google/api/client/http line
> > 78 Java Problem
> >
> > similarly below and in HttpRequestTest
> >
> > Make sure you set up your Eclipse project properly.
>
> Ah cannot @Override when implementing interfaces in Java 5. This too should
have
> been caught by maven. I am not sure what is wrong with my setup.
My Maven doesn't catch it either. I don't know why. But Eclipse catches it.
http://codereview.appspot.com/5482047/diff/12001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/ByteArrayContent.java
(right):
http://codereview.appspot.com/5482047/diff/12001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/ByteArrayContent.java:119:
protected InputStream getInputStream() {
Description Resource Path Location Type
The method getInputStream() from the type ByteArrayContent is
deprecated ByteArrayContentTest.java /google-http-client/src/test/java/com/google/api/client/http line
62 Java Problem
you may want to add @SuppressWarnings("deprecation") to that test
http://codereview.appspot.com/5482047/diff/12001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):
http://codereview.appspot.com/5482047/diff/12001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:726:
if (!isAllowEmptyContent()
Please also change MethodOverride to setAllowEmptyContent(false) and remove this
logic from there:
request.setContent(ByteArrayContent.fromString(null, " "));
http://codereview.appspot.com/5482047/diff/12001/google-http-client/src/test/...
File
google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java
(right):
http://codereview.appspot.com/5482047/diff/12001/google-http-client/src/test/...
google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java:90:
@Override
Please set up your Eclipse to catch these problems! In my Eclipse these show up
as errors. I'm actually surprised it's not that way for you by default.
Description Resource Path Location Type
The method getNextBackOffMillis() of type HttpRequestTest.MockBackOffPolicy must
override a superclass
method HttpRequestTest.java /google-http-client/src/test/java/com/google/api/client/http line
107 Java Problem
12 years, 2 months ago
(2012-01-23 22:09:36 UTC)
#10
http://codereview.appspot.com/5482047/diff/12001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/ByteArrayContent.java
(right):
http://codereview.appspot.com/5482047/diff/12001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/ByteArrayContent.java:119:
protected InputStream getInputStream() {
On 2012/01/23 18:30:56, yanivi wrote:
> Description Resource Path Location Type
> The method getInputStream() from the type ByteArrayContent is
>
deprecated ByteArrayContentTest.java /google-http-client/src/test/java/com/google/api/client/http line
> 62 Java Problem
>
> you may want to add @SuppressWarnings("deprecation") to that test
Done.
http://codereview.appspot.com/5482047/diff/12001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):
http://codereview.appspot.com/5482047/diff/12001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:726:
if (!isAllowEmptyContent()
On 2012/01/23 18:30:56, yanivi wrote:
> Please also change MethodOverride to setAllowEmptyContent(false) and remove
this
> logic from there:
>
> request.setContent(ByteArrayContent.fromString(null, " "));
Sorry, could you please elaborate on this comment? I do not understand what you
are recommending.
http://codereview.appspot.com/5482047/diff/12001/google-http-client/src/test/...
File
google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java
(right):
http://codereview.appspot.com/5482047/diff/12001/google-http-client/src/test/...
google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java:90:
@Override
On 2012/01/23 18:30:56, yanivi wrote:
> Please set up your Eclipse to catch these problems! In my Eclipse these show
up
> as errors. I'm actually surprised it's not that way for you by default.
>
> Description Resource Path Location Type
> The method getNextBackOffMillis() of type HttpRequestTest.MockBackOffPolicy
must
> override a superclass
>
method HttpRequestTest.java /google-http-client/src/test/java/com/google/api/client/http line
> 107 Java Problem
>
Done (with your help).
LGTM http://codereview.appspot.com/5482047/diff/7001/google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java File google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java (right): http://codereview.appspot.com/5482047/diff/7001/google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java#newcode69 google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:69: * {@link InputStream} from the source data whenever ...
12 years, 2 months ago
(2012-01-26 20:53:16 UTC)
#12
LGTM
http://codereview.appspot.com/5482047/diff/7001/google-http-client/src/main/j...
File
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java
(right):
http://codereview.appspot.com/5482047/diff/7001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:69:
* {@link InputStream} from the source data whenever invoked.
On 2012/01/23 18:30:56, yanivi wrote:
> On 2012/01/23 16:11:54, rmistry wrote:
> > On 2012/01/20 20:32:17, yanivi wrote:
> > > something like "for backwards compatibility this calls getInputStream, but
> > this
> > > will change in 1.8"
> >
> > Done.
>
> How will it change in 1.8? I assume it will be made abstract? You should be
> more specific.
Ping.
http://codereview.appspot.com/5482047/diff/12001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):
http://codereview.appspot.com/5482047/diff/12001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:726:
if (!isAllowEmptyContent()
On 2012/01/23 22:09:36, rmistry wrote:
> On 2012/01/23 18:30:56, yanivi wrote:
> > Please also change MethodOverride to setAllowEmptyContent(false) and remove
> this
> > logic from there:
> >
> > request.setContent(ByteArrayContent.fromString(null, " "));
>
> Sorry, could you please elaborate on this comment? I do not understand what
you
> are recommending.
This is for the google-api-java-client. Change
com.google.api.client.googleapis.MethodOverride.intercept() implementation using
the above instructions.
LGTM http://codereview.appspot.com/5482047/diff/12001/google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java File google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java (right): http://codereview.appspot.com/5482047/diff/12001/google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java#newcode726 google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:726: if (!isAllowEmptyContent() On 2012/01/26 20:53:17, yanivi wrote: > ...
12 years, 2 months ago
(2012-01-26 20:54:26 UTC)
#13
LGTM
http://codereview.appspot.com/5482047/diff/12001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):
http://codereview.appspot.com/5482047/diff/12001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:726:
if (!isAllowEmptyContent()
On 2012/01/26 20:53:17, yanivi wrote:
> On 2012/01/23 22:09:36, rmistry wrote:
> > On 2012/01/23 18:30:56, yanivi wrote:
> > > Please also change MethodOverride to setAllowEmptyContent(false) and
remove
> > this
> > > logic from there:
> > >
> > > request.setContent(ByteArrayContent.fromString(null, " "));
> >
> > Sorry, could you please elaborate on this comment? I do not understand what
> you
> > are recommending.
>
> This is for the google-api-java-client. Change
> com.google.api.client.googleapis.MethodOverride.intercept() implementation
using
> the above instructions.
This should go to http://codereview.appspot.com/5450097/
I will not submit this till http://codereview.appspot.com/5450097/ is ready to go as well. http://codereview.appspot.com/5482047/diff/7001/google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java File ...
12 years, 2 months ago
(2012-01-26 23:32:27 UTC)
#14
I will not submit this till http://codereview.appspot.com/5450097/ is ready to
go as well.
http://codereview.appspot.com/5482047/diff/7001/google-http-client/src/main/j...
File
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java
(right):
http://codereview.appspot.com/5482047/diff/7001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:69:
* {@link InputStream} from the source data whenever invoked.
On 2012/01/26 20:53:17, yanivi wrote:
> On 2012/01/23 18:30:56, yanivi wrote:
> > On 2012/01/23 16:11:54, rmistry wrote:
> > > On 2012/01/20 20:32:17, yanivi wrote:
> > > > something like "for backwards compatibility this calls getInputStream,
but
> > > this
> > > > will change in 1.8"
> > >
> > > Done.
> >
> > How will it change in 1.8? I assume it will be made abstract? You should
be
> > more specific.
>
> Ping.
Done.
http://codereview.appspot.com/5482047/diff/12001/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):
http://codereview.appspot.com/5482047/diff/12001/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:726:
if (!isAllowEmptyContent()
On 2012/01/26 20:54:26, yanivi wrote:
> On 2012/01/26 20:53:17, yanivi wrote:
> > On 2012/01/23 22:09:36, rmistry wrote:
> > > On 2012/01/23 18:30:56, yanivi wrote:
> > > > Please also change MethodOverride to setAllowEmptyContent(false) and
> remove
> > > this
> > > > logic from there:
> > > >
> > > > request.setContent(ByteArrayContent.fromString(null, " "));
> > >
> > > Sorry, could you please elaborate on this comment? I do not understand
what
> > you
> > > are recommending.
> >
> > This is for the google-api-java-client. Change
> > com.google.api.client.googleapis.MethodOverride.intercept() implementation
> using
> > the above instructions.
>
Got it. I will fix it in that CL.
> This should go to http://codereview.appspot.com/5450097/
It would really help to merge http://codereview.appspot.com/5572060/ into this one. http://codereview.appspot.com/5482047/diff/17002/google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java File google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java (right): http://codereview.appspot.com/5482047/diff/17002/google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java#newcode101 ...
12 years, 2 months ago
(2012-01-27 16:56:37 UTC)
#15
It would really help to merge http://codereview.appspot.com/5572060/ into this
one.
http://codereview.appspot.com/5482047/diff/17002/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java
(right):
http://codereview.appspot.com/5482047/diff/17002/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:101:
inputStream.close();
Perhaps nicer would be to provide a method to specify that the input stream
should be automatically closed at the end. For backwards compatibility, it
might be nicer to make the default true, but put a comment that this default
will be changed to false in the next release 1.8 (or just keep it true by
default).
What do you think?
We might consider a similar model for closing output stream, where the default
is not to close it, but it can be easily changed to auto-close it.
http://codereview.appspot.com/5482047/diff/21009/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java
(right):
http://codereview.appspot.com/5482047/diff/21009/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:25:
* The {@link #type} field is required. Subclasses should implement the {@link
#getLength()},
JavaDoc should specify that the input stream is not closed. Also specify that
this is a backwards-incompatible change.
Please check this comment for other implementations of HttpContent.
http://codereview.appspot.com/5482047/diff/21009/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:163:
inputStream.close();
do you want to close the input stream for this case? should we provide a
parameter whether to automatically close the input stream? note that this copy
method is used from writeTo
http://codereview.appspot.com/5482047/diff/21009/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/InputStreamContent.java
(right):
http://codereview.appspot.com/5482047/diff/21009/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/InputStreamContent.java:83:
* Should only be set to {@code true} if a BackOff Policy implementation is used
that adjusts the
This sentence here seems really bizarre. Shouldn't
getInputStream/newInputStream be responsible for that?
http://codereview.appspot.com/5482047/diff/21009/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/InputStreamContent.java:85:
* </p>
@since 1.7
http://codereview.appspot.com/5482047/diff/21009/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/InputStreamContent.java:94:
public InputStream getInputStream() {
you need to @Override newInputStream()
By the way newInputStream is probably not a good name either. What do you think
of something like readInputStream()? I'm trying to indicate that it means you
should start parsing the input stream from start, e.g. on a retry.
http://codereview.appspot.com/5482047/diff/21009/google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java File google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java (right): http://codereview.appspot.com/5482047/diff/21009/google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java#newcode25 google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:25: * The {@link #type} field is required. Subclasses should ...
12 years, 2 months ago
(2012-01-27 19:43:06 UTC)
#17
http://codereview.appspot.com/5482047/diff/21009/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java
(right):
http://codereview.appspot.com/5482047/diff/21009/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:25:
* The {@link #type} field is required. Subclasses should implement the {@link
#getLength()},
On 2012/01/27 16:56:37, yanivi wrote:
> JavaDoc should specify that the input stream is not closed. Also specify that
> this is a backwards-incompatible change.
>
> Please check this comment for other implementations of HttpContent.
Done.
http://codereview.appspot.com/5482047/diff/21009/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:163:
inputStream.close();
On 2012/01/27 16:56:37, yanivi wrote:
> do you want to close the input stream for this case? should we provide a
> parameter whether to automatically close the input stream? note that this
copy
> method is used from writeTo
Ah I missed this. Not closing it anymore and I updated the javadoc and updated
the one other reference outside this class.
http://codereview.appspot.com/5482047/diff/21009/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/InputStreamContent.java
(right):
http://codereview.appspot.com/5482047/diff/21009/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/InputStreamContent.java:83:
* Should only be set to {@code true} if a BackOff Policy implementation is used
that adjusts the
On 2012/01/27 16:56:37, yanivi wrote:
> This sentence here seems really bizarre. Shouldn't
> getInputStream/newInputStream be responsible for that?
Gave this a shot, let me know if it still does not make sense.
http://codereview.appspot.com/5482047/diff/21009/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/InputStreamContent.java:85:
* </p>
On 2012/01/27 16:56:37, yanivi wrote:
> @since 1.7
Done.
http://codereview.appspot.com/5482047/diff/21009/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/InputStreamContent.java:94:
public InputStream getInputStream() {
On 2012/01/27 16:56:37, yanivi wrote:
> you need to @Override newInputStream()
>
newInputStream calls getInputStream so till we get rid of getInputStream we do
not need to override newInputStream, is this correct?
> By the way newInputStream is probably not a good name either. What do you
think
> of something like readInputStream()? I'm trying to indicate that it means you
> should start parsing the input stream from start, e.g. on a retry.
Why not stick to what we had before getInputStream()? That name makes more sense
to me than readInputStream.
http://codereview.appspot.com/5482047/diff/21009/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/InputStreamContent.java:117:
public void setLength(long length) {
On 2012/01/27 17:57:24, yanivi wrote:
> Please change the return type to return this to follow the new style.
Done.
12 years, 1 month ago
(2012-02-08 18:15:17 UTC)
#20
http://codereview.appspot.com/5482047/diff/17002/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java
(right):
http://codereview.appspot.com/5482047/diff/17002/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:101:
inputStream.close();
On 2012/01/27 16:56:37, yanivi wrote:
> Perhaps nicer would be to provide a method to specify that the input stream
> should be automatically closed at the end. For backwards compatibility, it
> might be nicer to make the default true, but put a comment that this default
> will be changed to false in the next release 1.8 (or just keep it true by
> default).
>
> What do you think?
>
> We might consider a similar model for closing output stream, where the default
> is not to close it, but it can be easily changed to auto-close it.
Ping. I think we should consider an option to specify if the input stream
should be automatically closed, which I think is the typical case.
http://codereview.appspot.com/5482047/diff/17002/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:159:
public static void copy(InputStream inputStream, OutputStream outputStream)
throws IOException {
Similarly here it might be nice to provide a boolean parameter to specify if the
input stream should be closed. For backwards compatibility, we could keep the
current copy method that passes in true for the closeInputStream option.
http://codereview.appspot.com/5482047/diff/21009/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/InputStreamContent.java
(right):
http://codereview.appspot.com/5482047/diff/21009/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/InputStreamContent.java:94:
public InputStream getInputStream() {
On 2012/01/27 19:43:06, rmistry wrote:
> On 2012/01/27 16:56:37, yanivi wrote:
> > you need to @Override newInputStream()
> >
>
> newInputStream calls getInputStream so till we get rid of getInputStream we do
> not need to override newInputStream, is this correct?
>
> > By the way newInputStream is probably not a good name either. What do you
> think
> > of something like readInputStream()? I'm trying to indicate that it means
you
> > should start parsing the input stream from start, e.g. on a retry.
>
> Why not stick to what we had before getInputStream()? That name makes more
sense
> to me than readInputStream.
Yes, you are right. We should get rid of the whole newInputStream() and just
stick to the original naming getInputStream. Sorry for the original idea of
renaming it. Let's work on improving the JavaDoc on getInputStream though.
http://codereview.appspot.com/5482047/diff/37004/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpContent.java
(left):
http://codereview.appspot.com/5482047/diff/37004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpContent.java:44:
void writeTo(OutputStream out) throws IOException;
[optional] Should we disable the
org.codehaus.jackson.JsonGenerator.Feature.AUTO_CLOSE_TARGET feature? this
might not matter in practice since we never call JacksonFactory.close() as far
as I can tell.
http://codereview.appspot.com/5482047/diff/37004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpContent.java:44:
void writeTo(OutputStream out) throws IOException;
[optional] should we flush the output stream at the end of a writeTo
implementation?
http://codereview.appspot.com/5482047/diff/37004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpContent.java:44:
void writeTo(OutputStream out) throws IOException;
Should NetHttpRequest.execute() call connection.getOutputStream().close()?
URLConnection JavaDoc specifies that invoking close may free network resources.
http://codereview.appspot.com/5482047/diff/37004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpContent.java:44:
void writeTo(OutputStream out) throws IOException;
JavaDoc should specify in MultipartRelatedContent that the parts must not close
the output stream in writeTo.
http://codereview.appspot.com/5482047/diff/37004/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/InputStreamContent.java
(right):
http://codereview.appspot.com/5482047/diff/37004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/InputStreamContent.java:83:
* Should only be set to {@code true} if {@link #newInputStream} is used to
adjust the input
This still confuses me. I think possibly a better way to phrase this is that
newInputStream resets to the original position of the input stream.
http://codereview.appspot.com/5482047/diff/17002/google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java File google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java (right): http://codereview.appspot.com/5482047/diff/17002/google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java#newcode101 google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:101: inputStream.close(); On 2012/02/08 18:15:17, yanivi wrote: > On 2012/01/27 ...
12 years, 1 month ago
(2012-02-09 14:44:44 UTC)
#21
http://codereview.appspot.com/5482047/diff/17002/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java
(right):
http://codereview.appspot.com/5482047/diff/17002/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:101:
inputStream.close();
On 2012/02/08 18:15:17, yanivi wrote:
> On 2012/01/27 16:56:37, yanivi wrote:
> > Perhaps nicer would be to provide a method to specify that the input stream
> > should be automatically closed at the end. For backwards compatibility, it
> > might be nicer to make the default true, but put a comment that this default
> > will be changed to false in the next release 1.8 (or just keep it true by
> > default).
> >
> > What do you think?
> >
> > We might consider a similar model for closing output stream, where the
default
> > is not to close it, but it can be easily changed to auto-close it.
>
> Ping. I think we should consider an option to specify if the input stream
> should be automatically closed, which I think is the typical case.
Sorry, I missed this comment in the last round of review. Done.
http://codereview.appspot.com/5482047/diff/17002/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:159:
public static void copy(InputStream inputStream, OutputStream outputStream)
throws IOException {
On 2012/02/08 18:15:17, yanivi wrote:
> Similarly here it might be nice to provide a boolean parameter to specify if
the
> input stream should be closed. For backwards compatibility, we could keep the
> current copy method that passes in true for the closeInputStream option.
Done.
http://codereview.appspot.com/5482047/diff/37004/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpContent.java
(left):
http://codereview.appspot.com/5482047/diff/37004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpContent.java:44:
void writeTo(OutputStream out) throws IOException;
On 2012/02/08 18:15:18, yanivi wrote:
> JavaDoc should specify in MultipartRelatedContent that the parts must not
close
> the output stream in writeTo.
Done.
http://codereview.appspot.com/5482047/diff/37004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpContent.java:44:
void writeTo(OutputStream out) throws IOException;
On 2012/02/08 18:15:18, yanivi wrote:
> Should NetHttpRequest.execute() call connection.getOutputStream().close()?
> URLConnection JavaDoc specifies that invoking close may free network
resources.
Done.
http://codereview.appspot.com/5482047/diff/37004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpContent.java:44:
void writeTo(OutputStream out) throws IOException;
On 2012/02/08 18:15:18, yanivi wrote:
> [optional] should we flush the output stream at the end of a writeTo
> implementation?
Can we file the above two proposals as feature requests, I would love to get
this CL in for Media sooner rather than later.
http://codereview.appspot.com/5482047/diff/37004/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/InputStreamContent.java
(right):
http://codereview.appspot.com/5482047/diff/37004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/InputStreamContent.java:83:
* Should only be set to {@code true} if {@link #newInputStream} is used to
adjust the input
On 2012/02/08 18:15:18, yanivi wrote:
> This still confuses me. I think possibly a better way to phrase this is that
> newInputStream resets to the original position of the input stream.
Done.
waiting for 5450097 before reviewing http://codereview.appspot.com/5482047/diff/37004/google-http-client/src/main/java/com/google/api/client/http/HttpContent.java File google-http-client/src/main/java/com/google/api/client/http/HttpContent.java (left): http://codereview.appspot.com/5482047/diff/37004/google-http-client/src/main/java/com/google/api/client/http/HttpContent.java#oldcode44 google-http-client/src/main/java/com/google/api/client/http/HttpContent.java:44: void writeTo(OutputStream out) throws ...
12 years, 1 month ago
(2012-02-09 15:24:23 UTC)
#22
waiting for 5450097 before reviewing
http://codereview.appspot.com/5482047/diff/37004/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpContent.java
(left):
http://codereview.appspot.com/5482047/diff/37004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpContent.java:44:
void writeTo(OutputStream out) throws IOException;
On 2012/02/09 14:44:45, rmistry wrote:
> On 2012/02/08 18:15:18, yanivi wrote:
> > [optional] should we flush the output stream at the end of a writeTo
> > implementation?
>
> Can we file the above two proposals as feature requests, I would love to get
> this CL in for Media sooner rather than later.
TODO would certainly suffice here.
12 years, 1 month ago
(2012-02-09 16:18:28 UTC)
#23
http://codereview.appspot.com/5482047/diff/37004/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpContent.java
(left):
http://codereview.appspot.com/5482047/diff/37004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpContent.java:44:
void writeTo(OutputStream out) throws IOException;
On 2012/02/09 15:24:23, yanivi wrote:
> On 2012/02/09 14:44:45, rmistry wrote:
> > On 2012/02/08 18:15:18, yanivi wrote:
> > > [optional] should we flush the output stream at the end of a writeTo
> > > implementation?
> >
> > Can we file the above two proposals as feature requests, I would love to get
> > this CL in for Media sooner rather than later.
>
> TODO would certainly suffice here.
Done.
http://codereview.appspot.com/5482047/diff/17002/google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java File google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java (right): http://codereview.appspot.com/5482047/diff/17002/google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java#newcode101 google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:101: inputStream.close(); On 2012/02/09 14:44:45, rmistry wrote: > On 2012/02/08 ...
12 years, 1 month ago
(2012-02-09 17:03:07 UTC)
#24
http://codereview.appspot.com/5482047/diff/17002/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java
(right):
http://codereview.appspot.com/5482047/diff/17002/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:101:
inputStream.close();
On 2012/02/09 14:44:45, rmistry wrote:
> On 2012/02/08 18:15:17, yanivi wrote:
> > On 2012/01/27 16:56:37, yanivi wrote:
> > > Perhaps nicer would be to provide a method to specify that the input
stream
> > > should be automatically closed at the end. For backwards compatibility,
it
> > > might be nicer to make the default true, but put a comment that this
default
> > > will be changed to false in the next release 1.8 (or just keep it true by
> > > default).
> > >
> > > What do you think?
> > >
> > > We might consider a similar model for closing output stream, where the
> default
> > > is not to close it, but it can be easily changed to auto-close it.
> >
> > Ping. I think we should consider an option to specify if the input stream
> > should be automatically closed, which I think is the typical case.
>
> Sorry, I missed this comment in the last round of review. Done.
Just tested this with my resumable media upload sample and it throws an
exception because the input stream is closed in the default case which is why I
originally left it open.
I can make the original writeTo call this new method with closeInputStream=false
and add an upgrade warning in the javadoc of this class. What do you think?
http://codereview.appspot.com/5482047/diff/17002/google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java File google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java (right): http://codereview.appspot.com/5482047/diff/17002/google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java#newcode101 google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:101: inputStream.close(); On 2012/02/09 17:03:07, rmistry wrote: > On 2012/02/09 ...
12 years, 1 month ago
(2012-02-09 21:50:45 UTC)
#25
http://codereview.appspot.com/5482047/diff/17002/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java
(right):
http://codereview.appspot.com/5482047/diff/17002/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:101:
inputStream.close();
On 2012/02/09 17:03:07, rmistry wrote:
> On 2012/02/09 14:44:45, rmistry wrote:
> > On 2012/02/08 18:15:17, yanivi wrote:
> > > On 2012/01/27 16:56:37, yanivi wrote:
> > > > Perhaps nicer would be to provide a method to specify that the input
> stream
> > > > should be automatically closed at the end. For backwards compatibility,
> it
> > > > might be nicer to make the default true, but put a comment that this
> default
> > > > will be changed to false in the next release 1.8 (or just keep it true
by
> > > > default).
> > > >
> > > > What do you think?
> > > >
> > > > We might consider a similar model for closing output stream, where the
> > default
> > > > is not to close it, but it can be easily changed to auto-close it.
> > >
> > > Ping. I think we should consider an option to specify if the input stream
> > > should be automatically closed, which I think is the typical case.
> >
> > Sorry, I missed this comment in the last round of review. Done.
>
> Just tested this with my resumable media upload sample and it throws an
> exception because the input stream is closed in the default case which is why
I
> originally left it open.
>
> I can make the original writeTo call this new method with
closeInputStream=false
> and add an upgrade warning in the javadoc of this class. What do you think?
What I meant was that closeInputStream should be a field just like encoding and
type, not a parameter of the writeTo method. By default it should be true, but
for resumable media upload we should set it to false. No
backwards-compatibility issues that way, and it reduces the chance of
accidentally leaking resources from the input stream.
By contrast, having closeInputStream on the copy method makes sense.
http://codereview.appspot.com/5482047/diff/45004/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java
(right):
http://codereview.appspot.com/5482047/diff/45004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:30:
* Upgrade warning: in prior version 1.6 {@link #getInputStream} was protected,
it is now public.
better to put this comment in JavaDoc of getInputStream() I think
http://codereview.appspot.com/5482047/diff/45004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:63:
*/
@since 1.7 since visibility is higher??? I'm not sure.
http://codereview.appspot.com/5482047/diff/45004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:79:
*/
@since 1.7
http://codereview.appspot.com/5482047/diff/45004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:84:
copy(inputStream, out);
this should pass closeInputStream parameter to the copy method
http://codereview.appspot.com/5482047/diff/45004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:188:
*/
@since 1.7
http://codereview.appspot.com/5482047/diff/45004/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpContent.java
(right):
http://codereview.appspot.com/5482047/diff/45004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpContent.java:48:
* should not assume whether or not the output stream has been closed.
Add a recommendation to flush the output stream at the end if you don't close
it.
http://codereview.appspot.com/5482047/diff/45004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpContent.java:52:
* TODO(rmistry): Should we disable the JsonGenerator.Feature.AUTO_CLOSE_TARGET
feature?
this TODO should go inside JacksonFactory, and definitely not in a JavaDoc
comment
http://codereview.appspot.com/5482047/diff/45004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpContent.java:56:
* TODO(rmistry): Should we flush the output stream at the end of a writeTo
implementation?
The only implementations I'm aware of that don't flush the output stream are
ProtoHttpContent, MockHttpContent, LogContent, and AbstractInputStreamContent.
It takes 1 line of code to add out.flush() to each. I would just do it now
rather than add a TODO that you won't remember to do later.
alternatively, if you want to keep the TODO, please add the information I
mentioned here about which implementations to update and move it out of the
JavaDoc.
http://codereview.appspot.com/5482047/diff/45004/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/MultipartRelatedContent.java
(right):
http://codereview.appspot.com/5482047/diff/45004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/MultipartRelatedContent.java:101:
* {@link #writeTo} can be called multiple times for Multipart/Related content.
Parts must not
I think it would be better to put this comment at the class level, because users
are unlikely to look at JavaDoc of writeTo since they don't call it directly.
http://codereview.appspot.com/5482047/diff/17002/google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java File google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java (right): http://codereview.appspot.com/5482047/diff/17002/google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java#newcode101 google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:101: inputStream.close(); On 2012/02/09 21:50:45, yanivi wrote: > On 2012/02/09 ...
12 years, 1 month ago
(2012-02-10 14:42:51 UTC)
#26
http://codereview.appspot.com/5482047/diff/17002/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java
(right):
http://codereview.appspot.com/5482047/diff/17002/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:101:
inputStream.close();
On 2012/02/09 21:50:45, yanivi wrote:
> On 2012/02/09 17:03:07, rmistry wrote:
> > On 2012/02/09 14:44:45, rmistry wrote:
> > > On 2012/02/08 18:15:17, yanivi wrote:
> > > > On 2012/01/27 16:56:37, yanivi wrote:
> > > > > Perhaps nicer would be to provide a method to specify that the input
> > stream
> > > > > should be automatically closed at the end. For backwards
compatibility,
> > it
> > > > > might be nicer to make the default true, but put a comment that this
> > default
> > > > > will be changed to false in the next release 1.8 (or just keep it true
> by
> > > > > default).
> > > > >
> > > > > What do you think?
> > > > >
> > > > > We might consider a similar model for closing output stream, where the
> > > default
> > > > > is not to close it, but it can be easily changed to auto-close it.
> > > >
> > > > Ping. I think we should consider an option to specify if the input
stream
> > > > should be automatically closed, which I think is the typical case.
> > >
> > > Sorry, I missed this comment in the last round of review. Done.
> >
> > Just tested this with my resumable media upload sample and it throws an
> > exception because the input stream is closed in the default case which is
why
> I
> > originally left it open.
> >
> > I can make the original writeTo call this new method with
> closeInputStream=false
> > and add an upgrade warning in the javadoc of this class. What do you think?
>
> What I meant was that closeInputStream should be a field just like encoding
and
> type, not a parameter of the writeTo method. By default it should be true,
but
> for resumable media upload we should set it to false. No
> backwards-compatibility issues that way, and it reduces the chance of
> accidentally leaking resources from the input stream.
>
> By contrast, having closeInputStream on the copy method makes sense.
Done.
http://codereview.appspot.com/5482047/diff/45004/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java
(right):
http://codereview.appspot.com/5482047/diff/45004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:30:
* Upgrade warning: in prior version 1.6 {@link #getInputStream} was protected,
it is now public.
On 2012/02/09 21:50:45, yanivi wrote:
> better to put this comment in JavaDoc of getInputStream() I think
Done.
http://codereview.appspot.com/5482047/diff/45004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:63:
*/
On 2012/02/09 21:50:45, yanivi wrote:
> @since 1.7 since visibility is higher??? I'm not sure.
Not sure either, but Done.
http://codereview.appspot.com/5482047/diff/45004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:79:
*/
On 2012/02/09 21:50:45, yanivi wrote:
> @since 1.7
Done.
http://codereview.appspot.com/5482047/diff/45004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:84:
copy(inputStream, out);
On 2012/02/09 21:50:45, yanivi wrote:
> this should pass closeInputStream parameter to the copy method
Done.
http://codereview.appspot.com/5482047/diff/45004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/AbstractInputStreamContent.java:188:
*/
On 2012/02/09 21:50:45, yanivi wrote:
> @since 1.7
Done.
http://codereview.appspot.com/5482047/diff/45004/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/HttpContent.java
(right):
http://codereview.appspot.com/5482047/diff/45004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpContent.java:48:
* should not assume whether or not the output stream has been closed.
On 2012/02/09 21:50:45, yanivi wrote:
> Add a recommendation to flush the output stream at the end if you don't close
> it.
Done.
http://codereview.appspot.com/5482047/diff/45004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpContent.java:52:
* TODO(rmistry): Should we disable the JsonGenerator.Feature.AUTO_CLOSE_TARGET
feature?
On 2012/02/09 21:50:45, yanivi wrote:
> this TODO should go inside JacksonFactory, and definitely not in a JavaDoc
> comment
Done.
http://codereview.appspot.com/5482047/diff/45004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/HttpContent.java:56:
* TODO(rmistry): Should we flush the output stream at the end of a writeTo
implementation?
On 2012/02/09 21:50:45, yanivi wrote:
> The only implementations I'm aware of that don't flush the output stream are
> ProtoHttpContent, MockHttpContent, LogContent, and AbstractInputStreamContent.
> It takes 1 line of code to add out.flush() to each. I would just do it now
> rather than add a TODO that you won't remember to do later.
Done.
>
> alternatively, if you want to keep the TODO, please add the information I
> mentioned here about which implementations to update and move it out of the
> JavaDoc.
http://codereview.appspot.com/5482047/diff/45004/google-http-client/src/main/...
File
google-http-client/src/main/java/com/google/api/client/http/MultipartRelatedContent.java
(right):
http://codereview.appspot.com/5482047/diff/45004/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/http/MultipartRelatedContent.java:101:
* {@link #writeTo} can be called multiple times for Multipart/Related content.
Parts must not
On 2012/02/09 21:50:45, yanivi wrote:
> I think it would be better to put this comment at the class level, because
users
> are unlikely to look at JavaDoc of writeTo since they don't call it directly.
Makes sense. Done.
Issue 5482047: Changes to support 5450097
(Closed)
Created 12 years, 3 months ago by rmistry
Modified 12 years, 1 month ago
Reviewers: yanivi
Base URL: https://google-http-java-client.googlecode.com/hg/
Comments: 94