ContentUnitSaver should support creating content with related models inside its transaction
DeclarativeVersion (stages) currently supports creating content consisting of a single Content model. Many plugins such as RPM and Docker have complex content models. That is, each content (unit) is stored in multiple tables. The insert into these tables needs be committed atomically to prevent storing incomplete content. Further, the inserts need to happen in relational (referential) order and be compatible with bulk_create().
Add a hook to ContentUnitSaver that will pass the batch of DeclarativeVersion objects to a method called
save_related(batch). The base implementation of this will do nothing, but subclasses can perform bulk_save calls there. The call to save_related occurs after the content units have been saved, but during the same transaction.
#3 Updated by email@example.com over 2 years ago
The proposed solution of calling save_related() after the batch has been bulk created only supports one-to-many related models (with FK to Content).
Changing this to:
def _create_batch(batch) <move batch create code block here>
plugin writer would override as:
def _create_batch(batch): <create models where Content has FK to> super()._create_batch(batch) <create models with FK to Content>
This is step toward functional decomposition of the call and supports both one-to-many and many-to-one.
That said, having Content with FK to other models could introduce orphan clean up problems and can't think of a good use case for it. But, thinking the API should be as flexible as possible to avoid being short sighted.
In either case the new method should be protected (leading _).
#4 Updated by bmbouter over 2 years ago
I can't think of a specific situation, but I can imagine someone wanting to do some saving within the transaction prior to the content unit being saved. I think the hooks are simpler for this specific pattern. The hooks would be:
class CustomContentUnitSaver(ContentUnitSaver): def save_before(batch): """ """ def save_safter(batch): """ """
I think the hooks are less hazard prone because they don't have to call super(). Also it encourages them to separate their before/after code into distinct functions.
What do you think?
#5 Updated by firstname.lastname@example.org over 2 years ago
Calling super when overriding a method is common (recommended) practice and I'd be surprised if it posed a hazard. I prefer the approach I suggested mainly because it's a pattern I'm used to seeing. But, what you're proposing in #3953-4 will meet our needs as well.
#6 Updated by bmbouter over 2 years ago
- Subject changed from DeclarativeVersion should support creating content with related models. to ContentUnitSaver should support creating content with related models inside its transaction
- Assignee set to bmbouter
- Groomed changed from No to Yes
- Sprint set to Sprint 42
OK sweet. I'm going to make this now so that we can resolve this Errata bug. I'll take that assigned also.
Grooming per the comment 5.
Please register to edit this issue