+1 doing both sounds right.
-------- 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, Aug 7, 2019 at 11:15 PM Brian Bouterse <bmbou...@redhat.com> wrote: > > > On Wed, Aug 7, 2019 at 3:05 PM David Davis <davidda...@redhat.com> wrote: > >> 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. >> > +1 to doing both. Thank you for bringing this up. > > >> 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 >> >
_______________________________________________ Pulp-dev mailing list Pulp-dev@redhat.com https://www.redhat.com/mailman/listinfo/pulp-dev
