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
