Repository navigation
Hey Justin, I'm still using this one! - #88
Open
mpasternak wants to merge 45 commits into
Open
mpasternak wants to merge 45 commits into
mpasternak wants to merge 45 commits into
Conversation
fix assertion error on empty slug
Fix/empty slug
…b fork Resolved conflicts preferring upstream where it improves project health: - Took upstream's modern Python 3.9+ / Django 3.2+ requirements, GitHub Actions CI, ReadTheDocs config, and pyproject.toml. - Removed dead .travis.yml. - Kept iplweb fork identity: package name 'django-autoslug-iplweb' in setup.py / pyproject.toml, README badges pointing to iplweb fork URLs, and [bumpversion] sections in setup.cfg.
- Generated PEP 621 pyproject.toml with all metadata from setup.py/setup.cfg - Initialized uv (uv.lock gitignored as this is a library) - Removed obsolete packaging files: setup.py, setup.cfg, MANIFEST.in, _version_helper.py, requirements/*.txt - Updated docs/conf.py to import __version__ from autoslug package directly - Note: [bumpversion] config was removed from setup.cfg; will be re-added in Step 2 (version consolidation)
- Restored bumpversion-style config in pyproject.toml (was in setup.cfg) - Replaced legacy bumpversion with maintained bump-my-version fork - Both pyproject.toml and autoslug/__init__.py now bump together via 'uv run bump-my-version bump <part>' - Preserved original release-segment scheme (dev/gamma) from setup.cfg - Added bump-my-version to dev dependencies
- Added .pre-commit-config.yaml with ruff lint+format and standard hygiene hooks (trailing-whitespace, detect-private-key, etc.) - Configured ruff in pyproject.toml with basic flake8-equivalent rules (E, F, W) targeting py39 - Hooks run only on staged files - no existing code reformatted in bulk - Run 'pre-commit install' (already done in this repo) to enable
- Added pytest, pytest-django, pytest-cov to dev dependencies - Added [tool.pytest.ini_options] in pyproject.toml with DJANGO_SETTINGS_MODULE = 'autoslug.tests.settings' - Created autoslug/tests/settings.py mirroring the inline configuration previously in run_tests.py - Updated GitHub Actions workflow to use uv + pytest (was pip + run_tests.py) - Removed run_tests.py, runcover.sh, tox.ini (legacy py35-py38 envlist) - Tests verified: 32 passed, 1 failed, 1 xfailed (same result as run_tests.py; the 1 failure is a pre-existing upstream test, not a runner regression) Note: the deploy job (autopub) still uses pip - left untouched; may need adjustment if autopub does not yet read pyproject.toml-only projects.
Repo has been on git for years; these files no longer serve any purpose.
Verified autopub 0.4.1 supports PEP 621 / pure-pyproject.toml projects with
setuptools.build_meta backend (autopub just delegates to 'python -m build').
- Pinned autopub to 0.4.1 (avoid the 1.0 alpha line, which lacks a
setuptools plugin)
- Slimmed install line: dropped poetry, githubrelease, httpx==0.18.2
(stale workaround), and wheel (modern build handles it)
- Bumped actions/checkout@v3 -> v4, actions/setup-python@v4 -> v5
- Use Python 3.12 for the deploy job (was 3.9; deploy doesn't need to
match the test matrix)
- Use GITHUB_REPOSITORY env var instead of ${{ github.repository }}
interpolation in the shell command (idiomatic, avoids any potential
injection edge case)
- Removed unrecognized 'build-system = "setuptools"' key from
[tool.autopub] (autopub 0.4.1 doesn't read it)
Django 4.2 LTS reached extended-EOL on 2026-04-07. Currently in support: 5.2 LTS (until 2028-04) and 6.0. Next LTS will be 6.2 (April 2027). - requires-python: 3.9 -> 3.12 (lowest Python supported by Django 5.2/6.0 intersection) - dependencies: django>=3.2 -> django>=5.2 - classifiers: keep only Framework :: Django 5.2 and 6.0 - ruff target-version: py39 -> py312 - CI matrix: Python 3.12-3.14 x Django 5.2/6.0 (no exclusions needed - every cell is supported by Django's compatibility table) Tests pass on Django 6.0 with the same baseline (32 passed, 1 xfailed, 1 pre-existing failure).
- pyupgrade --py312-plus: auto-modernize Python syntax to py3.12 idioms - django-upgrade --target-version 5.2: auto-modernize Django patterns, using lowest supported Django version as the rewrite target so changes stay safe across the 5.2/6.0 matrix - Both hooks run on staged files only (pre-commit default), so they modernize new/changed code without touching legacy upstream code - Verified against the current tree: no rewrites needed (the upstream sync already brought the codebase to a modern py3 / Django baseline)
Django 6.0+ calls Field.pre_save() twice during INSERT compilation: once via Model.save_base() and a second time via the SQL compiler's pre_save_val (django/db/models/sql/compiler.py:1751). Without a guard, the second call re-runs slugify on the already-finalized slug. For non-idempotent slugify functions (e.g. one that replaces '-' with '_') this corrupts the value: 'hello_world-2' becomes 'hello_world_2', mangling the uniqueness suffix. This was the root cause of the test_custom_slugifier failure that we inherited from upstream — upstream did not yet account for the Django 6 double-call. Fix: guard pre_save with a per-instance sentinel attribute set on the first call within an INSERT cycle. A post_save signal handler clears the sentinel so subsequent save() calls (e.g. updates, or re-saving after manual modification) still process the slug normally. Also pin the test matrix's Django version reliably: - Use 'uv pip install --reinstall-package django' to actually downgrade Django to the matrix version (the previous 'uv pip install' was a no-op because uv saw the already-installed Django as satisfying the constraint, even when its version was higher than requested). - Use 'uv run --no-sync' for subsequent commands so uv doesn't undo the pin by re-resolving the project lockfile. Per-file lint ignores are added to pyproject.toml to suppress pre-existing upstream legacy issues (E501 line-too-long, E731 lambda assignment, F401 conditional imports, etc.) so pre-commit can run cleanly when we touch a legacy file. New files are not in the ignore list and remain fully linted.
Adds a clear support-matrix table covering the same cells that CI tests (Python 3.12-3.14 x Django 5.2 LTS / 6.0). Also documents which versions were intentionally dropped and why, so users hitting an older-version issue can self-diagnose without filing a bug.
Background
----------
generate_unique_slug used a plain SELECT to look for slug collisions:
rivals = manager.filter(**lookups)
if not rivals:
return slug
This is a TOCTOU race. Two transactions concurrently generating slugs
for the same input can both SELECT, both see no rival, both return the
same candidate slug, and both then INSERT - the second loses to the
UNIQUE constraint with IntegrityError, and depending on the surrounding
transaction graph this can also surface as a deadlock under PostgreSQL
or MySQL.
Fix
---
Wrap the uniqueness check in transaction.atomic() and use
select_for_update() so concurrent generators on the same slug-prefix
serialize at the database via row-level locks instead of racing.
- transaction.atomic(savepoint=False) nests cheaply if the caller is
already in a transaction, and opens one if they are not.
- NotSupportedError is caught for backends that don't support SELECT
FOR UPDATE (notably SQLite). The DB's UNIQUE constraint remains the
ultimate guarantee.
- .exists() materializes the lock-acquiring query without pulling rows.
Tests
-----
Added autoslug/tests/test_concurrency.py with two threading-based
TransactionTestCase tests:
- test_concurrent_create_does_not_collide
- test_concurrent_create_no_deadlock
Both are skipped on SQLite because SQLite drops SELECT FOR UPDATE and
serializes all writers anyway, masking the bug. To exercise them, point
DJANGO_SETTINGS_MODULE at a settings file using PostgreSQL or MySQL.
Adds an alternative Django settings module that spins up a PostgreSQL
container at test-session start, plus a CI job that uses it. This
finally lets us verify the TOCTOU fix from ab968d2 against a backend
that actually does row-level locking.
Verification (locally, against Postgres 16):
- Reverted autoslug/utils.py to its pre-ab968d2 state.
- Ran the concurrency tests against the Postgres settings:
DJANGO_SETTINGS_MODULE=autoslug.tests.settings_postgres uv run pytest
autoslug/tests/test_concurrency.py
Both tests failed with multiple
'duplicate key value violates unique constraint
autoslug_modelwithuniqueslug_slug_key'
IntegrityErrors — exactly the race the fix is intended to prevent.
- Restored the fix; both tests pass.
Implementation
--------------
- Added testcontainers[postgres] and psycopg[binary] to dev dependencies.
- New autoslug/tests/settings_postgres.py inherits from the base test
settings, starts a postgres:16-alpine container at import time, and
wires DATABASES to the container's DSN. atexit stops the container.
- New autoslug/tests/conftest.py auto-skips two upstream tests that pass
oversize values to a CharField. SQLite accepts those silently;
Postgres strictly rejects with DataError. Skipping is cleaner than
rewriting upstream tests we did not author.
- New CI job 'test-postgres' runs the suite with the Postgres settings
module on Python 3.12/3.14 x Django 5.2/6.0 (4 cells; the SQLite job
already covers the full 3x2 matrix and the Postgres job's purpose is
specifically to validate row-locking behaviour).
- The deploy job now waits on both 'test' and 'test-postgres'.
GitHub Actions ubuntu-latest runners have Docker pre-installed, so
testcontainers works without extra setup.
- Remove pypi/format badge (uninformative) - Remove pypi/status badge (redundant with version) - Remove pypi/dd badge (deprecated by shields.io, has been broken for years) - Remove ReadTheDocs badge — pointed to upstream's RTD project, not the iplweb fork (which has no separate RTD project) - Use direct GitHub Actions badge URL instead of shields.io proxy (more reliable, no CDN cache issues) - Add pypi/djversions badge (supported Django versions, derived from the Framework :: Django :: X.Y classifiers) - Add license badge linking to COPYING.LESSER - Add :alt: text for each badge for accessibility / non-image renderers
The dynamic shields.io `pypi/pyversions` and `pypi/djversions` badges read classifiers from the latest PyPI release; that release was published with stale/missing Django classifiers, so the Django badge rendered as "missing". Replace both with static shields.io badges that match the supported versions matrix already documented below them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Add Django 6.1 support
Sync bumpversion current_version with the released version (1.9.9 -> 1.10.0); pyproject `version` and autoslug/__init__.py were already at 1.10.0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CsQ5CxHBLmnruLLxfEfab5
Publishing used to hang off the `deploy` job in main.yml: every push to
master ran autopub, which published only if a RELEASE.md happened to be
sitting in the tree. Two problems with that, and the last release hit both.
The gate is invisible. Whether a push releases depends on a file that a
contributor may or may not have added in a pull request, so "merge this"
and "release this" are the same action with an unclear outcome.
And the job is not atomic. autopub's deploy commits the changelog, pushes
the tag, creates the GitHub release, and only then uploads to PyPI. The
GitHub release step returned 401 (hand-managed GH_TOKEN secret), so the
run died having already pushed tag v1.9.10 with nothing behind it on
PyPI. That tag is still orphaned.
So: publishing moves out to release.yml, triggered by a `v*` tag, which
is what the sibling packages in this family already do. main.yml keeps
only the tests.
release.yml is three jobs. `build` refuses to continue unless the tag
agrees with all three places this package keeps its version -- pyproject
`version`, autoslug.__version__ and the bumpversion `current_version` --
then runs the suite, builds, and twine-checks. `publish` is the only job
with `id-token: write`, and holds nothing else. `github-release` uses the
workflow's own GITHUB_TOKEN, which cannot expire the way GH_TOKEN did,
and takes its body from the CHANGELOG.rst section for that version.
Authentication is PyPI Trusted Publishing (OIDC), so secrets.PYPI_PASSWORD
and secrets.GH_TOKEN are gone from the YAML with no replacement. This
needs a matching publisher on PyPI (Manage -> Publishing): owner iplweb,
repo django-autoslug-iplweb, workflow release.yml, environment pypi.
Without it the first tagged release fails with `invalid-publisher`.
bump-my-version now tags (`tag = true`, `tag_name = "v{new_version}"`),
so cutting a release is one command plus a push. The autopub config is
removed from pyproject.toml and CONTRIBUTING.rst documents the new flow
in place of the RELEASE.md instructions.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
chore: publish to PyPI over trusted publishing, from a tag
A release is a chain of three jobs and a pushed tag fires it exactly once. When a link past the first one breaks -- a failed upload, an expired token, a transient GitHub outage -- the tag already exists, so nothing will start the chain again and the missing half cannot be redone. That is how tag v1.9.10 came to exist with nothing published behind it; deleting and re-pushing a tag was the only remedy. Add workflow_dispatch beside the tag trigger so the same run can be started again against the tag it was meant for. The dispatch dialog will happily point at a branch, and every step here reads the version out of github.ref_name -- from master that is "master", which would be compared against pyproject/__init__/bumpversion as if it were a version, and cut a GitHub release named after a branch. So the build job now begins by refusing any ref that is not a tag, and says to re-run with "Use workflow from" set to the tag. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ci: allow a release to be re-run by hand from its tag
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi Justin, I hope this message finds you well
I hate reviewing large PRs and I would probably close this one, point blank :-)
This is a PR that upgrades many of the crust accumulated over years -> uv, recent Python/Django versions, code upgrades etc. This PR also pulls in pytest and testcontainers for testing this over real PostgreSQL
THIS PR ALSO fixes TOCTOU bug in our code, that occured on PostgreSQL when django-autoslug models were used in concurrent processess. 467c325 This one. It will work great in SQLite, but it will break on concurrent runs.
A lot of code, oldest commit from 2017. Well.
Dare to merge? :-)