You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by tstanev <gi...@git.apache.org> on 2015/02/13 17:00:18 UTC
[GitHub] thrift pull request: Set Content-Type for HTTP request in Node.js ...
GitHub user tstanev opened a pull request:
https://github.com/apache/thrift/pull/378
Set Content-Type for HTTP request in Node.js wrapper. Done similar to th...
...e Python wrapper here:
https://github.com/apache/thrift/blob/master/lib/py/src/transport/THttpClient.py#L127
Without this header, connecting to some servers results in error response. (THRIFT-2998)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/tstanev/thrift adsk/nodejs_header
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/378.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #378
----
commit 0056ba8e1093b4bdde504d4f7bedc4874756f931
Author: stanevt <tr...@autodesk.com>
Date: 2015-02-13T15:45:59Z
Set Content-Type for HTTP request in Node.js wrapper. Done similar to the Python wrapper here https://github.com/apache/thrift/blob/master/lib/py/src/transport/THttpClient.py#L127 .
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #378: THRIFT-2998: Set Content-Type for HTTP request in ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/378
---
[GitHub] thrift pull request: THRIFT-2998: Set Content-Type for HTTP reques...
Posted by stigsb <gi...@git.apache.org>.
Github user stigsb commented on the pull request:
https://github.com/apache/thrift/pull/378#issuecomment-108482745
IMHO client code should always send the IANA-registered types, while servers should accept `application/x-thrift` as an alias for `application/vnd.apache.thrift.binary` (probably for a long time).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #378: THRIFT-2998: Set Content-Type for HTTP request in Node.js...
Posted by tstanev <gi...@git.apache.org>.
Github user tstanev commented on the issue:
https://github.com/apache/thrift/pull/378
By the way -- it's been nearly three years and I have since switched to another way of doing things. You can close the PR if nobody else needs it.
---
[GitHub] thrift issue #378: THRIFT-2998: Set Content-Type for HTTP request in Node.js...
Posted by tstanev <gi...@git.apache.org>.
Github user tstanev commented on the issue:
https://github.com/apache/thrift/pull/378
It's rebased now.
---
[GitHub] thrift issue #378: THRIFT-2998: Set Content-Type for HTTP request in Node.js...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/378
To properly test this fix I need to add http transport support to the nodejs cross programs, so I'll do that as part of the effort.
---
[GitHub] thrift pull request: THRIFT-2998: Set Content-Type for HTTP reques...
Posted by jfarrell <gi...@git.apache.org>.
Github user jfarrell commented on the pull request:
https://github.com/apache/thrift/pull/378#issuecomment-187493321
@RandyAbernethy agree with your suggested approach of committing this and creating a backlog story to tackle the larger IANA registered media type as a follow up
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-2998: Set Content-Type for HTTP reques...
Posted by tstanev <gi...@git.apache.org>.
Github user tstanev commented on the pull request:
https://github.com/apache/thrift/pull/378#issuecomment-108526460
I am only porting the fix from the python wrapper in order to make the node wrapper actually work. I don't know enough about Thrift to have a thought about what it should or should not do instead, but I am certain that with the server I attempted to use this with, only the application/x-thrift header worked. The IANA header did not work, so using the IANA header will not work in my case.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #378: THRIFT-2998: Set Content-Type for HTTP request in Node.js...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/378
@tstanev Thank you for putting in the time. The PR backlog on the project is getting better - there were over 100 PRs at the beginning of the year and we're down to 50, so every little bit helps. Of course today of all days Travis CI is having some pretty nasty issues with Docker Hub. :)
---
[GitHub] thrift pull request: THRIFT-2998: Set Content-Type for HTTP reques...
Posted by RandyAbernethy <gi...@git.apache.org>.
Github user RandyAbernethy commented on the pull request:
https://github.com/apache/thrift/pull/378#issuecomment-108011615
No worse than no header I suppose, however Thrift media types are now per protocol and registered with IANA:
http://www.iana.org/assignments/media-types/media-types.xhtml#application
- vnd.apache.thrift.binary
- vnd.apache.thrift.compact
- vnd.apache.thrift.json
Would be nice if we could mount a Thrift wide effort to get these into Thrift v1.0. If you need application/x-thrift in the mean time I suppose we could commit it with a TODO.
Thoughts?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-2998: Set Content-Type for HTTP reques...
Posted by larsxschneider <gi...@git.apache.org>.
Github user larsxschneider commented on the pull request:
https://github.com/apache/thrift/pull/378#issuecomment-107948285
@RandyAbernethy Looks like a good workaround for now. Any reason not to merge? :smile:
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---