jdiegodcp / ramlfications

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

Optional properties of resource types applies to differently named properties of resources #44

Closed xaratt closed 9 years ago

xaratt commented 9 years ago

I'm sorry if it's duplicate of 43.

Currently if resource type contains a method and resource with this resource type has another method name, resource will contains not only "own" parameters (e.g. queryParameters), but parameters from this resource type (e.g. formParameters) too.

Part of raml-file (full:

resourceTypes:
  - collection:
      description: A description of the collection resource type
      post?:
        formParameters:
          ids:
            displayName: IDs
            description: A list of IDs
...
/me:
  /widgets:
    displayName: current-user-saved-widgets
    type: collection
    put:
      description: Save Widgets for Current User
      queryParameters:
        ids:
          displayName: Widget IDs
          type: string

And ramlfications code calls:

>>> api = ramlfications.parse("/tmp/optional-bug-example.raml")
>>> api.resources
[ResourceNode(method='get', path='/me'), ResourceNode(method='put', path='/me/widgets')]
>>> api.resources[1].resource_type
ResourceTypeNode(name='collection')
>>> api.resources[1].query_params
[QueryParameter(name='ids')]
>>> api.resources[1].form_params
[FormParameter(name='ids')]

But resource type collection has formParameters only for optional post method, not for put.

And, second bug(?)

>>> api.resources[1].protocols
['HTTP']
>>> api.resources[1].absolute_uri
'https://example.com/v1/me/widgets'

If I have correct understanding of the raml spec, in this case absolute_uri should starts with HTTP, not HTTPS.

Thanks!

econchick commented 9 years ago

Hey thank you! The first one is definitely a bug - certainly related to #43. Traits might also behave the same (improper) way. I am in the middle of working on a patch that will fix this, so expect it in the next release (about a week - would be sooner if there wasn't a long weekend in the U.S. :)). I'll update this issue when it is released.

Re: the second issue - I see in the full RAML file that protocols is specifically defined with HTTP in the put method of the resource, which is the behavior that I'd expect. I'm on mobile so I can't try it myself right now - but does it return HTTPS if that line is removed?

xaratt commented 9 years ago

Thanks for the quick response!

After removing "protocols" from the put method I got results:

>>> api.resources[1].protocols
['HTTPS']
>>> api.resources[1].absolute_uri
'https://example.com/v1/me/widgets'

and after removing protocols from the root-level of raml I got the same result.

econchick commented 9 years ago

@xaratt ah great - so that seems okay then, right?

xaratt commented 9 years ago

I think there is some misunderstanding between us.

I hope that if resource contains section "protocols", absolute_uri should be created using data from this section, not from root's protocols:

Protocols A method can override an API's protocols value for that single method by setting a different value for the fields.

And if resource has no protocols, absolute_uri should use parent protocol.

econchick commented 9 years ago

Ah I misread the first one - I read HTTP instead of HTTPS in the absolute uri.

So yes you're right - I agree that it's the desired behavior and yet another bug :) thanks!

econchick commented 9 years ago

@xaratt - I just merged a PR (#46) that (hopefully) addresses your first concern. It'll be included in the next release on PyPI which may be in a week or so (want to fix some more bugs, including the other one you reported), but if you'd like to confirm that it works for you, I'd really appreciate it.

Thanks!

econchick commented 9 years ago

@xaratt OK hopefully the protocols issue is successfully addressed - I just merged in a fix. Let me know if it does/doesn't work for you.