raml-org / raml-js-parser

(deprecated) A RAML parser based on PyYAML written in CoffeScript and available for use as NodeJs module or in-browser.
195 stars 53 forks source link

Parsing schemas cause replacing reference of schema by schema object #168

Closed JaniszM closed 8 years ago

JaniszM commented 8 years ago

Like in subject, If I have schema in my raml e.g.: - Error: !include error-schema.json and I use it:

        "400": 
            description: Request validation error.
            body: 
                application/json: 
                    schema: Error
                    example: '{"status":1,"description":"sample description","code":1}'

than raml parser replaces schema reference by putting here (in 'schemas' and all 'schema' tags), whole schema object. In that case schemas become useless because are not being used...

dmartinezg commented 8 years ago

Hi @JaniszM, it looks like you want the un-transformed output from the parser, that can be done, simply by asking the parser not to transform the input via a feature flag like this:

https://github.com/raml-org/raml-js-parser/blob/master/test/specs/parser.js#L90

Although in that case, the RAML parser is hardly much better than a YAML parser which follows the !include tag.

plukevdh commented 8 years ago

The Java RAML Parser (v0.8) parses this differently with the result I believe @JaniszM is desiring. Rather than replacing the schema with the definition import, it leaves the schema name in place. Example RAML:

schemas:
  - SessionRequest: !include schemas/requests/session.json

/session:
  post:
    description:
      User Login.
    body:
      application/json:
        schema: SessionRequest
        example: !include examples/requests/session.json

From my Intellij debugger, when read via the Java RAML parser, the result looks like the following:

Note the "Schema" field reads "SessionRequest".

However, when this gets parsed by the JS RAML parser, the following is output (truncated for brevity):

    {
      "relativeUri": "/session",
      "methods": [
        {
          "description": "User Login.",
          "body": {
            "application/x-www-form-urlencoded": {
              "formParameters": {
                "email": {
                  "description": "Email Address.",
                  "type": "string",
                  "displayName": "email"
                },
                "password": {
                  "description": "Password.",
                  "type": "string",
                  "displayName": "password"
                }
              }
            },
            "application/json": {
              "schema": "{\n  \"title\": \"Session Request Schema\",\n  \"type\": \"object\",\n  \"properties\": {\n    \"email\": {\n      \"type\": \"string\"\n    },\n    \"password\": {\n      \"type\": \"string\"\n    }\n  },\n  \"required\": [\"email\", \"password\"]\n}\n",
              "example": "{\n  \"email\": \"test@articulate.com\",\n  \"password\": \"test1234\"\n}\n"
            }
          },
          "protocols": [
            "HTTPS"
          ],
          "method": "post"
        }
      ],
      "relativeUriPathSegments": [
        "session"
      ]
    },

In this case, the schema is dropped in place of the contents of the schema being referred to. In my case this a) duplicates data b) makes it harder to understand which previously defined schema is being referred to.

@JaniszM please correct me if I'm misinterpreting the original issue here. Either way, this is an issue for me. The behavior of the Java parser seems More Correctâ„¢ or at least is the behavior I'd like to see for the reasons stated above.

plukevdh commented 8 years ago

For reference on how the two parsers differ from a test perspective:

https://github.com/raml-org/raml-java-parser/blob/master/src/test/java/org/raml/parser/builder/SchemaBuilderTestCase.java#L73-L74

vs.

https://github.com/raml-org/raml-js-parser/blob/master/test/specs/parser.js#L5723-L5762

plukevdh commented 8 years ago

And really, to fix, all you have to not do is this: https://github.com/raml-org/raml-js-parser/blob/c8edf033a40c33a28161fa67a57fd710e7ddb005/src/composer.coffee#L61

Seems to me we don't want to apply the value of the schema here unless using a separate !include here. Thoughts @dmartinezg?

dmartinezg commented 8 years ago

Hi @plukevdh, you are right that is where we are transforming the output.

As I mentioned above, the way to avoid this transformation is by calling the parser with the flag transform to off.

raml.load(definition, 'filename.yml', {transform: false})
  .then(function(output){
    //do something with output here
  });
JaniszM commented 8 years ago

Sorry for late answer, I was on vacation :) @plukevdh you got it well I think. @dmartinezg I checked flag {transform: false}. Looks this is exactly what I need :) Thanks.

JaniszM commented 8 years ago

Ok, after checking it again I got serious problem. disabling transform makes whole structure of object like this visible in raml file. This mean that raml-object-to-raml lib cannot parse it anymore :( Is it possible any other way to use schemas like in transform disabled mode but still keep the structure of trasformed object?

plukevdh commented 8 years ago

Going to check out {transform: false} to see if it works for my case as well.

plukevdh commented 8 years ago

This does not follow the Java parser convention either as it doesn't perform any of the transformations. It doesn't collect the endpoints (/session) under the endpoints property. This seems like a very unfortunate divergence of the two parsers.

plukevdh commented 8 years ago

@dmartinezg any comment on what we might do to make the two parsers work more similarly?

dmartinezg commented 8 years ago

Hi @plukevdh and @JaniszM, I just pushed a branch, adding a specific flag for this, you can check https://github.com/raml-org/raml-js-parser/commit/4d49f906c7c3ad8dc302e8bea559c4dbfce4225c#diff-51b64dc80fa386af7ffcd496680224fdR124 to see how to disable the behaviour.

would this be enough for you?

plukevdh commented 8 years ago

Looks legit, I need to test though.

dmartinezg commented 8 years ago

I just pushed 0.8.15 to npmjs, it should make testing easier.

plukevdh commented 8 years ago

:+1: works as required. thanks for this solution. I'd be curious if it solves @JaniszM's problem as well.

JaniszM commented 8 years ago

Works perfectly :)