Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Django MPTT is no longer maintained - Migrate Existing Models to django-tree-queries #510

Closed
3 tasks done
lampwins opened this issue May 30, 2021 · 5 comments · Fixed by #3012
Closed
3 tasks done
Assignees
Labels
impact: breaking change This change or feature will remove or replace existing functionality; needs to be an X.0.0 release type: housekeeping Changes to the application which do not directly impact the end user
Milestone

Comments

@lampwins
Copy link
Member

lampwins commented May 30, 2021

Proposed Changes

The django-mptt project is no longer maintained so we need to find a new pattern for our tree use cases (Regions, rack groups, etc). I think we should revisit these use cases related to storage and access patterns to see if we really need the complexity of the MPTT structure, to begin with.

Justification

The django-mptt project is no longer maintained and as a core dependency, we need to find an alternative path.

TODO

Migrate

  • RackGroup
  • TenantGroup
  • InventoryItem

Footnote: Region is an MPTT model but should be collapsed into Location sometime in this release and either this issue or that one can be the one to "turn the lights off" on MPTT (See: #2517)

@glennmatthews glennmatthews added the type: housekeeping Changes to the application which do not directly impact the end user label Jun 3, 2021
@jathanism
Copy link
Contributor

Sounds like we really need to review for what, exactly, we're using MPTT, and how we can centralize the pattern. We already attacked this to some degree w/ the network model overhaul to decouple form Postgres using the MPTT pattern, but we could go deeper and make it more efficient by establishing some kind of base "tree" model that uses an adjacency list (self-referential foreign key) to a parent, and then have all the tree methods (get_ancestors, get_children, root, parent, etc.).

@glennmatthews
Copy link
Contributor

glennmatthews commented Jun 6, 2022

Requirements

The main requirements, as I understand them are:

  1. Reasonably efficient iteration over ancestors (get_ancestors())
    • nautobot/dcim/templates/dcim/site.html and others - for region in object.region.get_ancestors etc.
  2. Reasonably efficient iteration over children and/or descendants (get_children(), get_descendants())
    • nautobot.dcim.signals.handle_rackgroup_site_change - recursively setting the Site of all child RackGroups when a parent RackGroup changes Site
  3. Reasonably efficient tree membership queries (closely related to the previous two requirements)
    • TreeNodeMultipleChoiceFilter - "get me all Sites that belong to Region ABC or any of its descendants"
    • ConfigContextModelQuerySet - "get me all ConfigContexts that are assigned to Region XYZ or any of its ancestors"
  4. Reasonably efficient QuerySet annotation such as add_related_count
    • RegionListView - "How many Sites belong to each of the following Regions or any of their descendants"
    • RackGroupListView - "How many Racks belong to each of the following RackGroups or any of their descendants"
  5. Reasonably efficient insertion of leaf nodes (adding a new Region, RackGroup, TenantGroup, etc.)
  6. Reasonably efficient relocation of subtrees (changing the parent of a Region, etc.)
  7. Reasonably efficient hard-coded ordering of sibling nodes (e.g. sibling Regions are always ordered by name), as opposed to un-ordered trees
  8. Reasonably efficient tree traversal as QuerySet, respecting sibling ordering (e.g., depth-first ordering of Region QuerySet in order to render it into a hierarchical table)
  9. Reasonably efficient handling of wide trees (hundreds or thousands of sibling nodes under a given parent)
  10. Reasonably simple and foolproof API, easily usable with DRF, GraphQL, etc.
  11. Support for UUID PKs
  12. Compatibility with latest Django and Python versions / ongoing maintenance and support

Non-requirements

  1. Efficient arbitrary tree traversal (e.g. breadth-first search)
  2. Efficient user-selected ordering of sibling nodes (e.g. re-sorting or changing traversal order on the fly, as opposed to influencing order indirectly by changing the value of name or other field that is hard-coded for sorting) - see also Tree model tables should sort sensibly or not be sortable at all #1854
  3. Efficient deeply nested trees
    • Perhaps for IPAddress/Prefix models, where an IPv6 address could theoretically have 127 ancestor Prefixes
    • For Region/Location/RackGroup/TenantGroup, a tree depth of less than 10 would be typical and even 20 would be a rare exception
  4. Insertion of nodes at a specified position relative to their siblings (e.g. direct control over sibling order)

Comparison of approaches

Based on my limited research and understanding to date.

Requirement django-mptt django-treebeard django-tree-queries naive adjacency-list
1. efficient get_ancestors
2. efficient get_children, get_descendants
3. efficient tree membership queries
4. add_related_count feincms/django-tree-queries#21
5. efficient leaf insertion
6. efficient subtree relocation
7. efficient sibling ordering
8. DFS queryset
9. wide trees
10. simple foolproof API
11. UUID PKs
12. compatibility, maintenance ❓last release 2/2021

@jathanism
Copy link
Contributor

jathanism commented Jun 6, 2022

Naive implementation thoughts

Items 1 -3

I believe that this is not as scary as it might sound at first. I know, I invoke this a lot and when it comes to comparing networks to other types of trees there are some built-in assumptions, but generally when it comes to MPTT you have the triangulation of left, right, and node.

Therefore all of the ancestors/descendants/membership queries shouldn't ever be all that expensive, because you will always have the 3-tuple (left, right, node) of the source object at query time.

Item 4

Seeing as this is just an optional annotation, this should also be pretty simple to do.

Item 8

This is going to vary based on how ordering is determined to be done. Obviously, again, with networks it's implied they will be numerical, but as you had hinted if it's done by name or slug, that is actually quite efficient, but not necessarily deterministic. But certainly could be done in respect to the 3-tuple for tree position.

@glennmatthews
Copy link
Contributor

glennmatthews commented Jun 27, 2022

A feature we don't have or use at present is an efficient way to annotate a queryset with the tree path (ordered list of ancestors) of each node in the queryset (versus individually annotating each instance with something like get_ancestors()). To the best of my current understanding this would possibly be feasible with a materialized-path tree library such as one of the options provide by django-treebeard.

This would be useful for features like:

Perhaps django-tree-queries could add something like https://stevestedman.com/2012/02/generating-a-tree-path-with-a-cte/ as well?

@glennmatthews
Copy link
Contributor

glennmatthews commented Jun 28, 2022

Another useful feature I've identified a potential need/benefit for is the ability to select a subtree and/or set of nodes based on ancestor properties - e.g. "get me all of the Locations who have an ancestor of type Campus", "get me the subtrees of Locations descended from Location ABC or Location XYZ", "get me the subtrees of Locations descended from a Location that is linked to Site PDQ".

@glennmatthews glennmatthews added the impact: breaking change This change or feature will remove or replace existing functionality; needs to be an X.0.0 release label Sep 28, 2022
@bryanculver bryanculver changed the title Django MPTT is no longer maintained - need an alternative pattern for tree use cases Django MPTT is no longer maintained - Migrate Existing Models to django-tree-queries Nov 28, 2022
@glennmatthews glennmatthews self-assigned this Dec 15, 2022
@glennmatthews glennmatthews mentioned this issue Dec 15, 2022
9 tasks
@bryanculver bryanculver linked a pull request Dec 16, 2022 that will close this issue
9 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
impact: breaking change This change or feature will remove or replace existing functionality; needs to be an X.0.0 release type: housekeeping Changes to the application which do not directly impact the end user
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants