@ffejneslen's patch supporting custom JSON and XML request bodies #66

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
6 participants

Hi there,
This is an attempt to integrate the patch discussed in issue #51 - done by ffejneslen.

The line breaks were wrong in some of their code, so I converted them and merged as cleanly as I could.

The resulting code works for all the demo APIs apart from the one ffejneslen added to demonstrate custom request bodies ('/eventnotification').

The error it gives is:

Express
500 TypeError: /Users/graeme/git/iodocs-graeme/views/api.jade:1 > 1| h1=apiInfo.name 2| - if (session.authed && apiInfo.oauth && apiInfo.oauth.type =='three-legged') 3| - var authed ='authed' 4| - else Cannot read property 'name' of undefined
> 1| h1=apiInfo.name
2| - if (session.authed && apiInfo.oauth && apiInfo.oauth.type =='three-legged')
3| - var authed ='authed'
4| - else
Cannot read property 'name' of undefined
at Object.eval (eval at (/Users/graeme/git/iodocs-graeme/node_modules/jade/lib/jade.js:226:10))
at ServerResponse.res._render (/Users/graeme/git/iodocs-graeme/node_modules/express/lib/view.js:425:21)
at ServerResponse.res.render (/Users/graeme/git/iodocs-graeme/node_modules/express/lib/view.js:318:17)
at /Users/graeme/git/iodocs-graeme/app.js:800:9
at callbacks (/Users/graeme/git/iodocs-graeme/node_modules/express/lib/router/index.js:272:11)
at param (/Users/graeme/git/iodocs-graeme/node_modules/express/lib/router/index.js:246:11)
at param (/Users/graeme/git/iodocs-graeme/node_modules/express/lib/router/index.js:243:11)
at pass (/Users/graeme/git/iodocs-graeme/node_modules/express/lib/router/index.js:253:5)
at Router._dispatch (/Users/graeme/git/iodocs-graeme/node_modules/express/lib/router/index.js:280:5)
at Object.middleware [as handle] (/Users/graeme/git/iodocs-graeme/node_modules/express/lib/router/index.js:45:10)

I suspect I may have just made a merging error, but it anyone could point out my mistake that'd be most useful.

ffejneslen and others added some commits Nov 17, 2012

Graeme West
An attempt to merge in @ffejneslen 's fork, which supports JSON & XML…
… request bodies. The line endings on some files were wrong, so I had to convert them. I may not have merged app.js properly but we'll see…

Merge branch 'ffejnelsen-current-state'

Conflicts:
	app.js

So actually, this isn't as broken as it looked - I had just failed to set up my API config.

Looks like there are still a few gremlins (see issue #51 ), but more eyes on the problem would be great :)

Branch now working. I also cleaned up the line breaks (etc.) in Jeff's prior work to make merging easy.

Applies cleanly against master.

Contributor

mansilladev commented Jan 23, 2013

Graeme, will test today. Thanks!

thanks for your efforts here :-)

Contributor

mansilladev commented Jan 24, 2013

I've just tested the fork on Safari, Firefox and Chrome (on Mac)-- when I click "Add Header" or "Remove", nothing happens. Can someone else verify?

I got that too, but my custom headers still seemed to actually become part of the request.

I would be curious if the header you tried to add ended up in the request anyway -- I seem to recall that is how the code was working when I originally worked on it.. you could set a single header on the request... but I needed more hence the ability to configure more..

internally, there is a 'headers' array value that is getting set with the header sent in...

all the headers that are configured in are assigned to numbered members of that array.

I should clarify that although the first custom header added becomes part of the request, it doesn't seem to be possible to add multiple headers in this version - as @mansilladev points out, the 'Add Headers' and 'Remove' buttons are non-functional.

Any exciting news here? :)

app.js
- if(options.headers === void 0){
- options.headers = {}
- }
+
if (!options.headers['Content-Length']) {
if (requestBody) {
options.headers['Content-Length'] = requestBody.length;
@deflexor

deflexor Feb 16, 2013

When requestBody conatins multibyte chars, then it seems here we getting invalid Content-Length value, because requestBody.length returns number of chars, not octets as HTTP protocol requires. Maybe Buffer.byteLength(requestBody, 'utf8') would fit here ?

@capncodewash

capncodewash Feb 25, 2013

Yep, makes sense. Thanks.
Added in 2a9737d .

Now returning octet length instead of character length, at @deflexor's suggestion.

Anyone else want to test & integrate this? Keen to see it merged into mashery's version.

Merged in latest master.

Very nice to see improvement on what was done before ... thanks for the efforts!

Contributor

phairow commented Oct 9, 2014

closing this although it is a great feature and we should add the functionality to have structured xml. this example is certainly helpful.

a few comments on this pull request though.

  1. it should not contain personal api's so don't commit those.
  2. there are other changes outside of the actual feature request.
  3. at this point, we waited too long and there were additional features that this would conflict with.

it probably make sense for this to be implemented by leveraging the json schema forms which were recently introduced.

@phairow phairow closed this Oct 9, 2014

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