Skip to content

Apply rabbit suggestions on the e2e feature branch#3389

Merged
arkid15r merged 4 commits intoOWASP:feature/e2e-backendfrom
ahmedxgouda:e2e/apply-rabbit-suggestions
Jan 17, 2026
Merged

Apply rabbit suggestions on the e2e feature branch#3389
arkid15r merged 4 commits intoOWASP:feature/e2e-backendfrom
ahmedxgouda:e2e/apply-rabbit-suggestions

Conversation

@ahmedxgouda
Copy link
Collaborator

Proposed change

Resolves #(put the issue number here)

Add the PR description here.

Checklist

  • Required: I followed the contributing workflow
  • Required: I verified that my code works as intended and resolves the issue as described
  • Required: I ran make check-test locally: all warnings addressed, tests passed
  • I used AI for code, documentation, tests, or communication related to this PR

@github-actions
Copy link

PR validation failed: No linked issue and no valid closing issue reference in PR description

@github-actions github-actions bot closed this Jan 17, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 17, 2026

Summary by CodeRabbit

  • Chores

    • Reorganized environment example files for consistency.
    • Minor docstring clarifications for issue, PR, release, and post queries.
  • Refactor

    • Safer database operations and improved error handling.
    • Adjusted relation loading to improve query performance.
    • Migrated internal SQL handling to a composable format and updated type annotations.
  • Bug Fixes / API

    • Some fields may now be nullable — callers should handle missing values.
  • Tests

    • Updated tests to match SQL composition and behavior changes.

✏️ Tip: You can customize this high-level summary in your review settings.

Walkthrough

Refactors SQL in the dump_data management command to use psycopg2.sql compositions, updates tests accordingly, adjusts several GraphQL docstrings and resolver behaviors, changes ORM relation loading and a safe lookup, and reorders two frontend env variables.

Changes

Cohort / File(s) Summary
Dump data command
backend/apps/common/management/commands/dump_data.py
Replace f-string SQL with psycopg2.sql compositions (sql.SQL/sql.Identifier); add OperationalError handling; change signatures/returns to use sql.Composable and propagate composed SQL.
Dump data tests
backend/tests/apps/common/management/commands/dump_data_test.py
Remove mock_sql patch and param; update assertions to expect sql.SQL(...).format(sql.Identifier(...)) constructions.
GitHub API docstrings
backend/apps/github/api/internal/queries/issue.py, backend/apps/github/api/internal/queries/pull_request.py, backend/apps/github/api/internal/queries/release.py
Docstring edits: narrow distinct descriptions to "per author" (docstring-only changes).
PullRequest node & tests
backend/apps/github/api/internal/nodes/pull_request.py, backend/tests/apps/github/api/internal/nodes/pull_request_test.py
url resolver returns root.url directly (may be None) instead of ""; removed test that expected empty-string fallback.
User query relation loading
backend/apps/github/api/internal/queries/user.py
Change relation loading to select_related=["owasp_profile"] and prefetch_related=["user_badges__badge"].
Committee node type
backend/apps/owasp/api/internal/nodes/committee.py
Widen created_at return type from float to `float
Post query docstring
backend/apps/owasp/api/internal/queries/post.py
Docstring change: "Return the 5 most recent posts." → "Return the most recent posts."
Project health lookup
backend/apps/owasp/models/project_health_metrics.py
Replace .get(level=...) with .filter(level=...).first() to avoid exceptions and return None when missing.
Frontend env examples
frontend/.env.e2e.example, frontend/.env.example
Move NEXTAUTH_SECRET and NEXTAUTH_URL entries to the top of the files (values unchanged).
CI workflow Docker build
.github/workflows/update-nest-test-images.yaml
Change Docker build context and file path for fuzz-backend image.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested labels

ci, docker

Suggested reviewers

  • arkid15r
  • kasya
🚥 Pre-merge checks | ❌ 3
❌ Failed checks (2 warnings, 1 inconclusive)
Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is a template with placeholder text and incomplete checklist items; it does not provide meaningful information about the actual changes being made in this pull request. Replace the template with a clear description of the changes, including the main objectives (SQL composition refactoring, type updates, docstring changes) and link to the relevant issue.
Docstring Coverage ⚠️ Warning Docstring coverage is 68.75% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The PR title 'Apply rabbit suggestions on the e2e feature branch' is vague and does not clearly describe the main changes, which include SQL composition refactoring, type signature updates, docstring clarifications, and environment variable reorganization. Provide a more specific title that highlights the primary change (e.g., 'Refactor to use sql.Composable for SQL queries in dump_data command' or 'Apply CodeRabbit code review suggestions across multiple modules').

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ahmedxgouda ahmedxgouda reopened this Jan 17, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@backend/apps/common/management/commands/dump_data.py`:
- Around line 96-104: The DROP DATABASE cleanup in dump_data.py currently only
catches OperationalError inside the block that calls self._execute_sql (the DROP
DATABASE IF EXISTS {temp_db} SQL); broaden the exception handling to also catch
psycopg2.ProgrammingError (i.e., use except (OperationalError,
ProgrammingError):) and ensure ProgrammingError is imported where
OperationalError is imported so the best-effort cleanup won't raise an unhandled
ProgrammingError from _execute_sql.

coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 17, 2026
@ahmedxgouda ahmedxgouda marked this pull request as ready for review January 17, 2026 14:27
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
backend/apps/common/management/commands/dump_data.py (1)

109-120: Schema‑qualify email updates to prevent targeting wrong table if search_path is modified.

_table_list_query() explicitly filters WHERE table_schema = 'public', but _remove_emails() generates UPDATE {table} without schema qualification. If search_path is customized or contains a schema with a same-named table, the UPDATE could affect the wrong schema, leaving emails unmasked in the public tables being dumped.

Select table_schema alongside table_name and qualify updates with {schema}.{table}:

Proposed fix
-        table_list = self._execute_sql(temp_db, [self._table_list_query()])
-        self._execute_sql(temp_db, self._remove_emails([row[0] for row in table_list]))
+        table_list = self._execute_sql(temp_db, [self._table_list_query()])
+        self._execute_sql(
+            temp_db,
+            self._remove_emails([(row[0], row[1]) for row in table_list]),
+        )

     def _table_list_query(self) -> sql.Composable:
         return sql.SQL("""
-        SELECT table_name
+        SELECT table_schema, table_name
         FROM information_schema.columns
         WHERE table_schema = 'public' AND column_name = 'email';
         """)

-    def _remove_emails(self, tables: list[str]) -> list[sql.Composable]:
+    def _remove_emails(self, tables: list[tuple[str, str]]) -> list[sql.Composable]:
         return [
-            sql.SQL("UPDATE {table} SET email = '';").format(table=sql.Identifier(table))
-            for table in tables
+            sql.SQL("UPDATE {schema}.{table} SET email = '';").format(
+                schema=sql.Identifier(schema), table=sql.Identifier(table)
+            )
+            for schema, table in tables
         ]

@sonarqubecloud
Copy link

@arkid15r arkid15r merged commit 9e5ad1f into OWASP:feature/e2e-backend Jan 17, 2026
29 of 30 checks passed
arkid15r pushed a commit that referenced this pull request Jan 19, 2026
* Update dump_data

* Apply rabbit suggestions

* Update tests and dump_data command

* Update update-nest-test-images.yaml
arkid15r pushed a commit that referenced this pull request Jan 19, 2026
* Update dump_data

* Apply rabbit suggestions

* Update tests and dump_data command

* Update update-nest-test-images.yaml
@ahmedxgouda ahmedxgouda deleted the e2e/apply-rabbit-suggestions branch January 21, 2026 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments