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

Upgrade Sway dependency to 1.0.0 release #52

Merged
merged 4 commits into from
Aug 7, 2016
Merged

Upgrade Sway dependency to 1.0.0 release #52

merged 4 commits into from
Aug 7, 2016

Conversation

jolson88
Copy link
Contributor

@jolson88 jolson88 commented Aug 6, 2016

Based on the discussion in #25, I have grabbed irond13's forked changes and am creating this PR to make it easier to absorb. Based on feedback from @theganyo, I did a minor bump to 0.7.0 just in case. I can pull the version change back out if needed.

This fixes response validation checking for JSON objects which are broken with the current sway dependency. This restores that functionality.

Resolves #25

@coveralls
Copy link

coveralls commented Aug 6, 2016

Coverage Status

Coverage remained the same at 96.954% when pulling ab793fc on jolson88:master into cc89cb2 on theganyo:master.

@coveralls
Copy link

coveralls commented Aug 6, 2016

Coverage Status

Coverage remained the same at 96.954% when pulling efce7b8 on jolson88:master into cc89cb2 on theganyo:master.

@theganyo
Copy link
Collaborator

theganyo commented Aug 6, 2016

@jolson88 Thanks for this! Looks good. Have you had a chance to try it out?

@jolson88
Copy link
Contributor Author

jolson88 commented Aug 6, 2016

I haven’t yet. I couldn’t quite figure out yet how to hook it up into my swagger app. Do I need to hook into the validation response event? I thought I would be able to do it in my app.js, but it reports that on isn’t a function.

I’m sure there’s something silly I’m doing :). I couldn’t find any docs on it and am brand new to swagger-node-runner so code spelunking is going a bit slow right now :).

SwaggerRestify.create(config, function(err, swaggerRestify) {
if (err) {
throw err;
}

swaggerRestify.register(app);
swaggerRestify.runner.on('responseValidationError', function(validationResponse, req, res) {
    console.log('error');
    if (validationResponse.errors.length > 0) {
        res.send(500, { message: 'Server sent an invalid response' });
    }
});

var port = process.env.RECEIPT_SERVICE_PORT || 10000;
app.listen(port);

});

Thoughts? Or is the json_error_handler suppose to just handle it by itself?

@theganyo
Copy link
Collaborator

theganyo commented Aug 6, 2016

Sorry... I'm not sure what you are saying. What isn't a function?

@jolson88
Copy link
Contributor Author

jolson88 commented Aug 6, 2016

I tried adding the following code to subscribe to the responseValidationError on the runner:

SwaggerRestify.create(config, function(err, swaggerRestify) {
if (err) {
throw err;
}

swaggerRestify.register(app);
swaggerRestify.runner.on('responseValidationError', function(validationResponse, req, res) {
    console.log('error');
    if (validationResponse.errors.length > 0) {
        res.send(500, { message: 'Server sent an invalid response' });
    }
});

var port = process.env.RECEIPT_SERVICE_PORT || 10000;
app.listen(port);

});

But when I run my tests after that config, it throws an error of "TypeError: swaggerRestify.runner.on is not a function”

Once again, wouldn’t be surprised if I’m just doing something stupid :).

I did an initial try of just linking in my new swagger-node-runner locally that I just built, but it looks like there are other changes in Sway 1.0.0 that is causing other issues (I’m now getting errors around trying to reference a custom x-* field in my swagger). So I’m wondering if there’s going to be issues in the swagger router as well. I’ll try to keep on digging :).

@jolson88
Copy link
Contributor Author

jolson88 commented Aug 7, 2016

I got it working just fine using addListener instead:

swaggerRestify.runner.addListener('responseValidationError', function(validationResponse, req, res) {
    console.log('ERROR');
    if (validationResponse.errors.length > 0) {
        res.send(500, { message: 'Server sent an invalid response' });
    }
});

I haven’t been able to confirm it working yet. Not sure if the problem is in my Swagger file, or in Sway. But when run with DEBUG=*, it’s reporting there are no errors:

swagger:content response body type: string value: {"foo":1,"bar":2} +0ms
swagger validation result: +1ms { errors: [], warnings: [] }

The endpoint I’m calling expects to return a string and I made it an object to try to get it to fail. But it still passes :P. Based on what Sway is expecting, this appears to not be a problem with swagger-node-runner at this point after the fix in the PR.

I’ll continue to try digging to see if I can get to the bottom of this. Should be some fun spelunking into Sway since I haven’t do that before :).

Thanks again Scott!

@jolson88
Copy link
Contributor Author

jolson88 commented Aug 7, 2016

Confirmed through npm link that this is working for me now and JSON response validation is working again with the PR submitted and linked above.

If @theganyo decides to take this, after publishing a new version up to npm, should swagger-restify-mw also be updated to reflect this latest swagger-node-runner update? If so, there's probably also updating the restify project skeleton in swagger-node: http://github.com/swagger-api/swagger-node/blob/master/project-skeletons/restify/package.json. That project is still pulling in old 0.1.0, instead of newer 0.6.0 (let alone newer one if both node-runner and swagger-node need to be rev'ed).

Thoughts? I think that doing all three should get everything working again from a brand new swagger project create project using Restify as template.

@theganyo theganyo merged commit 56cbdc0 into apigee-127:master Aug 7, 2016
@theganyo
Copy link
Collaborator

theganyo commented Aug 7, 2016

Thanks!

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

Successfully merging this pull request may close these issues.

Response validation of object bodies broken
4 participants