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

Ignore parse the non-json request #2351

Closed
wants to merge 1 commit into from

Conversation

JacksonTian
Copy link

The bodyParser will eat the application/xml type post.

@JacksonTian
Copy link
Author

any maintainer?

@JacksonTian
Copy link
Author

ping

@RWOverdijk
Copy link
Contributor

@loicsaintroch ping.

@loicsaintroch
Copy link
Contributor

Can you write some tests before merging please?

@mikermcneil
Copy link
Member

@JacksonTian hmm.. this code should only being used for sockets-- have you tried it out? The bodyparser (skipper) is where you should look to customize the XML handling behavior.

@JacksonTian
Copy link
Author

Yes, in some sense, I must accept the XML data. In express/connect, the body parser only handle data by the content-type. I will add some test cases for this PR.

@mikermcneil
Copy link
Member

@RWOverdijk @loicsaintroch thanks as always :)

@JacksonTian ok so I've got a lot more context for this PR now after doing a bunch of work on the interpreter for v0.11 as my annual holiday project :p.

tldr;

The short answer is I'm happy to merge this if you wouldn't mind rebasing and adding a test. Here's a recent example of how I've been testing virtual requests-- (this is from the sockets hook, but you can use the same approach in core)

As of the latest code on master, Sails now uses the selfsame session and cookie parsing middleware as HTTP (finally!) This involved me doing some weird, arcane things to the virtual request interpreter. But now we're very much closer to a 100% parity simulation of the request and response streams coming out of the vanilla node HTTP server, since there's things like writeHead(), etc.

So most people aren't actually using a lot of the lower-level features of the virtual request interpreter atm (myself included), but there are a few reasons i spent so much time on it (and wrote so many tests):

  • I want the virtual request interpreter to be a pure implementation which matches Node core's HTTP incomingMessage and serverResponse streams so that sails.request() is all you need for writing tests
  • would be really cool to be able to effortlessly stream to (downloads) and from (uploads) connected socket.io clients, without having to think about them being any different from http clients. Same thing for serving views
  • I want to unify the middleware stack for socket.io / sails.request() / http requests, while still making things configurable.

These goals, particularly the 3rd point (re the middleware stack) are something I talked extensively with @dougwilson about, and also tie in with his efforts in express core to make things more modular (w/ jshttp + pillar).

Anyways, sorry for the long-winded explanation, but it's important to understand the background here. The implementation I'm hoping to get to involves pulling the middleware stack into either core or one or more core hooks. That's a little ways off, and for now, let's just get the solution for you merged (see the tldr above). Just wanted to let everyone know where things are headed.

I'll link to this issue from the roadmap entries on these topics for context.

@CWyrtzen
Copy link

Bumped~

@mikermcneil
Copy link
Member

@JacksonTian I'm going to close this for now- please feel free to open a new PR with a test if you still need this and I'll take a look:

The short answer is I'm happy to merge this if you wouldn't mind rebasing and adding a test. Here's a recent example of how I've been testing virtual requests-- (this is from the sockets hook, but you can use the same approach in core)

See my comment above for context about the long-term roadmap item which underlies this PR.

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

Successfully merging this pull request may close these issues.

5 participants