fix: add logger for creation update and suppression of important objects#502
fix: add logger for creation update and suppression of important objects#502npuchois-aphp wants to merge 1 commit intomainfrom
Conversation
📝 WalkthroughWalkthroughModule-level logging was added across six view modules to record user actions and request data; minor formatting/line-wrapping edits were made. destroy_many now returns HTTP 204 for bulk deletes. No public API signatures were changed. (48 words) Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
accesses/views/role.py (1)
65-76:⚠️ Potential issue | 🟠 MajorSame premature logging issue in
partial_update.Line 73 logs "Role updated" before
super().partial_update()on line 76 actually persists the change. Move the log statement after the super call.Proposed fix
def partial_update(self, request, *args, **kwargs): role = self.get_object() data = {'name': request.data.get('name', role.name) } data.update({right: request.data.get(right, getattr(role, right, False)) for right in all_rights }) try: roles_service.check_role_has_inconsistent_rights(data=data) - _logger.info(f"Role updated by user {request.user.username} - request data: {request.data}") except IntegrityError as e: return Response(data={"error": str(e)}, status=status.HTTP_400_BAD_REQUEST) - return super(RoleViewSet, self).partial_update(request, *args, **kwargs) + response = super(RoleViewSet, self).partial_update(request, *args, **kwargs) + _logger.info(f"Role updated by user {request.user.username} - request data: {request.data}") + return responseaccesses/views/access.py (1)
130-139:⚠️ Potential issue | 🟠 MajorSame premature logging in
close— log beforesuper().partial_update().Line 138 logs "Access closed" but the actual update hasn't been persisted yet (line 139). Move the log after the super call.
cohort/views/shared.py (1)
30-35:⚠️ Potential issue | 🟠 MajorLog emitted before
super().create()— records creation that hasn't happened yet.Line 34 logs the creation before
super().create()on line 35 persists the object. If the parentcreateraises an exception (validation, DB constraint, etc.), the log entry is misleading.Proposed fix
def create(self, request, *args, **kwargs): if type(request.data) is QueryDict: request.data._mutable = True request.data['owner'] = request.data.get('owner', request.user.pk) - _logger.info(f"User {request.user.username} created object {request.data}") - return super().create(request, *args, **kwargs) + response = super().create(request, *args, **kwargs) + _logger.info(f"User {request.user.username} created object {request.data}") + return response
🤖 Fix all issues with AI agents
In `@accesses/views/access.py`:
- Around line 120-126: The log in partial_update is emitted before the actual DB
write; move the _logger.info call so it runs after super().partial_update(...)
completes successfully (so place it after the call to super().partial_update in
the partial_update method), keep the try/except around
accesses_service.process_patch_data and only log using request.user.username and
request.data once the superclass update returns without raising to avoid
premature logging.
- Around line 110-116: The log "Access created" is emitted before the actual
creation occurs; move the _logger.info call to after the call to
super().create() inside the create method so it only runs on a successful
creation. Specifically, call accesses_service.process_create_data(...) as
before, then invoke response = super().create(request, *args, **kwargs), check
that the response indicates success (e.g., HTTP 2xx) and only then call
_logger.info with request.user.username and request.data; keep the ValueError
exception handling around process_create_data unchanged.
In `@accesses/views/role.py`:
- Around line 55-61: The log saying "Role created" is emitted before the actual
creation call; update RoleViewSet.create so that
roles_service.check_role_has_inconsistent_rights is still called first, but move
the _logger.info(...) call to after the call to super(RoleViewSet,
self).create(request, *args, **kwargs) and only log on successful creation
(e.g., when the response status indicates success), using request.user.username
and request.data for context; keep the IntegrityError handling as-is so failures
still return 400 without a misleading success log.
In `@admin_cohort/views/users.py`:
- Line 82: The current log call _logger_info.info(f"User created by user
{request.user.username} - request data: {request.data}") writes the full
request.data and may leak PII; replace this by sanitizing or allowlisting fields
before logging: extract only safe fields (e.g., username, email, first_name,
last_name) or explicitly redact sensitive keys (password, ssn, token,
credentials) from request.data, then log the sanitized payload alongside
request.user.username using the same logging site in admin_cohort.views.users so
the log no longer contains raw request.data.
In `@cohort/views/cohort_result.py`:
- Line 124: Log call _logger_info.info currently writes raw request.data and may
leak PII; before logging in cohort/views/cohort_result.py (the line using
_logger_info.info with request.user.username and request.data) sanitize or
redact sensitive fields from request.data using the same sanitization helper
used in admin_cohort/views/users.py (reuse that function rather than duplicating
logic), then log the sanitized payload and username instead of the raw
request.data.
🧹 Nitpick comments (2)
admin_cohort/views/users.py (1)
23-23: Inconsistent logger variable name across files.This file uses
_logger_infowhile most other files in this PR use_logger— all pointing to the samelogging.getLogger('info'). Consider unifying the name for grep-ability and consistency.cohort/views/cohort_result.py (1)
24-25: Missing blank line between logger declaration and class definition.Minor formatting nit — add a blank line after the logger for readability and consistency with other files in this PR.
_logger_info = logging.getLogger('info') + class CohortFilter(filters.FilterSet):
| def create(self, request, *args, **kwargs): | ||
| try: | ||
| accesses_service.process_create_data(data=request.data) | ||
| _logger.info(f"Access created by user {request.user.username} - request data: {request.data}") | ||
| except ValueError as e: | ||
| return Response(data={"error": str(e)}, status=status.HTTP_400_BAD_REQUEST) | ||
| return super().create(request, *args, **kwargs) |
There was a problem hiding this comment.
Log emitted before the access is actually created.
Line 113 logs "Access created" but super().create() on line 116 hasn't run yet. If the super call raises or returns an error response, the log is misleading. Move logging after the successful super().create().
Proposed fix
def create(self, request, *args, **kwargs):
try:
accesses_service.process_create_data(data=request.data)
- _logger.info(f"Access created by user {request.user.username} - request data: {request.data}")
except ValueError as e:
return Response(data={"error": str(e)}, status=status.HTTP_400_BAD_REQUEST)
- return super().create(request, *args, **kwargs)
+ response = super().create(request, *args, **kwargs)
+ _logger.info(f"Access created by user {request.user.username} - request data: {request.data}")
+ return response📝 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.
| def create(self, request, *args, **kwargs): | |
| try: | |
| accesses_service.process_create_data(data=request.data) | |
| _logger.info(f"Access created by user {request.user.username} - request data: {request.data}") | |
| except ValueError as e: | |
| return Response(data={"error": str(e)}, status=status.HTTP_400_BAD_REQUEST) | |
| return super().create(request, *args, **kwargs) | |
| def create(self, request, *args, **kwargs): | |
| try: | |
| accesses_service.process_create_data(data=request.data) | |
| except ValueError as e: | |
| return Response(data={"error": str(e)}, status=status.HTTP_400_BAD_REQUEST) | |
| response = super().create(request, *args, **kwargs) | |
| _logger.info(f"Access created by user {request.user.username} - request data: {request.data}") | |
| return response |
🤖 Prompt for AI Agents
In `@accesses/views/access.py` around lines 110 - 116, The log "Access created" is
emitted before the actual creation occurs; move the _logger.info call to after
the call to super().create() inside the create method so it only runs on a
successful creation. Specifically, call
accesses_service.process_create_data(...) as before, then invoke response =
super().create(request, *args, **kwargs), check that the response indicates
success (e.g., HTTP 2xx) and only then call _logger.info with
request.user.username and request.data; keep the ValueError exception handling
around process_create_data unchanged.
| transaction.on_commit(lambda: cohort_service.handle_cohort_creation(request=request, | ||
| cohort=response.data.serializer.instance, | ||
| global_estimate=global_estimate)) | ||
| _logger_info.info(f"Cohort created by user {request.user.username} - request data: {request.data}") |
There was a problem hiding this comment.
Same PII concern: raw request.data logged without sanitization.
Cohort creation payloads may contain query details or patient-related parameters. Apply the same sanitization approach recommended in admin_cohort/views/users.py.
🤖 Prompt for AI Agents
In `@cohort/views/cohort_result.py` at line 124, Log call _logger_info.info
currently writes raw request.data and may leak PII; before logging in
cohort/views/cohort_result.py (the line using _logger_info.info with
request.user.username and request.data) sanitize or redact sensitive fields from
request.data using the same sanitization helper used in
admin_cohort/views/users.py (reuse that function rather than duplicating logic),
then log the sanitized payload and username instead of the raw request.data.
npuchois-aphp
force-pushed
the
fix/3171-add-more-logs
branch
from
55c5bdd to
4808834
Compare
1 participant
Summary by CodeRabbit