[Pulp-dev] Master-detail inheritance in Pulp 3

Wed Jul 24 17:05:49 UTC 2019

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 <dawalker at 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>
>
> dawalker at redhat.com
> <https://www.redhat.com>
>
>
>
> On Wed, Jul 24, 2019 at 8:24 AM David Davis <daviddavis at 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 <bbouters at 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 <daviddavis at 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 <daviddavis at 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 <ipanova at 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 <daviddavis at 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 <bbouters at redhat.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On Tue, Apr 23, 2019 at 11:02 AM David Davis <daviddavis at 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 <
>>>>>>>>> daviddavis at 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 at redhat.com
>>>>>>>>> https://www.redhat.com/mailman/listinfo/pulp-dev
>>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>> Pulp-dev mailing list
>>>>>>> Pulp-dev at redhat.com
>>>>>>> https://www.redhat.com/mailman/listinfo/pulp-dev
>>>>>>>
>>>>>> _______________________________________________
>> Pulp-dev mailing list
>> Pulp-dev at redhat.com
>> https://www.redhat.com/mailman/listinfo/pulp-dev
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/pulp-dev/attachments/20190724/f9837f06/attachment.htm>