JCN-520: Omit props from arrays in logs#8
Conversation
📝 WalkthroughWalkthroughThe updates introduce a deep, pattern-based field exclusion mechanism for logging request and response data, supporting nested paths and wildcards. Documentation is expanded to clarify usage, and related tests are added to verify correct exclusion behavior. Dependencies are updated to remove Changes
Sequence Diagram(s)sequenceDiagram
participant API
participant Logger
participant Utils
API->>Logger: Log request/response data (with exclusion fields)
Logger->>Utils: Call omitRecursive(data, exclusionPatterns)
Utils->>Utils: Deep clone data
Utils->>Utils: Recursively remove fields matching patterns (supports *, **)
Utils-->>Logger: Return filtered data
Logger-->>API: Log filtered data
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.js⚙️ CodeRabbit Configuration File
Files:
🔇 Additional comments (4)
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. 🪧 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: 1
🧹 Nitpick comments (2)
README.md (1)
58-71: Clear documentation of the enhanced field exclusion feature.The documentation effectively explains the wildcard patterns and their usage. Consider adding examples to illustrate the patterns more clearly.
Consider adding examples after the warnings to clarify usage:
⚠️ **Warning**: - When using the wildcard `*` alone in the field path of the `excludeFieldsLogRequestData` or `excludeFieldsLogResponseBody` static getter, it will exclude all the fields in the log. - In case the field path is incorrect, it will not exclude any field. + +**Examples**: +- `'password'` - Excludes all 'password' fields recursively +- `'user.*.email'` - Excludes 'email' field from all array elements in 'user' +- `'data.**.secret'` - Excludes 'secret' fields at any depth under 'data' +- `'*'` - Excludes all fields (use with caution)lib/utils.js (1)
35-46: Consider performance implications of the ** wildcard.The current implementation tries every possible position when matching
**, which could be expensive for deeply nested objects. For a path of length n, it makes n+1 recursive calls.Consider adding a depth limit or warning in documentation about performance implications when using
**on deeply nested structures.Also, verify that edge cases with empty segments are handled correctly:
#!/bin/bash # Description: Check if there are any patterns with consecutive dots or leading/trailing dots in tests # Search for test cases with edge case patterns rg "excludeFields.*=.*\[.*[\'\"]\.+.*[\'\"].*\]" tests/ -A5 -B5 # Check if empty string patterns are tested rg "excludeFields.*=.*\[.*[\'\"][\'\"].*\]" tests/
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(1 hunks)lib/helpers/log.js(1 hunks)lib/utils.js(2 hunks)package.json(1 hunks)tests/dispatcher-test.js(26 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.js
Instructions used from:
Sources:
⚙️ CodeRabbit Configuration File
🧬 Code Graph Analysis (1)
lib/helpers/log.js (1)
lib/utils.js (1)
omitRecursive(99-112)
🪛 Biome (1.9.4)
lib/helpers/log.js
[error] 35-35: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🪛 GitHub Actions: Coverage Status
tests/dispatcher-test.js
[error] 1-1: Test coverage thresholds not met: lines (10.4%), functions (0%), branches (0%), statements (10.23%) below required 100%.
🔇 Additional comments (5)
package.json (1)
41-46: LGTM! Dependency cleanup aligns with implementation changes.The removal of
lodash.omitis appropriate since theomitRecursivefunction now useslodash.clonedeepfor deep cloning instead.lib/helpers/log.js (1)
34-34: Good defensive programming with empty array fallbacks.Adding
|| []prevents potential runtime errors whenexcludeFieldsLogRequestDataorexcludeFieldsLogResponseBodyare undefined.Also applies to: 40-40
tests/dispatcher-test.js (1)
782-1255: Comprehensive test coverage for the field exclusion feature.The tests thoroughly cover various scenarios including:
- Simple and nested field exclusion
- Wildcard patterns (
*and**)- Multiple exclusion patterns
- Edge cases (empty arrays, non-matching patterns)
- Deep nested structures
Well done on the thorough testing approach!
lib/utils.js (2)
67-90: Well-implemented recursive traversal and property removal.The function correctly:
- Handles both arrays and objects
- Builds the path incrementally
- Checks patterns before recursing deeper
- Safely deletes matching properties
27-28: Fix incorrect single-segment pattern matching logic.The current implementation only checks if the pattern matches the last segment of the path, which is incorrect. A single-segment pattern should match if it appears anywhere in the path as the final segment.
For example, with pattern
['password']:
- Path
['user', 'password']should match ✓- Path
['password']should match ✓- Path
['data', 'user', 'info', 'password']should match ✓- But path
['password', 'hash']should NOT match ✗The current logic would incorrectly reject valid matches and accept invalid ones. Consider this fix:
- // Special case: if pattern has only one segment, check if it matches the last path segment or is a wildcard - if(pattern.length === 1) - return path[path.length - 1] === pattern[0] || pattern[0] === '*'; + // Special case: if pattern has only one segment + if(pattern.length === 1) { + // If it's a wildcard, it matches any single-segment path + if(pattern[0] === '*') + return path.length === 1; + // Otherwise, check if the pattern matches the entire path (for root-level fields) + return path.length === 1 && path[0] === pattern[0]; + }However, based on the tests and use cases, it seems the intention might be to match the pattern as a leaf node anywhere in the object. In that case, this logic should be removed entirely and let the general recursive logic handle it.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/build-status.yml(1 hunks).github/workflows/coverage-status.yml(1 hunks).github/workflows/npm-publish.yml(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/npm-publish.yml
- .github/workflows/build-status.yml
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/coverage-status.yml
15-15: the runner of "actions/setup-node@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
| 'use strict'; | ||
|
|
||
| const omit = require('lodash.omit'); | ||
| const cloneDeep = require('lodash.clonedeep'); |
There was a problem hiding this comment.
siendo que mudaste todo a node 18 ya no necesitamos lodash.cloneDeep para esto es ir por structuredClone
| recurse(patterns, value, newPath); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
no se si viene de copy paste de otro lado o es nuevo pero me parece bastante nefasto en performance dar soporte al ** ya que hicimos eso por lo menos a nivel código intentaría atajarlo
forEachen esto es un rotundamente no... todos los MS van a pasar por acá eso es lento ->for ... of- este ya es un poco mas fino pero estamos recreando e iterando los array y además recursivo, para esto se puede evitar con .push() y .pop(), de esa forma solo vamos actualizando por iteración sin importar la profundidad, con push antes de entrar y pop al salir para mantener el array
- el Object.entries() se recontra tiene que ir, es Object.keys(), de esa forma evitamos la doble iteración de
entriespara "accder lindo" convaluea fin de cuenta lo único que cambia es el llamado arecursedel final
|
|
||
| object = { ...object }; // Avoid original object modification | ||
| // Convert dot-notation path patterns into arrays of path segments. | ||
| const patterns = pathPatterns.map(p => p.split('.')); |
There was a problem hiding this comment.
este ya solo para joderte jajajaja pero ya que estamos en borrar todo acá metes un for...of en vez de map y sigue mejorando
4 participants
Historia de usuario
https://janiscommerce.atlassian.net/browse/JCN-519
Sub-tarea
https://janiscommerce.atlassian.net/browse/JCN-520
Descripcion del requerimiento
Actualmente el paquete
@janiscommerce/apino permite omitir de los logs propiedades que estan dentro de arrays. Por ende, se muestran dichas propiedades con informacion sensible de seguridad lo cual no es adecuado ni seguro.Es necesario modificar los getters
excludeFieldsLogRequestDatayexcludeFieldsLogResponseBodypara que dichas propiedades puedan ser removidas de los logs en caso de ser especificado.Descripcion de la solucion
Al recibir los arrays
excludeFieldsLogRequestDataoexcludeFieldsLogResponseBody, se llama a un metodoomitRecursiveel cual fue modificado para lograr el requerimiento. Ahora, en vez de pasar solo nombres de propiedades, se puede hacer lo siguiente:Para la opcion del path, en caso de no saber el nombre root del log, se puede usar una wildcard
*y luego la propiedad a eliminar. Tambien, para los casos donde la propiedad esta dentro un array de objeto, se debe usar la wildcard*para que recorra cada objeto del array.Summary by CodeRabbit
Documentation
Bug Fixes
New Features
Tests
Chores