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

Error to deal with content-type which has multiple part like 'application/x-www-form-urlencoded; text/html; charset=utf-8' #22

Closed
daiwhea opened this issue Jul 1, 2015 · 14 comments
Milestone

Comments

@daiwhea
Copy link

daiwhea commented Jul 1, 2015

I use koa-better-body to parse the form data for a long time. It goes well. But when I got a form which has 'content-type' header like 'application/x-www-form-urlencoded; text/html; charset=utf-8', id won't parse the form at all.

Thanks!

@daiwhea
Copy link
Author

daiwhea commented Jul 1, 2015

Seems like it's the duty of koa. that.request.is() hasn't return the form type.

@tunnckoCore
Copy link
Member

@daiwhea Hi, very thanks!
New release is coming - working on store-cache, ip-filter and kind-error locally.
But yea, it is koa related issue, especially https://github.com/jshttp/type-is.

Actually.. When I think, you can give custom error types to koa-better-body options, but.. hm. Currently that is not so flexible.
So one way is to set text/html in the opts.extendTypes.form array, other to allow opts.extendTypes to be array and pass it directly to request.is()

I'll mark it as todo

Cheers, 🍺

@tunnckoCore tunnckoCore added this to the v2 milestone Jul 1, 2015
@tunnckoCore
Copy link
Member

Hm. Actually, try

var opts = {
  extendTypes: {
    form: ['x-www-form-urlencoded', 'text/html']
  }
}

I guess it should work, currently.

@daiwhea
Copy link
Author

daiwhea commented Jul 1, 2015

Thank you. I have tried but it doesn't work.

var opts = {
extendTypes: {
form: ['x-www-form-urlencoded', 'text/html']
}
}

Now I use an ugly use() in the top of app.js. But it's not that cute!

app.use(function* (next) {
if (this.request.header['content-type'] == 'application/x-www-form-urlencoded; text/html; charset=utf-8') {
this.request.header['content-type'] = 'application/x-www-form-urlencoded';
}
yield next;
});

@tunnckoCore
Copy link
Member

Hm, interesting. I'll review type-is and will PR there if needed.

@tunnckoCore
Copy link
Member

@daiwhea, would you try these

var opts = {
  extendTypes: {
    form: ['x-www-form-urlencoded', 'html']
  }
}

// and

var opts = {
  extendTypes: {
    form: ['application/x-www-form-urlencoded', 'text/html']
  }
}

I'm curious why it not work. And cant test it at the moment.

ref: koa.request docs I guess, cuz they both should be written in one style.

@tunnckoCore
Copy link
Member

I am also curious why is this header application/x-www-form-urlencoded; text/html; charset=utf-8, how it comes? I mean.. when form is submitted it is urlencoded, not text/html, if i remember?
Maybe an issue is on your side, on app side.

@tunnckoCore
Copy link
Member

Other than that... yea I should rush the process of incoming release, because a lot of things was changed from last updates.

@daiwhea
Copy link
Author

daiwhea commented Jul 2, 2015

Thank you!

Both won't work. This header comes from the greatest alipay notify. Alipay is an online payment api used in China.

In order to test, I used an use() to force the request.header['content-type'] = 'application/x-www-form-urlencoded; text/html; charset=utf-8'; of course it's only for locally testing.

app.use(function* log_raw_request(next) {
this.request.header['content-type'] = 'application/x-www-form-urlencoded; text/html; charset=utf-8';
console.log('log_raw_request was called',this.request.header['content-type'],this.request);
yield next;
});

@tunnckoCore
Copy link
Member

Mm, okey, thanks.

Also, you can use this.set to set the headers in koa.

@daiwhea
Copy link
Author

daiwhea commented Jul 2, 2015

Thanks. Will this.set() just set the koa.response headers? I need to set the request.header.

@tunnckoCore
Copy link
Member

O yea yea.

koa.response headers

this.set sets the response, my mistake.

@daiwhea
Copy link
Author

daiwhea commented Jul 3, 2015

Thank you sir.

I have visited jshttp/type-is#16 , and they have closed that issue. Yes, it's the duty of alipay api. But we cannot change it.

I also viewed your koa-better-body/index.js ["version": "1.0.17"] on line 105. There are 3 judgements. When alipay header comes, there is no judgement will be entered. Can we append the raw body in the last else branch?

if (that.request.is(opts.extendTypes.json)) {
  cache.fields = yield parse.json(that, options);
} else if (that.request.is(opts.extendTypes.form)) {
  options.limit = opts.formLimit;
  cache.fields = yield parse.form(that, options);
} else if (opts.multipart && that.request.is(opts.extendTypes.multipart)) {
  cache = yield parse.multipart(that, opts.formidable);
} else {
  console.log('not parsed');
  // should we add raw body here?
}

@tunnckoCore
Copy link
Member

I think this is resolved in refactor branch. More at #34.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants