[1522] Schema Router Logging Improvements #1821
Open
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 lifecycle logging (start/success/failure) to all schema router endpoints and introduces focused Foxx tests for the schema router, wiring them into the CMake Foxx test suite. Sequence diagram for schema router request lifecycle loggingsequenceDiagram
actor Client
participant SchemaRouter
participant Logger
participant ArangoDB
participant ErrorHandler
Client->>SchemaRouter: HTTP POST /schema/create
SchemaRouter->>Logger: logRequestStarted(client, correlationId, httpVerb, routePath, status, description)
SchemaRouter->>ArangoDB: _executeTransaction(...)
alt Transaction success
ArangoDB-->>SchemaRouter: sch
SchemaRouter->>Logger: logRequestSuccess(client, correlationId, httpVerb, routePath, status, description, extra)
SchemaRouter-->>Client: 200 OK [schema payload]
else Transaction failure
ArangoDB-->>SchemaRouter: error e
SchemaRouter->>Logger: logRequestFailure(client, correlationId, httpVerb, routePath, status, description, extra, error)
SchemaRouter->>ErrorHandler: handleException(e, res)
ErrorHandler-->>Client: Error response
end
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 - here's some feedback:
- In several catch blocks (
/create,/update,/revise,/delete,/view,/search) the failure logging accesses fields on variables that may still benull/undefined(e.g.,sch.own_id,sch_new.own_id,sch_old._id,result), which risks throwing a secondary error and hiding the original; consider guarding these with null checks or buildingextradefensively. - The failure logging payloads are inconsistent (e.g., some include
error: ewhile/deleteand/viewdo not); if downstream consumers rely on a consistent structure, it would be helpful to normalize the fields included in success/failure logs across all endpoints. - The
extrafields in logging for some endpoints include the full schema object or potentially largedefpayloads, which may be noisy or sensitive; consider restricting logged fields to identifiers/metadata only or truncating large objects to keep logs lean and safer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In several catch blocks (`/create`, `/update`, `/revise`, `/delete`, `/view`, `/search`) the failure logging accesses fields on variables that may still be `null`/`undefined` (e.g., `sch.own_id`, `sch_new.own_id`, `sch_old._id`, `result`), which risks throwing a secondary error and hiding the original; consider guarding these with null checks or building `extra` defensively.
- The failure logging payloads are inconsistent (e.g., some include `error: e` while `/delete` and `/view` do not); if downstream consumers rely on a consistent structure, it would be helpful to normalize the fields included in success/failure logs across all endpoints.
- The `extra` fields in logging for some endpoints include the full schema object or potentially large `def` payloads, which may be noisy or sensitive; consider restricting logged fields to identifiers/metadata only or truncating large objects to keep logs lean and safer.
## Individual Comments
### Comment 1
<location> `core/database/foxx/api/schema_router.js:83-92` </location>
<code_context>
router
.post("/create", function (req, res) {
+ let sch = null;
try {
+ logger.logRequestStarted({
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against using `sch` in the failure logger when it may still be null/undefined.
In `/create`, `sch` starts as `null` and is only set inside the transaction. If an error occurs before `sch = g_db.sch.save(...)` completes, the `catch` block will access `sch.own_id`, `sch.id`, and `sch.desc`, causing a new exception and hiding the original failure. Please either log `sch` directly (without field access) or only construct `extra` when `sch` is non-null.
</issue_to_address>
### Comment 2
<location> `core/database/foxx/tests/schema_router.test.js:10` </location>
<code_context>
+
+const schema_base_url = `${baseUrl}/schema`;
+
+describe("simple test for schema/create", () => {
+ before(() => {
+ // Ensure the required collections exist and are empty
</code_context>
<issue_to_address>
**suggestion (testing):** Add negative-path tests to cover error handling and edge cases for each endpoint
Right now the tests only cover the happy paths for create, update, revise, delete, view, and search, so the error-handling and logging paths aren’t exercised. Please add a few targeted negative tests, for example:
- `/create`: missing required fields (e.g. no `id` or `def`), and non-admin user if applicable.
- `/update` and `/revise`: `id` without `:version` suffix (expect `ERR_INVALID_PARAM` and correct HTTP status), and `id` with a non-existent version (expect `ERR_NOT_FOUND`).
- `/delete`: invalid `id` format and non-existent schema.
- `/view`: non-existent schema and invalid `id` format.
- `/search`: invalid query parameters (e.g. out-of-range paging) if validated by the implementation.
This will exercise the failure branches and their logging to help prevent regressions in validation and routing logic.
Suggested implementation:
```javascript
const { db } = require("@arangodb");
const schema_base_url = `${baseUrl}/schema`;
describe("schema router negative paths", () => {
it("should reject /create without required fields", () => {
const res = request.post({
url: `${schema_base_url}/create`,
body: {
// missing id and/or def on purpose
name: "invalid schema without id and def",
},
json: true,
});
// Adjust expected status code (e.g. 400 or 422) to match implementation
expect(res.statusCode).to.be.oneOf([400, 422]);
expect(res.json).to.have.property("error").that.is.true;
});
it("should reject /create with missing def", () => {
const res = request.post({
url: `${schema_base_url}/create`,
body: {
id: "sch/missing-def",
// def is missing
},
json: true,
});
expect(res.statusCode).to.be.oneOf([400, 422]);
expect(res.json).to.have.property("error").that.is.true;
});
it("should reject /update when id has no :version suffix", () => {
const res = request.put({
url: `${schema_base_url}/update`,
body: {
id: "sch/invalid-format-no-version",
def: { foo: "bar" },
},
json: true,
});
expect(res.statusCode).to.be.oneOf([400, 422]);
expect(res.json).to.have.property("error").that.is.true;
// Optionally assert error code string / number if available:
// expect(res.json).to.have.property("errorNum", "ERR_INVALID_PARAM");
});
it("should return not found for /update with non-existent version", () => {
const res = request.put({
url: `${schema_base_url}/update`,
body: {
id: "sch/non-existent:9999",
def: { foo: "bar" },
},
json: true,
});
expect(res.statusCode).to.be.oneOf([404, 410]);
expect(res.json).to.have.property("error").that.is.true;
// expect(res.json).to.have.property("errorNum", "ERR_NOT_FOUND");
});
it("should reject /revise when id has no :version suffix", () => {
const res = request.post({
url: `${schema_base_url}/revise`,
body: {
id: "sch/invalid-format-no-version",
def: { foo: "bar" },
},
json: true,
});
expect(res.statusCode).to.be.oneOf([400, 422]);
expect(res.json).to.have.property("error").that.is.true;
});
it("should return not found for /revise with non-existent version", () => {
const res = request.post({
url: `${schema_base_url}/revise`,
body: {
id: "sch/non-existent:9999",
def: { foo: "bar" },
},
json: true,
});
expect(res.statusCode).to.be.oneOf([404, 410]);
expect(res.json).to.have.property("error").that.is.true;
});
it("should reject /delete with invalid id format", () => {
const res = request.del({
url: `${schema_base_url}/delete`,
body: {
id: "invalid-id-format-without-slash-or-version",
},
json: true,
});
expect(res.statusCode).to.be.oneOf([400, 422]);
expect(res.json).to.have.property("error").that.is.true;
});
it("should return not found for /delete of non-existent schema", () => {
const res = request.del({
url: `${schema_base_url}/delete`,
body: {
id: "sch/non-existent:1",
},
json: true,
});
expect(res.statusCode).to.be.oneOf([404, 410]);
expect(res.json).to.have.property("error").that.is.true;
});
it("should return not found for /view of non-existent schema", () => {
const res = request.get({
url: `${schema_base_url}/view`,
qs: { id: "sch/non-existent:1" },
json: true,
});
expect(res.statusCode).to.be.oneOf([404, 410]);
expect(res.json).to.have.property("error").that.is.true;
});
it("should reject /view with invalid id format", () => {
const res = request.get({
url: `${schema_base_url}/view`,
qs: { id: "invalid-id-format" },
json: true,
});
expect(res.statusCode).to.be.oneOf([400, 422]);
expect(res.json).to.have.property("error").that.is.true;
});
it("should reject /search with invalid paging parameters", () => {
const res = request.get({
url: `${schema_base_url}/search`,
qs: {
// Assume offset/limit (or page/size) are validated in the implementation
offset: -1,
limit: 0,
},
json: true,
});
expect(res.statusCode).to.be.oneOf([400, 422]);
expect(res.json).to.have.property("error").that.is.true;
});
});
describe("simple test for schema/create", () => {
```
` what to customize (exact status codes, error fields, auth headers, etc).
Here are the edits:
<file_operations>
<file_operation operation="edit" file_path="core/database/foxx/tests/schema_router.test.js">
<<<<<<< SEARCH
const { db } = require("@arangodb");
const schema_base_url = `${baseUrl}/schema`;
describe("simple test for schema/create", () => {
=======
const { db } = require("@arangodb");
const schema_base_url = `${baseUrl}/schema`;
describe("schema router negative paths", () => {
it("should reject /create without required fields", () => {
const res = request.post({
url: `${schema_base_url}/create`,
body: {
// missing id and/or def on purpose
name: "invalid schema without id and def",
},
json: true,
});
// Adjust expected status code (e.g. 400 or 422) to match implementation
expect(res.statusCode).to.be.oneOf([400, 422]);
expect(res.json).to.have.property("error").that.is.true;
});
it("should reject /create with missing def", () => {
const res = request.post({
url: `${schema_base_url}/create`,
body: {
id: "sch/missing-def",
// def is missing
},
json: true,
});
expect(res.statusCode).to.be.oneOf([400, 422]);
expect(res.json).to.have.property("error").that.is.true;
});
it("should reject /update when id has no :version suffix", () => {
const res = request.put({
url: `${schema_base_url}/update`,
body: {
id: "sch/invalid-format-no-version",
def: { foo: "bar" },
},
json: true,
});
expect(res.statusCode).to.be.oneOf([400, 422]);
expect(res.json).to.have.property("error").that.is.true;
// Optionally assert error code string / number if available:
// expect(res.json).to.have.property("errorNum", "ERR_INVALID_PARAM");
});
it("should return not found for /update with non-existent version", () => {
const res = request.put({
url: `${schema_base_url}/update`,
body: {
id: "sch/non-existent:9999",
def: { foo: "bar" },
},
json: true,
});
expect(res.statusCode).to.be.oneOf([404, 410]);
expect(res.json).to.have.property("error").that.is.true;
// expect(res.json).to.have.property("errorNum", "ERR_NOT_FOUND");
});
it("should reject /revise when id has no :version suffix", () => {
const res = request.post({
url: `${schema_base_url}/revise`,
body: {
id: "sch/invalid-format-no-version",
def: { foo: "bar" },
},
json: true,
});
expect(res.statusCode).to.be.oneOf([400, 422]);
expect(res.json).to.have.property("error").that.is.true;
});
it("should return not found for /revise with non-existent version", () => {
const res = request.post({
url: `${schema_base_url}/revise`,
body: {
id: "sch/non-existent:9999",
def: { foo: "bar" },
},
json: true,
});
expect(res.statusCode).to.be.oneOf([404, 410]);
expect(res.json).to.have.property("error").that.is.true;
});
it("should reject /delete with invalid id format", () => {
const res = request.del({
url: `${schema_base_url}/delete`,
body: {
id: "invalid-id-format-without-slash-or-version",
},
json: true,
});
expect(res.statusCode).to.be.oneOf([400, 422]);
expect(res.json).to.have.property("error").that.is.true;
});
it("should return not found for /delete of non-existent schema", () => {
const res = request.del({
url: `${schema_base_url}/delete`,
body: {
id: "sch/non-existent:1",
},
json: true,
});
expect(res.statusCode).to.be.oneOf([404, 410]);
expect(res.json).to.have.property("error").that.is.true;
});
it("should return not found for /view of non-existent schema", () => {
const res = request.get({
url: `${schema_base_url}/view`,
qs: { id: "sch/non-existent:1" },
json: true,
});
expect(res.statusCode).to.be.oneOf([404, 410]);
expect(res.json).to.have.property("error").that.is.true;
});
it("should reject /view with invalid id format", () => {
const res = request.get({
url: `${schema_base_url}/view`,
qs: { id: "invalid-id-format" },
json: true,
});
expect(res.statusCode).to.be.oneOf([400, 422]);
expect(res.json).to.have.property("error").that.is.true;
});
it("should reject /search with invalid paging parameters", () => {
const res = request.get({
url: `${schema_base_url}/search`,
qs: {
// Assume offset/limit (or page/size) are validated in the implementation
offset: -1,
limit: 0,
},
json: true,
});
expect(res.statusCode).to.be.oneOf([400, 422]);
expect(res.json).to.have.property("error").that.is.true;
});
});
describe("simple test for schema/create", () => {
>>>>>>> REPLACE
</file_operation>
</file_operations>
<additional_changes>
1. Align the exact expected status codes with the implementation. For example, if the Foxx router uses `422` for validation errors instead of `400`, or uses `404` vs `410` for not-found semantics, update the `expect(res.statusCode).to.be.oneOf([...])` assertions to specific values.
2. Replace the generic `error` property assertions with whatever your Foxx error payload actually exposes (e.g. `errorNum`, `errorMessage`, or `code`). If your implementation uses symbolic codes like `ERR_INVALID_PARAM` and `ERR_NOT_FOUND`, add `expect` checks for those fields.
3. If your routes require authentication or an admin user, add the appropriate headers (e.g. `headers: { "x-user-id": "...", "x-role": "user" }`) and then add an extra negative test per endpoint using a non-admin or missing-auth context to exercise authorization failures.
4. If your actual route signatures differ (e.g. `/schema` vs `/schema/create`, or different HTTP verbs / parameter names), adjust the `url`, `body`, and `qs` usage in each test to match the real router definitions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+83
to
+92
| let sch = null; | ||
| try { | ||
| logger.logRequestStarted({ | ||
| client: req.queryParams?.client, | ||
| correlationId: req.headers["x-correlation-id"], | ||
| httpVerb: "POST", | ||
| routePath: basePath + "/create", | ||
| status: "Started", | ||
| description: "Create schema", | ||
| }); |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Guard against using sch in the failure logger when it may still be null/undefined.
In /create, sch starts as null and is only set inside the transaction. If an error occurs before sch = g_db.sch.save(...) completes, the catch block will access sch.own_id, sch.id, and sch.desc, causing a new exception and hiding the original failure. Please either log sch directly (without field access) or only construct extra when sch is non-null.
…of github.com:ORNL/DataFed into refactor-DAPS-1522-Schema-Router-Logging-Improvements CHECKPOINT
JoshuaSBrown
requested changes
Dec 26, 2025
Collaborator
There was a problem hiding this comment.
A few comments:
- in line with what we have been doing else where include the schema id in the description
- secondly, limit the char printed output in the desc if you are going to print it
- 3rd, the pub and sys attributes indicate visibility where it makes sense and they are defined they should be output.
- never, include the def attribute in the output, this will be much larger than the desc. So don't just dump the schema document into the log output.
Comment on lines
+164
to
+168
| extra: { | ||
| own_id: sch.own_id, | ||
| id: sch.id, | ||
| desc: sch.desc, | ||
| }, |
Collaborator
There was a problem hiding this comment.
Will this fail if sch is null?
Collaborator
There was a problem hiding this comment.
We don't want to include the desc. Or if we do, limit the char output. Incude pub and sys attributes if possible.
| httpVerb: "POST", | ||
| routePath: basePath + "/create", | ||
| status: "Started", | ||
| description: "Create schema", |
Collaborator
There was a problem hiding this comment.
It looks like the schema id is required, add it to the description.
| httpVerb: "POST", | ||
| routePath: basePath + "/create", | ||
| status: "Success", | ||
| description: "Create schema", |
Collaborator
There was a problem hiding this comment.
Add the schema id to the description
Collaborator
There was a problem hiding this comment.
The pub, and sys attributes are important for understanding the visibility of the schema that should be output, they should exist even though they are optional because they are default = True.
| httpVerb: "POST", | ||
| routePath: basePath + "/create", | ||
| status: "Failure", | ||
| description: "Create schema", |
Collaborator
There was a problem hiding this comment.
Add the schema id to the description.
| httpVerb: "POST", | ||
| routePath: basePath + "/update", | ||
| status: "Failure", | ||
| description: "Update schema", |
Collaborator
There was a problem hiding this comment.
Add the schema id to the description
| httpVerb: "POST", | ||
| routePath: basePath + "/delete", | ||
| status: "Started", | ||
| description: `Delete schema. ID: ${req.queryParams.id}`, |
Collaborator
There was a problem hiding this comment.
You did it here, not sure why you didn't do it on the other routes.
| routePath: basePath + "/view", | ||
| status: "Failure", | ||
| description: `View schema. ID: ${req.queryParams.id}`, | ||
| extra: sch, |
Collaborator
There was a problem hiding this comment.
This will print too much information, we don't want to include the desc. Or if we do, limit the char output.
Collaborator
There was a problem hiding this comment.
And we definately don't want to include the 'def' this will be bigger thant the desc.
| own_id: sch.own_id, | ||
| own_nm: sch.own_nm, | ||
| id: sch.id, | ||
| desc: sch.desc, |
Collaborator
There was a problem hiding this comment.
This will print too much information, we don't want to include the desc. Or if we do, limit the char output. Include pub, sys attributes.
| httpVerb: "POST", | ||
| routePath: basePath + "/revise", | ||
| status: "Success", | ||
| description: "Revise schema", |
| routePath: basePath + "/update", | ||
| status: "Success", | ||
| description: "Update schema", | ||
| extra: sch_new, |
Collaborator
There was a problem hiding this comment.
We don't want to include the desc. Or if we do, limit the char output. Incude pub and sys attributes if possible.
Collaborator
There was a problem hiding this comment.
And we definately don't want to include the 'def' this will be bigger thant the desc.
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 schema router and included tests for them
Tasks
Summary by Sourcery
Improve logging for schema router endpoints and add unit tests to validate schema CRUD and search behaviour.
Enhancements:
Tests: