Use unambiguous ref names

[kammala]

serializer type name may be ambiguous, e.g. in different applications of the same project one may have different Account with the same serializer class name but with different fields, etc.
In current implementation they will have the same $ref name in API spec which will lead to improper API representation.
Adding module name to ref name should solve this problem.

[rsichnyi]

Does this mean that for all existing projects all the model names will change? If so i'd say it's not acceptable since it will break every client app that uses existing apis.

[kammala]

Hello, @rsichny , thanks for your feedback!
Actually, this is a very good question and I don't know complete answer to it.

Yes, it will change names for all models in existing projects, but these names are not part of API itself, so I don't think it should not break anything.

Once generated with swagger-codegen client will not revise swagger.json, so these changes should not be a problem in this case.

[rsichnyi]

But this means that whenever someone generates a new client using codegen - they'll need to change ALL the code which uses the generated client.

Example 1: in our case QAs generate the new client using codegen for api schemas on each release and then they run test suite (thouthands of test cases) using the newly generated client. Global change of the names which are being output by codegen will produce enormous amount of work (they'll need to change each of the references to the old names).

Example 2: our partners use swagger definitions to generate the swagger client and use that client in their code. As soon as we decide to add a new field to api they'll need to re-generate the swagger client and all the integration will break.

My suggestion is to leave the old behavior by default, and either make this new behavior configurable, or only add prefixes for the cases when the schema is added to the list of definitions if the same definition already exist.

[kammala]

However in case of dynamic clients like bravado it will be definitely a problem(don't know if it will be a problem for swagger-js, doesn't seem so)

[rsichnyi]

Actually, since all the name conflicts can be resolved by using ref_name in serializer i don't think any change is necessary at all.

https://drf-yasg.readthedocs.io/en/stable/custom_spec.html#serializer-meta-nested-class

[kammala]

Yes, I was also thinking about adding some settings to switch this behaviour on keeping old one as a default.

However I still believe that it would be nice to have such unambiguous behavior by default, maybe in the next major release. Right now this behavior is very awkward: you will get improper API documentation in case on names clash without any warning and it is really hard to debug.

I will leave this PR open for now to gather more opinions and have a look if it is possible to make this problem more clear(raise a warning when app finds ambiguous name, e.g.)

[rsichnyi]

@kammala how about adding a warning into https://github.com/axnsan12/drf-yasg/blob/master/src/drf_yasg/openapi.py#L540 when the ref names overlap?

[axnsan12]

Others may use the swagger spec for live API clients or form generators, so changing stuff suddenly like this is still kinda awkward.

Plus, the problem this solves is not that common, I think. This solution might work behind a setting. Maybe a better one would be to always raise an exception when encountering multiple serializers with the same name, forcing explicit disambiguation by the user.

[axnsan12]

The warning should already be there:

drf-yasg/src/drf_yasg/inspectors/field.py

Lines 130 to 133 in 583e404

	if actual_serializer and actual_serializer != this_serializer: # pragma: no cover
	logger.warning("Schema for %s will override distinct serializer %s because they "
	"share the same ref_name", actual_serializer, this_serializer)

[rsichnyi]

@axnsan12 maybe just change to warnings.warn to make it more visible? (people often turn off logging for 3rd party libs)

[axnsan12]

I think I'll just change it to an exception 😄

[kammala]

However, this $ref is still not required in OpenAPI — we can either create definitions for schemas or put these definitions in place they are needed. I think, ideally this part should not be treated as a part of API cause it does not affect actual client-server protocol.

this warning doesn't work for some reason :(

[kammala]

actual_serializer is always None because of https://github.com/axnsan12/drf-yasg/blob/master/src/drf_yasg/openapi.py#L109

so this warning will never be printed :(

[kammala]

looks like #156 wasn't fixed