Repository navigation
fix(server): delete a branch named like a static branch route - #1091
Conversation
Branch names `merge`, `diff` and `merge-base` are valid, but the router
matches the static routes `/branches/merge`, `/branches/diff` and
`/branches/merge-base` before `/branches/{branch}`, so deleting such a
branch over HTTP answered 405 and the branch could not be removed. Each
static route now also answers DELETE by deleting the branch it spells,
through the same body as `DELETE /branches/{branch}`. The delete flow
test creates and deletes `merge`, `diff` and `merge-base` beside
`feature`; it fails with the 405 against the old router.
|
Recommendation: request changes. Review of head Why
What this change doesEach static route now also serves DELETE. The closure calls
Affected classThe broken contract is one rule. Every live branch name that create accepts can be removed with Only three requests break that rule today. The raw path segment is exactly These paths are a different case:
Create refuses an empty name, surrounding whitespace, a control character, Blocking
The three DELETE closures make today's delete succeed. They leave two routes on one URL. The static route still wins. The next method on Register one route,
Keep the current URLs. Keep Tradeoff and liabilityThis pull request removes a real hole. A remote It adds a copy. Five more static actions under One dispatcher is still a special case. It lives in one match, at the router that owns the clash, and the deleted name comes from the path. The path move in #1089 removes the special case. That is the right long-term change. It is out of scope for a fix that must keep current clients on these URLs. Spec and docsOpenAPI and the branch guide need no change for this bug. A gateway that selects the concrete path Sibling pull requests#1089 records this 405 as a separate bug and schedules the path move later. This pull request matches that sequence. The closure pattern should not become the way the next clash is fixed. #1076 renames the path extractor to ValidationReviewed head GQT cannot express this contract. The owner is On the head:
Result: On the head, exact names:
Before, the same delete tests with
A status probe with no branch created, old router:
The same probe on the head: the three literal names answer 404 A separate matchit 0.8.4 probe confirmed the path choices for Limitations: I did not run the full server suite, Clippy, or a live CLI process. Temporary tests were removed. Nothing was pushed. |
The previous fix gave each static branch action a DELETE closure, which
left two routes on one URL: the next method on `/branches/{branch}` or
the next static sibling would answer 405 again unless a closure were
copied, and each deleted name was a literal beside its path. One route
now owns `/branches/{branch}`: POST of `merge` runs the merge, GET of
`diff` or `merge-base` runs that read (HEAD follows GET), DELETE of any
captured name deletes that branch through the one delete body, and every
other method and name answers 405 with an `Allow` header naming what that
name serves. URLs, the spec and `branch_delete_body` are unchanged.
|
The blocking finding on WhyA branch may be named What this commit does
The URLs, the OpenAPI paths, and Optional
Tradeoff and liabilityThe violated contract is the HTTP interface: This removes the precedence hole and the three copied closures. The next action is one match arm, not a new static sibling. Five more actions stay in one function. The remaining special case is the name match, and the path move in #1089 removes it. Keeping the current URLs is the smaller break. A path move in this pull request would break current clients. The extra work on this path is one string match. That cost is not material. Correctness of the URL is the goal. Simplicity of a later path move stays available. This change does not add a second source of truth for branch identity, policy, or publication. Other collectionsI read the route table in
Callers, policy, and the spec
The CLI remote delete is OpenAPI still describes the operations clients call: One spelling changed with the single route. The handler sees the decoded segment. Sibling pull requests#1089 says this 405 is a separate bug, and it forbids a static segment beside a parameter. This router no longer has that pair under #1076 adds ValidationReviewed head GQT cannot express this contract. A Before, that test, with
Result: failed. Panic at A temporary probe on that same router, then removed:
After, exact These also passed on that head:
A temporary matrix on the head, then removed, also passed. It deleted Limitations: I did not run the full server suite, a live CLI process, or DST. Temporary tests were removed. Nothing was pushed. |
Brings in branch creation at a commit; the delete flow test's create requests name the new `from_commit` field. The branch route also takes the review's optional notes. Each action's name, method and handler now live in one `BranchAction` enum. The route keeps DELETE and sends every other method to one fallback, which runs the action the name and method select (HEAD runs a GET action) or answers 405 with that name's `Allow`, so PUT and PATCH name the right methods too. The release note and the test comment describe this route instead of the closures it replaced.
|
Both optional notes addressed in c7f780c, which also merges
|
|
Recommendation: fix the WhyA branch may be named What
|
Why
merge,diffandmerge-baseare valid branch names. The router matches the static routes/branches/merge,/branches/diffand/branches/merge-basebefore/branches/{branch}. SoDELETE /graphs/{graph_id}/branches/mergereached the POST-only merge route and answered 405, and such a branch could not be deleted over HTTP. An API audit found this, and the HTTP API conventions draft (#1089) records the rule that keeps it from recurring: no static segment beside a parameter.What
Each of the three static routes now also answers DELETE, which deletes the branch it spells. It goes through
server_branch_delete_named, the bodyDELETE /branches/{branch}already runs: the same policy check, admission and engine call. The spec is unchanged, since the operation is the documentedDELETE /branches/{branch}.Tests
data_routes::remote_branch_delete_flow_worksnow creates and deletesfeature,merge,diffandmerge-base. Against the old router it fails on the 405, whose body is not JSON. With the fix it passes.openapi::tests pass, and clippy passes with-D warnings.Backing issue / RFC