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

Strip slashes from definitionKey #1742

Closed
wants to merge 1 commit into from
Closed

Strip slashes from definitionKey #1742

wants to merge 1 commit into from

Conversation

silverbackdan
Copy link
Contributor

To allow shortNames with slashes and avoid swagger ui resolver error

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a

I'd like to update some resource's short names to include a prefix - e.g. component/hero instead of just hero.

It seemed the simplest way I found in previous issues was to change the short name. It works well except in the swagger ui which results in a Resolver error where it cannot resolve the reference if it has another slash in it.

The easiest fix I could think would be to strip the slashes as I've done in this PR.

Alternatively I'd look to tag each resource to group them together is they are an instance on AbstractComponent in my case. But tagging doesn't seem so easy to do dynamically at the moment, even with the DocumentationNormalizer.

Happy for feedback if there's a better solution though. I'd personally quite like a 'route_prefix' option instead but I'm not sure how useful it'd be for others.

To allow shortNames with slashes and avoid swagger ui resolver error
@silverbackdan
Copy link
Contributor Author

Not sure this is ideal really - I also had custom routes/controllers setup for one of my resources and there was an error because that custom route wasn't using the same short name as I had defined on the entity.

The documentation normalizer isn't really that flexible either. The object passed in is just an array of the classes and the docs outputted never reference the full class names so I'm not sure there's a very good way of adding documentation based on the resource/entity class for a given path or definition. This makes it tricky to dynamically add tags for the docs.

I'm not sure I like the hack of using the short name to prefix the route path either. The real problem I was having was to group the resources in the Swagger UI nicely which should really be done with tags as far as I can see.

Would you consider a PR to add a tags attribute to the ApiResource annotation? Happy to work on one if it wouldn't be a waste of time. Cheers.

@Simperfit
Copy link
Contributor

I guess the tag feature will be better, could you take care of this ? I'm closing this one, it seems not the right fix to me too.

@Simperfit Simperfit closed this Apr 6, 2018
@silverbackdan silverbackdan deleted the patch-1 branch April 6, 2018 14:06
@silverbackdan
Copy link
Contributor Author

@Simperfit Will do, I'll look at this PR as well as the enable_max_depth improvement as soon as I'm able to. Thanks!

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.

2 participants