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

Spike on the raml2obj schema #491

Closed
ddenisenko opened this issue Oct 3, 2016 · 15 comments
Closed

Spike on the raml2obj schema #491

ddenisenko opened this issue Oct 3, 2016 · 15 comments
Assignees
Milestone

Comments

@ddenisenko
Copy link
Contributor

It looks like https://github.com/raml2html/raml2obj has a number of improvements to the parser JSON format, we should check it and see if we can add some of those to the next generation of parser output.

@kevinrenskers
Copy link

That'd be great! Make sure to check the develop branch of raml2obj btw.

@sichvoge sichvoge changed the title Consider adding JSON format enhancements to the parser Spike on the RAML parser output schema Oct 4, 2016
@sichvoge sichvoge changed the title Spike on the RAML parser output schema Spike on the raml2obj schema Oct 4, 2016
@KonstantinSviridov KonstantinSviridov self-assigned this Oct 10, 2016
@KonstantinSviridov
Copy link
Contributor

I would apply the following changes:

  1. Switch arrays to maps for 'types', 'traits', 'resourceTypes', 'annotationTypes', 'securitySchemes' because map representation makes output JSON closer to the input RAML file. I would also switch resource.methods to map for the same reason.
  2. Insert resource.parentUrl.
  3. Insert resource.completeRelativeUri ( analogue of resource.uniqueId )
  4. Note that we already insert resource.absoluteUri

On the other hand, there are some transformations that I would not use in general case:

  1. Insert method.allUriParameters and resource.allUriParameters as it may significantly increase output size.
  2. Switch maps to array for 'responses', 'body', 'queryParameters', 'headers', 'properties', 'baseUriParameters', 'annotations' because map representation makes output JSON closer to the input RAML file.
  3. Apply types expansion until we define the algorithm. Note that even with algorithm defined the procedure may significantly increase output size.
  4. Apply the procedure of making examples consistent as it looses some information about RAML specification:
    • whether examples or example has actually been used
    • all properties of ExampleSpec but value i.e. strict, description, name, displayName and annotations.

@sichvoge
Copy link
Contributor

sichvoge commented Nov 9, 2016

@kevinrenskers whats your opinion for the points Konst highlighted around transformation he wouldn't do.

My take:

  1. I think allUriParameters can be easily calculated and doesn't necessarily have to be in the final output (it is also not part of the spec)
  2. We should be working on being close to the RAML input indeed, but also think about how easy it is to traverse through the JSON object. (see issue here). We also discussed that in issue Parser output inconsistencies #453.
  3. We don't need to do that, but we need to provide a consistent output for types as well. Sometimes the type field is an array and sometimes it isn't. Array's have two forms: type: array or Cat[] (this should be fixed by now though). Any form that can be either this or this, should be normalized to only one form, always!
  4. That for me relates to my comment on 3. Consistent does not mean that someone can have examples with value and other facets, or only examples. See example below.
examples:
  hello:
    cat: hello
examples:
  hello:
    value:
      cat: hello

In general, we should always provide the same output regardless if someone uses special shortcuts or syntactic sugar. For example, I should get the same output for these RAMLs:

#%RAML 1.0
title: Traits
baseUri:
  value: http://api.com

types:
  Cat:

/test:
  get:
    body:
      application/json:
        type: object
        properties:
          cats: Cat[]

and

#%RAML 1.0
title: Traits
baseUri: http://api.com

types:
  Cat:

/test:
  get:
    body:
      application/json:
        type: object
        properties:
          cats:
            type: array
            items: Cat

Both are exactly the same but I used different syntactical aspects. The output though should give you a single consistent output. That also encompass types / type vs schemas / schema in 1.0. We should only produce to types / type.

Would love to hear more opinions.

@sichvoge sichvoge reopened this Nov 9, 2016
@kevinrenskers
Copy link

kevinrenskers commented Nov 9, 2016

I agree with you. allUriParameters is purely a raml2html thing, it should not be part of the official parser.

My biggest problems with the parser are indeed the inconsistencies of the output, exactly like you described. Like types and examples. Also, there's examples and example which is just annoying to work with from the clients point of view. Just always make it an array, even if there is only one example. I really don't understand why the RAML spec has both of these in the first place. You say that always outputting examples loses the info of whether examples or example was used in the input RAML file. I wonder why any client rendering API docs would care. I just want consistent usable output without having to deal with so many double ways of doing things.

And like already mentioned plenty times before, just make the output easier to work with. So no wrapped objects inside of arrays and stuff. I would definitely switch maps to arrays as well. The fact that maps are closer to input RAML doesn't really matter to anyone actually working with the output?

I would like the addition of completeRelativeUri, and agree that uniqueId is purely raml2html related and should not be part of the parser output.

Most of all though, any type of type expansion should be handled by the official parser, as it's just much to hard too get this right myself (basically all bug reports for RAML 1 support in raml2html are about type handling, specifically arrays and inheritance). Output size really shouldn't be a concern I think? You can always do it behind a parameter if you don't always want to do it. My bet is that basically everyone will use that flag to auto expand though :)

@kevinrenskers
Copy link

Making stuff like example and examples consistent within raml2obj is not a huge problem btw, it's really the other stuff that is important I think. Like usable output (arrays of objects), all expansion already done. That would remove the need for 90% of raml2obj hacks.

@KonstantinSviridov
Copy link
Contributor

1. I do not have anything to say against allUriParameters disabled by default. My concern here is about chains of resources each having parameters:

/{param1}:
  /{param2}:
    /{param3}:
      /etc

In such cases definition of each parameter is inserted in all the subsequent resources, and thus, JSON size is a polynomial function of maximal resource depth. It does not seem to be unsafe in most cases, but one can easily imagine a critical case.

2. @sichvoge , we were discussing the maps vs arrays choice in September, and we ended with maps finaly (wich is variant "C" here). Thus I do not quite understand the reason of useing maps for 'types', 'traits', 'resourceTypes', 'annotationTypes', 'securitySchemes'; and arrays -- for 'responses', 'body', 'queryParameters', 'headers', 'properties', 'baseUriParameters', 'annotations'.
Maps seem more preferable to me because they provide just a little bit less convenient way of iterating (in comparison with arrays), but they allow by-property access, which arrays do not provide. Thus, my suggestion is to use only maps, unless we have strong reason for some other choice.

3. @sichvoge Right now type can have non array value only in a case as

types:
  T1:
    type:
      properties:
        p1:

when you obtain

"types": [
      {
        "T1": {
          "type": {
            "type": [
              "object"
            ],
            "properties": {
              "p1": {
                "type": [
                  "string"
                ]
            },
          }
        }
      }
    ]

How would we turn type value to array here? Is it just

"types": [
      {
        "T1": {
          "type": [     {
                 "type": [
                   "object"
                 ],
                 "properties": {
                   "p1": {
                     "type": [
                       "string"
                     ]
            }    ],
          }
        }
      }
    ]

?
@kevinrenskers In fact, I see only two questions about type expansion:

  1. What is the main principle: do we just replace type expressions by type definitions for type and items
    (and we shall need some distinguished property for union types which lists the components) or do we append facets of supertypes (including properties facet) to the set of facets of the type itself?
  2. How do we treat cycles in type definitions -- both legal and illegal?

4. Ok, I it seems reasonable to have uniform access to examples disregarding the facet used, but lets not loose specified facets of ExampleSpec (@sichvoge, I'm talking about how it is implemented in raml2obj). So, I suggest always moving the single example value to examples array and making its component satisfy the ExampleSpec schema which is like:

{
    "type": "object",
    "properties": {
      "name": {
        "type": "string",
        "description": "the key used in case of 'examples' facet and null in case of 'example' facet"
      },
      "displayName": { "type": "string" },
      "description": { "type": "string" },
      "annotations": { "type": "object" },
      "strict": { "type": "boolean" },
      "value": {
        "type": "string",
        "description": "string representation of the example"
      },
      "structuredValue": {
        "type": "any",
        "description": "object representation of the example, if possible"
      },
    }
}

@sichvoge
Copy link
Contributor

sichvoge commented Nov 21, 2016

Another improvement: for example, mediaType is either a single string or an array; the output should always be only one of that to keep conistency. I would go for the latter of course.

@KonstantinSviridov
Copy link
Contributor

@sichvoge Agree, let's use array for mediaType

@sichvoge
Copy link
Contributor

Thanks Konst. Do we have any issue or document somewhere that highlights all the changes? If not, what would be the best place? cc/ @ddenisenko

@KonstantinSviridov
Copy link
Contributor

No, I do not have a special place. The changes are enumerated only in the present issue. I guess, we could create an issue or a google doc.

@sichvoge
Copy link
Contributor

sichvoge commented Nov 30, 2016

I'd go for a new issue and update it when ever we've done a decision. Can you start with that and consolidate any changes we discussed so far and bring that into some kind of table format? For example:

Schema Field Current Type Change
mediaType Its either a string or an array Only array

We can extend this table when ever there is a new decision. Please link to the new issue here.

@KonstantinSviridov
Copy link
Contributor

Ok

@KonstantinSviridov
Copy link
Contributor

@sichvoge here it is: #562

@KonstantinSviridov
Copy link
Contributor

Once again about the difference between foramts of Resource.methods and ResourceType.methods.

Actually, I do not like both of them. I would use a map under the methods key:

"methods": {
  "get" : {
  },
  "post": {
   }
}

What do you think, guys?

@ddenisenko ddenisenko added this to the Sprint 6 core milestone Dec 1, 2016
@ddenisenko
Copy link
Contributor Author

I guess this one can be closed. Please reopen if there are still matters to discuss.

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

No branches or pull requests

4 participants