Skip to content

Fixes unexpected error when request using pulsar 2.4.2 proxy - #157

Merged
sijie merged 3 commits into
streamnative:masterfrom
zymap:fix-proxy-error
Dec 20, 2019
Merged

Fixes unexpected error when request using pulsar 2.4.2 proxy#157
sijie merged 3 commits into
streamnative:masterfrom
zymap:fix-proxy-error

Conversation

@zymap

@zymap zymap commented Dec 16, 2019

Copy link
Copy Markdown
Member

Motivation

When sending a GET or DELETE request to the Pulsar proxy, the server
always threw an IllegalArgumentException. By my test that's because
we are setting the Content-Type for a request without the request body.
We remove the Content-Type if the request is GET or DELETE.

Modifications

  • Remove the header Content-Type for the GET and DELETE requests

---

*Motivation*

When sending a GET or DELETE request to the Pulsar proxy, the server
always threw an IllegalArgumentException. By my test that's because
we are setting the Content-Type for a request without the request body.
We remove the Content-Type if the request is GET or DELETE.

*Modifications*

- Remove the header Content-Type for the GET and DELETE requests
@zymap zymap added the type/bug label Dec 16, 2019
@zymap zymap added this to the 0.3.0 milestone Dec 16, 2019
@zymap
zymap requested review from sijie and wolfstudy December 16, 2019 15:09
@zymap zymap self-assigned this Dec 16, 2019

@sijie sijie left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zymap does pulsarctl work for 2.4.1 or 2.4.0? What has been changed in pulsar 2.4.2 to fail pulsarctl?

@zymap

zymap commented Dec 17, 2019

Copy link
Copy Markdown
Member Author

@sijie
Yes. Pulsarctl works well on 2.4.1 and 2.4.0. I think the problem due to the apache/pulsar#5361. I am not sure whether the server will think there is a request body when we add the header Content-Type for GET or DELETE. If there have a request body, the code ReplayableProxyContentProvider will save the body. Using curl -H "Content-Type: application/json" http://localhost:8080/admin/v2/tenats will reproduce the problem.

@sijie

sijie commented Dec 17, 2019

Copy link
Copy Markdown
Member

@zymap : if apache/pulsar#5361 is the root cause, shouldn't we look into that issue? Why do you think removing Content-Type: application/json from GET and DELETE requests is the right fix?

@zymap

zymap commented Dec 17, 2019

Copy link
Copy Markdown
Member Author

@sijie Yes. we should. I look into the issue, the request.contentLength will be -1 if we using GET or DELETE and specified the header Content-Type

Also, I find the https://tools.ietf.org/html/rfc7231#section-3.1.1.5,
A sender that generates a message containing a payload body SHOULD generate a Content-Type header field in that message unless the intended media type of the enclosed representation is unknown to the sender. If a Content-Type header field is not present, the recipient MAY either assume a media type of "application/octet-stream" ([RFC2046], Section 4.5.1) or examine the data to determine its type.. These words just tell we should add Content-Type when we have a request body. I am not sure should we add the Content-Type even if we have not the request body.

@sijie

sijie commented Dec 17, 2019

Copy link
Copy Markdown
Member

the request.contentLength will be -1 if we using GET or DELETE and specified the header Content-Type

Why it is -1? Is -1 the right value?

These words just tell we should add Content-Type when we have a request body. I am not sure should we add the Content-Type even if we have not the request body.

if that's the case, your fix is incorrect, no? If as what you said, wee should add content type based on whether we have content or not. but you are fixing based on the request type. why do you think a delete request will not have content?

@zymap

zymap commented Dec 17, 2019

Copy link
Copy Markdown
Member Author

-1 is a default value in Jetty. In Pulsarctl the content length is 0.

if that's the case, your fix is incorrect, no? If as what you said, wee should add content type based on whether we have content or not. but you are fixing based on the request type. why do you think a delete request will not have content?

Ah. You are right, I should add content-type based on whether we have content or not.

@zymap

zymap commented Dec 18, 2019

Copy link
Copy Markdown
Member Author

@sijie PTAL. Thanks.

@sijie
sijie merged commit 09b7313 into streamnative:master Dec 20, 2019
tisonkun pushed a commit to tisonkun/pulsar-client-go that referenced this pull request Aug 15, 2023
…ative/pulsarctl#157)

*Motivation*

When sending a GET or DELETE request to the Pulsar proxy, the server
always threw an IllegalArgumentException. By my test that's because
we are setting the Content-Type for a request without the request body.
We remove the Content-Type if the request is GET or DELETE.

*Modifications*

- Remove the header Content-Type if the content body is empty.
tisonkun pushed a commit to apache/pulsar-client-go that referenced this pull request Aug 16, 2023
…ative/pulsarctl#157)

*Motivation*

When sending a GET or DELETE request to the Pulsar proxy, the server
always threw an IllegalArgumentException. By my test that's because
we are setting the Content-Type for a request without the request body.
We remove the Content-Type if the request is GET or DELETE.

*Modifications*

- Remove the header Content-Type if the content body is empty.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants