feat(external-api): knowledge base resource - #2132
aliaska-varieva wants to merge 1 commit into
Conversation
Expose the knowledge base on the External API under /api/v1/knowledge-base, running the shared api-lib domain services (KnowledgeBaseService, KnowledgeBaseTagService, KnowledgeBaseAttachmentService) in-process on behalf of the API key owner, the same way the tickets resource does. - KnowledgeBaseController: list/read/move items, add/remove tag, folder tree, create/rename/delete folder, archived articles, create/update article, publish/unpublish/archive/unarchive, tags, attachment upload/download/delete - KnowledgeBaseReadService batches tags + attachment metadata per page and turns unknown ids into 404 (the domain services throw a generic IllegalArgumentException that GraphQL surfaces as a payload error) - KnowledgeBaseMapper + REST DTOs; customers exposed under the product name - ErrorCode.KNOWLEDGE_BASE_ITEM_NOT_FOUND - KnowledgeBaseTagService.addTagToItem is now idempotent, mirroring the ticket tag service, so repeated public-API calls do not create duplicate assignments - OpenAPI description lists the resource; API version bumped to 1.2.0
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| public Tag requireTag(String tagId) { | ||
| return tagRepository.findById(tagId) | ||
| .filter(tag -> tag.getEntityType() == TagEntityType.KNOWLEDGE_ARTICLE) | ||
| .orElseThrow(() -> new NotFoundException(ErrorCode.TAG_NOT_FOUND, "Tag not found: " + tagId)); | ||
| } |
There was a problem hiding this comment.
🦩 🟠 [warn/recommended] OFJAVA-013 KnowledgeBaseReadService.requireTag throws generic NotFoundException instead of typed KnowledgeBaseTagNotFoundException
requireTag throws com.openframe.core.exception.NotFoundException directly with ErrorCode.TAG_NOT_FOUND, rather than a dedicated exception type mirroring the KnowledgeBaseItemNotFoundException pattern introduced in this same PR. This is inconsistent with the pattern established by requireItem/requireItem(id, type) in the same class and with OPENFRAM-003-14's requirement that domain error conditions use named exception classes. It also means the 404-vs-other-tag-types nuance (a tag belonging to a different TagEntityType) is indistinguishable from a truly nonexistent tag at the exception-type level.
Evidence
public Tag requireTag(String tagId) {
return tagRepository.findById(tagId)
.filter(tag -> tag.getEntityType() == TagEntityType.KNOWLEDGE_ARTICLE)
.orElseThrow(() -> new NotFoundException(ErrorCode.TAG_NOT_FOUND, "Tag not found: " + tagId));
}
🤖 Prompt for AI agents
In openframe-external-api-service-core/src/main/java/com/openframe/external/service/KnowledgeBaseReadService.java around lines 55-59, address this code-review finding: KnowledgeBaseReadService.requireTag throws generic NotFoundException instead of typed KnowledgeBaseTagNotFoundException.
requireTag throws com.openframe.core.exception.NotFoundException directly with ErrorCode.TAG_NOT_FOUND, rather than a dedicated exception type mirroring the KnowledgeBaseItemNotFoundException pattern introduced in this same PR. This is inconsistent with the pattern established by requireItem/requireItem(id, type) in the same class and with OPENFRAM-003-14's requirement that domain error conditions use named exception classes. It also means the 404-vs-other-tag-types nuance (a tag belonging to a different TagEntityType) is indistinguishable from a truly nonexistent tag at the exception-type level.
The flagged code:
```
public Tag requireTag(String tagId) {
return tagRepository.findById(tagId)
.filter(tag -> tag.getEntityType() == TagEntityType.KNOWLEDGE_ARTICLE)
.orElseThrow(() -> new NotFoundException(ErrorCode.TAG_NOT_FOUND, "Tag not found: " + tagId));
}
```
Make the minimal change that resolves the finding; do not refactor unrelated code.
confidence: 45 — react 👍/👎 to teach the reviewer
🦩 Flamingo Code Review1 finding(s) — 0 action required · 1 recommended · 0 informational Mode: advisory · Rules cited: Need another pass? Commits pushed after this review are not reviewed automatically.
Prefer typing? Comment React 👍/👎 on inline comments to teach the reviewer. |
1 participant
Summary
Exposes the knowledge base on the External API under
/api/v1/knowledge-base, following the tickets pattern: the controller runs the shared api-lib domain services (KnowledgeBaseService,KnowledgeBaseTagService,KnowledgeBaseAttachmentService) in-process on behalf of the API key owner. No domain logic is duplicated.External API (openframe-external-api-service-core)
KnowledgeBaseController— 21 operations:parentId/type/statuses/tagIds/search), read, move, add/remove tagchildrenAction=MOVE|ARCHIVE)KnowledgeBaseReadServicebatches tags + attachment metadata per page and turns unknown ids into 404. The domain services throw a genericIllegalArgumentExceptionfor a missing item (GraphQL surfaces it as a payload error); a REST contract needs a proper 404.KnowledgeBaseMapper+ REST DTOs indto/knowledgebase; customer ids exposed under the product name as in the tickets API. List responses omit articlecontent; single-item reads include it.Shared code
ErrorCode.KNOWLEDGE_BASE_ITEM_NOT_FOUNDKnowledgeBaseTagService.addTagToItemis now idempotent (mirrorsTicketTagService), so a repeated public-API call does not create duplicate tag assignments.Not included
ExternalKnowledgeBaseTestsuite).Test plan
mvn -o compile -pl openframe-external-api-service-core -amfrom sourcemvn -o compile -pl openframe-api-service-core -am(consumer of the api-lib change)/external-api/swagger-ui