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

Wed Aug 7 19:20:36 UTC 2019

Seems right to me. Just curious what happens if the plugin doesn't define
default_related_name?

On Wed, Aug 7, 2019 at 3:16 PM Dana Walker <dawalker at redhat.com> wrote:

> +1
>
> Sound logic.
>
>
> 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, Aug 7, 2019 at 3:05 PM David Davis <daviddavis at 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.
>>
>> Thoughts?
>>
>> David
>>
>>
>> On Fri, Aug 2, 2019 at 8:08 AM David Davis <daviddavis at 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 <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
>>>>>
>>>> _______________________________________________
> 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/20190807/9ba6c7c8/attachment.htm>