refactor: enhance mask_sensitive_info in BaseAPI #3555
Conversation
WalkthroughRefactors BaseAPI's sensitive-info masking to use a location-aware mapping (PLACEHOLDER, DEFAULT_MASK_MAP) with helpers (_get_sensitive_info_mapping, _get_sensitive_info_overrides, _mask_sensitive_info). Removes subclass SENSITIVE_INFO overrides and adds comprehensive tests for the new masking behavior. No other runtime flows changed. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant BaseAPI
Caller->>BaseAPI: mask_sensitive_info(log)
BaseAPI->>BaseAPI: _get_sensitive_info_mapping()
BaseAPI->>BaseAPI: _mask_sensitive_info(log.request_headers, mapping["headers"])
BaseAPI->>BaseAPI: _mask_sensitive_info(log.data, mapping["data"])
BaseAPI->>BaseAPI: _mask_sensitive_info(log.request_body, mapping["body"])
BaseAPI->>BaseAPI: _mask_sensitive_info(log.output, mapping["output"])
BaseAPI-->>Caller: masked log
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@sagarvora |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3555 +/- ##
===========================================
+ Coverage 60.98% 61.03% +0.04%
===========================================
Files 134 135 +1
Lines 13788 13841 +53
===========================================
+ Hits 8409 8448 +39
- Misses 5379 5393 +14
... and 12 files with indirect coverage changes 🚀 New features to boost your workflow:
|
![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 (4)
india_compliance/gst_india/api_classes/test_mask_sensitive_info.py (4)
70-70: Inconsistent BaseAPI instantiation pattern.The test uses
BaseAPI.__new__(BaseAPI)here butBaseAPI()in other tests. For consistency and clarity, use the standard constructor pattern throughout.- api = BaseAPI.__new__(BaseAPI) + api = BaseAPI()
125-125: Inconsistent BaseAPI instantiation pattern.Same issue as the previous test - use consistent instantiation pattern.
- api = BaseAPI.__new__(BaseAPI) + api = BaseAPI()
156-156: Inconsistent instantiation pattern for custom API class.For consistency with the BaseAPI instantiation pattern, use the standard constructor.
- api = CustomAPI.__new__(CustomAPI) + api = CustomAPI()
172-172: Inconsistent instantiation pattern for custom API class.For consistency, use the standard constructor.
- api = NoOverrideAPI.__new__(NoOverrideAPI) + api = NoOverrideAPI()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
india_compliance/gst_india/api_classes/base.py(3 hunks)india_compliance/gst_india/api_classes/nic/e_invoice.py(1 hunks)india_compliance/gst_india/api_classes/nic/e_waybill.py(1 hunks)india_compliance/gst_india/api_classes/taxpayer_base.py(0 hunks)india_compliance/gst_india/api_classes/test_mask_sensitive_info.py(1 hunks)
🧠 Learnings (3)
india_compliance/gst_india/api_classes/nic/e_waybill.py (3)
Learnt from: karm1000
PR: #3532
File: india_compliance/gst_india/utils/e_waybill.py:327-336
Timestamp: 2025-07-22T11:45:43.419Z
Learning: In the e-waybill update functions (india_compliance/gst_india/utils/e_waybill.py), fields like place_of_change and state use hardcoded "-" values in old_values dictionaries because these values are not stored in the system, so there's no way to retrieve the actual previous values.
Learnt from: karm1000
PR: #3354
File: india_compliance/gst_india/report/summary_of_itc_availed/summary_of_itc_availed.py:278-281
Timestamp: 2025-06-25T08:19:02.607Z
Learning: In the Summary of ITC Availed report (india_compliance/gst_india/report/summary_of_itc_availed/summary_of_itc_availed.py), the summary dictionaries created by get_initial_summary() are never empty - they always contain tax field keys (igst_amount, cgst_amount, sgst_amount, cess_amount) with 0 values, so checking for empty dictionaries is unnecessary.
Learnt from: vorasmit
PR: #3326
File: india_compliance/gst_india/api_classes/nic/e_invoice.py:84-92
Timestamp: 2025-07-12T13:31:12.352Z
Learning: In the e-Invoice API implementation (india_compliance/gst_india/api_classes/nic/e_invoice.py), the base class and StandardEInvoiceAPI subclass have different is_ignored_error method implementations because enriched and standard APIs receive errors in different formats. The base class checks if the error message starts with an error code from the message field, while StandardEInvoiceAPI checks the ErrorCode field in ErrorDetails.
india_compliance/gst_india/api_classes/nic/e_invoice.py (2)
Learnt from: vorasmit
PR: #3326
File: india_compliance/gst_india/api_classes/nic/e_invoice.py:84-92
Timestamp: 2025-07-12T13:31:12.352Z
Learning: In the e-Invoice API implementation (india_compliance/gst_india/api_classes/nic/e_invoice.py), the base class and StandardEInvoiceAPI subclass have different is_ignored_error method implementations because enriched and standard APIs receive errors in different formats. The base class checks if the error message starts with an error code from the message field, while StandardEInvoiceAPI checks the ErrorCode field in ErrorDetails.
Learnt from: vorasmit
PR: #3326
File: india_compliance/gst_india/api_classes/nic/e_invoice.py:137-139
Timestamp: 2025-07-06T06:08:22.165Z
Learning: Sandbox credentials for e-Invoice API testing can be hardcoded in the EnrichedEInvoiceAPI class. These are test credentials provided by the API service (company_gstin: "02AMBPG7773M002", username: "adqgsphpusr1", password: "Gsp@1234") and are meant to be shared for ease of testing by developers.
india_compliance/gst_india/api_classes/test_mask_sensitive_info.py (1)
Learnt from: vorasmit
PR: #3399
File: india_compliance/gst_india/utils/gstr_1/test_gstr_1_books_data.py:791-822
Timestamp: 2025-05-29T15:22:04.761Z
Learning: In the india_compliance test suite, the IntegrationTestCase framework automatically handles database rollbacks after test completion, which means functions that modify global state (like GST Settings and Item Tax Templates) in setUpClass won't persist beyond the test run and don't require manual cleanup.
🧬 Code Graph Analysis (1)
india_compliance/gst_india/api_classes/test_mask_sensitive_info.py (1)
india_compliance/gst_india/api_classes/base.py (4)
BaseAPI(19-375)_get_sensitive_info_mapping(314-338)mask_sensitive_info(297-312)_get_sensitive_info_overrides(340-348)
💤 Files with no reviewable changes (1)
- india_compliance/gst_india/api_classes/taxpayer_base.py
🧰 Additional context used
🧠 Learnings (3)
india_compliance/gst_india/api_classes/nic/e_waybill.py (3)
Learnt from: karm1000
PR: #3532
File: india_compliance/gst_india/utils/e_waybill.py:327-336
Timestamp: 2025-07-22T11:45:43.419Z
Learning: In the e-waybill update functions (india_compliance/gst_india/utils/e_waybill.py), fields like place_of_change and state use hardcoded "-" values in old_values dictionaries because these values are not stored in the system, so there's no way to retrieve the actual previous values.
Learnt from: karm1000
PR: #3354
File: india_compliance/gst_india/report/summary_of_itc_availed/summary_of_itc_availed.py:278-281
Timestamp: 2025-06-25T08:19:02.607Z
Learning: In the Summary of ITC Availed report (india_compliance/gst_india/report/summary_of_itc_availed/summary_of_itc_availed.py), the summary dictionaries created by get_initial_summary() are never empty - they always contain tax field keys (igst_amount, cgst_amount, sgst_amount, cess_amount) with 0 values, so checking for empty dictionaries is unnecessary.
Learnt from: vorasmit
PR: #3326
File: india_compliance/gst_india/api_classes/nic/e_invoice.py:84-92
Timestamp: 2025-07-12T13:31:12.352Z
Learning: In the e-Invoice API implementation (india_compliance/gst_india/api_classes/nic/e_invoice.py), the base class and StandardEInvoiceAPI subclass have different is_ignored_error method implementations because enriched and standard APIs receive errors in different formats. The base class checks if the error message starts with an error code from the message field, while StandardEInvoiceAPI checks the ErrorCode field in ErrorDetails.
india_compliance/gst_india/api_classes/nic/e_invoice.py (2)
Learnt from: vorasmit
PR: #3326
File: india_compliance/gst_india/api_classes/nic/e_invoice.py:84-92
Timestamp: 2025-07-12T13:31:12.352Z
Learning: In the e-Invoice API implementation (india_compliance/gst_india/api_classes/nic/e_invoice.py), the base class and StandardEInvoiceAPI subclass have different is_ignored_error method implementations because enriched and standard APIs receive errors in different formats. The base class checks if the error message starts with an error code from the message field, while StandardEInvoiceAPI checks the ErrorCode field in ErrorDetails.
Learnt from: vorasmit
PR: #3326
File: india_compliance/gst_india/api_classes/nic/e_invoice.py:137-139
Timestamp: 2025-07-06T06:08:22.165Z
Learning: Sandbox credentials for e-Invoice API testing can be hardcoded in the EnrichedEInvoiceAPI class. These are test credentials provided by the API service (company_gstin: "02AMBPG7773M002", username: "adqgsphpusr1", password: "Gsp@1234") and are meant to be shared for ease of testing by developers.
india_compliance/gst_india/api_classes/test_mask_sensitive_info.py (1)
Learnt from: vorasmit
PR: #3399
File: india_compliance/gst_india/utils/gstr_1/test_gstr_1_books_data.py:791-822
Timestamp: 2025-05-29T15:22:04.761Z
Learning: In the india_compliance test suite, the IntegrationTestCase framework automatically handles database rollbacks after test completion, which means functions that modify global state (like GST Settings and Item Tax Templates) in setUpClass won't persist beyond the test run and don't require manual cleanup.
🧬 Code Graph Analysis (1)
india_compliance/gst_india/api_classes/test_mask_sensitive_info.py (1)
india_compliance/gst_india/api_classes/base.py (4)
BaseAPI(19-375)_get_sensitive_info_mapping(314-338)mask_sensitive_info(297-312)_get_sensitive_info_overrides(340-348)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Python Unit Tests
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (8)
india_compliance/gst_india/api_classes/nic/e_invoice.py (1)
14-14: LGTM: Clean migration to structured sensitive info masking.The removal of the
SENSITIVE_INFOextension aligns perfectly with the new structured masking approach in the base class. The previously masked keys ("password", "Password", "AppKey") are now properly handled by theDEFAULT_MASK_MAPin their appropriate locations (headers, data, body).india_compliance/gst_india/api_classes/nic/e_waybill.py (1)
19-19: LGTM: Consistent migration to structured masking approach.The removal of the
SENSITIVE_INFOextension is consistent with the refactor. The previously masked keys ("password", "app_key") are now covered by the base class'sDEFAULT_MASK_MAPin their appropriate data locations.india_compliance/gst_india/api_classes/test_mask_sensitive_info.py (1)
7-176: Excellent comprehensive test coverage for the new masking system.The test suite thoroughly validates all aspects of the refactored sensitive information masking functionality:
- ✅ Basic mapping structure validation
- ✅ Location-specific masking (headers vs. output vs. data vs. body)
- ✅ Request body handling
- ✅ Missing data edge cases
- ✅ False positive prevention
- ✅ Override mechanism functionality
- ✅ Default fallback behavior
The tests properly verify that sensitive information is masked only in appropriate locations and that the new structured approach works as intended.
india_compliance/gst_india/api_classes/base.py (5)
1-1: Appropriate import addition for deep copy functionality.The
copyimport is necessary for the deep copy operation in_get_sensitive_info_mappingto prevent modification of the class constant.
22-43: Excellent structured approach to sensitive information mapping.The new
DEFAULT_MASK_MAPprovides significant improvements over the previous flat tuple approach:
- Location-aware masking: Keys are only masked in relevant contexts (e.g., "x-api-key" only in headers)
- Comprehensive coverage: Includes all sensitive key variations and locations
- Clear categorization: Easy to understand which keys are masked where
The
PLACEHOLDERconstant provides consistent masking across the codebase.
303-312: Clean and focused refactor of the masking logic.The refactored
mask_sensitive_infomethod is much cleaner and more maintainable:
- Delegates location-specific masking to the helper method
- Uses the structured mapping for precise masking
- Maintains the same interface while improving functionality
314-348: Well-designed override mechanism with proper deep copy protection.The
_get_sensitive_info_mappingmethod implementation is excellent:
- Deep copy protection: Prevents accidental modification of the class constant
- Flexible override system: Allows subclasses to customize specific locations only
- Complete replacement strategy: Overrides replace entire lists per location for clarity
- Null handling: Properly handles empty override lists
The template method pattern in
_get_sensitive_info_overridesprovides a clean extension point for subclasses.
350-360: Simple and focused helper method for masking operations.The
_mask_sensitive_infohelper method is well-implemented:
- Null-safe: Handles missing target or sensitive_keys gracefully
- Simple logic: Clear and focused on the single responsibility of masking
- Consistent placeholder: Uses the class constant for consistent masking
vorasmit
changed the title
Fixes: #1969
Sensitive Info Masking Refactor
Problem Statement
The original
mask_sensitive_infomethod in the BaseAPI class had several issues:Solution
Before (Generic Approach)
After (Specific + Override Approach)
Benefits
1. Location-Specific Masking
2. Incremental Overrides
Subclasses only need to specify what's different:
3. Easy Customization Examples
Example 1: API that only needs custom headers
Example 2: API that doesn't return sensitive info
Example 3: API with special body fields
Migration Guide
For Base API Usage
No changes needed - existing code continues to work.
For Subclasses Using Full Override
Testing
The refactor includes comprehensive tests that verify:
Summary by CodeRabbit