feat: resolve circular json serialization issue #382
Conversation
WalkthroughThe pull request introduces the Changes
Sequence DiagramsequenceDiagram
participant Logger as Logging System
participant Flatted as Flatted Library
participant Transport as SlogerrTransport
Logger->>Flatted: Request object serialization
Flatted-->>Logger: Serialize with circular reference support
Logger->>Transport: Pass serialized log object
Transport->>Transport: Log object using flatted stringify
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Harish-osmosys
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
apps/api/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
apps/api/package.json(1 hunks)apps/api/src/common/logger/slogerr.transport.ts(3 hunks)apps/api/src/config/logger.config.ts(2 hunks)
🔇 Additional comments (3)
apps/api/src/common/logger/slogerr.transport.ts (1)
7-7: LGTM: Proper implementation of circular reference handlingThe replacement of
JSON.stringifywithflatted.stringifyis the correct approach to handle circular references in log objects.Also applies to: 44-44
apps/api/src/config/logger.config.ts (1)
11-11: LGTM: Consistent use of circular reference handlingThe implementation correctly uses
flatted.stringifyfor log serialization, maintaining consistency with the transport implementation.Also applies to: 43-43
apps/api/package.json (1)
54-54: Verify the flatted package version for known issuesThe package version
^3.3.2allows for minor updates. Let's verify this version for any known issues or vulnerabilities.✅ Verification successful
Package version ^3.3.2 is safe to use
The package is actively maintained with no reported security issues or critical bugs. Recent GitHub activity shows regular updates and prompt issue resolution.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for security advisories and latest versions of the flatted package # Check npm for latest versions npm view flatted versions --json | jq -r '.[-5:]' # Check for security advisories npm audit [email protected] --json || trueLength of output: 740
Script:
#!/bin/bash # Check GitHub releases and npm advisories for flatted package # Get latest releases from GitHub gh release list --repo lukeed/flatted --limit 5 # Direct check of npm advisories (alternative method) curl -s "https://registry.npmjs.org/-/npm/v1/security/advisories/search?package=flatted" | jq . # Get package description and repository info npm view flatted repository.url descriptionLength of output: 511
Script:
#!/bin/bash # Check GitHub activity and issues for WebReflection/flatted # Get latest releases gh release list --repo WebReflection/flatted --limit 5 # Check recent issues gh issue list --repo WebReflection/flatted --state all --limit 5 # Get latest commit date gh api repos/WebReflection/flatted/commits/master --jq '.commit.committer.date'Length of output: 891
| this.logger.log( | ||
| `Failed to send log to Slogerr. Status: ${response.status}, Message: ${response.statusText}`, | ||
| ); | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider keeping error level logging for failed requests
Changing from error to log level reduces the visibility of failed log submissions. Failed logging attempts should typically be treated as errors to ensure proper monitoring and alerting.
- this.logger.log(
+ this.logger.error(
`Failed to send log to Slogerr. Status: ${response.status}, Message: ${response.statusText}`,
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.logger.log( | |
| `Failed to send log to Slogerr. Status: ${response.status}, Message: ${response.statusText}`, | |
| ); | |
| } else { | |
| this.logger.error( | |
| `Failed to send log to Slogerr. Status: ${response.status}, Message: ${response.statusText}`, | |
| ); | |
| } else { |
There was a problem hiding this comment.
updated in latest commit
There was a problem hiding this comment.
@Harish-osmosys Thank you for implementing the change. I can confirm that error-level logging has been restored for failed requests.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the current content of the file
rg "Failed to send log to Slogerr" -A 2 "apps/api/src/common/logger/slogerr.transport.ts"
Length of output: 317
Script:
#!/bin/bash
# Check with more context lines
rg "Failed to send log to Slogerr" -B 2 -A 2 "apps/api/src/common/logger/slogerr.transport.ts"
Length of output: 424
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
this.logger.error(
`Failed to send log to Slogerr. Status: ${response.status}, Message: ${response.statusText}`,
);
when i tried using this - if their was any issue in slogger config wrong type or wrong api key - the error log is generated which due to the new transport again route to this slogger transport so the loop continue so to avoid this i have made this as a warn log only instead of error log
There was a problem hiding this comment.
@Harish-osmosys I apologize for my previous suggestion and thank you for the detailed explanation. You're absolutely right - using error logging here would create an infinite loop when there are configuration issues, as the error logs would keep getting routed back through the same transport. Your solution to use info logging is the correct approach to break this potential infinite loop.
✏️ Learnings added
Learnt from: Harish-osmosys
PR: OsmosysSoftware/osmo-x#382
File: apps/api/src/common/logger/slogerr.transport.ts:69-72
Timestamp: 2025-01-15T09:30:40.760Z
Learning: In the SlogerrTransport class (apps/api/src/common/logger/slogerr.transport.ts), failed attempts to send logs to Slogerr should use info-level logging (this.logger.log) instead of error-level logging to prevent infinite loops, as error logs would be routed back through the same transport when there are configuration issues.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/api/src/config/logger.config.ts (1)
Line range hint
1-1: Consider adding tests for circular reference handling.While the implementation of flatted looks good, consider adding tests to verify:
- Circular reference handling in various log object structures
- Performance impact of using flatted vs JSON.stringify
- Error scenarios and their proper logging levels
This will ensure the changes are robust and maintainable.
Would you like me to help create test cases for these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
apps/api/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
apps/api/package.json(1 hunks)apps/api/src/common/logger/slogerr.transport.ts(3 hunks)apps/api/src/config/logger.config.ts(2 hunks)
🔇 Additional comments (5)
apps/api/package.json (1)
54-54: Verify the latest stable version of flatted.The package addition looks good, but let's verify we're using the latest stable version.
✅ Verification successful
Package version is up to date
The installed version (^3.3.2) matches the latest version available on npm registry.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check latest version and security advisories for flatted echo "Latest version from npm:" npm view flatted version echo -e "\nSecurity advisories:" npm audit flattedLength of output: 517
apps/api/src/common/logger/slogerr.transport.ts (2)
7-7: LGTM! Import looks good.The import of
stringifyfrom flatted is correctly placed.
44-44: LGTM! Correctly using flatted for circular JSON handling.The replacement of JSON.stringify with flatted's stringify will properly handle circular references in the log info object.
apps/api/src/config/logger.config.ts (2)
11-11: LGTM! Import looks good.The import of
stringifyfrom flatted is correctly placed.
43-43: Verify circular reference handling in log objects.The change to use flatted's stringify is good, but let's verify it handles all our log object structures correctly.
✅ Verification successful
The circular reference handling is verified but over-engineered for current usage
The codebase shows consistent use of simple log objects (strings, errors, basic objects) with no circular references. While
flatted.stringify()will work correctly, it's more complex than needed since the current logging patterns don't involve circular references. RegularJSON.stringify()would suffice for all current use cases.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for complex log objects that might have circular references echo "Searching for complex log objects:" rg -B 5 -A 5 "logger\.(log|error|warn|debug|verbose)\(" --type ts echo -e "\nSearching for custom logger implementations:" ast-grep --pattern 'class $_ implements $_ { $$$ logger$_ = $_ $$$ }'Length of output: 72755
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
API PR Checklist
Pre-requisites
.env.examplefile with the required values as applicable.PR Details
PR details have been updated as per the given format (see below)
feat: add admin login endpoint)Additional Information
ready for reviewshould be added if the PR is ready to be reviewed)Note: Reviewer should ensure that the checklist and description have been populated and followed correctly, and the PR should be merged only after resolving all conversations and verifying that CI checks pass.
Description:
This Pr uses the new npm package stringify method instead of the json to avoid circular Json serialization issue
Related changes:
flattedScreenshots:
Query request and response:
N/A
Documentation changes:
N/A
Test suite/unit testing output:
N/A
Pending actions:
N/A
Additional notes:
N/A
Summary by CodeRabbit
New Features
flattedlibrary to improve logging capabilities.Improvements
Chores
flattedversion 3.3.2.