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

Validation error when responding with cached content #91

Open
tanzim opened this issue Dec 23, 2015 · 10 comments
Open

Validation error when responding with cached content #91

tanzim opened this issue Dec 23, 2015 · 10 comments

Comments

@tanzim
Copy link

tanzim commented Dec 23, 2015

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
Copy link
Contributor

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
Copy link
Author

tanzim commented Dec 23, 2015

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
Copy link
Contributor

@tanzim
Copy link
Author

tanzim commented Dec 23, 2015

Yup, following that 👍

@tanzim
Copy link
Author

tanzim commented Dec 23, 2015

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
Copy link
Contributor

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
Copy link
Author

tanzim commented Dec 23, 2015

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
Copy link
Contributor

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
Copy link
Author

tanzim commented Mar 16, 2016

Any update on this? Thanks!

@theganyo
Copy link
Contributor

@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.

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