Add content_type table to the dump data command#4042
Add content_type table to the dump data command#4042ahmedxgouda wants to merge 8 commits intoOWASP:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughTruncates Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can customize the tone of the review comments and chat replies.Configure the |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tests/apps/common/management/commands/dump_data_test.py (1)
183-216:⚠️ Potential issue | 🟡 Minor
test_dump_data_with_custom_tablesdoesn't assertpublic.django_content_typeis present.When
--table public.custom_tableis passed, argparse appends to the default list, so--table=public.django_content_typewill be incmd_args. The test should assert this to catch any future regression where the default entry is accidentally dropped.✅ Suggested assertion
assert "--table=public.owasp_*" in cmd_args assert "--table=public.github_*" in cmd_args assert "--table=public.custom_table" in cmd_args +assert "--table=public.django_content_type" in cmd_args🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/common/management/commands/dump_data_test.py` around lines 183 - 216, The test test_dump_data_with_custom_tables fails to assert that the default table entry was preserved; update the test to also assert that "--table=public.django_content_type" is present in cmd_args after call_command. Locate the assertions near the end of test_dump_data_with_custom_tables (they reference run_call, cmd_args and mock_run.call_args_list) and add a line asserting "--table=public.django_content_type" in cmd_args to ensure the default list isn't dropped when passing "--table public.custom_table".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/Makefile`:
- Around line 123-127: The load-data-staging Makefile target is calling
exec-db-command-fuzz (which connects to fuzz-nest-db) causing a
credential/container mismatch; add a new Makefile target named
exec-db-command-staging (modeled after exec-db-command and exec-db-command-fuzz)
that runs commands against the staging container (staging-nest-db) using the
staging docker-compose file (docker-compose/staging/compose.yaml), then change
load-data-staging to invoke exec-db-command-staging instead of
exec-db-command-fuzz and keep the CMD environment/psql/pg_restore arguments
as-is for nest_user_staging/nest_db_staging.
---
Outside diff comments:
In `@backend/tests/apps/common/management/commands/dump_data_test.py`:
- Around line 183-216: The test test_dump_data_with_custom_tables fails to
assert that the default table entry was preserved; update the test to also
assert that "--table=public.django_content_type" is present in cmd_args after
call_command. Locate the assertions near the end of
test_dump_data_with_custom_tables (they reference run_call, cmd_args and
mock_run.call_args_list) and add a line asserting
"--table=public.django_content_type" in cmd_args to ensure the default list
isn't dropped when passing "--table public.custom_table".
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4042 +/- ##
=======================================
Coverage 99.39% 99.39%
=======================================
Files 520 520
Lines 16442 16445 +3
Branches 2241 2241
=======================================
+ Hits 16343 16346 +3
Misses 62 62
Partials 37 37
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Could you look into making this code DRYer?
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/Makefile`:
- Around line 62-67: The current exec-backend-command-it target delegates to
exec-backend-command-dev which drops interactive TTY (-it) behavior; update the
else branch to delegate to the interactive dev target
(exec-backend-command-dev-it) or otherwise invoke the interactive variant so CMD
runs with TTY preserved (i.e., replace @$(MAKE) exec-backend-command-dev
CMD="$(CMD)" 2>/dev/null with a call to the interactive dev target that
preserves -it behavior).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/run-ci-cd.yamlbackend/Makefile
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/run-ci-cd.yaml
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Please revert unrelated changes.
| echo "Truncating content_type table" && | ||
| psql -h localhost -U nest_user_e2e -d nest_db_e2e -c "TRUNCATE django_content_type CASCADE;" && |
There was a problem hiding this comment.
Could you look into making this code DRYer?
I meant repetitive TRUNCATE django_content_type CASCADE by that.
There was a problem hiding this comment.
I thought combining the repeated make commands to one command is a good idea. Commands like load-data have an identical behavior, the only difference is the environment.
There was a problem hiding this comment.
It's difficult to read/understand. Even if we do it -- it should be done in a different PR.
There was a problem hiding this comment.
I reverted the changes. I think I can make it more readable in another PR. I will implement a common makefile target and pass environment variables for each specific target:
_load-data-common:
@echo "Truncating content_type table"
@CMD="psql -U $(DB_USER) -d $(DB_NAME) -c \"TRUNCATE django_content_type CASCADE;\"" $(MAKE) $(EXEC_DB_TARGET)
@echo "Loading Nest $(DB_ENV) data"
@CMD="pg_restore -U $(DB_USER) -d $(DB_NAME) < ./backend/data/nest..dump" $(MAKE) $(EXEC_DB_TARGET)There was a problem hiding this comment.
Maybe it'd be better extend the purge data script and run it before the data loading one?
There was a problem hiding this comment.
Sounds a good idea, but this should be in another PR right? If you don't have any comments, you may merge it and I will work on that in the next PR.
There was a problem hiding this comment.
No, that was a suggestion on how to avoid the truncate command related code duplication.
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
It's also seems it won't work for staging recently migrated to ECS.
| echo "Truncating content_type table" && | ||
| psql -h localhost -U nest_user_e2e -d nest_db_e2e -c "TRUNCATE django_content_type CASCADE;" && |
There was a problem hiding this comment.
It's difficult to read/understand. Even if we do it -- it should be done in a different PR.
ahmedxgouda
force-pushed
the
fix/dump-data
branch
from
4d3fe4d to
24128af
Compare
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/apps/common/management/commands/purge_data.py">
<violation number="1" location="backend/apps/common/management/commands/purge_data.py:35">
P1: Truncating `django_content_type` with `CASCADE` wipes global Django permissions that this dump flow does not restore.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
2 participants
Proposed change
Resolves #4030
django_content_typetable to the tables to dump in thedump_datacommand.Checklist
make check-testlocally: all warnings addressed, tests passed