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

Change forcing POST to optional GET #70

Closed
Orvisky opened this Issue Feb 22, 2016 · 4 comments

Comments

Projects
None yet
3 participants
@Orvisky
Copy link

Orvisky commented Feb 22, 2016

Hi,

i have problem with this line:

/node_modules/jayson/lib/server/middleware.js

//  405 method not allowed if not POST
if(!utils.isMethod(req, 'POST')) return error(405, { 'allow': 'POST' });

Can u make small new version, with something like this ?

if (options.allowGet) {
    if(!utils.isMethod(req, 'POST') && !utils.isMethod(req, 'GET')) return error(405, { 'allow': 'POST or GET' });
} else {
    if(!utils.isMethod(req, 'POST')) return error(405, { 'allow': 'POST' });
}
@tedeh

This comment has been minimized.

Copy link
Owner

tedeh commented Feb 22, 2016

I think that it is contrived to force the request method to be POST, because the middleware is not responsible for any body parsing at all. However, it is not possible to have a request body with GET requests so it would be unclear where Jayson would get the actual request from. The relevant line is https://github.com/tedeh/jayson/blob/master/lib/server/middleware.js#L30 which states that you need req.body to be set because it needs to be used further down.

I might consider dropping the method and content-type requirement in the middleware and just create a setting for specifying where the middleware gets the request from. This would default to req.body like it does now.

@eapearson

This comment has been minimized.

Copy link

eapearson commented Feb 24, 2016

Although the json-rpc spec is transport agnostic, the utility of GET seems limited. The discussions I've seen show lots of confusion about how to implement it. But I assume there must be a good reason to use it sometimes. I wonder how widespread it is?

I don't think it is a good idea to drop the method and content-type requirements as such, but making the range of values accepted be configurable would be great. I use jayson for mocking services, and the services I deal with have their own twisted idea of json-rpc. I sometimes end up having to make local source changes to jayson just to get them to work. So I need jayson to both reflect that interpretation of json-rpc and also sometimes break the rules (although in the latter case I do complain to the service authors, usually to no avail.)

@tedeh

This comment has been minimized.

Copy link
Owner

tedeh commented Feb 26, 2016

I'm generally against cluttering the API with too many configuration options, so I think that the POST requirement will stay as it is. In any case, it is very simple to rig your own middleware that you may impose any requirement you wish on, like I demonstrated in this earlier issue here:

#68 (comment)

Find it hard to imagine anyone is doing JSON-RPC requests within a GET request, but it should be possible to parse/stringify the querystring as a request using a package like qs

@eapearson

This comment has been minimized.

Copy link

eapearson commented Feb 26, 2016

Great solution, thanks.

@tedeh tedeh closed this May 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.