Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standalone appearance of nested routes #220

Merged
merged 1 commit into from
Mar 2, 2015

Conversation

croeck
Copy link
Contributor

@croeck croeck commented Feb 25, 2015

Rerences issue #130 and the comment by @antek-drzewiecki. Also includes the README update.

Here is what I did:

  • Build combined_namespace_routes that includes all routes if no nested namespace shall appear as standalone resource. If one shall appear as standalone resource it is not added to the parent_route but will be its own route.
  • Transform the full namespace path to a more URL friendly format by replacing _ with - and / with _ or use a custom url for the route if specified in the swagger_name option.
  • Added a basic spec that tests the exposure for APIs with header and path versioning.

Let me know what you think!

@@ -191,6 +191,27 @@ You can specify a swagger nickname to use instead of the auto generated name by
desc 'Get a full list of pets', nickname: 'getAllPets'
```

## Expose nested namespace as standalone route
Use the `nested = false` option to expose nested actions as standalone resources.
This option can help to struture and keep the swagger schema simple.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: struCture

@dblock
Copy link
Member

dblock commented Feb 26, 2015

By flattening these aren't you documenting routes that don't exist? What am I missing?

@croeck
Copy link
Contributor Author

croeck commented Feb 26, 2015

Hm, I don't really get what you are trying to say ;-)
Perhaps that is also because of the (at least to me) somewhat confusing names (routes, namespaces, resources, ...).

The main goal of my implementation is to realize what was described in issue #130:

The documentation should generate:

store: Operations about store
get      /store/    "lists all stores"

order: Operations about order
get      /store/{storeId}/order/{orderId}
delete /store/{storeId}/order/{orderId}
post    /store/{storeId}/order/

instead of :

store: Operations about store
get      /store/    "lists all stores"
get      /store/{storeId}/order/{orderId}
delete /store/{storeId}/order/{orderId}
post    /store/{storeId}/order/

Therefore I specify the nested: false option on the store/:store_id/order namespace.

From my point of view, all the routes (Grape::Route) did already exist before, however they were seen as operations on a resource (here: the store) instead of being operations of the namespace(-resource) themselves.

The pull request still has some major flaws, especially with nested namespaces, etc.
If you do like this approach I would update the pull request with some fixes and more specs.

(route.instance_variable_get(:@options)[:namespace] == "/#{name}" || route.instance_variable_get(:@options)[:namespace] == "/:version/#{name}")
end.compact

if namespace.options[:nested] == false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unless namespace.options[:nested], or better, switch the order inside the if and do if namespace.options[:nested] ... else

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically you are right, but this would change the default behavior. If I want to keep the current default behavior I can either check for if namespace.options[:nested] == false (current) or if !namespace.options.key?(:nested) || namespace.options[:nested] (opposite).

We could also change the key name to sth. like as_resource: true or standalone: true, but personally I prefer the nested: false and would keep the current if statement.

@dblock
Copy link
Member

dblock commented Mar 1, 2015

Ok, I get this now. I am down with merging this whenever you're ready (and amend with some minor things from my comments above).

I wonder whether we should refactor swagger_name and nested into swagger: { name: ..., nested: ... } as these options will continue piling up? I am worried that tomorrow Grape will decide to use nested for other purposes and we'll have a bigger problem on our hands.

@croeck
Copy link
Contributor Author

croeck commented Mar 1, 2015

I updated the pull request once again. Included changes are:

  • your recommendation to put the options in to the dedicated swagger key

  • handling of more complex nesting cases (see spec for more info), for instance:

    namespace 'store/orders', swagger: { nested: false } do
    post :order_idx
    namespace 'actions' do
    get 'dummy'
    end
    namespace 'actions2', swagger: { nested: false } do
    get 'dummy2'
    namespace 'actions22' do
    get 'dummy22'
    end
    end
    end

@dblock
Copy link
Member

dblock commented Mar 2, 2015

This is great, I am merging.

dblock added a commit that referenced this pull request Mar 2, 2015
Standalone appearance of nested routes
@dblock dblock merged commit a0d446d into ruby-grape:master Mar 2, 2015
@dblock dblock mentioned this pull request Mar 11, 2015
@dblock
Copy link
Member

dblock commented Mar 11, 2015

I ran into a regression/issue with this change, see https://github.com/tim-vandecasteele/grape-swagger/issues/227. Digging through it now, but would appreciate some help.

@croeck
Copy link
Contributor Author

croeck commented Mar 11, 2015

Confirmed, seems to be caused by these changes. I'm looking through the code right now and give an update once I found or resolve the issue.

@dblock
Copy link
Member

dblock commented Mar 11, 2015

I have a fix in https://github.com/tim-vandecasteele/grape-swagger/pull/228, would you please check it out?

@croeck
Copy link
Contributor Author

croeck commented Mar 11, 2015

Seems good and should fix the issue.

@stefan-kolb
Copy link
Contributor

Functionality seems to be broken in 0.20. As far as we can see, the code and documentation of this feature was migrated but the tests aren't.

@dblock
Copy link
Member

dblock commented May 2, 2016

Are you looking at the 0.20 tag for everything? Open an issue?

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

Successfully merging this pull request may close these issues.

3 participants