Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Response validation of object bodies broken #25

Closed
krivin opened this issue Dec 12, 2015 · 14 comments · Fixed by #52
Closed

Response validation of object bodies broken #25

krivin opened this issue Dec 12, 2015 · 14 comments · Fixed by #52

Comments

@krivin
Copy link

krivin commented Dec 12, 2015

76f5051 seems to have broken response validation, since v. 0.6.0 of Sway can no longer validate JSON encoded strings as objects, see apigee-127/sway#48.

@theganyo
Copy link
Collaborator

Thanks. This should be addressed automatically when @whitlockjc updates Sway.

@whitlockjc
Copy link
Member

This has been fixed in Sway but it will not be released immediately. I'm hoping to cut sway@v1.0.0 within the next few days and it will be included in that.

@irond13
Copy link
Contributor

irond13 commented Feb 16, 2016

I've just tried manually forcing the sway dependency to sway#master, however swagger-node-runner is no longer compatible with, specifically as it relates to validating responses. This is due to breaking change apigee-127/sway@52d4c43. The relevant swagger-node-runner line is https://github.com/theganyo/swagger-node-runner/blob/master/lib/connect_middleware.js#L163.

What is the plan for 1.0.0 regarding swagger-node-runner and sway? Are they going to be bumped together? Or is some of this going to be handled by the new sway-connect project? As it stands, there doesn't seem to be any way to do response validation, barring going back to swagger-node-runner 0.5.x.

@irond13
Copy link
Contributor

irond13 commented Feb 16, 2016

FWIW, I've created a fork which makes swagger-node-runner compatible with sway@master - irond13@e837376. I can submit a pull request if need be.

@theganyo
Copy link
Collaborator

Yes, it's unfortunate that Sway 1.0 broke compatibility, but since you've worked it out already, your PR is certainly welcome!

No, there is no plan to incorporate Sway directly into this project nor to use sway-connect. But perhaps I just don't understand why you are suggesting that either of those would be good options?

@whitlockjc
Copy link
Member

I didn't break things on a whim but for consistency, and I didn't take it lightly. Not only that but I was up front that the API can and will change up to 1.0.0 which is completely common 0.x.x since even semver treats these releases as "for initial development. Anything may change at any time. The public API should not be considered stable." So while it is unfortunate, it was necessary. I will help however I can with compatibility and will be releasing 1.0.0 shortly.

@theganyo
Copy link
Collaborator

@whitlockjc I wasn't blaming you for your choice to break compatibility, but if it seemed that way to you, I apologize.

@irond13
Copy link
Contributor

irond13 commented Feb 16, 2016

@theganyo my comments regarding swagger-node-runner/sway and 1.0.0 were more around whether you plan on bumping the sway dep to 1.0.x once it is released. As it stands, using the latest published minor range of swagger-node-runner (0.6.x), response validation of JSON objects is broken when using Express. It seems that there is no workaround other than using the latest sway version (or forking).

Regarding the PR, I'll submit it as soon as sway 1.0.0 is published (at the moment, my fork has a sway dep. of sway#master). Do note that there are other breaking changes as well though, besides the response validation stuff mentioned above. There were also test failures for the mock and sample response features. I got all the tests passing again by making changes to both the tests and the swagger_router fitting (see irond13@e837376).

Since this would also result in breaking compatibility for swagger-node-runner clients, this would mean swagger-node-runner would need at least a minor bump (arguably a major bump would be more appropriate). Hence why I asked whether swagger-node-runner would be matching the sway version bump :)

@whitlockjc That makes perfect sense (the major bump definitely seems appropriate here). I'm just looking for the best way to get response validation working. Are there any outstanding pieces of work that are needed before sway 1.0.0 gets released?

@theganyo
Copy link
Collaborator

Gotcha. Most likely this will be a minor bump to 0.7.0, but nothing has been decided yet.

@whitlockjc
Copy link
Member

@theganyo, not at all. Just providing context to those that don't work with me. :-)

@krivin
Copy link
Author

krivin commented Aug 3, 2016

@theganyo: Any news on when you're upgrading to sway@v1.0.0?

@jolson88
Copy link
Contributor

jolson88 commented Aug 6, 2016

@theganyo I'm also curious if there is a plan for this.

@irond13 Can you submit a PR for the changes you made? I suppose I can fork your changes and submit the PR if I need to.

It would great to get the ball rolling on this with how long sway 1.0 has been out. I'm trying to use this library at work and I know I'd really like for the response validation on JSON objects to work so I can ensure my responses from my API server stay in-sync with my Swagger doc.

I can try helping out any way that helps :).

@jolson88
Copy link
Contributor

jolson88 commented Aug 6, 2016

Okay, I went ahead and forked, cherry-picked @irond13 changes, and submitted a PR. This upgrades to sway 1.0.0.

Looks like some CI failures occurred for node 0.12 and 0.10. I'll try to get those patched up as well.

@jolson88
Copy link
Contributor

jolson88 commented Aug 6, 2016

Okay, all checks are passing for v0.12 and v0.10 now.

So I suppose it's ready for you to take a look if you wish @theganyo

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

Successfully merging a pull request may close this issue.

5 participants