Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multipart body altering #7

Open
smelfungus opened this issue Nov 21, 2018 · 13 comments
Open

Multipart body altering #7

smelfungus opened this issue Nov 21, 2018 · 13 comments
Assignees
Labels
bug Something isn't working

Comments

@smelfungus
Copy link

Hi there! Thank you for that nice interceptor.
We've faced fancy issue here:
While trying to upload multipart body containing file with OkHttpProfiler connected, our backend notifies body validation error. With no OkHttpProfiler connected everything fires just ok.
During further inspection we're getting next raw body starting:

--79f57c45-d2a7-440b-8646-081067778104Content-Disposition: form-data; name="file"; filename="file"Content-Type: image/jpegContent-Length: 38511<file body going next>

I'm not sure about validity of that boundary appendix and produced Content-Length.
Is it somehow altered by OkHttpProfiler? Thank you!

@itkacher
Copy link
Owner

Hi!
I have had no problems with the multipart body.
I will investigate this issue. I have no idea right now
Thanks for sharing

@itkacher itkacher self-assigned this Nov 21, 2018
@itkacher itkacher added question Further information is requested bug Something isn't working and removed question Further information is requested labels Nov 21, 2018
@itkacher
Copy link
Owner

itkacher commented Nov 21, 2018

Is this a real file size? Filename with ".jpg" or ".jpeg" extension here name="file"; filename="file"?
Also, can you provide a log from the logcat (Verbose level) like:

V/OKPRFL_9o0ubvx_RQB: --275a8191-f116-4421-a4cb-b2fc6fd56eb3
    Content-Disposition: form-data; name="file"; filename="Document_1.jpg"
    Content-Type: image/jpeg
    Content-Length: 200248

Only headers of file part
I can't reproduce any altering.

@smelfungus
Copy link
Author

@itkacher thank you for such a quick response.
The thing I discovered for now is switching off late response proceeding solves the issue:

    public static class PrematureOkHttpProfilerInterceptor implements Interceptor {
        private final DataTransfer dataTransfer = new LogDataTransfer();
        private final DateFormat format = new SimpleDateFormat("ddhhmmssSSS", Locale.US);

        @Override
        public Response intercept(Chain chain) throws IOException {
            Request originalRequest = chain.request();
            Response originalResponse = chain.proceed(originalRequest);

            String id = generateId();
            long startTime = System.currentTimeMillis();
            dataTransfer.sendRequest(id, originalRequest);
            try {
                dataTransfer.sendResponse(id, originalResponse);
                dataTransfer.sendDuration(id, System.currentTimeMillis() - startTime);
                return originalResponse;
            } catch (Exception e) {
                dataTransfer.sendException(id, e);
                dataTransfer.sendDuration(id, System.currentTimeMillis() - startTime);
                throw e;
            }
        }

        private String generateId() {
            long timeAndDay = Long.parseLong(format.format(new Date()));
            return Long.toString(timeAndDay, Character.MAX_RADIX);
        }
    }

Output:

POST /api/content/upload

Accept:application/json
Authorization:Bearer ***
Content-Type:application/json

--d3276c57-519a-4fdf-a304-407adb7c9455Content-Disposition: form-data; name="file"; filename="file"Content-Type: image/jpegContent-Length: 0--d3276c57-519a-4fdf-a304-407adb7c9455--

And looses intercepted parts' content.

Filename with ".jpg" or ".jpeg" extension here

Yep, that is intended.
Will provide requested info ASAP.

@smelfungus
Copy link
Author

Failed request with original interceptor:
https://gist.github.com/DummyCo/4f2ddfccd8d8015c91bf55e100e4e4d8
Succeed request with premature chain processing:
https://gist.github.com/DummyCo/1328c57bd81d076986c2a405f9a00bb8

@itkacher
Copy link
Owner

Thanks a lot. I will try to fix soon. Sorry about the issue.

@itkacher
Copy link
Owner

itkacher commented Nov 21, 2018

Request header is:
OKPRFL_9obdan0_RQH: Content-Type:application/json
Are you sure, that this is correct?
Because it can be a reason.
I guess it needs to be like this.
Content-Type: multipart/form-data; boundary=cb57c87e-f1dd-492c-b142-2ffca7ecad04
Can you provide the library (OkHttp or Retrofit) and how do exactly you send the file (java/kotlin code), please?

@itkacher
Copy link
Owner

I did some request fixes with encoding, not sure that this is it.
Can you test it, please?
https://github.com/itkacher/OkHttpProfiler/raw/bugfix/7/okhttpprofiler-1.0.5.jar
Just add this file to the libs folder and add to the gradle
implementation fileTree(dir: 'libs', include: ['*.jar'])
Also, can you set the correct type of MultipartBody?

RequestBody requestBody = new MultipartBody.Builder()
                .setType(MultipartBody.FORM)

Because a server needs to know the boundary for correct request parsing.
I will be grateful if you help me with this.
Thanks

@smelfungus
Copy link
Author

Are you sure, that this is correct?

I agree that it's not too healthy to provide wrong Content-Type here but our server consumes it for now, will work on it further anyway, but I'm not too sure that it may cause this particular problem because request is delivered with no issues using series of other custom and opensource interceptors for different purposes. Nevertheless, will fix it ASAP, thank you.

Going to test new version out and notify about the results!

@itkacher
Copy link
Owner

itkacher commented Nov 22, 2018

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Type

boundary
For multipart entities the boundary directive is required, which consists of 1 to 70 characters from a set of characters known to be very robust through email gateways, and not ending with white space. It is used to encapsulate the boundaries of the multiple parts of the message. Often the header boundary is prepended by two dashes in the body and the final boundary also have a two dashes appended to it.

This header is important for the multipart form data.
The server parses content between two bound as one part of the request.
In theory, your request shouldn't work at all.
But I agree with you, that this is strange behavior that it works without Interceptor.
I can't reproduce it now, so I need more data for the problem understanding.
Can you provide the code of request generation I will try your case?
I will install a NodeJS server and try to send a request and compare the content of it with/without interceptor.

@smelfungus
Copy link
Author

smelfungus commented Nov 22, 2018

Thank you for clarifying. I managed to fix headers and some internal stream feeding flow, here are some interesting points:

  • Retrofit version: 2.5.0
  • okhttp version: 3.12.0

REST service upload method signature:

@Multipart
@POST("content/upload")
Observable<ModelResponse<Content>> uploadContent(
		@Part MultipartBody.Part part
);

Request preparing & uploading flow:

InputStreamRequestBody inputStreamRequestBody = new InputStreamRequestBody(
		MediaType.parse(mimeType),
		inputStream
);

CountingRequestBody countingRequestBody = new CountingRequestBody(
		inputStreamRequestBody,
		(bytesWritten, contentLength) -> {
			//...
		}
);

MultipartBody.Part filePart = MultipartBody.Part.createFormData(
		"file",
		"file",
		countingRequestBody
);

restManager.uploadContent(filePart);

Now that's still firing ok with no OkHttpProfiler interceptor and fails with it getting new unexpected end of stream.
At least we have valid multipart header for now :D

New output:
https://gist.github.com/DummyCo/3e78f660567a9c2d31ebec4e77a9c521

And one more interesting observation. In case we're creating prepared RequestBody with byte array:

ByteArrayOutputStream buffer = new ByteArrayOutputStream();

int nRead;
byte[] data = new byte[16384];

while ((nRead = inputStream.read(data, 0, data.length)) != -1) {
	buffer.write(data, 0, nRead);
}

RequestBody requestBody = RequestBody.create(
		MediaType.parse("image/jpeg"),
		buffer.toByteArray()
);

MultipartBody.Part filePart = MultipartBody.Part.createFormData(
		"file",
		"file",
		requestBody
);

It fires ok with or without interceptor:
https://gist.github.com/DummyCo/aeb5409b9b167364a875b33f66e3881a

@smelfungus
Copy link
Author

smelfungus commented Nov 22, 2018

I'm comparing successful and failed request bodies side-by-side and getting small diffs present...

@itkacher
Copy link
Owner

@DummyCo can you remove a reference from this issue square/okhttp#3585 ?
I am sure, that this is not a problem of OkHttp.
The other tested way is to send a file

val mimeType = FileUtil.getMimeType(filePath)
val mediaType = MediaType.parse(mimeType ?: filePath)
val file = File(filePath)
val requestBody: RequestBody = RequestBody.create(mediaType, file)
val data = MultipartBody.Part.createFormData("file", file.name, requestBody)

I will test 'CountingRequestBody' and define the problem

@smelfungus
Copy link
Author

smelfungus commented Nov 22, 2018

Sure, just wanted to provide author implementations, but I'm afraid reference is set automatically by GitHub and can't be undone 😥
Talking about file-based ways: we're pretty limited about this approach because we're forced to work with URIs and resolved streams, so content can be represented by not file-backed resource.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants