feat(external-api): knowledge base resource - #2132
Merged
Merged
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}
Comment on lines
+55
to
+59
| 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)); | ||
| } |
Contributor
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
Contributor
🦩 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. |
- attachment download-url/delete answer 404 (KNOWLEDGE_BASE_ATTACHMENT_NOT_FOUND) for an unknown id instead of the domain's generic 400 - requireTag throws a typed KnowledgeBaseTagNotFoundException, matching KnowledgeBaseItemNotFoundException - cap attachment fileName at 255 chars (it becomes part of the storage path)
Adds ExternalApiMockMvc, a standalone MockMvc wired like the deployed app (ApiCaller resolver, both exception handlers, bean + method validation, the module's Jackson setup), and covers every knowledge base endpoint: happy paths with domain-call arguments, 404/400/409 error contract, content omitted from lists, tag and attachment pre-checks.
…roxy and infrastructure Controller tests (MockMvc) for every endpoint of the existing resources, mapper and read-service unit tests, RestProxyService forwarding against a loopback server, and the cross-cutting pieces: ApiKeyPrincipalResolver, ApiCaller binding, ExternalCursors, exception handlers, Jackson and OpenAPI config.
…cs and proxy gaps Found while covering the module with tests. Nothing is removed or renamed; successful requests behave as before. - tickets: add/remove tag and add note on an unknown ticket answer 404 TICKET_NOT_FOUND instead of a generic 400; note update/delete check that the note belongs to the ticket in the path and answer 404 TICKET_NOTE_NOT_FOUND otherwise (the ticket id used to be ignored). Pre-checks live in TicketReadService, so the shared domain services still report these the way the dashboard's GraphQL layer expects - customers: can-archive and status answer 404 CUSTOMER_NOT_FOUND for an unknown customer, as documented, instead of 400 - devices: @Valid on the status/nickname bodies, so a missing status is a 400 at the REST layer instead of reaching the domain as null - OpenAPI: the common 400/401/429/500 responses are a GlobalOpenApiCustomizer; a plain OpenApiCustomizer bean is not applied to the grouped docs Swagger UI serves - integrations proxy: HEAD is forwarded instead of failing with 500 - Jackson: instants always carry three fraction digits (…05.000Z, not …05Z) - mappers: null item/organization lists map to empty lists; null ticket filters map to empty lists like empty ones; device tag createdBy is mapped (TagService.buildTag carries TagAssignment.taggedBy)
ProxyUrlResolver.resolve rebuilds the target from the decoded path and query, so a percent-encoded reserved character changes meaning on the way to the tool (q=a%26b arrives as q=a&b, %2B as +, %2F inside a segment as a separator). resolve() itself is untouched: the gateway routes all Fleet / MeshCentral / tool traffic through it, and its output stays byte-identical (now pinned by ProxyUrlResolverTest). The new opt-in resolvePreservingEncoding() forwards the raw path and query, delegates to resolve() for anything it cannot handle on the raw form, and is used by the external API tools proxy only.
…tion errors Same classes of problem as found in the external API, on the dashboard side: - organizations: can-archive and status answer 404 ORGANIZATION_NOT_FOUND for an unknown id instead of 400 (same message; other domain refusals stay 400) - devices: @Valid on the status body, so a missing status is reported on the "status" field at the REST layer instead of as a service parameter violation - GraphQL: a ConstraintViolationException from a @validated data fetcher is a VALIDATION_ERROR carrying the constraint message, not a generic INTERNAL_ERROR
romanivan-flamingo
approved these changes
Sep 20, 2026
aliaska-varieva
added a commit
that referenced
this pull request
Sep 21, 2026
Follow-up to #2188, after testing deviceLogs against qa. Drop the regex filter. Search is now words in, words out: `contains` requires every term, `excludes` rejects lines holding one, both matched literally and case-insensitively. Regex bought little that those two do not, and cost a dependency to validate, a second syntax for users, and patterns the edge proxy rejects before they reach the service. It shipped in 6.34.21 but no client uses it yet, so removing it now is free. Drop the field name from the @SiZe messages on DeviceLogFilterInput: the GraphQL error handler already prefixes the offending field, so qa answered "contains: contains cannot hold more than 5 terms". Add GraphQLExceptionHandlerTest over the bean-validation branch (400 with the violation message) and the unexpected-failure branch (500, message withheld). That branch reached main in #2132 after 6.34.21 was cut, which is why qa still returned INTERNAL_ERROR for six search terms when I tested.
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
2 participants
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.
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 item, tag and attachment ids into 404 (typedKnowledgeBase{Item,Tag,Attachment}NotFoundException). 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_FOUND,ErrorCode.KNOWLEDGE_BASE_ATTACHMENT_NOT_FOUNDKnowledgeBaseTagService.addTagToItemis now idempotent (mirrorsTicketTagService), so a repeated public-API call does not create duplicate tag assignments.Fixes found while writing the tests
Nothing is removed or renamed and successful requests behave as before; the shared domain services keep throwing what the dashboard's GraphQL layer expects (the 404s are pre-checks in the REST adapters).
External API
TICKET_NOT_FOUND(was 400); note update/delete verify the note belongs to the ticket in the path → 404TICKET_NOTE_NOT_FOUNDotherwise (the ticket id used to be ignored).can-archiveandstatus→ 404CUSTOMER_NOT_FOUNDfor an unknown customer, as documented (was 400).@Validon the status/nickname bodies.GlobalOpenApiCustomizer, so they reach the grouped docs Swagger UI serves.ProxyUrlResolver.resolvePreservingEncoding().ProxyUrlResolver.resolve()is untouched — the gateway's Fleet / MeshCentral / tool traffic keeps byte-identical upstream URLs, now pinned byProxyUrlResolverTest.…05.000Z); null lists in log/tool/ticket-filter mappers map to empty lists; device tagcreatedByis mapped.Dashboard API (
openframe-api-service-core)can-archiveandstatus→ 404ORGANIZATION_NOT_FOUNDfor an unknown id (was 400, same message).@Validon the status body.ConstraintViolationExceptionfrom@Validatedfetchers →VALIDATION_ERRORwith the constraint message (was a genericINTERNAL_ERROR).Tests
Unit tests for the whole
openframe-external-api-service-coremodule (~600 tests, no Spring context, ~10s):ExternalApiMockMvctest helper — standalone MockMvc wired like the deployed app (ApiCallerresolver, both exception handlers, bean + method validation, the module's Jackson setup).TicketReadService;RestProxyServiceforwarding against a loopback server;ApiKeyPrincipalResolver,ApiCallerbinding,ExternalCursors, exception handlers, Jackson and OpenAPI config.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)mvn -o test -pl openframe-api-service-core,openframe-external-api-service-core,openframe-gateway-service-core -am— green (JDK 21)/external-api/swagger-ui