Skip to content
This repository has been archived by the owner on Aug 4, 2023. It is now read-only.

Make the router work with controllers named only by operationId #218

Merged
merged 3 commits into from
May 28, 2015

Conversation

bachp
Copy link
Contributor

@bachp bachp commented May 24, 2015

If x-swagger-router-controller is not set the router will try to match
the handler only by operationId. This makes it easier to use swagger-tools
without having to add the non standard field x-swagger-router-controller
to the swagger specification file.

If x-swagger-router-controller is not set the router will try to match
the handler only by operationId. This makes it easier to use swagger-tools
without having to add the non standard field x-swagger-router-controller
to the swagger specification file.
@whitlockjc
Copy link
Member

What's the purpose of this? The idea is that {ControllerName}_{operationId|method} becomes the key to find route handlers and if you remove the need for ControllerName, where do the routes get matched up against? I assume you're just creating your own controllers map yourself and don't see the need for the convention in place?

@bachp
Copy link
Contributor Author

bachp commented May 25, 2015

The routes just match based on the operationid. According to the spec

[operationid] A friendly name for the operation. The id MUST be unique among all operations described in the API. Tools and libraries MAY use the operation id to uniquely identify an operation.

The operationid itself should be sufficient to match the function to be called.
In the case I'm trying to solve I require a module that exports all my handlers as functions and I pass this as the controller option to the router. If I don't set x-swagger-router-controller in my spec file (which I would like to avoid) the handlers don't get called. As it resolves to undefined_{operationId} instead of just {operationId}

Maybe I'm doing somthing wrong?

@vlad-x
Copy link

vlad-x commented May 25, 2015

I stumbled upon this too, handlerName resolves to undefined_{operationId} if the x-swagger-router-controller field is not set in the swagger spec.

This is definitely a bug, please merge this pull request.

@whitlockjc
Copy link
Member

Well, the purpose of using undefined_{operationId} was to force you to set the x-swagger-router-controller. One of the reasons x-swagger-router-controller was nice was because if you set it, you could optionally use operationId as it is not a required property but without x-swagger-router-controller you'd obviously violate the uniqueness constraint across operations as the default controller handler name is the operation's HTTP method.

I'm not against accepting the pull request because simpler naming is never a bad thing. Do you mind adding a unit test for this? Not only is that a requirement anyways but it would also showcase how you intend people to use this. It would also be nice to update the docs in docs/Middleware.md as well but I can handle that if need be.

@bachp
Copy link
Contributor Author

bachp commented May 26, 2015

What I don't understand is why it should be better to force people to add a non standard x-swagger-router-controller instead of telling the users that operationId is required.

In my opinion it makes more sense to require the users to set operationId without a fallback. Because as far as understand the purpose of operationId is to create this relation.

I will try to add a unit test and update the documentation.

@whitlockjc
Copy link
Member

I do not disagree, nor have I been attempting to push back. I just was trying to explain the reasons behind this originally. One thing that has happened with swagger-tools is that most of the features were written for Swagger 1.2 and were then later ported to work with Swagger 2.0 when that spec was finalized/released. We are currently in the process of breaking swagger-tools into smaller, feature specific modules and during this process we are rewriting based on our knowledge now and not what was when we wrote it originally.

In the end, the only thing holding this up is a unit test.

@earth2marsh
Copy link
Contributor

👍

I've been wondering the same thing, glad to see it's not far off.

@gaberoffman
Copy link

I'd love to see any pre-alpha stuff of breaking swagger-tools into smaller, feature specific modules as its current structure is keeping us from using it.

@whitlockjc
Copy link
Member

@gaberoffman Do you mind sharing your opinion on what its current state is causing you grief? File and issue and give me the details.

whitlockjc added a commit that referenced this pull request May 28, 2015
Make the router work with controllers named only by operationId
@whitlockjc whitlockjc merged commit 880293d into apigee-127:master May 28, 2015
@whitlockjc
Copy link
Member

@bachp Thanks for the contribution. When we work on #219 and #221, I will likely need to expand upon this patch a little.

whitlockjc added a commit that referenced this pull request May 28, 2015
@bachp
Copy link
Contributor Author

bachp commented May 28, 2015

@whitlockjc Yes I imagine that, but as an intermediate solution it's goood, as this should not break any existing code while it is still usable with operationId only.

@whitlockjc
Copy link
Member

Agreed. I like what is there now, just saying we might change it so that you don't have to manually build the controllers option and we'd do it based on convention like we do now, but without the x-swagger-router-controller.

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

Successfully merging this pull request may close these issues.

5 participants