Use JSON.stringify for object interpolation in log statements to avoi…#311
Use JSON.stringify for object interpolation in log statements to avoi…#311rhopp wants to merge 2 commits intoredhat-appstudio:mainfrom
Conversation
Review Summary by QodoUse JSON.stringify for object interpolation in log statements
WalkthroughsDescription• Replace direct object interpolation with JSON.stringify in log statements • Prevents [object Object] output in logs for better debugging • Improves log readability for Azure API errors and responses • Applies consistent object serialization across three service files Diagramflowchart LR
A["Log statements with objects"] -- "Apply JSON.stringify" --> B["Serialized JSON in logs"]
B -- "Result" --> C["Readable object output instead of [object Object]"]
File Changes1. src/api/azure/http/azure-http.client.ts
|
Code Review by Qodo
1.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAzure HTTP client error handling was reworked: added a sanitizeErrorData helper and robust mapping of various error shapes into Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/azure/http/azure-http.client.ts`:
- Around line 48-52: The error handler logs error.request using JSON.stringify
which throws on circular structures; instead pull the request URL from the
AxiosError config and log that (e.g., use error.config?.url or combine
error.config?.baseURL + error.config?.url) so the this.logger.error call in the
Azure DevOps API Error "no response received" branch doesn't crash; update the
branch that currently references JSON.stringify(error.request) to log the
extracted URL (and optionally method) plus any safe scalar fields from
error.request rather than stringifying the whole object.
In `@src/api/azure/services/azure-variable-group.service.ts`:
- Line 42: The current info log in azure-variable-group.service.ts uses
JSON.stringify(response) which may expose secrets; update the logging to only
emit non-sensitive identifiers (e.g., response.id, response.name,
response.description) or call a sanitization helper (e.g.,
redactSensitiveFields(response) or sanitizeAzureResponse(response)) that
removes/redacts properties like variables, value, secret, content or any nested
credential fields before logging via this.logger.info; ensure the log uses the
sanitized object or explicit fields instead of the full response object.
In `@src/api/rhdh/developerhub.ts`:
- Line 45: The current this.logger.info call logs the full
componentScaffoldOptions (variable componentScaffoldOptions) which may contain
secrets; change it to avoid leaking sensitive data by either (a) lowering the
level to this.logger.debug when dumping the full options, or (b) replacing the
logged payload with a safe subset (e.g., { templateRef:
componentScaffoldOptions.templateRef, repoUrl:
componentScaffoldOptions.values?.repoUrl }) and keep the log at INFO; update the
call at the site where the component is created (the existing this.logger.info
line) to implement one of these approaches.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/api/azure/http/azure-http.client.tssrc/api/azure/services/azure-variable-group.service.tssrc/api/rhdh/developerhub.ts
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
rhopp
force-pushed
the
stringifyObjects
branch
from
6f5e4cf to
17256fa
Compare
coderabbitai
bot
added
bug-fix
and removed
enhancement
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/api/azure/http/azure-http.client.ts (1)
79-84: Minor: Dead code path in config extraction.Line 82 attempts
error.request?.config, but the Axioserror.requestproperty is a Node.jshttp.ClientRequestobject which doesn't have aconfigproperty—this will always beundefinedand fall through toerror.config.Not a bug (the fallback works correctly), but you can simplify to just use
error.config:🧹 Optional cleanup
} else if (error.request) { azureError = new AzureApiError('No response received from Azure DevOps', undefined, undefined, error); try { - const { method, baseURL, url, timeout } = error.request?.config ?? error.config ?? {}; + const { method, baseURL, url, timeout } = error.config ?? {}; this.logger.error(`Azure DevOps API Error: No response received - ${method?.toUpperCase()} ${baseURL ?? ''}${url ?? ''} (timeout: ${timeout})`); } catch { /* logging must not break error propagation */ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/azure/http/azure-http.client.ts` around lines 79 - 84, The axios error handling contains a dead path using error.request?.config; simplify the config extraction by removing the request branch and just destructure from error.config (e.g. const { method, baseURL, url, timeout } = error.config ?? {}), then keep the existing logging inside the try/catch and preserve the AzureApiError construction; update the code paths in the same error handling block that reference error.request?.config so only error.config is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/api/azure/http/azure-http.client.ts`:
- Around line 79-84: The axios error handling contains a dead path using
error.request?.config; simplify the config extraction by removing the request
branch and just destructure from error.config (e.g. const { method, baseURL,
url, timeout } = error.config ?? {}), then keep the existing logging inside the
try/catch and preserve the AzureApiError construction; update the code paths in
the same error handling block that reference error.request?.config so only
error.config is used.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/api/azure/http/azure-http.client.tssrc/api/azure/services/azure-variable-group.service.tssrc/api/rhdh/developerhub.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/api/azure/services/azure-variable-group.service.ts
|
/retest |
2 similar comments
|
/retest |
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/azure/http/azure-http.client.ts`:
- Around line 86-89: The logging currently interpolates the raw error
(this.logger.error(`Azure DevOps API Error: Request setup failed - ${error}`))
which can produce “[object Object]”; update the catch branch in
azure-http.client.ts to serialize the error safely before logging: if the value
is an Error log error.stack (or error.message if stack missing); otherwise
attempt JSON.stringify(error) with a try/catch fallback to util.inspect(error)
or String(error); keep creating azureError (AzureApiError) as before but pass
the serialized message to this.logger.error to ensure useful output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fcc2dd04-3bbb-4e86-b249-fd83421cd5e7
📒 Files selected for processing (3)
src/api/azure/http/azure-http.client.tssrc/api/azure/services/azure-variable-group.service.tssrc/api/rhdh/developerhub.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/api/azure/services/azure-variable-group.service.ts
| azureError = new AzureApiError('Request setup failed', undefined, undefined, error); | ||
| try { | ||
| this.logger.error(`Azure DevOps API Error: Request setup failed - ${error}`); | ||
| } catch { /* logging must not break error propagation */ } |
There was a problem hiding this comment.
Avoid raw object interpolation in setup-failure logging.
This branch can still emit [object Object] for non-Error values, which weakens debugging output.
Suggested fix
} else {
azureError = new AzureApiError('Request setup failed', undefined, undefined, error);
try {
- this.logger.error(`Azure DevOps API Error: Request setup failed - ${error}`);
+ const detail = error instanceof Error ? error.message : sanitizeErrorData(error);
+ this.logger.error(`Azure DevOps API Error: Request setup failed - ${detail}`);
} catch { /* logging must not break error propagation */ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/azure/http/azure-http.client.ts` around lines 86 - 89, The logging
currently interpolates the raw error (this.logger.error(`Azure DevOps API Error:
Request setup failed - ${error}`)) which can produce “[object Object]”; update
the catch branch in azure-http.client.ts to serialize the error safely before
logging: if the value is an Error log error.stack (or error.message if stack
missing); otherwise attempt JSON.stringify(error) with a try/catch fallback to
util.inspect(error) or String(error); keep creating azureError (AzureApiError)
as before but pass the serialized message to this.logger.error to ensure useful
output.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/azure/http/azure-http.client.ts`:
- Around line 82-83: The conditional uses a dead branch error.request?.config
(error.request is an http.ClientRequest and has no config); update the
extraction to pull method, baseURL, url, timeout only from error.config (e.g.,
replace the destructuring that uses error.request?.config ?? error.config ?? {}
with just error.config ?? {}), and keep the existing this.logger.error call
(Azure DevOps API Error...) unchanged so it logs method/baseURL/url/timeout from
the valid AxiosError config; target the destructuring near the this.logger.error
call in azure-http.client.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fcd1883d-77e3-4396-af96-6a41c2f2d593
📒 Files selected for processing (3)
src/api/azure/http/azure-http.client.tssrc/api/azure/services/azure-variable-group.service.tssrc/api/rhdh/developerhub.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/api/azure/services/azure-variable-group.service.ts
- src/api/rhdh/developerhub.ts
| const { method, baseURL, url, timeout } = error.request?.config ?? error.config ?? {}; | ||
| this.logger.error(`Azure DevOps API Error: No response received - ${method?.toUpperCase()} ${baseURL ?? ''}${url ?? ''} (timeout: ${timeout})`); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/api/azure/http/azure-http.client.ts | sed -n '75,95p'Repository: redhat-appstudio/tssc-test
Length of output: 1415
🏁 Script executed:
rg -n "error\.request" src/api/azure/http/azure-http.client.tsRepository: redhat-appstudio/tssc-test
Length of output: 210
🏁 Script executed:
rg -n "error\.request\?*\.config|error\.request\.config" --type ts src/Repository: redhat-appstudio/tssc-test
Length of output: 211
🏁 Script executed:
rg -n "import.*axios|import.*AxiosError" src/api/azure/http/azure-http.client.tsRepository: redhat-appstudio/tssc-test
Length of output: 52
🏁 Script executed:
rg -n "AxiosError|error:" src/api/azure/http/azure-http.client.ts -B 2 -A 2Repository: redhat-appstudio/tssc-test
Length of output: 626
🏁 Script executed:
# Check if error is explicitly typed and how it's imported
rg -n "catch\s*\(\s*error" src/api/azure/http/azure-http.client.ts -A 1Repository: redhat-appstudio/tssc-test
Length of output: 52
🏁 Script executed:
# Look for axios client setup to understand error handling context
head -50 src/api/azure/http/azure-http.client.tsRepository: redhat-appstudio/tssc-test
Length of output: 1754
🏁 Script executed:
# Check for AxiosError type definitions or error type annotations
rg -n "catch.*error.*AxiosError|error\s*:\s*AxiosError|as AxiosError" src/api/azure/http/azure-http.client.tsRepository: redhat-appstudio/tssc-test
Length of output: 52
🏁 Script executed:
# Check BaseHttpClient to understand what error types propagate
head -100 src/api/common/http/base-http.client.tsRepository: redhat-appstudio/tssc-test
Length of output: 3206
🏁 Script executed:
# Search for axios usage in BaseHttpClient
rg -n "error\.request\|\.request\?\." src/api/common/http/base-http.client.tsRepository: redhat-appstudio/tssc-test
Length of output: 52
🏁 Script executed:
# Check ApiError class to understand error.cause
rg -n "class ApiError|error\.cause" src/api/common/errors/api.errors.ts -A 5Repository: redhat-appstudio/tssc-test
Length of output: 435
🏁 Script executed:
# Look at the full error handling block to understand context
sed -n '60,95p' src/api/azure/http/azure-http.client.tsRepository: redhat-appstudio/tssc-test
Length of output: 2065
🏁 Script executed:
# Check the full ApiError class structure
cat -n src/api/common/errors/api.errors.tsRepository: redhat-appstudio/tssc-test
Length of output: 3862
🏁 Script executed:
# Verify that in the else if (error.request) block, error is indeed AxiosError
# by checking what properties are being accessed
rg -n "error\.request|error\.config|error\.response" src/api/azure/http/azure-http.client.ts | head -20Repository: redhat-appstudio/tssc-test
Length of output: 520
Remove dead code path—error.request doesn't have a config property.
In AxiosError, error.request is the Node.js http.ClientRequest object, which has no config property. The expression error.request?.config will always evaluate to undefined, making it unnecessary dead code. Simplify to access only error.config:
Suggested fix
- const { method, baseURL, url, timeout } = error.request?.config ?? error.config ?? {};
+ const { method, baseURL, url, timeout } = error.config ?? {};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/azure/http/azure-http.client.ts` around lines 82 - 83, The
conditional uses a dead branch error.request?.config (error.request is an
http.ClientRequest and has no config); update the extraction to pull method,
baseURL, url, timeout only from error.config (e.g., replace the destructuring
that uses error.request?.config ?? error.config ?? {} with just error.config ??
{}), and keep the existing this.logger.error call (Azure DevOps API Error...)
unchanged so it logs method/baseURL/url/timeout from the valid AxiosError
config; target the destructuring near the this.logger.error call in
azure-http.client.ts.
…d [object Object] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ing in Azure HTTP client Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jkopriva
force-pushed
the
stringifyObjects
branch
from
6edd6d8 to
c974b69
Compare
|
@rhopp: The following test has Failed, say /retest to rerun failed tests.
Inspecting Test ArtifactsTo inspect your test artifacts, follow these steps:
mkdir -p oras-artifacts
cd oras-artifacts
oras pull quay.io/konflux-test-storage/rhtap-team/rhtap-cli:e2e-4.20-tdhg5Test results analysis<not enabled> OCI Artifact Browser URL<not enabled> |
3 participants
…d [object Object]
Example of log before this change:
Summary by CodeRabbit
Bug Fixes
Chores