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
