feat: link tags - #1
Conversation
|
Warning Review limit reachedNext included review available in 47 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (9)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (14)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe SDK adds tag CRUD operations, link tagging and filtering, bulk tag updates, tag filters for statistics and exports, and documentation and tests for these capabilities. ChangesTag support
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This change adds SDK tag management, link tagging, filtering, bulk updates, and tag-aware stats and exports. The covered request and response behaviors are ready to merge with no identified current risk. Sequence Diagram(s)sequenceDiagram
participant Application
participant spoo.Client
participant c.do
participant TagsAPI
Application->>spoo.Client: Call CreateTag, UpdateTag, or DeleteTag
spoo.Client->>c.do: Build and send typed request
c.do->>TagsAPI: Issue HTTP request to /api/v1/tags
TagsAPI-->>c.do: Return tag response or error
c.do-->>spoo.Client: Decode typed result
spoo.Client-->>Application: Return result or error
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 12 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Zingzy
left a comment
There was a problem hiding this comment.
Verdict
Mergeable. Nothing blocks. The wire contract matches the live v2.4.0 spec on every path, method, query key and body field I traced, and the tri-state handling on UpdateURLParams.TagIDs does exactly what the API needs (list replaces, [] or null clears, omitted keeps). One design call to make on the BulkTags signature before it is frozen into a public API; the rest is small.
Checked against the real thing, not the PR body: diffed origin/main...origin/feat/link-tags, read every Go file in full, ran go vet ./... and go test -race -count=1 ./... at caba6aa (both green), and compared the committed openapi.json to upstream spoo/main (byte-for-byte equal as parsed JSON, so the 2,260-line refresh is exactly what the drift job requires and nothing else).
Contract trace
Every item below was verified by reading the SDK code and its test assertions against the spec, not the description.
POST /api/v1/shorten:tag_idsomitted when empty (shorten.go:23, asserted inTestShortenTagIDs). Correct, since on create an absent list and an empty list mean the same thing.PATCH /api/v1/urls/{id}:Opt[[]string]withomitzero(urls.go:256).TestUpdateURLTagIDspins all four bodies:{"tag_ids":["t1","t2"]},{"tag_ids":[]},{"tag_ids":null}, and omitted. Matches the spec text "the list replaces the stored one; null or [] clears it".GET /api/v1/urlsfilter JSON:tagIds,tagNamesas arrays,tagsMatchas a string, riding the existingfilterparam (urls.go:118-126). Matches the filter param description in the spec.GET /api/v1/statsandGET /api/v1/export:tagandtag_idgo through the existingvalues()comma-join, which is the format both query params document. Per-link routes reject them before any request goes out (stats.go:117-124), andTestPerLinkCallsRejectTagFiltersproves no request is sent.POST /api/v1/urls/bulk/tags:{ids, add, remove}with emptyadd/removeomitted (bulk.go:88-92). The spec marks onlyidsrequired, so omitting is right.PATCH /api/v1/tags/{id}:UpdateTagParamsuses plain strings withomitempty, so the SDK can never emiticon: null(a 422 upstream). Good call, and consistent with howUpdateURLParamsuses plain strings forLongURLandAliasand reservesOptfor fields where null means something.- Decoding:
Tag.Color,Tag.Icon,TagRef.Color,TagRef.Iconare plain strings, so a palette or icon key added server-side later decodes without a release.updated_at: nulldecodes to a zeroTimestamp(TestListTagsUnwrapsItems). - Response types:
tagsis present onUrlResponse,UrlListItemandUpdateUrlResponsein the spec and onShortURL,URLItemandUpdatedURLin the SDK.ClaimUrlsResponseand the public stats envelope carry no tags upstream, so nothing is missing.
Blocker
None.
Should-decide
1. BulkTags(ctx, ids, add, remove []string) has two adjacent same-typed slice parameters (bulk.go:87). c.BulkTags(ctx, ids, x, nil) and c.BulkTags(ctx, ids, nil, x) both compile and both look right at the call site; the second one strips tags the caller meant to add. The sibling bulk methods dodge this because their second argument is a string or a time.Time, so a swap fails to compile. This is the only bulk method that mutates in two opposite directions from one call, and it is the one where a swap is silent. Blast radius is the caller's own links (recoverable, 200 OK, no error), so should-decide rather than blocker, but a signature is the one thing an SDK cannot fix after v1 users pick it up. Fix: BulkTags(ctx, ids, BulkTagChange{Add: ..., Remove: ...}) with a two-field struct, one request as today, named at the call site. README and the test adjust in place.
Nits
2. Doc claims a 422 the spec does not state (stats.go:117, stats.go:142-144, export.go:43-44). The per-link stats route description says only that short_code and url_id "do not exist here"; it says nothing about tag or tag_id, and its filters dimension list simply omits them. Rejecting client-side is the right behaviour either way (a silently ignored filter would return unfiltered data as if it were filtered), but the comments assert a server status code for tag/tag_id that the contract does not promise. Say "not accepted on the per-link routes" and drop the 422 for the tag pair.
3. The Tag doc comment hard-codes the palette (tags.go:10-12). The type keeps Color and Icon as strings precisely so a new key decodes without an SDK release; the doc enumerating nine colours will be stale the first time that happens and nobody will remember to bump it. Name one or two as examples and point at the API docs for the current set.
4. Spelling register (README.md:123, tags.go:33, three "colour" in prose). The repo writes American everywhere (serialize in opt.go and timestamp.go, behavior in version_test.go) and the field is Color. The API spec descriptions use British, which is probably where it came from, but the SDK prose should match itself.
5. Set([]string(nil)) marshals as null (urls.go:244-246). Behaviourally identical to Null[[]string]() since both clear upstream, so no bug. One clause in the UpdateURLParams doc ("a nil slice clears like Null") saves someone a test.
Public text
Clean. Grepped every added line, the commit message and the PR body for em-dashes, en-dashes, AI or agent mentions, and ticket ids: none. The CodeRabbit block in the PR body is bot-appended, not author text.
Comments
Fit the house style. // zero until the tag is first edited on Tag.UpdatedAt states a nullable the type cannot show, and the one-line headers on the new tests match the ones already in shorten_test.go and bulk_test.go. Nothing narrates a change or talks to the reviewer.
Merge and release
Merging publishes nothing; a Go module version tag does. The API side is live for every account with no flag, so once a tag is cut this is usable immediately. The Version = "dev" telemetry string is a pre-existing issue and not this PR's to fix, but it will be visible on the first request users make with the new methods.
Good
- Every new test asserts the exact request body or query as a string, not just "a request happened".
TestUpdateURLTagIDspinning all four bodies is the kind of test that catches a futureomitempty/omitzeromixup. UpdateTagParamsnot reaching forOptwas the right instinct;Optstays where null carries meaning, exactly as its doc comment promises.TagDeletionnext toDomainDeletion,ListTagsunwrappingitemsthe same way the other list calls do, path ids throughurl.PathEscapelikeUpdateURL. Nothing here reads as if a different person wrote it.
Tags went live on the API in spoo v2.4.0 and this SDK had no way to reach them. Adds the tag endpoints, tag_ids on shorten and update, tags on every link response, the tagIds/tagNames/tagsMatch list filters, the tag and tag_id stats and export filters, and BulkUpdateTags next to the other bulk methods. Refreshes openapi.json so spec drift stays green.
|
Applied all five findings in the same commit.
gofmt, go vet, go test -race and golangci-lint are all green at the new head. |
caba6aa to
21d0c11
Compare
What
TagandTagReftypes;ListTags,CreateTag,UpdateTag(PATCH, every field optional),DeleteTag(reportslinks_updated)TagIDsonShortenRequest(omitted when empty) andUpdateURLParams(Opt[[]string]:Setreplaces the list,Set([]string{})orNullclears it, omitted keeps it)Tags []TagRefonShortURL,URLItemandUpdatedURLTagIDs,TagNames,TagsMatchonListURLsOptions, serialised into the existingfilterJSON astagIds,tagNames,tagsMatchtagandtag_idstats and export filters throughStatsQuery.Filters, comma-joined like the other filters; rejected client-side on the per-link routes likeshort_codeandurl_idBulkTags(ctx, ids, add, remove)posting{ids, add, remove}to/api/v1/urls/bulk/tags, returning*BulkResultopenapi.jsonrefreshed from upstream main so the spec drift job passesWhy
Tags are live on the API (spoo v2.4.0) and this SDK is how people script against it. Without these methods the only route was the raw request passthroughs.
How proven
The 14 new tests (
go test -run 'Tag|BulkTags' -v .) all pass and assert method, path, query and exact body againsthttptestservers. Not run against production.Summary by CodeRabbit