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

context.is or request.is return null #440

Closed
zppro opened this issue May 5, 2015 · 8 comments
Closed

context.is or request.is return null #440

zppro opened this issue May 5, 2015 · 8 comments

Comments

@zppro
Copy link

zppro commented May 5, 2015

nodejs v0.12.2
koa v0.20.0

app.use(function *(next){
    if(this.request.is('application/json')||this.is('json')){
        // Query
        var builder = sqlBuilder();


        var request = builder().request(); // or: var request = connection.request();

        yield request.query('select * from Pub_User', function(err, recordset) {
            // ... error checks


            //console.log('check user '+JSON.stringify(recordset));
            console.log('get recordset');
            _self.message = 'hello';

        });
        console.log('check user');
    }
    yield next;
});

so , if(this.request.is('application/json')||this.is('json')){
is not work ,it return null
but in fidder the request header is

GET http://192.168.1.165:3000/ HTTP/1.1
User-Agent: Fiddler
Content-Type: application/json
Host: 192.168.1.165:3000

why?

@tj
Copy link
Member

tj commented May 5, 2015

looks ok to me if that really is the request at the bottom there, can you reproduce with a small script or in a test?

@dougwilson
Copy link
Contributor

Your request actually needs body content. Having body content would mean you should have a Transfer-Encoding header or a Content-Length header, which you are missing.

If you are not trying to parse a body, you should instead use the Accept request header and this.request.accepts('json').

@zppro
Copy link
Author

zppro commented May 5, 2015

this.request.accepts('json') works!
and add content-length:0 also works!!

@zppro zppro closed this as completed May 5, 2015
@tj
Copy link
Member

tj commented May 5, 2015

oh yea hahaha good call, missed that

@yanickrochon
Copy link
Contributor

I have this problem with koa@1.1.2.

The request is sent via jQuery : $.getJSON('/url/');

and this.is('text'); and this.request.is('text'); always returns null. Also, this.accepts('html'); returns html.

The request looks like this (note: the actual response is a 302 error page, as this.status = 302; is set, since the validation is wrong and a text/json request is assumed to be text/html)

selection_038

@dougwilson
Copy link
Contributor

Hi @yanickrochon there is nothing with those response, as both are correct:

  1. .is('text') is null because your request is a GET request; it has no body, so the request is nothing.
  2. .accepts('html') returns html because yes, the browser accepts html/text, as the Accept header includes */*, which means it'll accept anything.

@yanickrochon
Copy link
Contributor

@dougwilson hmm... alright for this.is(...), except that it still does not honor the documentation, where it should return undefined, not null (docs should be updated in any case), and I don't understand why a body is required since content-type is sent anyhow?

As for this.accepts(...), there should be a strict option that ignores wildcard and return only explicit accepted values. Something like this.accepts('html', true);. This would make things more easier to test as I don't want to manually test all other content types, simply because html can match */* in every cases, even if totally irrelevant.

@dougwilson
Copy link
Contributor

except that it still does not honor the documentation, where it should return undefined, not null (docs should be updated in any case)

Make a documentation pull request :) You never mentioned anything about the doc irregularity in your original post, sorry.

and I don't understand why a body is required since content-type is sent anyhow?

You have no Content-Type on the request in your example, only in the response. You are calling .is on request, not response, thus the method is explaining to you properties of the request (this.is is just a shortcut to this.request.is)

As for this.accepts(...), there should be a strict option that ignores wildcard and return only explicit accepted values. Something like this.accepts('html', true);. This would make things more easier to test as I don't want to manually test all other content types, simply because html can match / in every cases, even if totally irrelevant.

Please open a new issue with your proposal for discussion, and it can certainly be discussed. I didn't see you make any mention in your original post regarding this proposal, sorry. SInce you were posted in an existing issue, I could only assume you were posting something related to the original issue in this thread and not something off-topic, I'm sorry.

Please create new issues for the various things.

@koajs koajs locked and limited conversation to collaborators Dec 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants