So I've been working on the change to have plugin writers manually specify default_related_name. In terms of pulpcore, I see two options:
1. Just document that this needs to be done 2. Enforce that default_related_name needs to be defined and raise an exception if it is not. I'd suggest we do both options 1 and 2. First of all, it's quite easy to forget and overlook this detail in the docs. Raising an error could save a plugin writer a lot of pain/grief later on. Secondly, there are a number of plugins already and it's quite possible some plugin writers could miss this email thread or the new documentation. Normally, I am all for giving plugin writers freedom but this seems like one case we may want to try to enforce a solution to prevent problems later on. Thoughts? David On Fri, Aug 2, 2019 at 8:08 AM David Davis <davidda...@redhat.com> wrote: > Thanks for the feedback. It sounds like no one is opposed to having plugin > writers specify default_related_name for Detail models so I plan to move > forward with this solution next week if no one objects. > > David > > > On Thu, Aug 1, 2019 at 4:39 AM Ina Panova <ipan...@redhat.com> wrote: > >> I am in favour of manually setting default_related_name and document this >> in plugin writers guide. >> >> >> >> -------- >> Regards, >> >> Ina Panova >> Senior Software Engineer| Pulp| Red Hat Inc. >> >> "Do not go where the path may lead, >> go instead where there is no path and leave a trail." >> >> >> On Wed, Jul 24, 2019 at 7:06 PM David Davis <davidda...@redhat.com> >> wrote: >> >>> I think my main concern with the solution to remove model inheritance is >>> that we either only apply it to the Content model and run the risk of >>> having conflicts in other Master/Detail models (unlikely but possible). Or >>> we apply it to all M/D models which is a huge undertaking (unless we can >>> create some general solution?). >>> >>> David >>> >>> >>> On Wed, Jul 24, 2019 at 10:09 AM Dana Walker <dawal...@redhat.com> >>> wrote: >>> >>>> I like your solution using default_related_name [0] manually, as Brian >>>> noted [1], it's more explicit and therefore more pythonic. >>>> >>>> That in mind, Daniel's alternative, not using model inheritance for the >>>> Content models [2], while less simple a change initially, potentially had >>>> significant performance gains and is also more explicit and pythonic. >>>> >>>> Should we still pursue this more complex fix for the improvements to >>>> bulk_create since we'd rather have breaking changes early in development >>>> than need to address them later? >>>> >>>> Or am I putting the cart before the horse by seeking optimization too >>>> early? >>>> >>>> [0] https://pulp.plan.io/issues/4681#note-19 >>>> [1] https://pulp.plan.io/issues/4681#note-20 >>>> [2] https://pulp.plan.io/issues/4681#note-11 >>>> >>>> Dana Walker >>>> >>>> She / Her / Hers >>>> >>>> Software Engineer, Pulp Project >>>> >>>> Red Hat <https://www.redhat.com> >>>> >>>> dawal...@redhat.com >>>> <https://www.redhat.com> >>>> >>>> >>>> >>>> On Wed, Jul 24, 2019 at 8:24 AM David Davis <davidda...@redhat.com> >>>> wrote: >>>> >>>>> I want to bump this thread again. We've only had one person weigh in >>>>> and this is a major change that'll affect all Pulp 3 plugins that we need >>>>> to address soon. Please respond here or on the issue with feedback. >>>>> >>>>> David >>>>> >>>>> >>>>> On Sun, Jul 21, 2019 at 10:49 AM Brian Bouterse <bbout...@redhat.com> >>>>> wrote: >>>>> >>>>>> Thanks for the investigation and demo patch. I posted a +1 for the >>>>>> manual option with reasoning here: >>>>>> https://pulp.plan.io/issues/4681#note-20 >>>>>> >>>>>> Other ideas and perspectives are welcome. I hope we can resolve this >>>>>> issue soon as we approach RC4. >>>>>> >>>>>> On Wed, Jul 17, 2019 at 12:55 PM David Davis <davidda...@redhat.com> >>>>>> wrote: >>>>>> >>>>>>> I did some investigation and posted my findings[0]. Basically, it >>>>>>> would be possible to solve this problem by defining default_related_name >>>>>>> either manually or automatically on detail models. I don't know if we >>>>>>> want >>>>>>> to go this route so feedback is appreciated. >>>>>>> >>>>>>> [0] https://pulp.plan.io/issues/4681#note-19 >>>>>>> >>>>>>> David >>>>>>> >>>>>>> >>>>>>> On Mon, Apr 29, 2019 at 2:16 PM David Davis <davidda...@redhat.com> >>>>>>> wrote: >>>>>>> >>>>>>>> It seems like most people are in favor of setting the OneToOneField >>>>>>>> or perhaps the default_related_name on the detail model. I think >>>>>>>> there’s >>>>>>>> also some interest in seeing how we can do this automatically for >>>>>>>> plugins. >>>>>>>> I’ve added this feedback to the issue: >>>>>>>> >>>>>>>> https://pulp.plan.io/issues/4681#note-8 >>>>>>>> >>>>>>>> David >>>>>>>> >>>>>>>> >>>>>>>> On Wed, Apr 24, 2019 at 6:22 AM Ina Panova <ipan...@redhat.com> >>>>>>>> wrote: >>>>>>>> >>>>>>>>> I would avoid making changes in class naming. So +1 for the >>>>>>>>> OneToOneField definition. >>>>>>>>> >>>>>>>>> >>>>>>>>> -------- >>>>>>>>> Regards, >>>>>>>>> >>>>>>>>> Ina Panova >>>>>>>>> Software Engineer| Pulp| Red Hat Inc. >>>>>>>>> >>>>>>>>> "Do not go where the path may lead, >>>>>>>>> go instead where there is no path and leave a trail." >>>>>>>>> >>>>>>>>> >>>>>>>>> On Tue, Apr 23, 2019 at 6:45 PM David Davis <davidda...@redhat.com> >>>>>>>>> wrote: >>>>>>>>> >>>>>>>>>> The default_related_name setting is something that django >>>>>>>>>> provides. Subclasses can also explicitly define their OneToOneField >>>>>>>>>> parent >>>>>>>>>> link as well: >>>>>>>>>> >>>>>>>>>> content_ptr = models.OneToOneField(Content, >>>>>>>>>> on_delete=models.CASCADE, parent_link=True, >>>>>>>>>> related_name='rpm_package') >>>>>>>>>> >>>>>>>>>> I am not sure what you mean by 'robust' but if a plugin subclass >>>>>>>>>> doesn't do either of these, it may not work with other plugins. >>>>>>>>>> >>>>>>>>>> I think the question now would be whether we should just document >>>>>>>>>> this or try to do it automagically for plugins? >>>>>>>>>> >>>>>>>>>> David >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Tue, Apr 23, 2019 at 12:31 PM Brian Bouterse < >>>>>>>>>> bbout...@redhat.com> wrote: >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Tue, Apr 23, 2019 at 11:02 AM David Davis < >>>>>>>>>>> davidda...@redhat.com> wrote: >>>>>>>>>>> >>>>>>>>>>>> I think I found another solution that might work best: defining >>>>>>>>>>>> 'default_related_name' on subclassed master-detail models. So >>>>>>>>>>>> Package in >>>>>>>>>>>> pulp_rpm would define its default_related_name as "rpm_package". >>>>>>>>>>>> >>>>>>>>>>> Would we be making 'default_related_name' or is that something >>>>>>>>>>> Django is providing? If it's something Pulp would be providing >>>>>>>>>>> perhaps >>>>>>>>>>> defining the explicit one-to-one field is better. Any plugin that >>>>>>>>>>> takes the >>>>>>>>>>> step of defining the one-to-one field will insulate themselves from >>>>>>>>>>> other >>>>>>>>>>> plugins. If plugins don't take that step they will still work, just >>>>>>>>>>> not as >>>>>>>>>>> robustly. Am I thinking about this correctly? >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> David >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On Tue, Apr 23, 2019 at 10:29 AM David Davis < >>>>>>>>>>>> davidda...@redhat.com> wrote: >>>>>>>>>>>> >>>>>>>>>>>>> I wanted to email the pulp-dev list about a major problem[0] >>>>>>>>>>>>> that was recently encountered in Pulp 3 that affects how the Pulp >>>>>>>>>>>>> 3 plugin >>>>>>>>>>>>> API functions. >>>>>>>>>>>>> >>>>>>>>>>>>> # Problem >>>>>>>>>>>>> >>>>>>>>>>>>> In the plugin API we rely on inheritance to allow plugin >>>>>>>>>>>>> writers to import functionality into their plugin. This includes >>>>>>>>>>>>> models >>>>>>>>>>>>> such as Remote and Content that are inherited by plugins. We rely >>>>>>>>>>>>> on >>>>>>>>>>>>> django's multi-table inheritance[1] for these models. >>>>>>>>>>>>> >>>>>>>>>>>>> Behind the scenes, django defines a OneToOneField and a >>>>>>>>>>>>> reverse accessor. This field is not namespace so if two >>>>>>>>>>>>> subclasses have the >>>>>>>>>>>>> same name, you get an error ("Reverse accessor for >>>>>>>>>>>>> 'Package.content_ptr' >>>>>>>>>>>>> clashes with reverse accessor for 'Package.content_ptr'.") >>>>>>>>>>>>> >>>>>>>>>>>>> To give an actual example, both the Debian and RPM plugins >>>>>>>>>>>>> implement a Package class. This causes an error to be raised when >>>>>>>>>>>>> a user >>>>>>>>>>>>> installs both plugins. Django tries to define a 'package' reverse >>>>>>>>>>>>> accessor >>>>>>>>>>>>> for both subclasses and blows up. >>>>>>>>>>>>> >>>>>>>>>>>>> # Potential Solutions >>>>>>>>>>>>> >>>>>>>>>>>>> ## Class Naming >>>>>>>>>>>>> >>>>>>>>>>>>> The first solution I can think of which is probably also the >>>>>>>>>>>>> simplest and most straightforward would be to require plugin >>>>>>>>>>>>> writers to >>>>>>>>>>>>> namespace their master/detail subclass names. So Package would be >>>>>>>>>>>>> RpmPackage. This places the onus on plugin writers to name their >>>>>>>>>>>>> master/detail classes correctly. >>>>>>>>>>>>> >>>>>>>>>>>>> ## Defining OneToOneField >>>>>>>>>>>>> >>>>>>>>>>>>> The other solution would be to either manually define the >>>>>>>>>>>>> OneToOneField on the subclasses[2] and specify a namespaced field >>>>>>>>>>>>> name. >>>>>>>>>>>>> There may be a way to do this dynamically (ie magically) in the >>>>>>>>>>>>> parent >>>>>>>>>>>>> somehow as well. >>>>>>>>>>>>> >>>>>>>>>>>>> ## Abstract Class >>>>>>>>>>>>> >>>>>>>>>>>>> Lastly, we could redefine master models as abstract >>>>>>>>>>>>> classes[3]. I can think of at least one or two places (e.g. >>>>>>>>>>>>> content field >>>>>>>>>>>>> on RepositoryVersionContent, publisher field on Publication) that >>>>>>>>>>>>> would >>>>>>>>>>>>> have to switch their relationships to generic relationships in >>>>>>>>>>>>> order to >>>>>>>>>>>>> accommodate this change. >>>>>>>>>>>>> >>>>>>>>>>>>> There might be other solutions I am not thinking of so feel >>>>>>>>>>>>> free to propose something. Also, quick feedback would be greatly >>>>>>>>>>>>> appreciated as this is going to be a major change in our plugin >>>>>>>>>>>>> API. >>>>>>>>>>>>> >>>>>>>>>>>>> [0] https://pulp.plan.io/issues/4681 >>>>>>>>>>>>> [1] >>>>>>>>>>>>> https://docs.djangoproject.com/en/2.2/topics/db/models/#multi-table-inheritance >>>>>>>>>>>>> [2] >>>>>>>>>>>>> https://docs.djangoproject.com/en/2.2/topics/db/models/#specifying-the-parent-link-field >>>>>>>>>>>>> [3] >>>>>>>>>>>>> https://docs.djangoproject.com/en/2.2/topics/db/models/#abstract-base-classes >>>>>>>>>>>>> >>>>>>>>>>>>> David >>>>>>>>>>>>> >>>>>>>>>>>> _______________________________________________ >>>>>>>>>>>> Pulp-dev mailing list >>>>>>>>>>>> Pulp-dev@redhat.com >>>>>>>>>>>> https://www.redhat.com/mailman/listinfo/pulp-dev >>>>>>>>>>>> >>>>>>>>>>> _______________________________________________ >>>>>>>>>> Pulp-dev mailing list >>>>>>>>>> Pulp-dev@redhat.com >>>>>>>>>> https://www.redhat.com/mailman/listinfo/pulp-dev >>>>>>>>>> >>>>>>>>> _______________________________________________ >>>>> Pulp-dev mailing list >>>>> Pulp-dev@redhat.com >>>>> https://www.redhat.com/mailman/listinfo/pulp-dev >>>>> >>>> _______________________________________________ >>> Pulp-dev mailing list >>> Pulp-dev@redhat.com >>> https://www.redhat.com/mailman/listinfo/pulp-dev >>> >>
_______________________________________________ Pulp-dev mailing list Pulp-dev@redhat.com https://www.redhat.com/mailman/listinfo/pulp-dev
