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

Fri Aug 2 12:08:35 UTC 2019

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 <ipanova at 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 <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/20190802/5576f2c4/attachment.htm>