apigee-127 / volos

Key Node.js modules for building production-quality APIs
Other
205 stars 63 forks source link

Validation error when responding with cached content #91

Open tanzim opened 8 years ago

tanzim commented 8 years ago

A follow up to #90
I may have spoken too soon. While the caching works as expected, it seems that it doesn't play quite well with swagger response validation. It seems that cached response streamed to the http response [1] with the headers etc. set and when the validator's response end is called, data passed to res.end [2] is undefined. This leads response validation error and an attempt to send headers twice (since the cache already wrote to the response)

Without further debugging I can't say much at this point sadly. I'm on Node 4.2.1 by the way. Seeing that the cache encoder is using the stream module, this could be important.

[1] https://github.com/apigee-127/volos/blob/master/cache/common/lib/cache-encoder.js#L92 [2] https://github.com/apigee-127/swagger-tools/blob/master/middleware/swagger-validator.js#L176

theganyo commented 8 years ago

What version of swagger-node-runner are you on? The newest versions (0.6.4+) no longer attempt to hijack the response pipe and instead just allow you to listen for errors. See this: https://github.com/theganyo/swagger-node-runner/releases/tag/v0.6.4

tanzim commented 8 years ago

Ah, I was on 0.5x. Just updated to 0.6x and it seem that 0.6 introduced a few changes that breaks a few existing things. Working through them currently...

theganyo commented 8 years ago

This should help: https://github.com/theganyo/swagger-node-runner/releases/tag/v0.6.0

tanzim commented 8 years ago

Yup, following that :+1:

tanzim commented 8 years ago

Good and bad news. Good news is 0.6 upgrade went well with a few minor changes. Bad news is, if I try introducing Volos, I see a new error [1]. Specifically, req.swagger.swaggerObject at https://github.com/apigee-127/volos/blob/master/swagger/lib/app-middleware.js#L51 is undefined. I assume the swaggerObject is the parsed swagger definition itself, but couldn't see where it might be initialized :/

pipes exit fitting: express_compatibility +1ms
pipes post-flight fitting: express_compatibility +0ms
pipes:content output (context[output]): undefined +0ms
pipes pre-flight fitting: volosFitting +0ms
pipes input: undefined +0ms
pipes enter fitting: volosFitting +0ms
swagger creating volos chain for: login +0ms
pipes caught error: TypeError: Cannot read property 'x-a127-services' of undefined
  at /Users/t/Programming/hello-world/node_modules/volos-swagger/lib/helpers.js:84:19
  at Array.forEach (native)
  at Object.createResources (/Users/t/Programming/hello-world/node_modules/volos-swagger/lib/helpers.js:83:12)
  at Object.middleware [as app] (/Users/t/Programming/hello-world/node_modules/volos-swagger/lib/app-middleware.js:51:32)
theganyo commented 8 years ago

Hmm. Looks like I perhaps cleaned up too much during the upgrades. I'll have to look into that, but a quick fix should be for you to assign it in your fitting. Just something like this:

context.request.swagger.swaggerObject = swaggerExpress.runner.swagger

tanzim commented 8 years ago

I think the whole https://github.com/apigee-127/volos/blob/master/swagger/lib/app-middleware.js file needs a review. Are there tests? req.swagger.path isn't defined too (I suspect it should be the api path). Anyway, I'll leave you to it for now. happy holidays and thanks for the help.

theganyo commented 8 years ago

I'm pretty sure you're correct. Unfortunately, I won't have time to get into that for a couple weeks, so if you're blocked, you may still need to use a bit of chewing gum to add that field back in for now.

tanzim commented 8 years ago

Any update on this? Thanks!

theganyo commented 8 years ago

@tanzim I'm very sorry, I've been otherwise occupied recently and I haven't had anyone else with these issues. Let's start over... are you using these modules?

https://www.npmjs.com/package/volos-swagger-apply https://www.npmjs.com/package/volos-swagger-oauth

The reason I ask is that I'm fairly certain those are being successfully used as-is today.