[DAPS-1522] Topic Router Logging Improvements #1801
Conversation
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
megatnt1122
added
Type: Refactor
Contributor
Reviewer's GuideAdds structured request logging around topic router endpoints and introduces basic integration tests for the topic Foxx service, wiring them into the CMake Foxx test suite. Sequence diagram for topic router logging on list topics endpointsequenceDiagram
actor Client
participant TopicRouter as TopicRouter_list_topics
participant GLib as GLib_support
participant Logger as Logger
participant DB as ArangoDB
participant ErrorHandler as ErrorHandler
Client->>TopicRouter: GET /topic/list/topics?client=clientId
TopicRouter->>GLib: getUserFromClientID(clientId)
GLib-->>TopicRouter: client
TopicRouter->>Logger: logRequestStarted(clientId, correlationId, GET, topic/list/topics, Started, List topics)
TopicRouter->>DB: execute list topics query
DB-->>TopicRouter: result
alt success
TopicRouter-->>Client: 200 OK with result
TopicRouter->>Logger: logRequestSuccess(clientId, correlationId, GET, topic/list/topics, Success, List topics, result)
else failure
TopicRouter->>Logger: logRequestFailure(clientId, correlationId, GET, topic/list/topics, Failure, List topics, result, error)
TopicRouter->>ErrorHandler: handleException(error, response)
ErrorHandler-->>Client: error response
end
Class diagram for topic router logging integrationclassDiagram
class TopicRouter {
+get_list_topics(req, res)
+get_view(req, res)
+get_search(req, res)
}
class Logger {
+logRequestStarted(client, correlationId, httpVerb, routePath, status, description)
+logRequestSuccess(client, correlationId, httpVerb, routePath, status, description, extra)
+logRequestFailure(client, correlationId, httpVerb, routePath, status, description, extra, error)
}
class GLib_support {
+getUserFromClientID(clientId)
+handleException(error, res)
}
class ArangoDB_t {
+exists(id)
+document(id)
+query(queryString, params)
}
TopicRouter --> Logger : uses
TopicRouter --> GLib_support : uses
TopicRouter --> ArangoDB_t : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `core/database/foxx/api/topic_router.js:12` </location>
<code_context>
const g_lib = require("./support");
+const logger = require("./lib/logger");
+const basePath = "topic";
module.exports = router;
</code_context>
<issue_to_address>
**question:** Consider whether `basePath` should include the leading slash to keep logged route paths consistent with actual routes.
Right now the routes are defined as `/list/topics`, `/view`, `/search`, but logged as `topic/list/topics` (no leading slash). If any tooling or consumers expect paths to start with `/` or to match the Foxx route definitions, this mismatch could cause confusion in logs and dashboards. Consider standardising this to a leading-slash form (e.g. `"/topic"`) or logging the full route path exactly as defined.
</issue_to_address>
### Comment 2
<location> `core/database/foxx/api/topic_router.js:19` </location>
<code_context>
router
.get("/list/topics", function (req, res) {
+ const client = g_lib.getUserFromClientID(req.queryParams.client);
try {
+ logger.logRequestStarted({
</code_context>
<issue_to_address>
**issue (bug_risk):** Client lookup happening outside the `try` block can prevent error handling and logging from running if it throws.
In this handler (and in the `/view` and `/search` handlers), `g_lib.getUserFromClientID` is called before the `try`. If it throws (e.g., malformed client ID, DB error), `logRequestStarted`, `logRequestFailure`, and `g_lib.handleException` are all skipped and the exception escapes unhandled. Please move this lookup into the existing `try` block or wrap it in its own `try/catch` so these failures are logged and mapped to proper HTTP responses.
</issue_to_address>
### Comment 3
<location> `core/database/foxx/api/topic_router.js:21-28` </location>
<code_context>
.get("/list/topics", function (req, res) {
+ const client = g_lib.getUserFromClientID(req.queryParams.client);
try {
+ logger.logRequestStarted({
+ client: client?._id,
+ correlationId: req.headers["x-correlation-id"],
+ httpVerb: "GET",
+ routePath: basePath + "/list/topics",
+ status: "Started",
+ description: "List topics",
+ });
+
var qry,
</code_context>
<issue_to_address>
**suggestion (bug_risk):** `logRequestStarted` in `/search` is outside the `try`, unlike the other handlers, which may lead to inconsistent error handling.
In `/search`, `logRequestStarted` runs before the `try`, unlike `/list/topics` and `/view` where it’s inside. If this call throws (e.g., due to serialization, transport, or config issues), the request can fail before the main logic runs and `logRequestFailure`/`handleException` won’t be invoked. To align behavior and avoid unhandled failures, move `logRequestStarted` into the `try` or wrap it in its own small `try/catch`.
Suggested implementation:
```javascript
try {
logger.logRequestStarted({
client: client?._id,
correlationId: req.headers["x-correlation-id"],
httpVerb: "POST",
routePath: basePath + "/search",
status: "Started",
description: "Search topics",
});
} catch (logErr) {
// Prevent logging failures from breaking the request handler.
// Adjust this to use your central error logger if available.
console.error("Failed to log request start for /search", logErr);
}
```
Because I can’t see the full `/search` handler, you should:
1. Ensure this `logger.logRequestStarted` block is located at the start of the `/search` route handler, before the main `try` that wraps the business logic (if one exists).
2. If your codebase has a centralized logging/monitoring utility for internal errors (e.g. `logger.logInternalError` or similar), replace `console.error` in the `catch` with that, to stay consistent with the rest of the file.
3. Optionally, if the `/search` handler already has a top-level `try { ... } catch (err) { ... }`, you can alternatively move `logger.logRequestStarted` inside that main `try` block instead of wrapping it in its own `try/catch`, to match exactly how `/list/topics` and `/view` are structured.
</issue_to_address>
### Comment 4
<location> `core/database/foxx/api/topic_router.js:73-80` </location>
<code_context>
});
res.send(result);
+ logger.logRequestSuccess({
+ client: client?._id,
+ correlationId: req.headers["x-correlation-id"],
+ httpVerb: "GET",
+ routePath: basePath + "/list/topics",
+ status: "Success",
+ description: "List topics",
+ extra: result,
+ });
} catch (e) {
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Logging full `result`/`topic` objects in `extra` may have performance and data-exposure implications.
Across these handlers, `extra` is set to the full `result`/`topic` for both success and failure. For large or sensitive payloads (e.g. topic lists, embedded content, user fields), this can bloat logs and expose data unnecessarily. Consider logging only key identifiers/summary fields or a redacted subset instead, particularly for `/list/topics` and `/search` where responses can be large.
Suggested implementation:
```javascript
logger.logRequestSuccess({
client: client?._id,
correlationId: req.headers["x-correlation-id"],
httpVerb: "GET",
routePath: basePath + "/list/topics",
status: "Success",
description: "List topics",
// Only log summary info to avoid large/sensitive payloads in logs
extra: {
topicCount: Array.isArray(result) ? result.length : undefined,
},
});
```
```javascript
logger.logRequestFailure({
client: client?._id,
correlationId: req.headers["x-correlation-id"],
httpVerb: "GET",
routePath: basePath + "/list/topics",
status: "Failure",
description: "List topics",
// Only log summary info to avoid large/sensitive payloads in logs
extra: {
topicCount: Array.isArray(result) ? result.length : undefined,
},
error: e,
});
```
To fully implement your comment across the codebase, you likely need to:
1. Apply the same pattern to other topic-related handlers (e.g. `/search`, `/list`, detail endpoints) where `extra` currently logs full `result`/`topic` payloads.
2. Consider extracting a small helper, e.g. `summarizeResultForLog(result)` or `summarizeTopicForLog(topic)`, that returns only identifiers and counts (IDs, names, counts) and use that helper consistently in all `logger.logRequestSuccess` / `logger.logRequestFailure` calls.
3. For search endpoints, log summary fields such as `{ query, resultCount }` instead of full result arrays or documents, to avoid performance and data-exposure issues.
</issue_to_address>
### Comment 5
<location> `core/database/foxx/tests/topic_router.test.js:36-45` </location>
<code_context>
+ });
+ });
+
+ it("should successfully run the list route", () => {
+ db.u.save({
+ _key: "fakeUser",
+ _id: "u/fakeUser",
+ name: "fake user",
+ name_first: "fake",
+ name_last: "user",
+ is_admin: true,
+ max_coll: 50,
+ max_proj: 10,
+ max_sav_qry: 20,
+ email: "[email protected]",
+ });
+
+ db.t.save({
+ _key: "10",
+ });
+
+ // arrange
+ // TODO: make encoded query params less hard coded
+ const request_string = `${topic_base_url}/view?client=u/fakeUser&id=10`;
+ // act
+ const response = request.get(request_string);
+ // assert
+ expect(response.status).to.equal(200);
</code_context>
<issue_to_address>
**issue (testing):** Test name and URL under test are inconsistent
The test name mentions the list route, but it actually hits `/view` (`/topic/view?client=u/fakeUser&id=10`). This is confusing for future readers and coverage assumptions. Please either rename the test to reflect `/view` or change the URL to the intended list route (e.g. `/list/topics`).
</issue_to_address>
### Comment 6
<location> `core/database/foxx/tests/topic_router.test.js:62-60` </location>
<code_context>
+ it("should successfully run the search route", () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding search edge-case tests (no matches / empty phrase) to better cover `/search` behavior
Right now we only assert the happy-path match. Given the logic in this route (tokenization, phrase parsing, etc.), please add at least one edge-case test, e.g. a search with no results and/or an empty/whitespace-only phrase (if valid). This helps verify router behavior and logging under less ideal inputs.
Suggested implementation:
```javascript
it("should run the search route for happy-path and edge cases", () => {
// Create user
db.u.save({
_key: "fakeUser",
is_admin: true,
});
// Create ArangoSearch View (if missing)
if (!db._view("topicview")) {
db._createView("topicview", "arangosearch", {
links: {
```
Because only the beginning of the test body is visible, you’ll need to extend the body of this `it` block where the actual `/search` request is currently made. After the existing “happy-path” search assertion, add at least:
1. A “no matches” search:
- Issue a GET to `/search` with a phrase that you know will not match any seeded topic (e.g. `__no_such_phrase__`).
- Assert `response.status` is 200 (or the expected status) and that the response body contains an empty result set (e.g. `expect(response.json.results).to.have.length(0)` or equivalent for your schema).
- Optionally assert that any expected logging/metadata fields are present.
2. An empty/whitespace phrase search (if the route allows it):
- Issue a GET to `/search` with `phrase=""` or `" "`.
- Assert the status code and body semantics that your router defines for this case (e.g. 200 with empty results, or a 400 validation error).
- Add explicit assertions for body shape so regressions in input validation or tokenization are caught.
Place these additional calls after your current “happy-path” search request and keep using the existing `request`/`topic_base_url` helpers so the tests stay consistent with the rest of the file.
</issue_to_address>
### Comment 7
<location> `core/database/foxx/tests/topic_router.test.js:56-58` </location>
<code_context>
+
+ // arrange
+ // TODO: make encoded query params less hard coded
+ const request_string = `${topic_base_url}/view?client=u/fakeUser&id=10`;
+ // act
+ const response = request.get(request_string);
+ // assert
+ expect(response.status).to.equal(200);
</code_context>
<issue_to_address>
**suggestion (testing):** No test for behavior when `client` is missing or invalid in the query
These changes now rely on `g_lib.getUserFromClientID(req.queryParams.client)` in all three routes, and all current tests pass a valid `client` query (with `/search` using a slightly different shape). Please add at least one test where `client` is missing and one where it’s malformed, to confirm the router’s behavior in those cases (e.g., logging a null client vs. returning an explicit error) and to validate the robustness of the new logging lookup.
Suggested implementation:
```javascript
db.t.save({
_key: "10",
});
// arrange
// TODO: make encoded query params less hard coded
const request_string = `${topic_base_url}/view?client=u/fakeUser&id=10`;
// act
const response = request.get(request_string);
// assert
expect(response.status).to.equal(200);
});
it("should handle missing client query param on view route", () => {
// Create user
db.u.save({
_key: "fakeUser",
is_admin: true,
});
// Create topic
db.t.save({
_key: "11",
});
// arrange: omit client query param entirely
const request_string = `${topic_base_url}/view?id=11`;
// act
const response = request.get(request_string);
// assert
// Expect the router to reject requests without a client identifier
expect(response.status).to.equal(400);
});
it("should handle malformed client query param on view route", () => {
// Create user
db.u.save({
_key: "fakeUser",
is_admin: true,
});
// Create topic
db.t.save({
_key: "12",
});
// arrange: pass a malformed client (does not match expected encoding/shape)
const malformedClient = "not/a/valid/client";
const request_string = `${topic_base_url}/view?client=${encodeURIComponent(
malformedClient
)}&id=12`;
// act
const response = request.get(request_string);
// assert
// Expect the router to detect and reject malformed client identifiers
expect(response.status).to.equal(400);
});
it("should successfully run the search route", () => {
// Create user
db.u.save({
_key: "fakeUser",
is_admin: true,
});
// Create ArangoSearch View (if missing)
if (!db._view("topicview")) {
```
1. If the actual router behavior for missing or malformed `client` is different (e.g. returns `200` but logs a null client, or uses a different status like `422`), update the `expect(response.status).to.equal(400);` assertions in the two new tests to match the real behavior.
2. If the router returns a JSON error payload (e.g. `{ error: "invalid client" }`), add additional expectations on `response.json` or `response.body` (depending on the existing test conventions in this file) to assert the error shape/message.
3. If topics with keys `"11"` and `"12"` conflict with other tests’ fixtures, adjust the `_key` values in the new tests to unused ones that are consistent with the rest of the suite.
</issue_to_address>
### Comment 8
<location> `core/database/foxx/tests/topic_router.test.js:133` </location>
<code_context>
const paging = body[body.length - 1].paging;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {paging} = body[body.length - 1];
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const g_lib = require("./support"); | ||
| const logger = require("./lib/logger"); | ||
|
|
||
| const basePath = "topic"; |
Contributor
There was a problem hiding this comment.
question: Consider whether basePath should include the leading slash to keep logged route paths consistent with actual routes.
Right now the routes are defined as /list/topics, /view, /search, but logged as topic/list/topics (no leading slash). If any tooling or consumers expect paths to start with / or to match the Foxx route definitions, this mismatch could cause confusion in logs and dashboards. Consider standardising this to a leading-slash form (e.g. "/topic") or logging the full route path exactly as defined.
Comment on lines
+36
to
+45
| it("should successfully run the list route", () => { | ||
| db.u.save({ | ||
| _key: "fakeUser", | ||
| _id: "u/fakeUser", | ||
| name: "fake user", | ||
| name_first: "fake", | ||
| name_last: "user", | ||
| is_admin: true, | ||
| max_coll: 50, | ||
| max_proj: 10, |
Contributor
There was a problem hiding this comment.
issue (testing): Test name and URL under test are inconsistent
The test name mentions the list route, but it actually hits /view (/topic/view?client=u/fakeUser&id=10). This is confusing for future readers and coverage assumptions. Please either rename the test to reflect /view or change the URL to the intended list route (e.g. /list/topics).
JoshuaSBrown
reviewed
Dec 3, 2025
JoshuaSBrown
reviewed
Dec 3, 2025
JoshuaSBrown
reviewed
Dec 3, 2025
JoshuaSBrown
reviewed
Dec 3, 2025
JoshuaSBrown
reviewed
Dec 3, 2025
JoshuaSBrown
reviewed
Dec 3, 2025
JoshuaSBrown
reviewed
Dec 3, 2025
JoshuaSBrown
requested changes
Dec 3, 2025
Collaborator
JoshuaSBrown
left a comment
JoshuaSBrown
left a comment
There was a problem hiding this comment.
Some progress, keep it up.
Collaborator
|
Looking at the log output. 
When passing to topic search, make sure that you add a descriptive field to the Extra part for the key. Bad Extra: Good Extra: { items_found: } That way when it displays in the logs it is clear what is being returned. Also we need to remove _rev and _key from the topic view content. topics should contain the following, creator and title attributes according to what I am seeing in the support.js file. |
JoshuaSBrown
reviewed
Dec 3, 2025
JoshuaSBrown
requested changes
Dec 3, 2025
Collaborator
JoshuaSBrown
left a comment
JoshuaSBrown
left a comment
There was a problem hiding this comment.
Make sure you look at what is being logged.
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.
Ticket
#1522
Description
Logging improvements to topic rounter
Tasks
Summary by Sourcery
Add structured logging around topic router endpoints and introduce basic integration tests for topic routing.
Enhancements:
Tests: