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

Possible solution for tags generation #7184

Merged
merged 26 commits into from
Feb 28, 2020
Merged

Possible solution for tags generation #7184

merged 26 commits into from
Feb 28, 2020

Conversation

dhaval-mehta
Copy link
Contributor

@dhaval-mehta dhaval-mehta commented Feb 9, 2020

This merge request adds logic to generate open API tags automatically. It also allows the user to override auto-generated tags as an argument to the constructor of AutoSchema. This solution will not break the encapsulation of schema generation. Inspiration: #7103 (comment)

There is another merge request(#7182) with a possible solution. You can merge whichever solution is the best.

This Fixes: #7103
This closes: #7104 & #7177

@dhaval-mehta dhaval-mehta changed the title Possible solution for Tag generation Possible solution for tags generation Feb 9, 2020
@dhaval-mehta dhaval-mehta changed the title Possible solution for tags generation possible solution for tags generation Feb 9, 2020
@dhaval-mehta dhaval-mehta changed the title possible solution for tags generation Possible solution for tags generation Feb 9, 2020
Copy link
Member

@kevin-brown kevin-brown left a comment

Choose a reason for hiding this comment

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

In general I'm +0 on these changes. I've got one in-line review comment about being consistent in how we generate the default tag names.

rest_framework/schemas/openapi.py Outdated Show resolved Hide resolved
docs/api-guide/schemas.md Outdated Show resolved Hide resolved
@dhaval-mehta
Copy link
Contributor Author

In general I'm +0 on these changes. I've got one in-line review comment about being consistent in how we generate the default tag names.

Thanks for your suggestion. I have done given changes. Additionally, I have done the following changes:

  1. People use - or _ to separate words in URL (example: order-items or order_items). To make everything consistent, I will replace all - or _ with white space.

@dhaval-mehta
Copy link
Contributor Author

@carltongibson I await your valuable reply at your earliest convenience.

PascalCaseXYZ | ['pascal case xyz']
IPAddressView | ['ip address']

2. If View is not an instance of ViewSet, tag name will be first element from the path. Also, any `-` or `_` in path name will be replaced by a space.
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we ought to just use the path style as the default case?

I think that'd be more obvious, more consistent across a code base, and a little more simple.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I will implement the path style as the default case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I think the default implementation should use the path. Folks can pass tags, or subclass for something cleverer.

Copy link
Member

@tomchristie tomchristie left a comment

Choose a reason for hiding this comment

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

Looks like there's some great stuff here. I'd personally like to see it start slimmed down as much as possible.

@carltongibson
Copy link
Collaborator

Hi @dhaval-mehta. Yes, I can see that you like viewsets and want this particular implementation. But a lot of people don't use viewsets at all. And there are many different configurations and conventions. I'm just not convinced that favouring this one particular approach is correct. I will think on it.

@tomchristie
Copy link
Member

rather than just going with the much simpler path approach, and letting folks do something different

Seconded. 😃

@dhaval-mehta
Copy link
Contributor Author

dhaval-mehta commented Feb 20, 2020

Hi @dhaval-mehta. Yes, I can see that you like viewsets and want this particular implementation. But a lot of people don't use viewsets at all. And there are many different configurations and conventions. I'm just not convinced that favouring this one particular approach is correct. I will think on it.

Should I remove viewset specific implementation?

@carltongibson
Copy link
Collaborator

Hey @dhaval-mehta. Yes, I think so, please. Let's just go with the path approach.

Folks can create a subclass if they use viewsets heavily and set settings.DEFAULT_SCHEMA_CLASS to have it used everywhere by default.

@dhaval-mehta
Copy link
Contributor Author

Hey @dhaval-mehta. Yes, I think so, please. Let's just go with the path approach.

Folks can create a subclass if they use viewsets heavily and set settings.DEFAULT_SCHEMA_CLASS to have it used everywhere by default.

I have removed the viewset specific code.

@dhaval-mehta
Copy link
Contributor Author

Hey @dhaval-mehta. Yes, I think so, please. Let's just go with the path approach.

Folks can create a subclass if they use viewsets heavily and set settings.DEFAULT_SCHEMA_CLASS to have it used everywhere by default.

Hi @carltongibson, This is a nice idea. With your permission, I would like to add this idea in documentation with an example.

@dhaval-mehta dhaval-mehta removed the request for review from kevin-brown February 20, 2020 17:57
@dhaval-mehta
Copy link
Contributor Author

Hi, @carltongibson I am working on the global tag object, where users can pass dict as a tag. I request the following you.

  1. If you think everything is good, merge this PR because:

    1. I am going to open a PR that has this code + global tag objects. If you won't merge then these changes will come as the difference in that PR.
    2. Possible merge conflict may come with Openapi custom operation name #7190 in schemas.md
  2. My approach for global tag objects is:
    allow users to pass a dict as tags to the schema generator class. In this case, we need to override get_schema_view and allow users to pass a custom generator instance (as of now, only class can be passed.)
    If you think my approach needs any changes, kindly let me know.

@carltongibson
Copy link
Collaborator

Hi @dhaval-mehta Slowly, slowly. 🙂 Let’s finish this off before moving on. I need time to review it finally and go over the docs changes. Sorry if that’s a bit slow for you, but you have to remember I do this in my free time, and alas I’m not going to review on the weekend.

Then we should discuss further changes in an issue before spending time on code. Firstly, how much demand is there really for the dict format of tags? It might be that we choose to not support it.

@dhaval-mehta
Copy link
Contributor Author

Sorry. I was unaware about this situation. I am working in start up where code is running on production just 2-3 days after PR. Anyway thank you so much for your efforts.

@carltongibson
Copy link
Collaborator

No problem. Therein lies the difference between comercial software and open source. 🙂

Thank you for your efforts on this one. We will get it in.

@tomchristie
Copy link
Member

Looking great. @carltongibson let me know if you'd like my time on this too.

@carltongibson
Copy link
Collaborator

It’s on my list for this week.

Copy link
Collaborator

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

Hi @dhaval-mehta.

Good work on this one, thank you! 🥇

@duncanmcdowell
Copy link

@dhaval-mehta where did you land on global tags generation. I'm currently looking to implement logic on this but don't want to re-invent the wheel if others have made good progress.

@dhaval-mehta
Copy link
Contributor Author

dhaval-mehta commented May 7, 2020

@carltongibson I have 2 different approaches for global tag generation.

  1. accept global tags as an argument to SchemaGenerator.
  2. Guide users on how to create a subclass and override schema generation to generate global tags. For example https://github.com/encode/django-rest-framework/pull/7268/files#diff-6d2713ae8040f965de07518cc4ef8a95R187

I am personally inclined with the approach: 1

@carltongibson
Copy link
Collaborator

Yep, currently you'd need to override get_schema().

Happy to add some args to SchemaGenerator in principle for the most common cases. Not sure if tags counts there or not. (Likely yes?)

What I want to avoid is a argument for every possible option: that way lies madness. Keep the interface clean for the common cases that face most users. Subclassing isn't hard for the few that need the more exotic options.

@dhaval-mehta
Copy link
Contributor Author

SchemaGenerator deals with Open API object

I believe only 2 fields tags and security will be used widely and we have not implemented them. Hence I am not expecting more arguments to SchemaGenerator.

For mid to big size of projects & for companies with multiple teams, the global tags are a must. I think adding tags as a parameter to SchemaGenerator is not a bad idea.

@carltongibson
Copy link
Collaborator

Hey @dhaval-mehta. That sounds reasonable. Fancy doing a PR for those either of you? Thanks!

@dhaval-mehta
Copy link
Contributor Author

@carltongibson I will pick this task today.

@carltongibson
Copy link
Collaborator

@dhaval-mehta One issue is the inferface: you don't generally instantiate SchemaGenerator yourself. Tags I can see as a CLI option but security...? (Hence create a subclass for your project may still be needed there.) Let's see what you come up with. Thanks

@dhaval-mehta
Copy link
Contributor Author

The following approach is just a thought:

Tags are user-defined and vary user to user. On the other hand, security depends on authentication classes.

I am thinking that let authentication classes to provide the schema related methods like paginator classes do(get_paginated_response_schema).

For any custom authentication class, the user/library needs to override those methods.

pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
sigvef pushed a commit to sigvef/django-rest-framework that referenced this pull request Dec 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add OpenAPI tags to schemas.
5 participants