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

Ina Panova ipanova at redhat.com
Thu Aug 1 08:38:51 UTC 2019 

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 <daviddavis at 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 <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
>>>
>> _______________________________________________
> 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/20190801/15f68c58/attachment.htm>

More information about the Pulp-dev mailing list