jdiegodcp / ramlfications

Python parser for RAML
https://ramlfications.readthedocs.org
Apache License 2.0
234 stars 50 forks source link

How to inherit resourceTypes by resource? #23

Closed postatum closed 8 years ago

postatum commented 8 years ago

Hi guys. First of all, thanks a lot for all you work. Enjoying your parser a lot so far.

When working with resourceTypes I've faced an issue: How do I make resource inherit from one of resourceTypes properly in ramlfications?

Docs section says that

When a resource has a trait and/or type assigned to it, or a resource type has another resource type or a trait assigned to it, it inherits its properties.

I have following RAML file:

#%RAML 0.8

---
title: Example REST API
documentation:
    - title: Home
      content: Welcome to the example API.
baseUri: http://example.com
version: v1
resourceTypes:
    - collection:
        description: The collection of items
        get:
            description: Get all items, optionally filtered
        post:
            description: Create a new items object

/items:
    type: collection

What I expect after parsing this file is:

But when I parse it and check results in console, that's what I see:

In [1]: import ramlfications

In [2]: api = ramlfications.parse('example.raml')

In [3]: api.resources
Out[3]: [ResourceNode(method=None, path='/items')]

In [4]: resource = api.resources[0]

In [5]: resource.description
Out[5]: ---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
.......
TypeError: __repr__ returned non-string (type NoneType)

I tried defining methods on /items route as follows and was hoping inheritance will have this way but had no luck again.

...
/items:
    type: collection
    get:
    post:

Please give an advice on what is the correct way to achieve what I want using ramlfications?

Thanks for your time! :)

econchick commented 8 years ago

Hey @postatum - so sorry for the delay; was traveling a lot. (also, slight nitpick: guys, there's only one main dev here, and I'm a woman :smiley: )

I think you've stumbled on a bug; I agree that resource.description should inherit the get description defined above. Thanks so much for bringing it to my attention!

postatum commented 8 years ago

Hi @econchick. Thanks for answering.

also, slight nitpick: guys, there's only one main dev here, and I'm a woman

Sorry, didn't mean to offend you :) English isn't my mother tongue and for some reason I thought "guys" refers to group of any people, not just men.

I agree that resource.description should inherit the get description defined above.

resource.description was just an example. The more important issue (or maybe it works like this on purpose) is that if resourceType defines methods "a, b, c" and resource uses that resource type, it feels like there should be generated 3 resources - for methods a, b and c.

econchick commented 8 years ago

I've been rereading the raml spec - If a resource type has three methods, and a resource inherits it like the following:

resourceTypes:
  foo:
    get:
      description: get some foo
    post:
      description: post some foo
/foobar
  type: foo

I would expect it to inherit both get and post and both of their descriptions. However, if it was like this:

resourceTypes:
  foo:
    get?:
      description: get some foo
    post?:
      description: post some foo
/foobar
  type: foo
  get:

I would expect /foobar to only inherit get and its description (notice the ? in the methods of the resourceTypes).

Seems like ramlfications has some wonky behavior when inheriting traits. Looks like I got some work to do!

postatum commented 8 years ago

Hi again @econchick.

Thanks for working on this issue! We would love to use it :)

I've just tried to use version 0.1.8 and I had troubles with it.

Here is my RAML file https://github.com/postatum/ramses-example/blob/99425204_use_resourcetypes/example.raml. When I look at resources generated from it, here is what I see (formatted to make it easier to see things):

[ResourceNode(method='post', path='/users'),
 ResourceNode(method='get', path='/users'),

 ResourceNode(method='get', path='/users/{username}'),

 ResourceNode(method='get', path='/users/{username}/settings'),
 ResourceNode(method='post', path='/users/{username}/settings'),

 ResourceNode(method='get', path='/users/{username}/groups'),
 ResourceNode(method='post', path='/users/{username}/groups'),

 ResourceNode(method='get', path='/users/{username}/profile'),
 ResourceNode(method='post', path='/users/{username}/profile'),
 ResourceNode(method='patch', path='/users/{username}/profile'),

 ResourceNode(method='post', path='/stories'),
 ResourceNode(method='get', path='/stories'),
 ResourceNode(method='delete', path='/stories'),

 ResourceNode(method='get', path='/stories/{id}')]

As you can see, only the first method from resourcetype is inherited.

So I looked in the code here https://github.com/spotify/ramlfications/blob/0.1.8/ramlfications/parser.py#L824-L849 and I think I found few issues (please excuse me if I didn't understood some code correctly):

  1. resourceType methods are only inherited when resource does not define ANY methods, because line 839 is only run if no methods found in resource. Should inherited methods be generated as well as methods defined in resource?
  2. In lines 824-829 only first method of resourceType is inherited, because _resource_type_lookup returns only first resourceType method.
  3. Idea of "inherit only if resource doesn't define any methods" breaks because first inherited method is added before check in line 830. E.g. if resource doesn't define any methods but uses resourceType, first method from that resourceType will be added to methods var and check in line 830 will be True though that check seems to be checking that "resource defines methods".
econchick commented 8 years ago

Hey @postatum - thanks for the update!

I'm working on a bit of a rewrite to clean out the spaghetti that the code has become. And I'll definitely write a few test cases for this to fix it.

How high of a priority/issue is this for you folks? I'll adjust my prioritization if necessary (juggling a lot of things at the moment).

econchick commented 8 years ago

A clarification question: w/r/t your example RAML file, would you expect /stories/{id} to inherit all 5 methods since its parent is of the collection type? What about /users/{username}/?

postatum commented 8 years ago

Hi @econchick.

A clarification question: w/r/t your example RAML file, would you expect /stories/{id} to inherit all 5 methods since its parent is of the collection type? What about /users/{username}/?

I expect /stories/{id} and /users/{username} to inherit all methods from item resourceType. I also expect /stories and /users to inherit all methods from collection resourceType.

PS. @jstoiko will answer your question regarding priorities later.

econchick commented 8 years ago

@postatum I just re-read the RAML spec, and if I understand correctly, I think /stories/{id} and /users/{username} should not inherit resource types. I'm particularly looking at this line under Resource Types and Traits:

Resource type definitions MUST NOT incorporate nested resources; they cannot be used to generate nested resources when they are applied to a resource, and they do not apply to its existing nested resources.

Do you think it should be otherwise?

postatum commented 8 years ago

Here is how I understood it:

Resource type definitions MUST NOT incorporate nested resources;

Since resourceType is similar to resources, users may think they can define nested resources in resourceType and this says - no you can't.

they cannot be used to generate nested resources when they are applied to a resource,

Same as first, but if nested resource is already defined in resourceType, that resource should not be generated.

and they do not apply to its existing nested resources.

If resourceType is applied to "/stories" it will not be applied to "/stories/{id}" and other nested resources.

But in our case, "/stories" and "/stories/{id}" apply their own resourceTypes. "collection" for "/stories" and "item" for "/stories/{id}".

We can try to ask RAML team to interpret that piece of spec.

econchick commented 8 years ago

Ah sure yea - definitely makes sense. I missed that item was applied to the nested resources.

But If it weren't applied, would you agree that the child resource would not inherit its parent's type?

postatum commented 8 years ago

But If it weren't applied, would you agree that the child resource would not inherit its parent's type?

I agree. I think resource should only use resourceType if it's directly applied to it.

econchick commented 8 years ago

awesome thank you :)

jstoiko commented 8 years ago

It's not a blocker however our RAML files will look a lot nicer once this is fixed :)