From 26b0e2bb92caf2d16cabe455792350f20d6f42ca Mon Sep 17 00:00:00 2001 From: saurabh Date: Thu, 4 Dec 2025 20:55:21 +0530 Subject: [PATCH 1/2] Fixed #36620 -- Fixed workflow to summarize coverage in PRs. Follow-up to a89183e63844a937aacd3ddb73c4952ef869d2cc, which was reverted in e4c4a178aa642f8493b7ae2c0ad58527af51f67e because a change to the workflow trigger resulted in the PR branch not being checked out. We used this opportunity to reimplement the coverage tracing and coverage commenting in a two-workflow pattern with more granular permissions. To reduce duplicative workflows, we removed the existing python test workflow on PRs, at least until we run more distinct configurations on GitHub actions. The run with coverage tracing enabled is sufficient for now. The existing workflow still runs on pushes to main. We can revisit when adding more test configurations. --- .github/workflows/coverage_comment.yml | 72 +++++++++++++++++++++++++ .github/workflows/coverage_tests.yml | 75 ++++++++++++++++++++++++++ .github/workflows/tests.yml | 1 + zizmor.yml | 1 + 4 files changed, 149 insertions(+) create mode 100644 .github/workflows/coverage_comment.yml create mode 100644 .github/workflows/coverage_tests.yml diff --git a/.github/workflows/coverage_comment.yml b/.github/workflows/coverage_comment.yml new file mode 100644 index 000000000000..d63e6f4a75ad --- /dev/null +++ b/.github/workflows/coverage_comment.yml @@ -0,0 +1,72 @@ +name: Coverage Comment + +on: + workflow_run: + workflows: ["Coverage Tests"] + types: + - completed + +permissions: + contents: read + pull-requests: write + +jobs: + comment: + if: > + github.event.workflow_run.event == 'pull_request' && + github.event.workflow_run.conclusion == 'success' && + github.repository == 'django/django' && + github.event.workflow_run.pull_requests[0] != null + name: Post Coverage Comment + runs-on: ubuntu-latest + timeout-minutes: 60 + steps: + - name: Download diff coverage report + uses: actions/download-artifact@v4 + with: + name: diff-coverage-report-${{ github.event.workflow_run.pull_requests[0].number }} + github-token: ${{ secrets.GITHUB_TOKEN }} + run-id: ${{ github.event.workflow_run.id }} + + - name: Post/update PR comment + env: + PR_NUMBER: ${{ github.event.workflow_run.pull_requests[0].number }} + uses: actions/github-script@v8 + with: + script: | + const fs = require('fs'); + const reportPath = 'diff-cover-report.md'; + let body = 'No coverage data available.'; + if (fs.existsSync(reportPath)) { + body = fs.readFileSync(reportPath, 'utf8'); + } + const commentBody = '### 📊 Coverage Report for Changed Files\n\n```\n' + body + '\n```\n\n**Note:** Missing lines are warnings only. Some lines may not be covered by SQLite tests as they are database-specific.\n\nFor more information about code coverage on pull requests, see the [contributing documentation](https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/unit-tests/#code-coverage-on-pull-requests).'; + + const prNumber = parseInt(process.env.PR_NUMBER); + if (isNaN(prNumber)) { + core.setFailed('PR number is not available or invalid.'); + return; + } + + const { data: comments } = await github.rest.issues.listComments({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + }); + + for (const c of comments) { + if ((c.body || '').includes('📊 Coverage Report for Changed Files')) { + await github.rest.issues.deleteComment({ + owner: context.repo.owner, + repo: context.repo.repo, + comment_id: c.id, + }); + } + } + + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + body: commentBody, + }); diff --git a/.github/workflows/coverage_tests.yml b/.github/workflows/coverage_tests.yml new file mode 100644 index 000000000000..af463bb29502 --- /dev/null +++ b/.github/workflows/coverage_tests.yml @@ -0,0 +1,75 @@ +name: Coverage Tests + +on: + pull_request: + paths-ignore: + - 'docs/**' + - '**/*.md' + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +permissions: + contents: read + +jobs: + coverage: + if: github.repository == 'django/django' + name: Coverage Tests (Windows) + runs-on: windows-latest + timeout-minutes: 60 + steps: + - name: Checkout + uses: actions/checkout@v5 + with: + fetch-depth: 0 + persist-credentials: false + + - name: Set up Python + uses: actions/setup-python@v6 + with: + python-version: '3.14' + cache: 'pip' + cache-dependency-path: 'tests/requirements/py3.txt' + + - name: Install dependencies + run: | + python -m pip install --upgrade pip wheel + python -m pip install -r tests/requirements/py3.txt -e . + python -m pip install 'coverage[toml]' diff-cover + + - name: Run tests with coverage + env: + PYTHONPATH: ${{ github.workspace }}/tests + COVERAGE_PROCESS_START: ${{ github.workspace }}/tests/.coveragerc + RUNTESTS_DIR: ${{ github.workspace }}/tests + run: | + python -Wall tests/runtests.py -v2 + + - name: Generate coverage report + if: success() + env: + COVERAGE_RCFILE: ${{ github.workspace }}/tests/.coveragerc + RUNTESTS_DIR: ${{ github.workspace }}/tests + run: | + python -m coverage combine + python -m coverage report --show-missing + python -m coverage xml -o tests/coverage.xml + + - name: Generate diff coverage report + if: success() + run: | + if (Test-Path 'tests/coverage.xml') { + diff-cover tests/coverage.xml --compare-branch=origin/main --fail-under=0 > diff-cover-report.md + } else { + Set-Content -Path diff-cover-report.md -Value 'No coverage data available.' + } + + - name: Upload diff coverage report + if: success() + uses: actions/upload-artifact@v4 + with: + name: diff-coverage-report-${{ github.event.pull_request.number }} + path: diff-cover-report.md + retention-days: 1 diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index b709ed514540..079fae63283c 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -19,6 +19,7 @@ permissions: jobs: windows: + if: github.event_name == 'push' runs-on: windows-latest strategy: matrix: diff --git a/zizmor.yml b/zizmor.yml index 76e53f73cc29..23630337b48c 100644 --- a/zizmor.yml +++ b/zizmor.yml @@ -1,6 +1,7 @@ rules: dangerous-triggers: ignore: + - coverage_comment.yml - labels.yml - new_contributor_pr.yml unpinned-uses: From 17d644c8e257c2ea5cc738fb7a9c47989e29bf09 Mon Sep 17 00:00:00 2001 From: Tim Graham Date: Wed, 3 Dec 2025 18:06:53 -0500 Subject: [PATCH 2/2] Added DatabaseFeatures.prohibits_dollar_signs_in_column_aliases. This is also applicable on CockroachDB. --- django/db/backends/base/features.py | 4 ++++ django/db/backends/postgresql/compiler.py | 11 +---------- django/db/backends/postgresql/features.py | 1 + django/db/models/sql/compiler.py | 8 ++++++++ tests/annotations/tests.py | 7 +++++-- 5 files changed, 19 insertions(+), 12 deletions(-) diff --git a/django/db/backends/base/features.py b/django/db/backends/base/features.py index 1e957d105ed3..38af239cbe75 100644 --- a/django/db/backends/base/features.py +++ b/django/db/backends/base/features.py @@ -415,6 +415,10 @@ class BaseDatabaseFeatures: # Does the Round() database function round to even? rounds_to_even = False + # Should dollar signs be prohibited in column aliases to prevent SQL + # injection? + prohibits_dollar_signs_in_column_aliases = False + # A set of dotted paths to tests in Django's test suite that are expected # to fail on this database. django_test_expected_failures = set() diff --git a/django/db/backends/postgresql/compiler.py b/django/db/backends/postgresql/compiler.py index 08d78e333a5d..48d0ccfd9d06 100644 --- a/django/db/backends/postgresql/compiler.py +++ b/django/db/backends/postgresql/compiler.py @@ -1,6 +1,6 @@ from django.db.models.sql.compiler import ( # isort:skip SQLAggregateCompiler, - SQLCompiler as BaseSQLCompiler, + SQLCompiler, SQLDeleteCompiler, SQLInsertCompiler as BaseSQLInsertCompiler, SQLUpdateCompiler, @@ -25,15 +25,6 @@ def __str__(self): return "UNNEST(%s)" % ", ".join(self) -class SQLCompiler(BaseSQLCompiler): - def quote_name_unless_alias(self, name): - if "$" in name: - raise ValueError( - "Dollar signs are not permitted in column aliases on PostgreSQL." - ) - return super().quote_name_unless_alias(name) - - class SQLInsertCompiler(BaseSQLInsertCompiler): def assemble_as_sql(self, fields, value_rows): # Specialize bulk-insertion of literal values through UNNEST to diff --git a/django/db/backends/postgresql/features.py b/django/db/backends/postgresql/features.py index 5e4ed320be8e..30d06c95c500 100644 --- a/django/db/backends/postgresql/features.py +++ b/django/db/backends/postgresql/features.py @@ -70,6 +70,7 @@ class DatabaseFeatures(BaseDatabaseFeatures): supports_nulls_distinct_unique_constraints = True supports_no_precision_decimalfield = True can_rename_index = True + prohibits_dollar_signs_in_column_aliases = True test_collations = { "deterministic": "C", "non_default": "sv-x-icu", diff --git a/django/db/models/sql/compiler.py b/django/db/models/sql/compiler.py index 20f06ad168a8..9068d87a8974 100644 --- a/django/db/models/sql/compiler.py +++ b/django/db/models/sql/compiler.py @@ -553,6 +553,14 @@ def quote_name_unless_alias(self, name): for table names. This avoids problems with some SQL dialects that treat quoted strings specially (e.g. PostgreSQL). """ + if ( + self.connection.features.prohibits_dollar_signs_in_column_aliases + and "$" in name + ): + raise ValueError( + "Dollar signs are not permitted in column aliases on " + f"{self.connection.display_name}." + ) if name in self.quote_cache: return self.quote_cache[name] if ( diff --git a/tests/annotations/tests.py b/tests/annotations/tests.py index 10cd05db63f4..6336cabafae9 100644 --- a/tests/annotations/tests.py +++ b/tests/annotations/tests.py @@ -1545,8 +1545,11 @@ def test_alias_filtered_relation_sql_injection_dollar_sign(self): qs = Book.objects.alias( **{"crafted_alia$": FilteredRelation("authors")} ).values("name", "crafted_alia$") - if connection.vendor == "postgresql": - msg = "Dollar signs are not permitted in column aliases on PostgreSQL." + if connection.features.prohibits_dollar_signs_in_column_aliases: + msg = ( + "Dollar signs are not permitted in column aliases on " + f"{connection.display_name}." + ) with self.assertRaisesMessage(ValueError, msg): list(qs) else: