'output' field should be accessed using a protected accessor

[pdeva]

currently if somebody wants to override openOutput() to say send compressed data, they cant assign to output cause its private and also cause no accessor is used throughout the code to access it. This means replacing a bunch of code just to modify the output stream creation.

The output field should be protected and accessed using getOutput() throughout the code.

[kevinsawicki]

How about having a helper called createOutputStream() that returns an OutputStream that is called from openOutput() that by default just returns the value of getConnection().getOutputStream()?

You could override that and wrap the connection's output stream in another output stream if desired.

[pdeva]

true. though I am not sure that alone would do. Here is the gist of the code I had to write to overcome some of the deficiencies in HttpRequest. Points to note there:

1. there needs to be a getOutput() method that returns the outputstream variable since the send() methods dont capture all use cases.
2. constructor for HttpRequestException should be either public or there needs to be a method newHttpRequestException() method, so we dont have to subclass the exception just to throw it.

[kevinsawicki]

On point 1, currently a RequestOutputStream is used internally to wrap the connection's OutputStream.

Are you saying you want to replace RequestOutputStream completely or you just want to replace the OutputStream it wraps in your sub-classes?

[kevinsawicki]

I made the HttpRequestException constructor public in commit aeccee6

[pdeva]

I forgot to actually post the gist in my last comment. here it is: https://gist.github.com/pdeva/6314946

[pdeva]

point 1 relates to the writeData(HttpURLConnection conn, boolean isDeflate, JSONStreamAware obj) method in the gist.

Here the JSONStreamAware needs to be passed an OutputStream object. currently there is no way to get a reference to the outputstream from the httprequest object.

also the copy() method is incorrect. It checks for -1 as a terminator. Many times, especially while sending binary/compressed data, -1 is a real value. The method instead needs to take in a length param.

[kevinsawicki]

So in your example you want to send the data compressed to the server right?

I think this is something useful to build into the library but I also want to make extensions like yours work.

[pdeva]

i think any situation where you are acccepting a byte array and automatically assume -1 is a terminator inside that array is fundamentally incorrect.

[kevinsawicki]

Isn't -1 what the docs say as the end of stream?

http://docs.oracle.com/javase/7/docs/api/java/io/ByteArrayInputStream.html#read()

[pdeva]

it only says: "If pos equals count, then -1 is returned to indicate end of file."

nowhere does it say that the presence of -1 in the stream indicates end of stream.

Prashant

Sent from my iPhone

On Aug 23, 2013, at 9:05 AM, Kevin Sawicki [email protected] wrote:

Isn't -1 that what the docs say as the end of stream?

http://docs.oracle.com/javase/7/docs/api/java/io/ByteArrayInputStream.html#read()

— Reply to this email directly or view it on GitHubhttps://github.com/kevinsawicki/http-request/issues/52#issuecomment-23171021 .

[pdeva]

oops I read the wrong read() method in the last comment. the one you pointed to also says values in the range 0 to 255 are valid. Java uses a 2s complement system and leaves one bit for the sign, so -1 will be 255. (Byte.MaxValue is 128).

Prashant

Sent from my iPhone

On Aug 23, 2013, at 9:05 AM, Kevin Sawicki [email protected] wrote:

Isn't -1 that what the docs say as the end of stream?

http://docs.oracle.com/javase/7/docs/api/java/io/ByteArrayInputStream.html#read()

— Reply to this email directly or view it on GitHubhttps://github.com/kevinsawicki/http-request/issues/52#issuecomment-23171021 .

[kevinsawicki]

Could you point to the line of code in HttpRequest.java that you think is not checking the end of streams properly?

[pdeva]

oh my bad. i thought you were looking for a -1 in the byte array stream. this is what happens when you are dealing with a bug and wondering if its your code or the library ;)

[kevinsawicki]

No worries, thanks for letting me know, I want to circle back to your original point and rephrase to make sure I understand what you are looking for:

In your example you don't want to have the internal class RequestOutputStream used at all, you want to completely take over the output stream written in the various send methods so you can compress it on the way out?

[pdeva]

Actually there is no RequestOutputStream class in my example... https://gist.github.com/pdeva/6314946

The class that should not be present is that entire ConnectionDataWriter class.

[kevinsawicki]

Yeah, sorry, RequestOutputStream is internal to HttpRequest and is what is currently returned from openOutput().

I asked because the output field in HttpRequest assumes a RequestOutputStream and not just an OutputStream.

[pdeva]

yes, that assumption definitely prevents us from doing what we need to do

Prashant

On Wed, Aug 28, 2013 at 11:14 AM, Kevin Sawicki [email protected]:

Yeah, sorry, RequestOutputStream is internal to HttpRequest and is what is currently returned from openOutput().

I asked because the output field in HttpRequest assumes a RequestOutputStream and not just an OutputStream.

— Reply to this email directly or view it on GitHubhttps://github.com/kevinsawicki/http-request/issues/52#issuecomment-23435597 .