Project

Profile

Help

Story #5574

Provide an easy to use way to ensure the ref_name attribute convention

Added by gmbnomis about 1 year ago. Updated 11 months ago.

Status:
CLOSED - CURRENTRELEASE
Priority:
Normal
Assignee:
Category:
-
Sprint/Milestone:
Start date:
Due date:
% Done:

100%

Estimated time:
Platform Release:
Groomed:
No
Sprint Candidate:
No
Tags:
Sprint:
Quarter:

Description

Due to issue #5562, pulpcore_plugin proposes a convention for the ref_name attribute of serializer Meta classes.

The current convention needs to be changed, as there is a (vague) recommendation to use a dot for namespaces (see for example https://github.com/OAI/OpenAPI-Specification/issues/1362). This is supported in some tools, but neither in swagger codegen, see https://github.com/swagger-api/swagger-codegen/issues/7326 nor in OpenAPITools/openapi-generator. openapi-generator handles the dot like other special characters (and just removes it from the class name, capitalizing the next word).

Thus, use <app_name>.<model class name> for ref_name. Ref names ("definitions" in the JSON API file) look like "file.FileRemote" then.

The generator creates model classes like "FileFileRemote". For models defined by core, we do not introduce a separate namespace (core.) but leave them as they are today.

For Modelserializers derived from pulpcore's ModelSerializer we can provide a proper default for ref_name.

Associated revisions

Revision 9e37fab3 View on GitHub
Added by gmbnomis 12 months ago

Set default ref_name attribute for a ModelSerializers's Meta class.

Use the init_subclass method added in Python 3.6 to customize subclasses of pulpcore's ModelSerializer class.

If it is not yet defined, set the Meta.ref_name attribute according to the best practice established within Pulp ('.').

closes #5574 https://pulp.plan.io/issues/5574

Revision 87199523 View on GitHub
Added by gmbnomis 12 months ago

Improve documentation on explicit ref_name requirement

re #5574 https://pulp.plan.io/issues/5574

History

#1 Updated by gmbnomis about 1 year ago

  • Description updated (diff)

#2 Updated by gmbnomis about 1 year ago

  • Description updated (diff)

#3 Updated by fao89 about 1 year ago

  • Tracker changed from Issue to Story
  • % Done set to 0

#4 Updated by gmbnomis about 1 year ago

I found a way to set the value automatically (see https://github.com/pulp/pulpcore/pull/345). However, I was unaware that this convention will also change the name of the classes in the API client. For example, Repository is CoreRepository now.

We could suppress setting ref_name for classes in core.

But the ref_name will affect the plugin clients as well. For example, the pulp_file client has model classes like FileFilecontent, FileFiledistribution, FileFiledistribution. The naming looks odd.

We could use <app_name>_<model class name> for ref_name instead, which will yield ref_names like file_FileContent and client model classes like FileFileContent.

#5 Updated by gmbnomis about 1 year ago

gmbnomis wrote:

We could use <app_name>_<model class name> for ref_name instead, which will yield ref_names like file_FileContent and client model classes like FileFileContent.

I thought about this again and I think we should avoid the remapping of ref_name to CamelCase class names by the openapi generator. The default in drf yasg is to take the class name of the serializer minus the "Serializer" suffix (if it is present). Thus, the references in the openapi json use CamelCase by default and look like class names.

Using <capitalized app_name><model class name> for ref_name seems to be the better choice. In this case the ref_name and the resulting model class name should be the same, e.g. FileFileContent

#6 Updated by gmbnomis 12 months ago

After today's IRC discussion, I had a deeper look into this:

There seems to be a (vague) recommendation to use a dot for namespaces (see for example https://github.com/OAI/OpenAPI-Specification/issues/1362). This is supported in some tools, but not neither in swagger codegen, see https://github.com/swagger-api/swagger-codegen/issues/7326 nor in OpenAPITools/openapi-generator. openapi-generator handles the dot like other special characters (and just removes it from the class name, capitalizing the next word).

Thus, the right thing to do seems to use <app_name>.<model class name>. Ref names ("definitions" in the JSON API file) would look like "file.FileRemote" then.

The generator would create model classes like "FileFileRemote" still. People using other tooling might get properly namespaced modules. But, at least for the time being, we are stuck with classes like FileFileRemote (unless we want to explore workarounds when creating the bindings).

For models defined by core, we have the option to namespace them using "core." or leave them as they are today. Given the current class model names, I prefer the latter, but I am fine with both.

#7 Updated by bmbouter 12 months ago

gmbnomis wrote:

After today's IRC discussion, I had a deeper look into this:

Thank you!

There seems to be a (vague) recommendation to use a dot for namespaces (see for example https://github.com/OAI/OpenAPI-Specification/issues/1362). This is supported in some tools, but not neither in swagger codegen, see https://github.com/swagger-api/swagger-codegen/issues/7326 nor in OpenAPITools/openapi-generator. openapi-generator handles the dot like other special characters (and just removes it from the class name, capitalizing the next word).

Thus, the right thing to do seems to use <app_name>.<model class name>. Ref names ("definitions" in the JSON API file) would look like "file.FileRemote" then.

I agree

The generator would create model classes like "FileFileRemote" still. People using other tooling might get properly namespaced modules. But, at least for the time being, we are stuck with classes like FileFileRemote (unless we want to explore workarounds when creating the bindings).

I'm ok with this. It's a little ugly, but it's namespaced!

For models defined by core, we have the option to namespace them using "core." or leave them as they are today. Given the current class model names, I prefer the latter, but I am fine with both.

+1 to leaving core as is today. We don't need it to be namespaced if everything else is.

#8 Updated by gmbnomis 12 months ago

  • Description updated (diff)

#10 Updated by gmbnomis 12 months ago

  • Status changed from NEW to MODIFIED
  • % Done changed from 0 to 100

#11 Updated by bmbouter 11 months ago

  • Sprint/Milestone set to 3.0.0

#12 Updated by bmbouter 11 months ago

  • Status changed from MODIFIED to CLOSED - CURRENTRELEASE

Please register to edit this issue

Also available in: Atom PDF