raml-org / raml-spec

RAML Specification
http://raml.org
3.87k stars 858 forks source link

The "parameters" property in RAML 1.0 annotation type declarations is flawed #196

Closed oberlies closed 8 years ago

oberlies commented 9 years ago

In annotations one may want to have something to the right of the colon:

(myAnnotation): something

The way to specify the format of that "something" is through a parameters property in the annotation type declaration. According to the spec, that property takes a map of parameter names to types, so e.g.

annotationTypes:
  myAnnotation:
    parameters:
      p1: string
      p2: string

then allows annotations of the following format:

(myAnnotation):
  p1: my value of p1
  p2: my value of p2

But then the "something" could never be a basic type. The examples and the current parser implementation seem to imply that there is a hack for this: to use the parameter name value. So

annotationTypes:
  myAnnotation:
    parameters:
      value: string

allows the following annotation:

(myAnnotation): my value

This is IMHO a bad design, that could be fixed by replacing the parameters property with a value property. This property would expect a type reference or inline type as value so that the examples above would look like this:

annotationTypes:
  mySimpleAnnotation:
    value: string
  myComplexAnnotation:
    value:
      type: object
      properties:
        p1: string
        p2: string
petrochenko-pavel-a commented 9 years ago

This is how it is done in Java (from which a lot of ideas was borrowed from) but I like suggestion pretty much.

Regards, Pavel

usarid commented 9 years ago

This seems like a great simplification with no downside.

petrochenko-pavel-a commented 9 years ago

agree.

ddenisenko commented 9 years ago

Wait guys.

Am I getting this right, that we're going to simplify things in one area, changing current

  myComplexAnnotation:
    parameters:
      value:
        type: object
        properties:
          p1: string
          p2: string

to

  myComplexAnnotation:
    value:
      type: object
      properties:
        p1: string
        p2: string

and at the same time we make things more difficult by changing current

annotationTypes:
  myAnnotation:
    parameters:
      p1: string
      p2: string

to

annotationTypes:
  myAnnotation:
    value:
      type: object
      properties:
        p1: string
        p2: string

Is that correct?

oberlies commented 9 years ago

@ddenisenko: Yes, this is the proposal.

But the main benefit of the proposed change is that it removes the hack with the special property key "value". This hack leads to slack in the annotation type declaration. What kind annotation would you expect from this declaration:

annotationTypes:
  myAnnotation:
    parameters:
      value: string

Would you expect

(myAnnoation): foo

or

(myAnnotation):
  value: foo

AFAIK, the workbench currently tolerates both, but IMHO this ambiguity is bad.


And the implementation seems currently buggy. Try to get the workbench to allow an annotation with a string array as parameter. If the hack was followed consistently, the declaration would be

annotationTypes:
  myAnnotation:
    parameters:
      value: string[]

But it is not. I couldn't find a way to get an annotation (myAnnotation): [foo, bar] accepted by the parser.

So, apparently the special key "value" was even confusing the parser implementers. So just remove it and go for the standard type syntax instead.

sichvoge commented 9 years ago

Indeed, we should agree an a consistent format. TBH, looking at through the different proposals I would stick to parameters and to

(myAnnotation):
  value: foo

and avoid:

(myAnnotation): foo

I know it's a simplification, but having the keyword parameters is much clearer to me what that is than value.

ddenisenko commented 9 years ago

@sichvoge I would certainly not recommend removing

(myAnnotation): foo

format. This is going to be the most common use case for annotations.

ddenisenko commented 9 years ago

@oberlies I do agree that what you propose is more consistent and less "hack" then what we currently have. What we need to understand is: 1) Does the change make code more compact in general? 2) Does this change make the common use cases more compact or otherwise?

oberlies commented 9 years ago

I know it's a simplification, but having the keyword parameters is much clearer to me what that is than value.

value is not set in stone. It could also be called parameter - and of course the parameter could be an object (i.e. { type: object, properties: {...}}). Only the plural-s is the problem.

sichvoge commented 9 years ago

I think where I want to go is exactly that. The plural is the problem as you can have multiple parameters and not a single object. For example I could have multiple objects that describe a certain behaviour for some kind of testing tools. Being the first object information about the API, the second about the HTTP method, and maybe a third to describe some test patterns.

oberlies commented 8 years ago

@sichvoge Your use case can be easily modeled with a (single) parameter object with multiple properties. There is no need to allow multiple parameters to support your use case.

So multiple parameters

sichvoge commented 8 years ago

That does not really sound right for me as they are three independent objects that have different purposes and should not combined into a single object.

What do you mean by your last two points?

oberlies commented 8 years ago

@sichvoge What is the difference between an annotation with one unnamed parameter with an object value and an annotation multiple parameters? In YAML, they look exactly the same:

(oneObjectParam):
  myProperty1: foo
  myProperty2: bar
(mulitipleParams):
  myParam1: foo
  myParam2: bar

Right now, there are two different annotation specification syntaxes for these annotations, although the annotations are used in exactly the same way:

annotationTypes:
  oneObjectParam:
    parameters:
      value:
        type: object
        properties:
          myProperty1: string
          myProperty1: string
  multipleParams:
    parameters:
      myProperty1: string
      myProperty1: string

This ambiguity is confusing. There should only be a single annotation specification syntax these annotations:

annotationTypes:
  oneObjectParam:
    parameter:
      type: object
      properties:
        myProperty1: string
        myProperty1: string
...
ddenisenko commented 8 years ago

As I said already, I generally do not mind accepting the proposal of @oberlies , it is more consistent than our current approach. The only question is if/when we have time to support that in parsers and API Workbench.

petrochenko-pavel-a commented 8 years ago

@ddenisenko generally it is more consistent, but it is very not coherent with usual syntax for annotations in languages like Java. Also I really like word parameters as very descriptive. Regards, Pavel

usarid commented 8 years ago

@petrochenko-pavel-a when you say "in languages like Java" do you actually mean "in Java" or even "only in Java"? ;-) I don't see that sort of behavior in C# attributes or in Python decorators.

sichvoge commented 8 years ago

depends on how you look at it right, you can see a single class that defines an annotation or attribute in programming as the single object (value/parameter) that has multiple properties.

For example that C# class

[System.AttributeUsage(System.AttributeTargets.Class |
                       System.AttributeTargets.Struct)
]
public class Author : System.Attribute
{
    private string name;
    public double version;

    public Author(string name)
    {
        this.name = name;
        version = 1.0;
    }
}

would translate into

annotationTypes:
  Author:
    parameters:
      name: string
      version: number

where the outcome or how you use the annotation is the same as you would define it like the following:

annotationTypes:
  Author:
    parameters:
      value:
        type: object
        properties:
          name: string
          version: number

I think if we shorten it to the code below and only use that way of a notation, the parameter property is a little bit misleading. It feels strange to define a parameter for Author with multiple properties as the one who is defining it. I definitely prefer the first as this is cleaner to me.

annotationTypes:
  Author:
    parameter:
        type: object
        properties:
          name: string
          version: number
usarid commented 8 years ago

But note that C# has no magic value -- it's just based on positional and named parameters.

Anyway, it seems we have 2 options proposed so far:

Current spec:

#%RAML 1.0
title: Test simplifying annotation suggestions
types:
  url:
    pattern: "^http(s?)://"
  testConditions:
    properties:
      []: url
annotationTypes:
  deprecated:
    description: Valueless indication that the target is deprecated
  internalName:
    description: String-valued internal name
    parameters:
      value: string # This could be omitted as it's the default type
  iconPath:
    description: The path to an icon file
    parameters:
      value: url
  registered:
    description: Something listed in the registry
    parameters:
      path: url
      status:
        enum: [ draft, final ]
  testHarness:
    description: Metadata for testing
    parameters:
      preconditions: testConditions
      postconditions: testConditions

Oberlies proposal:

#%RAML 1.0
title: Test simplifying annotation suggestions
types:
  url:
    pattern: "^http(s?)://"
  testConditions:
    properties:
      []: url
annotationTypes:
  deprecated:
    description: Valueless indication that the target is deprecated
  internalName:
    description: String-valued internal name
    value: string # This could be omitted as it's the default type
  iconPath:
    description: The path to an icon file
    value: url
  registered:
    description: Something listed in the registry
    value:
      type: object # This could be omitted as it's implied by `properties`
      properties:
        path: url
        status:
          enum: [ draft, final ]
  testHarness:
    description: Metadata for testing
    value:
      properties:
        preconditions: testConditions
        postconditions: testConditions

I'm thinking we could take it one step further and simply treat annotation types as extending data types. That would lead to a 3rd proposal:

Data type extension proposal:

#%RAML 1.0
title: Test simplifying annotation suggestions
types:
  url:
    pattern: "^http(s?)://"
  testConditions:
    properties:
      []: url
annotationTypes:
  deprecated:
    description: Valueless indication that the target is deprecated
  internalName:
    description: String-valued internal name
    type: string # This could be omitted as it's the default type
  iconPath:
    description: The path to an icon file
    type: url
  registered:
    description: Something listed in the registry
    type: object # This could be omitted as it's implied by `properties`
    properties:
      path: url
      status:
        enum: [ draft, final ]
  testHarness:
    description: Metadata for testing
    properties:
      preconditions: testConditions
      postconditions: testConditions

No magic values; no parameter vs parameters; these are just like data type definitions but with a few extra properties.

Can anyone see any ambiguities or confusion this would introduce?

sichvoge commented 8 years ago

That aligns to what I thought as well. As you defining objects/types that are basically just annotations. You are doing the same in Java and C#. You define a class (type) and add extra properties to declare them as annotations/attributes.

usarid commented 8 years ago

@oberlies @ddenisenko @petrochenko-pavel-a @aldonline let us know what you think too -- and anyone else please chime in.

ddossot commented 8 years ago

:+1: for the data type extension proposal, I don't see any confusion with other types and the unification is great. Elegance is oftentimes a trait of good design :wink:

petrochenko-pavel-a commented 8 years ago

+1 for data type extension proposal to.

ddenisenko commented 8 years ago

Last proposal looks great for me as long as we clearly say that annotations are not types and just use types syntax for convenience.

Annotations may be actually types inside the implementation (API Workbench) to reuse type handling code, but should be officially differentiated from types in the spec. In example, types can not extend annotations, types can not have properties of annotation type etc etc.

Also this produces a number of questions. In example, can annotation be of another annotation type (extend other annotation)?

usarid commented 8 years ago

Annotation types are not types, they have the same syntax as types but with extra facets. So types are not affected at all by this proposal: a property within a type must itself be a type, not an annotation type, etc. And as for inheritance of annotation types (good question!): while I'm sure there could be some value, there would also be confusion, so the answer is no: annotation types have no notion of inheritance amongst themselves. In fact, the easiest way to think of them is just as types that prescribe the metadata that can be added in various places in a RAML API definition, and have a few extra facets added to their top-level declarations to allow that, such as where they can be used (allowedTargets).

oberlies commented 8 years ago

+1 for the data type extension proposal

justjacksonn commented 8 years ago

Funny @usarid when I was reading through all the proposals I was thinking why do we need value: and parameters: why not just have type: and your proposal is spot on with what I was thinking. +1 from me as well.

usarid commented 8 years ago

Thanks guys. We'll "make it so".