Repository navigation
Walk the graph breadth first so depth is the shortest path - #85
Merged
Merged
Conversation
A depth-first walk measures the depth of the route it took, so a graph with links back to its parents fails with "nested more than 128 levels deep" even when every object is a few levels from the root. A valid store of 25 orders and a valid ring of 200 objects both fail. The specs cover the rules of a breadth-first walk: the depth is the shortest path, an object that two routes reach is reported with the shorter route, a depth error is reported once and is never false, and the results come shallowest first. Twelve of the fifteen tests fail on the depth-first walk, without a stack overflow. Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
The depth-first walk measured the depth of the route it took. A graph with links back to its parents, such as the navigation properties of an ORM model, has a long route even when every object is a few levels from the root, and a valid store of 25 orders failed with a false "nested more than 128 levels deep" error. The walk now keeps a queue and calls nothing recursively. An object is marked when it is queued, so the first path that reaches it is the shortest one, and a depth error is always true. A collection that a property holds is a queue step of its own, so every step is one level. Paths are chains of steps and become strings only for an object that has a result. The limit of 128 stays, as a stop for a computed property that builds a new object on each read. The results come shallowest first, so Items[0].Name now follows Last.Name in a model with First, Items and Last. An object that two routes reach is reported with its shorter path. Ties keep the first property. Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
…paths Add an "Order of results" section to the README, say that the depth is the shortest path, and drop the changelog note about stack use for deep collections, which no longer holds. Add a BREAKING changelog entry for the result order, the shortest path in a member name and the depth rule. Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
The Equals-ancestor stop marks an object on its shortest path, so a longer path that passes no equal ancestor never walks it, and an invalid object below it passes. v2.3.3 found it for LongFirst. The two specs for it fail on the breadth-first walk. A third spec, EfRegionCopy, records a known limit and is skipped. Also pin what already holds: an object that two paths stop at is validated once, an equal ancestor at level 129 gets no depth error, a level is finished for all parents before the next one starts, and a collection that a property holds is enumerated when its step is reached, not when the property is read. Move the result-order test from UpgradeCompatibilityTests, which checks the 2.2 contract, to BreadthFirstWalkTests, because the order changed in 3.0. Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
The Equals-ancestor check ran when an object came out of the queue, after the object was marked on its shortest path. If that path passed an equal ancestor, the object was validated but not walked, and every longer path to it was dropped, so an invalid object below it passed. v2.3.3 found it when the first path in property order passed no equal ancestor. Check for an equal ancestor when the object is queued, and do not mark an object that stops. It is validated once, however many paths stop at it. A later path that passes no equal ancestor queues it and walks it, and skips the validation. That also finds the object when the shortest path is the first in property order, which v2.3.3 missed. An object that every path reaches through an equal ancestor is still not walked. That is a known limit (EfRegionCopy, skipped). Also fix three comments, and say that the depth limit stops one new object per read. Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
…ming Update the README for the equal-ancestor rule, the order of items that are collections, and the limit of the depth check for a property that builds two or more new objects per read. Add two BREAKING changelog bullets: the equal-ancestor rule with its known limit, and the later enumeration of a collection that a property holds. Make two examples clearer and remove a duplicate definition in a test summary. Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
Since the maximum depth, a guard that breaks makes the walk fail with a depth error and does not overflow the stack. Say so, and keep the note about what happened before 3.0. The comments about a record's generated Equals are unchanged, because that recursion is still real. Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
The maximum depth added in #83 counted the length of the route the depth-first walk happened to take. A graph with links back to its parents, such as the navigation properties of an ORM model, has a long route even when every object is a few levels from the root. A valid store of 25 orders failed with a false "nested more than 128 levels deep" error, and so did a valid ring of 200 objects. Each of those objects was in fact validated by another route, so the message was untrue and the validation failed for a valid model.
The walk is now breadth first, so the first path that reaches an object is its shortest one and a depth error is always true. It also uses a queue and no recursion, so the depth of a graph no longer uses the stack.
Summary
The commits follow the usual order for this repo: specs, then the fix, then docs, twice (the rewrite, then the fixes from review).
Test plan
-p:IncludeNetFramework=true)