Skip to content

FIX/TEST: Invalid moves#2102

Open
ianhi wants to merge 5 commits intomainfrom
ian/fewer-moves
Open

FIX/TEST: Invalid moves#2102
ianhi wants to merge 5 commits intomainfrom
ian/fewer-moves

Conversation

@ianhi
Copy link
Copy Markdown
Collaborator

@ianhi ianhi commented Apr 29, 2026

Fixed two bugs with move.

  1. You could move a group inside of itself - including the root group!
  2. you could move a node to have an array as it's parent

Added a descriptive error message for each of these to help the user.

Also we didn't have any hypothesis tests exercising invalid move paths so I've added those - which would have caught the above bugs.

ianhi added 3 commits April 27, 2026 16:42
Previously, `session.move("/a", "/a/c")` silently succeeded but produced an
unrepresentable move: `/a` would need to be both an ancestor and a descendant
of itself. Rendering the resulting tree dropped the whole subtree.

Reject these moves up front with `MoveIntoSelfOrDescendant`. Also introduce
`MoveDestinationParentMissing` to give a move-specific message when the
destination's parent doesn't exist (previously surfaced as the generic
`AncestorNodeNotFound`).

Hypothesis coverage gap: `valid_moves` deliberately excluded `source` and its
descendants from destination candidates, so these malformed moves were never
generated. Add `invalid_moves`, `tree_and_invalid_moves`, and
`tree_and_mixed_moves` strategies that produce moves tagged with the expected
rejection reason (or `None` for valid moves), covering self, descendant,
missing-parent, overwrite, and source-missing cases.
from __future__ import annotations

import itertools
import posixpath
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use pathlib.PurePosixPath instead?
https://docs.python.org/3/library/pathlib.html#pathlib.PurePosixPath
(which is likely overkill, and posixpath is already better than f-strings from before, so just a suggestion, not a strong requirement)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants