feat(api): return skill labels from the skill listing endpoints on request - #730
Conversation
…quest
Skill labels were reachable only one skill at a time, through
/api/{v1,web}/skills/{namespace}/{slug}/labels, so a client rendering a list had
to issue a follow-up request per row.
Add includeLabels=true to GET /api/v1/skills and GET /api/web/skills. The labels
array is populated only when the parameter is set and left out of the payload
otherwise, so existing responses are byte-identical.
Labels for the whole page are resolved by SkillLabelProjectionService in three
queries — assignments, definitions, translations — rather than three per skill.
Closes #710
Signed-off-by: FenjuFu <fufenjupku@gmail.com>
24af499 to
3329900
Compare
XiaoSeS
left a comment
There was a problem hiding this comment.
维护者建议调整 API 形状:请把当前 includeLabels=true 改成通用扩展参数 include=labels。
原因:
- 这是列表接口的可选关联展开能力,不应为每个可选关系增加一个布尔参数。
- 后续如果还要展开 author、stats、versions 等字段,可以自然扩展为
include=labels,stats,API 兼容性更好。 - 当前 PR 尚未合入,直接调整参数名不会破坏已发布行为。
建议实现边界:
- 支持
GET /api/v1/skills?include=labels。 - 支持
GET /api/web/skills?include=labels。 - 默认不传
include时响应保持现状,不返回 labels。 - 当前只接受
labels;未知 include 值建议返回 400,避免调用方拼错参数后静默降级。 - 同步更新测试和 OpenAPI 生成文件。
这个调整完成后,再按维护者验证流程做本地镜像/runtime 验证。
Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
|
@XiaoSeS Thanks for pushing the API-shape follow-ups. I reviewed the current head fc448c5: both list endpoints now use include=labels, unsupported include values return 400 before search work begins, tests cover the parser/controller behavior, and the generated OpenAPI file is updated. All remote checks, DCO, and CLA are green. Your earlier changes-requested review is still the active review decision; could you re-review or dismiss it now that your two commits are on the branch? |
XiaoSeS
left a comment
There was a problem hiding this comment.
已按维护者建议完成调整并验证:\n\n- API 参数从 includeLabels=true 改为通用扩展参数 include=labels。\n- /api/v1/skills 与 /api/web/skills 默认响应保持不返回 labels。\n- include=labels 返回 labels 数组;未知 include 值返回 400。\n- 已补充 controller/parser/projection 测试并同步 OpenAPI 生成文件。\n- 本地源码验证、latest main + PR head 的 release Compose 运行态验证均通过。\n\n本轮 review 解除前置 changes-requested,后续可按维护者合并流程处理。
Summary
GET /api/v1/skillsandGET /api/web/skillsacceptincludeLabels=trueand return each skill's labels inline./api/{v1,web}/skills/{namespace}/{slug}/labels, so a client rendering a list had to issue a follow-up request per row.{ "items": [ { "slug": "global/demo-skill", "displayName": "Demo Skill", "labels": [ { "slug": "audited", "type": "PRIVILEGED", "displayName": "Audited" }, { "slug": "automation", "type": "RECOMMENDED", "displayName": "Automation" } ] } ], "nextCursor": null }The issue named
/api/v1/skillsfirst and/api/web/skillsas the alternative, so both surfaces take the parameter. The payload shape is the existingSkillLabelDto(slug,type,displayName), same as the per-skill labels endpoint, with the display name localized throughLabelLocalizationServiceexactly as it is there.The default response does not change
labelsisnullunless the caller opts in, and both records carry@JsonInclude(NON_NULL)on that component only — not on the record — so no other nullable field starts being dropped. WithoutincludeLabels, responses are byte-identical to today, which matters for/api/v1/skills: it is the ClawHub compatibility surface with legacy clients on it.SkillSummaryResponseandClawHubSkillListResponse.SkillListItemeach keep a constructor at the previous arity that delegates withnull, so the seven existing construction sites are untouched.One batch, not one per row
Naively reusing the per-skill path would issue three queries per row — 75 for a default page of 25.
SkillLabelProjectionServiceresolves the whole page in three: assignments (findBySkillIdIn), definitions, translations. The unit test asserts that arity directly withverify(..., times(1)), so a regression to N+1 fails the build rather than quietly slowing the endpoint down.Validation
I could not run
mvn test— no JDK is available on the machine this was written on, so CI is the first real execution. Tests are included and I would ask a reviewer to weight CI over my checklist here:SkillLabelProjectionServiceTest— per-skill grouping, thetype-then-slugordering, assignments whose definition is missing, empty/null pages touching no repository, and the three-query batch arity.SkillSearchControllerTest—labelsabsent by default, populated withincludeLabels=true, and an empty array for a skill that has none.ClawHubCompatAppServiceTest— the same two cases on the/api/v1/skillscompatibility surface.Risk
includeLabelssee no change.skill_labeland the label definition tables are read as they already are.Notes
SkillLabelAppService.toDtosnow overlaps withSkillLabelProjectionService.toDto. Folding the former into the projection service would mean changingSkillLabelAppService's constructor, which I left alone to keep this diff to the feature; worth doing as a separate cleanup.