fix(cli): prevent docs-scraper endpoint-name collisions from dropping routes#3789
Draft
krMaynard wants to merge 1 commit into
Draft
fix(cli): prevent docs-scraper endpoint-name collisions from dropping routes#3789krMaynard wants to merge 1 commit into
krMaynard wants to merge 1 commit into
Conversation
… routes groupByResource disambiguated colliding endpoint names with a counter keyed on the original derived name: the second GET /users became get_users_2. But that generated suffix could equal the *natural* name of another documented route (e.g. GET /users/users_2), and because endpoints are stored in a map, the later assignment silently overwrote the earlier one — a real API operation vanished from the generated spec with no error. Allocate each name against the set of names already used in the resource, incrementing the suffix until an unused name is found. Naming stays deterministic in documentation order, ordinary _2/_3 disambiguation is unchanged, and a natural name that already ends in a numeric suffix can no longer be clobbered. Adds regression coverage for the collision case (three routes, none dropped) and for ordinary deterministic suffixing. Refs #11
Contributor
Merge Protections🔴 2 of 2 protections blocking · waiting on 🤖 CI and 🙋 you
🔴 require-ready-label-and-ciWaiting for
This rule is failing.
🔴 🚦 Auto-queueWaiting for
This rule is failing.When all merge protections are satisfied and these conditions match, this pull request will be queued automatically.
|
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.
Intent
The docs scraper's endpoint-name disambiguation could assign the same generated name to two different documented routes. Because endpoints are stored in a map, the later assignment silently overwrote the earlier one — a real API operation disappeared from the generated spec with no error. It happened when a generated suffix (e.g.
get_users_2for a secondGET /users) collided with another route's natural name (e.g.GET /users/users_2).Issue: Fixes #3786
Approach
In
groupByResource, allocate each endpoint name against the set of names already used in the resource, incrementing the numeric suffix until an unused name is found — instead of counting collisions only by the original derived name. Iterating in documentation order keeps naming deterministic; ordinary_2/_3disambiguation is unchanged; and a natural name that already ends in a numeric suffix can no longer be clobbered.Scope
Primary area:
internal/docspec(documentation-derived spec builder).Why this belongs in this repo: machine code — endpoint-name derivation in the docs-to-spec path, shared by every doc-sourced generation.
Risk
Low. The only behavioral change is in the previously-broken collision case (a route that used to be dropped is now kept under a unique name). Ordinary disambiguation output is identical.
Output Contract
Endpoint-name derivation can affect generated command names, so this is output-relevant.
scripts/golden.sh verifypasses all 32 cases unchanged — no existing fixture exercises the pathological collision, and ordinary suffixing is byte-identical, so the fix only adds a previously-missing endpoint in the collision case. Regression coverage is added at the unit level ininternal/docspecrather than as a new golden fixture.Verification
go test ./internal/docspec/— passes, incl. new cases: three-route collision (none dropped) and ordinary deterministic_2/_3suffixing.scripts/golden.sh verify— 32/32 cases pass (no generated-output drift).go build ./...,go vet ./internal/docspec/,gofmt -l— clean.Generator/template change: verified generated output via
scripts/golden.sh verify(32 cases) plus unit regression tests on the naming logicGenerator/template change: covered the collision variant and ordinary-suffix variant, not only happy path
Generator/template change: change is localized to name allocation in
groupByResource; no emitted definition/call-site gates involvedAI / Automation Disclosure