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

Use ref_name to disambiguate multiple components with identical names #27

Closed
jayvdb opened this issue Apr 23, 2020 · 15 comments
Closed
Labels
enhancement New feature or request

Comments

@jayvdb
Copy link
Contributor

jayvdb commented Apr 23, 2020

WARNING #813: Encountered 2 components with identical names "User" and different classes <class 'cookie.serializers.UserSerializer'> and <class 'oscarapi.serializers.login.UserSerializer'>. This will very likely result in an incorrect schema. Try renaming one.

I think drf-yasg uses ref_name to bypass this problem

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

@tfranzel tfranzel added the enhancement New feature or request label Apr 23, 2020
@tfranzel
Copy link
Owner

i guess renaming is not an option when 2 libraries collide with each other. i suppose there is no way around this.

813 warnings? that is a lot...

@jayvdb
Copy link
Contributor Author

jayvdb commented Apr 23, 2020

It is a lot (mostly caused by django-oscar-api, and a complex auth system), but drf-yasg failed miserably, so I am a huge fan of drf-spectacular atm.

tfranzel added a commit that referenced this issue May 3, 2020
@tfranzel
Copy link
Owner

tfranzel commented May 3, 2020

i looked at the use cases again for this issue and i see 3 distinct cases:

  1. 2 or more utilized libraries have colliding serializers, e.g. UserSerializer
    • Fix: at least one library naming has be be changed
  2. owned code collides with utilized library
    • Fix: either rename owned serializer or change lib serializer
  3. collisions in owned code
    • Fix: choose reasonable naming yourself

when it comes to lib code (case 1&2) decoration via @extend_schema is likely not practical, so there is no sane way around the extensions. For that reason i tried simplifying the extensions for those cases.

All you have to do is define something like this anywhere (*) in YOUR code. setting the target_class attr and overriding get_name is all that is necessary.

class Lib2XSerializerRename(OpenApiSerializerExtension):
target_class = x_lib2 # also accepts import strings
def get_name(self):
return 'RenamedLib2X'

@MissiaL @jayvdb what do you think? (@foarsitter that general mechanic might also be interesting for you) closing this for now. feel free to suggest a better approach.

(*) anywhere == anywhere the interpreter visits at least once. I recommend having a schema.py in your project and doing a from yourproject.schema import * in your settings.py

@tfranzel tfranzel closed this as completed May 3, 2020
@jayvdb
Copy link
Contributor Author

jayvdb commented May 3, 2020

feel free to suggest a better approach.

I already did.

drf-yasg uses ref_name, which is a better approach for libraries to use, and is widely implemented - that search is quite restrictive - more examples can be found with broader search, and that is only OSS code, not the many OSS forks which have been modified to fix this, and non-public django projects running in production.

@MissiaL
Copy link
Contributor

MissiaL commented May 3, 2020

I will support @jayvdb
In my large project we often use ref_name. This is a good and convenient option.

@tfranzel tfranzel reopened this May 3, 2020
@tfranzel
Copy link
Owner

tfranzel commented May 3, 2020

@jayvdb yes you already did, but imho it does not solve the issues outlined in my comment. how do you solve case 1 ref_name?

let's say you use 2 libraries that both have UserSerializer you are still out of luck with ref_name, unless you patch those libraries and inject a compatible ref_name. The SerializerExtension solve exactly that problem without awkward lib code hacking.

@MissiaL i can understand that is is more convenient because your code base already supports it. What i don't really understand is why it is more convenvient to use a ref_name instead of properly naming the serializers in the first place. i guess this just diverges significantly from the code bases i worked on. as mentioned above i don't understand how ref_name generally solves the library naming collision.

i was not aware that that other libs support ref_name. then it is worth supporting i guess. that is in addition to extensions.

@jayvdb
Copy link
Contributor Author

jayvdb commented May 4, 2020

how do you solve case 1 ref_name?

You raise an issue in their project to support drf-yasg & drf-spectacular. And fork and fix it yourself, submit a PR, and use git+...@openapi in your own deps.

It isnt a zero-sum game. Your extension approach is great, but it doesnt mean this library cant also be compatible with drf & drf-yasg where it helps.

@jayvdb
Copy link
Contributor Author

jayvdb commented May 4, 2020

Note that duplicate serializers is implicit in using oscar, which requires "forking" their apps to extend them in order to customise the site. So I receive lots of

Warning #29: Encountered 2 components with identical names "Partner" and different classes <class 'my.partner.serializers.PartnerSerializer'> and <class 'oscarapi.serializers.product.PartnerSerializer'>. This will very likely result in an incorrect schema. Try renaming one.

Renaming my models definitely breaks oscar, but renaming the serializer may not -- however it would deviate from their instructions which makes the site less understandable/umaintainable by someone else.

I could add an 'extension' for all of them, but it is much easier to add ref_name, and drf-spectacular should be able to differentiate between them. In fact I only want my forked one - the other one should be hidden. I've yet to dig into why their one is being 'seen' by drf-spectacular.

@tfranzel
Copy link
Owner

tfranzel commented May 4, 2020

Renaming my models definitely breaks oscar, ...

no one suggested renaming the models. model names are not used in the schema anyway.

however it would deviate from their instructions which makes the site less understandable/umaintainable by someone else.

i absolutely agree.

at the beginning of the issue i wasn't aware of the full picture. there was only the "what" but not the "why" and "how". after the clarifications this makes a lot more sense. for future issues, we should try amd get those questions straightened out from the get-go.

I've yet to dig into why their one is being 'seen' by drf-spectacular.

this is interesting. please do. spectacular only processes what it finds in the views. let me know what you find, then we can see how to solve it.

Warning #29: Encountered 2 components with identical names "Partner" and different classes <class 'my.partner.serializers.PartnerSerializer'> and <class 'oscarapi.serializers.product.PartnerSerializer'>. This will very likely result in an incorrect schema. Try renaming one.

i guess that is leaking issue. as far as i understand those would disappear if the leaking stopped.

how would you use ref_name to solve that particular issue here? i think i'm still missing the last piece of the puzzle.

@tfranzel
Copy link
Owner

tfranzel commented May 4, 2020

just as clarification. i absolutely support adding ref_name. i'm just not sure whether and how it completely solves this particular problem.

@jayvdb
Copy link
Contributor Author

jayvdb commented May 5, 2020

how would you use ref_name to solve that particular issue here?

I import the oscar serializer and subclass it to add ref_name = 'MyPartner' under Meta. That works with drf-yasg.

In addition to supporting ref_name, it would be better if the default/worstcase fallback to auto-disambiguate the two (using qualname? or even random gibberish) so then the result of multiple similar-name components can be seen in the generated doc, which will result in better support experience, especially for any 'leaking' of installed objects the project isnt expecting to be in their spec.

@tfranzel
Copy link
Owner

tfranzel commented May 5, 2020

I import the oscar serializer and subclass it to add ref_name = 'MyPartner' under Meta. That works with drf-yasg.

does that mean you have 2 partner serializers in the schema? is that as intended?

i did this https://django-oscar-api.readthedocs.io/en/latest/usage/customizing_oscarapi.html for the 2 example serializers and encountered no leaking issues yet. i have a basic oscar setup with the api plugin and those changes. can you provide a snippet for reproduction?

In addition to supporting ref_name, it would be better if the default/worstcase fallback to auto-disambiguate the two (using qualname? or even random gibberish)

absolutely. that makes sense. if random, i guess it would need to be pseudo random otherwise the schema would change on every run.

@tfranzel
Copy link
Owner

tfranzel commented May 7, 2020

i added ref_name support. however, i explicitly excluded ref_name=None functionality for inlining serializer definitions as this might silently break client generation. my experience was that some code generators can't deal with inlined entitities.

@jayvdb
Copy link
Contributor Author

jayvdb commented May 8, 2020

Awesome. I see the result! Very helpful for me working out those shadows.

@tfranzel
Copy link
Owner

tfranzel commented May 8, 2020

awesome. are those issues completely gone now? if yes you can close this. i'll keep a mental note for the remaining qualname thing.

@jayvdb jayvdb changed the title Multiple components with identical names Use ref_name to disambiguate multiple components with identical names May 8, 2020
@jayvdb jayvdb closed this as completed May 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants