Skip to content
This repository has been archived by the owner on Jan 21, 2024. It is now read-only.

Add RAMLVersion option #29

Merged
merged 2 commits into from
Apr 8, 2017
Merged

Conversation

camiloforero
Copy link
Contributor

All the way down at https://github.com/mulesoft-labs/osprey-method-handler/blob/master/osprey-method-handler.js#L422 when the validate function is created, it expects to find the RAML version of the document at options.RAMLVersion. However, this option is never set, returning undefined.
Therefore, later on, here: https://github.com/mulesoft-labs/node-raml-validate/blob/master/raml-validate.js#L361 the raml validator will default to version 0.8.
I was trying to run an mock based on a RAML 1.0 document, and although it mostly worked, it couldn't handle array data types, throwing an error that looked like this:

{
"errors": [
{
"type": "json",
"dataPath": "preferencias",
"keyword": "repeat",
"schema": false,
"data": [
"a",
"b"
],
"message": "invalid json (repeat, false)"
}
],
"stack": "BadRequestError: Request failed to validate against RAML definition\n at createValidationError (/usr/lib/node_modules/osprey-mock-service/node_modules/osprey/node_modules/osprey-method-handler/osprey-method-handler.js:742:14)\n at ospreyJsonBody (/usr/lib/node_modules/osprey-mock-service/node_modules/osprey/node_modules/osprey-method-handler/osprey-method-handler.js:455:21)\n at handle (/usr/lib/node_modules/osprey-mock-service/node_modules/osprey/node_modules/compose-middleware/lib/index.js:56:16)\n at dispatch (/usr/lib/node_modules/osprey-mock-service/node_modules/osprey/node_modules/compose-middleware/lib/index.js:39:20)\n at next (/usr/lib/node_modules/osprey-mock-service/node_modules/osprey/node_modules/compose-middleware/lib/index.js:37:24)\n at /usr/lib/node_modules/osprey-mock-service/node_modules/osprey/node_modules/body-parser/lib/read.js:129:5\n at invokeCallback (/usr/lib/node_modules/osprey-mock-service/node_modules/osprey/node_modules/body-parser/node_modules/raw-body/index.js:262:16)\n at done (/usr/lib/node_modules/osprey-mock-service/node_modules/osprey/node_modules/body-parser/node_modules/raw-body/index.js:251:7)\n at IncomingMessage.onEnd (/usr/lib/node_modules/osprey-mock-service/node_modules/osprey/node_modules/body-parser/node_modules/raw-body/index.js:307:7)\n at emitNone (events.js:67:13)"
}

After this fix, it now correctly handles the document as a RAML 1.0 one, and the mockup works correctly

All the way down at https://github.com/mulesoft-labs/osprey-method-handler/blob/master/osprey-method-handler.js#L422 when the validate function is created, it expects to find the RAML version of the document at options.RAMLVersion. However, this option is never set, returning undefined. 
Therefore, later on, here: https://github.com/mulesoft-labs/node-raml-validate/blob/master/raml-validate.js#L361 the raml validator will default to version 0.8. 
I was trying to run an mock based on a RAML 1.0 document, and although it mostly worked, it couldn't handle array data types, throwing an error that looked like this:

{
  "errors": [
    {
      "type": "json",
      "dataPath": "preferencias",
      "keyword": "repeat",
      "schema": false,
      "data": [
        "a",
        "b"
      ],
      "message": "invalid json (repeat, false)"
    }
  ],
  "stack": "BadRequestError: Request failed to validate against RAML definition\n    at createValidationError (/usr/lib/node_modules/osprey-mock-service/node_modules/osprey/node_modules/osprey-method-handler/osprey-method-handler.js:742:14)\n    at ospreyJsonBody (/usr/lib/node_modules/osprey-mock-service/node_modules/osprey/node_modules/osprey-method-handler/osprey-method-handler.js:455:21)\n    at handle (/usr/lib/node_modules/osprey-mock-service/node_modules/osprey/node_modules/compose-middleware/lib/index.js:56:16)\n    at dispatch (/usr/lib/node_modules/osprey-mock-service/node_modules/osprey/node_modules/compose-middleware/lib/index.js:39:20)\n    at next (/usr/lib/node_modules/osprey-mock-service/node_modules/osprey/node_modules/compose-middleware/lib/index.js:37:24)\n    at /usr/lib/node_modules/osprey-mock-service/node_modules/osprey/node_modules/body-parser/lib/read.js:129:5\n    at invokeCallback (/usr/lib/node_modules/osprey-mock-service/node_modules/osprey/node_modules/body-parser/node_modules/raw-body/index.js:262:16)\n    at done (/usr/lib/node_modules/osprey-mock-service/node_modules/osprey/node_modules/body-parser/node_modules/raw-body/index.js:251:7)\n    at IncomingMessage.onEnd (/usr/lib/node_modules/osprey-mock-service/node_modules/osprey/node_modules/body-parser/node_modules/raw-body/index.js:307:7)\n    at emitNone (events.js:67:13)"
}

After this fix, it now correctly handles the document as a RAML 1.0 one, and the mockup works correctly
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling d5112c5 on camiloforero:patch-1 into ** on mulesoft-labs:master**.

SO it doesn't fail the integration test
@coveralls
Copy link

coveralls commented Mar 15, 2017

Coverage Status

Changes Unknown when pulling ccce605 on camiloforero:patch-1 into ** on mulesoft-labs:master**.

@jstoiko
Copy link
Contributor

jstoiko commented Mar 15, 2017

good call! thanks for catching this and submitting a PR to fix it.

Will review, merge and release. We should probably think of a way to (unit) test this RAML version behavior somehow.

@camiloforero
Copy link
Contributor Author

The simpler one could be to add more RAML 1.0 test cases where the new functionalities, such as data types, are tested. Or arrays; I understand that 0.8 doesn't have them and that's why it broke.

To check specifically for RAML version behaviour... I could not think of a way to do that with the current code, as the error message doesn't say which RAML version was used to do the validation checks.
Adding it would involve adding a "version" attribute to the Validation object here: https://github.com/mulesoft-labs/node-raml-validate/blob/master/raml-validate.js#L138, setting it correctly as "RAML10" or "RAML08" here https://github.com/mulesoft-labs/node-raml-validate/blob/master/raml-validate.js#L281 and here https://github.com/mulesoft-labs/node-raml-validate/blob/master/raml-validate.js#L198 everywhere they call the "toValidationObject" function, and then putting this on the error message at https://github.com/mulesoft-labs/osprey-method-handler/blob/master/osprey-method-handler.js#L788.

Then an unit test would only have to check the error message and see if the "version" attribute has the expected value

@jstoiko
Copy link
Contributor

jstoiko commented Mar 16, 2017

I like the idea of adding the RAMLVersion to the validation errors in raml-validate. We might (arguably) have to increment the major for that though since it would affect the responses.

What other RAML 1.0 tests were you thinking of? There are already quite a bit in raml-validate and osprey-method-handler but maybe you were referring to tests in osprey or osprey-mock-service in which case one option could be to test against an invalid RAML file, one that contains RAML 1.0 features with a RAML 0.8 header, and vice versa. Thoughts?

@camiloforero
Copy link
Contributor Author

Well, checking the osprey-mock-service tests, they only deal with GET requests; I think at least a POST would add a good deal of coverage.
I remember the mock service also wroke when I sent arrays in both GETs and POSTs, so a test for that would be good
I checked out the tests in osprey and they are far more complete there than they are in here. I wonder how much of that code is reusable, perhaps adding some examples to the fixtures there and checking that they are sent correctly by the mock service?

@jstoiko jstoiko merged commit 72390c8 into mulesoft-labs:master Apr 8, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants