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

Override of Enums in QueryParams. #112

Open hadesbox opened 9 years ago

hadesbox commented 9 years ago

So if you define a queryParam inside a trait, and you use this trait for one method but then you override it on the method definition, the enums get added/joined/merged instead of overriden.

Imgur Here is the sample RAML file.

#%RAML 0.8

---
title: Data API
version: 2
documentation:
    - title: Description
      content: the doc
baseUri: https://api.midomain.com/api/v2
traits:
  - dateBounded:
      queryParameters:
        group_by:
          required: true
          displayName: Group by
          description: Time aggregation period
          example: month
          enum: [day, week, month]
          type: string      

/zipcodes/{zipcode}:
  displayName: Zipcodes
  uriParameters:
    zipcode:
      required: true
      displayName: Zipcode
      description: Zipcode to be retrieved
      example: 57500
      type: string 

  /consumption_pattern:
    is: [dateBounded]
    displayName: Consumption pattern by zipcode
    description: This service returns, for an specific zipcode....
    get:
      description: Consumption patterns by time and week/day for a zipcode
      queryParameters:
        group_by:
          required: true
          displayName: Group by
          description: Time aggregation period
          default: month
          example: month
          enum: ["month"]
          type: string

In this example the enum [day, month week] defined in the trait, is not overriden by the enum defined in the method definition which is only month. As you can see in the console screenshot, the enum says [day, week, month, month], and it should be only [month] so the raml parser is merging/joining the enums rather than overriding them.

This is the output JSON of the resources properties, after it gets rendered by the raml-js-parser

[
   {
      "displayName": "Zipcodes",
      "uriParameters": {
         "zipcode": {
            "required": true,
            "displayName": "Zipcode",
            "description": "Zipcode to be retrieved",
            "example": 57500,
            "type": "string"
         }
      },
      "relativeUri": "/zipcodes/{zipcode}",
      "resources": [
         {
            "is": [
               "dateBounded"
            ],
            "displayName": "Consumption pattern by zipcode",
            "description": "This service returns, for an specific zipcode....",
            "relativeUri": "/consumption_pattern",
            "methods": [
               {
                  "queryParameters": {
                     "group_by": {
                        "required": true,
                        "displayName": "Group by",
                        "description": "Time aggregation period",
                        "example": "month",
                        "enum": [
                           "day",
                           "week",
                           "month",
                           "month"
                        ],
                        "type": "string",
                        "default": "month"
                     }
                  },
                  "description": "Consumption patterns by time and week/day for a zipcode",
                  "method": "get",
                  "securedBy": [
                     "basic"
                  ]
               }
            ],
            "relativeUriPathSegments": [
               "consumption_pattern"
            ]
         }
      ],
      "relativeUriPathSegments": [
         "zipcodes",
         "{zipcode}"
      ]
   }
]
hadesbox commented 9 years ago

While this is not a critical Bug, today I tried to troubleshoot it. So far no success BUT... I've discovered that the problem is on the traits.coffee file, more specifically on apply_traits and/or apply_trait.

When a trait is applied to the resource (or method), there is a missing condition somewhere that should check if that particular property is not overwrriten, so the trait its NOT applied.

I would like to ask Norberto what happen if a queryparameter defined on a trait, that later is overwriten in the speification, should all properties be merged or overwritted values prevail.!!!!

hadesbox commented 9 years ago

So the whole enchilada is in src/nodes.coffee

class @MappingNode extends @CollectionNode
  id: 'mapping'

  clone: ->
    properties = []
    for property in @value
      name  = property[0].clone()
      value = property[1].clone()

      properties.push [name, value]

    temp = new @constructor(@tag, properties, @start_mark, @end_mark, @flow_style)
    return temp

The thing as I suspect, is that the clone method should be comparing against the original Mapping Node/property, make a DEEP comparisons of each property, and in the case that a parent property exist with the same name (i.e. queryParam X is defined on method and in trait, header Y is defined in method and in trait), it should NOT be cloned... this will prevent merging of properties in traits and explicit properties in methods.

Sadly I don't know coffee script enough to propose a pull request.

dmartinezg commented 9 years ago

@hadesbox this is a really good catch, and thanks for the analysis.

I am not entirely sure how to fix this either (in terms of the functionality, not the code, that part should be easy)... We need to think about whether there are cases in which we want arrays mixed in, and cases where the array should be entirely overwritten.

If there is a case where we want this dual behaviour (sometimes overwritten and sometimes mixed in), the fix will not be simple at all...

hadesbox commented 9 years ago

I know it wont be simple... I tried to find a way but the deep cloning function its kinda complex to modify...

I would propose that the RAML workgroup defines how and when the overwrite of traits with explicit properties in the services should happen. The parser should NOT do what I "think" or "feel", but what the Spec says.

I mentioned something here so maybe its clarified in 1.0 and then this can be implemented on the js parser :).