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

David Davis daviddavis at redhat.com
Wed Aug 7 19:46:37 UTC 2019 

An exception would be raised whenever the pulp app starts that says, "The
setting 'default_related_name' has not been defined for detail model X".

David


On Wed, Aug 7, 2019 at 3:20 PM Austin Macdonald <amacdona at redhat.com> wrote:

> 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/743d287b/attachment.htm>

More information about the Pulp-dev mailing list