Skip to content
This repository has been archived by the owner on Feb 12, 2022. It is now read-only.

Override of Enums in QueryParams. #112

Open
hadesbox opened this issue Oct 2, 2014 · 4 comments
Open

Override of Enums in QueryParams. #112

hadesbox opened this issue Oct 2, 2014 · 4 comments

Comments

@hadesbox
Copy link

hadesbox commented Oct 2, 2014

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
Copy link
Author

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
Copy link
Author

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
Copy link

@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
Copy link
Author

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 :).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants