add first child if there's no last child

[graingert]

fixes #140 and #220

[graingert]

it looks like what's happening here is the self.numchild attribute has been updated but the database doesn't have the child there

[graingert]

?

[hazho]

I tested in my end, your changes are currently better than mine, but both need more tests for

[graingert]

I tested in my end, your changes are currently better than mine, but both needs more tests to be done on

Thanks, do you have a repeat for this crash it seems to be intermittently happening for me

[hazho]

Thanks, do you have a repeat for this crash it seems to be intermittently happening for me

it been more than 2 years I'm working on it without any issues till before two days, I'm using it with wagtail-cms, I was getting this error only when I was initializing my first time data using django-shell (based on Python shell), the rest of treebeard nodes are being created by website's editors/authors who use buttons that being controlled by views.

[wanglophile]

Any update on merging this? Seems like a relatively straight-forward fix.

[hazho]

It seems no one maintaining this repo anymore, or they forgot about this PR..!

[janbaykara]

@jrief seeing as you were the last contributor to merge into main, would you mind having a look? The bugs this PR addresses are quite frequent in my experience of Wagtail development.

[solarissmoke]

I'm happy to review this, but I do think it will need tests to cover the change and illustrate the issue it is fixing.

[gasman]

As things stand this is fixing a symptom of #220, rather than the bug itself, and causing a performance regression along the way.

For background: the AttributeError: 'NoneType' object has no attribute '_inc_path' error occurs when a previous insertion has failed, leaving the parent node in an inconsistent state where the numchild field has been incremented but no child has been created. (This is one of the consistency issues fixed by fix_tree.) The is_leaf() method works on the basis that the numchild field is reliable as an indicator of "does this node have children or not" - if this is the case, then we can rely on get_last_child() returning a valid node whenever is_leaf() is false; but if not, we end up calling _inc_path on the child node that ought to exist but doesn't.

By skipping the is_leaf check as in this proposed fix, we're no longer reliant on numchild, and thus avoid the error - but we also lose the performance optimisation where if we know there are no existing children, we can immediately build the new child's path string without having to do a database lookup to retrieve the current last child. This could be remedied by replacing:

last_child = self.node.get_last_child()

in the new version with:

if self.node.is_leaf():
    last_child = None
else:
    last_child = self.node.get_last_child()

However, there's still an open question about whether silently correcting for inconsistent numchild values is a good thing - for the sake of developer friendliness - or whether we're better off letting them fail loudly so that inconsistencies can be spotted early and fixed with fix_tree before they cause more subtle problems.