Normalize issue ids in relation getters - #210
Open
mkwlsn wants to merge 1 commit into
Open
Conversation
GetIssue normalizes a bare id, but six relation getters did not, so callers that pass the raw argument resolve the issue and then silently get nothing back for its relations. The visible case is `td show`. cmd/show.go:113 keeps the raw arg in issueID. GetIssue on line 138 succeeds because it normalizes its own local copy, leaving issueID bare. Lines 144-158 pass that to GetLogs, GetLatestHandoff, GetLinkedFiles, GetDependencies, GetBlockedBy, and GetStartSnapshot, each of which queries `abc123` against rows keyed `td-abc123`. Every one returns empty, and every call discards its error, so nothing surfaces. Result: `td show abc123` and `td show td-abc123` return the same issue, but the bare form omits CURRENT HANDOFF, SESSION LOG, GIT STATE, and the BLOCKED BY / BLOCKS edges. On a real issue that was 24 lines against 53. The output ends cleanly, so it reads as complete, and a blocked issue appears unblocked. Fixed in the DB layer rather than at the callsite, matching how AddComment, AddHandoff, and about forty other call sites already normalize. A callsite fix would leave the next caller of these getters exposed to the same bug. marcus#199 named GetLatestHandoff as part of the same defect. Its write paths now normalize, so `td comment abc123` works, but the read path it described did not. Test fails without the change, with bare returning 0 where prefixed returns 1. make fmt clean, make test green.
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
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.
GetIssuenormalizes a bare id, but six relation getters do not. A caller that passes the raw argument resolves the issue and then silently gets nothing back for its relations.Repro
Both return the same issue with an identical first line. On a real issue the bare form printed 24 lines and the prefixed form 53. There is no error, and the output ends cleanly on SESSIONS INVOLVED, so it reads as a complete record. A blocked issue shows no blocker.
Root cause
cmd/show.go:113keeps the raw argument:GetIssueon line 138 succeeds because it normalizes its own local copy, soissueIDis still bare. Lines 144 to 158 pass it toGetLogs,GetLatestHandoff,GetLinkedFiles,GetDependencies,GetBlockedBy, andGetStartSnapshot. Each queriesabc123against rows keyedtd-abc123and returns empty. Each call discards its error with_, so nothing surfaces.The fix
Normalize in the six getters, matching how
AddComment,AddHandoff, and about forty other call sites ininternal/dbalready do it.I went with the DB layer rather than
issueID = issue.IDat the callsite because the callsite fix leaves the next caller of these getters exposed to the same bug. Happy to switch to the one-line version if you would rather keep normalization at the CLI boundary.Relationship to #199
#199 named
GetLatestHandoff(issueID)as part of this defect. Its write paths now normalize, sotd comment abc123succeeds and storestd-abc123correctly. The read path it described was still open, and that is what this fixes. I have not used a closing keyword since the scope of that issue is yours to judge.Tests
internal/db/bare_id_relations_test.gocoversGetDependencies,GetBlockedBy, andGetLogs, asserting the bare and prefixed forms agree. Without the change it fails:make fmtclean.make testgreen across all packages, including the e2e suite.Note on scope
td dep addstill rejects bare ids withFOREIGN KEY constraint failed, which is a loud failure rather than a silent one, so I left it alone. Worth deciding separately whether bare ids should be accepted uniformly or rejected uniformly. Right nowtd depandtd treeaccept them,td dep addrejects them, andtd showaccepted them while quietly returning less.