-
-
Notifications
You must be signed in to change notification settings - Fork 175
Add new LogBox configuration option to disallow serializing complex objectts #615
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: development
Are you sure you want to change the base?
Conversation
…bjects A new top level config item named `allowSerializingComplexObjects` controls whether LogBox will attempt to serialize and log out complex objects in `extraInfo` arguments. Defaults to `true`. Turning this to `false` can catch instances where large you using lots of processing time converting objects to XML or JSON unnecessarily. Removing these can increase performance and stability of running applications.
Test Results 42 files + 6 936 suites +133 7m 31s ⏱️ +56s Results for commit 3fb11d5. ± Comparison against base commit c3afabf. This pull request removes 1 and adds 3 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a new configuration option allowSerializingComplexObjects
to LogBox that controls whether complex objects can be serialized in the extraInfo
parameter of log messages. When set to false, it can help identify performance bottlenecks caused by unnecessary object serialization during logging.
- Added
allowSerializingComplexObjects
configuration property with default value oftrue
- Implemented validation logic to detect and prevent complex object serialization when disabled
- Added comprehensive test coverage for the new functionality including nested object detection
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
system/logging/config/LogBoxConfig.cfc | Adds the new configuration property and parsing logic |
system/logging/Logger.cfc | Implements complex object detection and validation with exception throwing |
system/logging/LogBox.cfc | Passes the configuration setting to logger instances |
tests/specs/logging/LoggerTest.cfc | Adds test cases for the new serialization control functionality |
return checkForComplexObjects( v ); | ||
} ); | ||
} | ||
|
||
if ( isStruct( arguments.value ) ) { | ||
return structSome( arguments.value, function( k, v ){ | ||
return checkForComplexObjects( v ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The recursive check for complex objects could cause performance issues with deeply nested or large data structures. Consider adding a depth limit or size limit to prevent excessive recursion that could impact logging performance.
return checkForComplexObjects( v ); | |
} ); | |
} | |
if ( isStruct( arguments.value ) ) { | |
return structSome( arguments.value, function( k, v ){ | |
return checkForComplexObjects( v ); | |
return checkForComplexObjects( v, depth + 1 ); | |
} ); | |
} | |
if ( isStruct( arguments.value ) ) { | |
return structSome( arguments.value, function( k, v ){ | |
return checkForComplexObjects( v, depth + 1 ); |
Copilot uses AI. Check for mistakes.
return false; | ||
} | ||
|
||
if ( isArray( arguments.value ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In all reality, if we are going to NOT allow anything complex, I would say don't even check for here down, why go through the trouble of iterate through each of the values, this is time consuming, error proned and just slow.
} ); | ||
} | ||
|
||
if ( isStruct( arguments.value ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
@@ -456,4 +474,46 @@ component accessors="true" { | |||
return canLog( this.logLevels.DEBUG ); | |||
} | |||
|
|||
private boolean function checkForComplexObjects( required any value ){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
document the function, no excuses now, use chatgpt
} | ||
|
||
// CFML Exceptions are safe | ||
if ( isCFMLException( arguments.value ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the cfml
, this is now. boxlang + cfml engine, use abstractions: isException()
return true; | ||
} | ||
|
||
private boolean function isCFMLException( required any value ){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change method name to isException
and document it
Description
A new top level config item named
allowSerializingComplexObjects
controls whether LogBox will attempt to serialize and log out complex objects inextraInfo
arguments. Defaults totrue
. Turning this tofalse
can catch instances where you are using lots of processing time converting objects to XML or JSON unnecessarily. Removing these can increase performance and stability of running applications.Jira Issues
https://ortussolutions.atlassian.net/browse/LOGBOX-83
Type of change
Please delete options that are not relevant.
Checklist