feat: Add option Parse.nodeLogging to fully log Parse.Object in Node.js environments
#1594
Conversation
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. |
|
Does this influence existing built-in log messages? If so, maybe this should be optional, because logging object attributes may expose sensitive data in the logs. |
|
Yes it does, that's why it defaults to |
|
Alright, maybe we can add a short warning note about that to the option JSdoc. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## alpha #1594 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 63 63
Lines 6167 6174 +7
Branches 1464 1465 +1
=========================================
+ Hits 6167 6174 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The symbols are scoped locally per object and doesn't influence existing built in logs @mtrezza This is ready for review |
|
I believe I was referring to when using the Parse SDK in Cloud Code, whether that will output additional logs to the requester, which may expose information about the object, for example in case of an error. |
|
The requester will never see this right? |
|
That depends on what the server sends to the requester in case of an error. I don't know whether there is currently an error handling anywhere in Parse Server that would cause the full object being returned to the requester. In any case, I think my question back then was referring to the log files on the server side. Sensitive information should also not be in the log files by default. Maybe that's good for development, so if it's off by default that should be fine. I would just add an extra warning note to the option's doc that this should not be enabled in production. Maybe log a warning as well. |
📝 Walkthrough""" WalkthroughA new configuration flag, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Parse
participant CoreManager
participant ParseObject
User->>Parse: Set Parse.nodeLogging = true
Parse->>CoreManager: Set NODE_LOGGING = true
User->>ParseObject: new ParseObject(...)
ParseObject->>CoreManager: Get NODE_LOGGING
alt NODE_LOGGING is true
ParseObject->>ParseObject: Attach custom inspect method
end
User->>ParseObject: Call inspect method (via util.inspect)
ParseObject->>User: Return formatted string representation
""" 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/__tests__/ParseObject-test.js (2)
3537-3544: EnsureNODE_LOGGINGflag is always reset
The test manually setsNODE_LOGGINGback tofalseat the end, but if the assertion fails, the flag remainstrueand may leak into subsequent tests. Consider using a shared teardown or atry/finallyto guarantee cleanup. For example, add anafterEachhook at the top of this suite:+describe('ParseObject pin', () => { + beforeEach(() => { + ParseObject.enableSingleInstance(); + jest.clearAllMocks(); + mockLocalDatastore.isEnabled = true; + }); + afterEach(() => { + CoreManager.set('NODE_LOGGING', false); + });
3539-3542: Make the string comparison in the assertion more resilient
Using.toBewith a raw multi-line string is brittle—any minor formatting change will break the test. You could instead match on key parts or use a regex/snapshot:// Example with regex: expect(o[symbol]()).toMatch( /ParseObject: className: Person, id: undefined\s+Attributes:\s+\{\s+"foo": "bar"\s+\}/ );Or leverage Jest snapshots:
expect(o[symbol]()).toMatchSnapshot();src/ParseObject.ts (2)
189-195: Avoid per-instance function allocation
Allocating a new inspect function on every instance increases memory footprint. Consider defining the custom inspect method once onParseObject.prototypewhenNODE_LOGGINGis enabled, rather than inside the constructor.
189-195: Warn developers about sensitive data exposure
Enabling detailed logging can inadvertently expose PII or other sensitive attributes. It would be prudent to emit aconsole.warnwhenNODE_LOGGINGis set totrue, reminding users to avoid enabling this in production environments.src/CoreManager.ts (1)
358-358: Document and type annotate the newNODE_LOGGINGflag.The new flag would benefit from an inline comment explaining its purpose, default, and caution for sensitive data. For example:
~// Enable detailed Node.js logging of Parse.Object instances. Off by default to avoid logging sensitive data. ~NODE_LOGGING: false,Additionally, consider extending the
Configinterface with:/** Enable detailed Node.js logging of Parse.Object instances */ NODE_LOGGING?: boolean;so that the flag is explicitly typed in TypeScript.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/CoreManager.ts(1 hunks)src/Parse.ts(1 hunks)src/ParseObject.ts(1 hunks)src/__tests__/Parse-test.js(1 hunks)src/__tests__/ParseObject-test.js(1 hunks)types/Parse.d.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (Node 22, 22.4.1)
🔇 Additional comments (5)
src/CoreManager.ts (1)
358-358: Good implementation of theNODE_LOGGINGflag with sensible default valueSetting the default value to
falseis a security-conscious choice that aligns with the PR objectives of avoiding unintentional exposure of sensitive data in logs.types/Parse.d.ts (1)
325-329: LGTM - Property type definition properly documentedThe type declaration and JSDoc are consistent with other similar properties in this file, making it clear that this is a boolean configuration flag.
src/Parse.ts (1)
317-326: LGTM - Well-implemented property accessorThe getter and setter implementation follows the established pattern in this codebase, properly delegating to CoreManager for the actual storage of the configuration value.
src/__tests__/Parse-test.js (1)
209-214: LGTM - Comprehensive test caseThis test appropriately verifies the default state and the ability to toggle the setting, ensuring the feature can be enabled and disabled as needed.
src/ParseObject.ts (1)
189-195: Correct usage ofutil.inspect.customsymbol
The custom inspect method correctly leveragesSymbol.for('nodejs.util.inspect.custom')to provide detailed output—including class name, ID, and a nicely formatted JSON of attributes—whenNODE_LOGGINGis enabled.
|
I've added a note to the JSDoc of |
mtrezza
changed the title
Parse.nodeLogging to fully log Parse.ObjectParse.nodeLogging to fully log Parse.Object
mtrezza
changed the title
Parse.nodeLogging to fully log Parse.ObjectParse.nodeLogging to fully log Parse.Object
mtrezza
changed the title
Parse.nodeLogging to fully log Parse.ObjectParse.nodeLogging to fully log Parse.Object in Node.js environments
|
LGTM! |
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
New Pull Request Checklist
Issue Description
Currently, when logging a Parse.Object in NodeJS, the only thing that is logged is
ParseObject { _objCount: 376, className: 'Person' }, which more often than not isn't overly helpful.Related issue:
n/aApproach
Adds
Parse.nodeLoggingwhich logs the object out to:TODOs before merging
Summary by CodeRabbit