feincms / django-tree-queries

Adjacency-list trees for Django using recursive common table expressions. Supports PostgreSQL, sqlite, MySQL and MariaDB.
https://django-tree-queries.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
427 stars 27 forks source link

Tree Fields Not Accurate After Model.objects.create() #40

Open JSv4 opened 2 years ago

JSv4 commented 2 years ago

Once again, great library, and I'm back to trying to use it again after using mptt but running into a real PITA concurrency issue where it has trouble creating distinct, unrelated nodes concurrently without corrupting the tree_ids. Given the project is unmaintained, potential fixes look like they've gotten stuck in PR limbo.

Anyway, I had a lot of trouble getting the right django-tree-queries "tree field" annotations on a model in a certain situation that I've finally solved. I'd like to confirm I understand the library behavior properly and suggest some documentation updates (which I'm happy to contribute as a PR).

Here's what I was doing.

I have a model that inherits from TreeNode. I also set a custom manager via TreeQuerySet.as_manager(with_tree_fields=True). I want to be able to, given a pk for an instance of this model type, make a copy of the instance with some, slightly different attributes and then make the original instance a child of the new instance. It's essentially a rudimentary VCS.

I ran into two problems trying to do this "version control":

  1. When I create the new object via Model.objects.create(...), I kept getting an error that tree_depth was not an attribute on the new instance. I tried to refresh_from_db() thinking maybe that could help, but no dice. Eventually, I realized that, if I took the pk of the new instance and immediately refetched it via Model.objects.get(id=id), I got the tree field annotations.
  2. One thing that was throwing me a bit was the original instance had the proper parent attribute but the tree_depth was still 0. As with the new instance, refresh_from_db() was no help. Like the new instance, I eventually realized I needed to requery it from the database and my problem was solved.

After discovering working this out and then very closely rereading your documentation, I think this is expected behavior as you state the manager adds the tree fields to queries by default:

Create a manager using TreeQuerySet.as_manager(with_tree_fields=True) if you want to add tree fields to queries by default.

So, I think it's the case that a database refresh will have no effect on these fields as they're not actual database fields but rather calculated fields. Furthermore, create(), I guess, isn't a query?

So, couple questions:

  1. Can you confirm my understanding is correct and that the only way to get the proper, accurate tree fields is to use objects.get() or objects.filter()?
  2. Not having written a Django manager before, is there an easy way to extend the TreeQuerySet code to add these annotations to the model .create() behavior too?
  3. Likewise, do you know if there is a way to override the refresh_from_db() behavior to annotate the tree fields?
  4. Finally, regardless of your answer to 1, 2 or 3, would you welcome some update documentation making this behavior clearer? It took me some time to figure this out, and, while it may be clear to people familiar with the inner workings of Django managers, I'd love to save the less enlightened some time.
matthiask commented 2 years ago

Hi,

Thank you!

  1. Yes, your understanding is correct. That's the only way (for now)
  2. The parent foreign key is a boring old Django ForeignKey; if e.g. parent.tree_depth exists it would be relatively easy to set node.tree_depth = node.parent.tree_depth + 1 after initialization or creation. The danger of concurrency issues should be small since those fields are never persisted to the database anyway. I'm not sure I would want to add this code to django-tree-queries though: The database is the single source of truth for now and I don't think I want to change that (because of bugs that will happen as always)
  3. I don't think there's an easy way to extend refresh_from_db. I think it internally also does teh equivalent of a get() followed by reassigning attributes so there's not a big difference from obj.refresh_from_db() to obj = Model.objects.get(pk=obj.pk) (but I could be wrong)
  4. Yes, please. That would be awesome!