diff --git a/django/db/migrations/loader.py b/django/db/migrations/loader.py index 66944c7ab7b0..db81f6f78f91 100644 --- a/django/db/migrations/loader.py +++ b/django/db/migrations/loader.py @@ -109,11 +109,11 @@ def load_disk(self): if was_loaded: reload(module) self.migrated_apps.add(app_config.label) - migration_names = { + migration_names = [ name for _, name, is_pkg in pkgutil.iter_modules(module.__path__) if not is_pkg and name[0] not in "_~" - } + ] # Load migrations for migration_name in migration_names: migration_path = "%s.%s" % (module_name, migration_name) diff --git a/django/db/models/base.py b/django/db/models/base.py index b58e7e3e524d..8d7cbdd32d2e 100644 --- a/django/db/models/base.py +++ b/django/db/models/base.py @@ -1801,7 +1801,7 @@ def _check_composite_pk(cls): meta = cls._meta pk = meta.pk - if not isinstance(pk, CompositePrimaryKey): + if meta.proxy or not isinstance(pk, CompositePrimaryKey): return errors seen_columns = defaultdict(list) diff --git a/docs/internals/_images/contribution_process.svg b/docs/internals/_images/contribution_process.svg new file mode 100644 index 000000000000..6e005a572bbe --- /dev/null +++ b/docs/internals/_images/contribution_process.svg @@ -0,0 +1,4 @@ + + + +
Needs Triage
Ready For Checkin
Accepted
Needs Patch
Needs PR Review
Closed
fixed
Closed
duplicate
invalid
needsinfo
wontfix
worksforme
Ticket Progress
100%
5%
Waiting On Author
Django Maintainers
Mergers
Anyone in the Community
Triagers
Reviewers
Bug Fixers
 \ No newline at end of file diff --git a/docs/internals/_images/triage_process.graffle b/docs/internals/_images/triage_process.graffle deleted file mode 100644 index 67bdbfb60057..000000000000 Binary files a/docs/internals/_images/triage_process.graffle and /dev/null differ diff --git a/docs/internals/_images/triage_process.pdf b/docs/internals/_images/triage_process.pdf deleted file mode 100644 index 558553e2dd7c..000000000000 Binary files a/docs/internals/_images/triage_process.pdf and /dev/null differ diff --git a/docs/internals/_images/triage_process.svg b/docs/internals/_images/triage_process.svg deleted file mode 100644 index 6fbf1cbcc7f2..000000000000 --- a/docs/internals/_images/triage_process.svg +++ /dev/null @@ -1,282 +0,0 @@ - - - - - - - - - - - - - - - - - - - - - - - Canevas 1 - - - Calque 1 - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - Closed tickets - resolution - - - - - - - Open tickets - triage state - - - - - - - Ready for - Checkin - - - - - - - Accepted - - - - - - - Unreviewed - - - - - - - duplicate - - - - - - - fixed - - - - - - - invalid - - - - - - - needsinfo - - - - - - - worksforme - - - - - - - wontfix - - - - - - - - - - - - completed - - - - - - stopped - - - - - - in progress - - - - - - - - Ticket triagers - - - - - - - - Mergers - - - - - - status - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - The ticket was already reported, was - already rejected, isn't a bug, doesn't contain - enough information, or can't be reproduced. - - - - - - - - - - - - - The ticket is a - bug and should - be fixed. - - - - - - - - - - - - - The ticket has a patch which applies cleanly and includes all - needed tests and docs. A merger can commit it as is. - - - - - - - - - - - - - - diff --git a/docs/internals/contributing/new-contributors.txt b/docs/internals/contributing/new-contributors.txt index 99f70b770a83..63835289ebb5 100644 --- a/docs/internals/contributing/new-contributors.txt +++ b/docs/internals/contributing/new-contributors.txt @@ -28,7 +28,7 @@ If an `unreviewed ticket`_ reports a bug, try and reproduce it. If you can reproduce it and it seems valid, make a note that you confirmed the bug and accept the ticket. Make sure the ticket is filed under the correct component area. Consider writing a patch that adds a test for the bug's behavior, even if -you don't fix the bug itself. See more at :ref:`how-can-i-help-with-triaging` +you don't fix the bug itself. See more at :ref:`how-can-i-help-with-triaging`. Review patches of accepted tickets ---------------------------------- diff --git a/docs/internals/contributing/triaging-tickets.txt b/docs/internals/contributing/triaging-tickets.txt index 62556dbfbb7c..66f0cc661ec5 100644 --- a/docs/internals/contributing/triaging-tickets.txt +++ b/docs/internals/contributing/triaging-tickets.txt @@ -49,47 +49,58 @@ attribute easily tells us what and who each ticket is waiting on. Since a picture is worth a thousand words, let's start there: -.. image:: /internals/_images/triage_process.* - :height: 750 - :width: 600 +.. image:: /internals/_images/contribution_process.* :alt: Django's ticket triage workflow -We've got two roles in this diagram: +We have four roles in this diagram. Maintainers (also known as Fellows) usually +take part in all of them, but anyone in the Django community can participate in +any role except merger. The :ref:`merger role ` is granted by a +vote of the :ref:`Steering Council `. -* Mergers: people with commit access who are responsible for making the - final decision to merge a change. +* Triagers: anyone can take on this role by checking whether a ticket describes + a real issue and keeping the tracker organized. -* Ticket triagers: anyone in the Django community who chooses to - become involved in Django's development process. Our Trac installation - is intentionally left open to the public, and anyone can triage tickets. - Django is a community project, and we encourage :ref:`triage by the - community`. +* Bug fixers: anyone can contribute by opening a pull request and working on a + solution for a ticket. -By way of example, here we see the lifecycle of an average ticket: +* Reviewers: anyone can review pull requests and suggest improvements. -* Alice creates a ticket and sends an incomplete pull request (no tests, +* Mergers: people with commit access who make the final decision to merge a + change. + +Our Trac system is intentionally open to the public, and anyone can help by +working on tickets. Django is a community project, and we encourage +:ref:`triage and collaboration by the community +`. This could be you! + +For example, here's the typical lifecycle of a ticket: + +* Alice creates a ticket and opens an incomplete pull request (missing tests, incorrect implementation). -* Bob reviews the pull request, marks the ticket as "Accepted", "needs tests", - and "patch needs improvement", and leaves a comment telling Alice how the - patch could be improved. +* Bob reviews the pull request, marks the ticket as "Accepted", sets the + flags "needs tests" and "patch needs improvement", and leaves a comment + explaining how Alice can improve the patch. This puts the ticket + automatically into the "waiting on author" queue within the "accepted" stage. -* Alice updates the pull request, adding tests (but not changing the - implementation). She removes the two flags. +* Alice updates the pull request, adding tests (but not yet fixing the + implementation), and removes the two flags. The ticket moves into the "needs + PR review" queue. -* Charlie reviews the pull request and resets the "patch needs improvement" - flag with another comment about improving the implementation. +* Charlie reviews the pull request, sets the "patch needs improvement" flag + again, and leaves another comment suggesting changes to the implementation. + The ticket moves back to the "waiting on author" queue. -* Alice updates the pull request, fixing the implementation. She removes the - "patch needs improvement" flag. +* Alice updates the pull request again, this time fixing the implementation, + and removes the "patch needs improvement" flag. The ticket moves once more + into the "needs PR review" queue. * Daisy reviews the pull request and marks the ticket as "Ready for checkin". -* Jacob, a :ref:`merger `, reviews the pull request and merges - it. +* Jacob, a :ref:`merger `, reviews and merges the pull request. -Some tickets require much less feedback than this, but then again some tickets -require much much more. +Some tickets move through these steps quickly, while others take more time and +discussion. Each contribution helps Django improve. .. _triage-stages: @@ -104,16 +115,15 @@ Unreviewed The ticket has not been reviewed by anyone who felt qualified to make a judgment about whether the ticket contained a valid issue or ought to be closed -for any of the various reasons. +for any reasons. Unreviewed tickets appear in the "triage" queue. Accepted -------- -The big gray area! The absolute meaning of "accepted" is that the issue -described in the ticket is valid and is in some stage of being worked on. -Beyond that there are several considerations: +The absolute meaning of "accepted" is that the issue described in the ticket is +valid and actionable. It is broken out into three queues: -* **Accepted + No Flags** +* **Needs Patch** (Accepted + No Flags) The ticket is valid, but no one has submitted a patch for it yet. Often this means you could safely start writing a fix for it. This is generally more @@ -126,14 +136,14 @@ Beyond that there are several considerations: ` and received community and :ref:`Steering Council ` approval, or been accepted in a DEP. -* **Accepted + Has Patch** +* **Needs PR Review** (Accepted + Has Patch) The ticket is waiting for people to review the supplied solution. This means downloading the patch and trying it out, verifying that it contains tests and docs, running the test suite with the included patch, and leaving feedback on the ticket. -* **Accepted + Has Patch + Needs ...** +* **Waiting On Author** (Accepted + Has Patch + Needs fixes) This means the ticket has been reviewed, and has been found to need further work. "Needs tests" and "Needs documentation" are self-explanatory. "Patch @@ -339,10 +349,10 @@ bring the issue to the `Django Forum`_ instead. .. _how-can-i-help-with-triaging: -How can I help with triaging? -============================= +How can I help with development? +================================ -The triage process is primarily driven by community members. Really, +The development process is primarily driven by community members. Really, **ANYONE** can help. To get involved, start by `creating an account on Trac`_. If you have an diff --git a/docs/releases/5.2.8.txt b/docs/releases/5.2.8.txt index a75db9cbc8ca..62cb32f55c29 100644 --- a/docs/releases/5.2.8.txt +++ b/docs/releases/5.2.8.txt @@ -16,3 +16,6 @@ Bugfixes * Fixed a bug in Django 5.2 where ``QuerySet.first()`` and ``QuerySet.last()`` raised an error on querysets performing aggregation that selected all fields of a composite primary key. + +* Fixed a bug in Django 5.2 where proxy models having a ``CompositePrimaryKey`` + incorrectly raised a ``models.E042`` system check error. diff --git a/tests/composite_pk/test_checks.py b/tests/composite_pk/test_checks.py index c33f2ee2eb69..21796aa87111 100644 --- a/tests/composite_pk/test_checks.py +++ b/tests/composite_pk/test_checks.py @@ -268,3 +268,38 @@ class Bar(Foo): ), ], ) + + def test_proxy_model_can_subclass_model_with_composite_pk(self): + class Foo(models.Model): + pk = models.CompositePrimaryKey("a", "b") + a = models.SmallIntegerField() + b = models.SmallIntegerField() + + class Bar(Foo): + class Meta: + proxy = True + + self.assertEqual(Foo.check(databases=self.databases), []) + self.assertEqual(Bar.check(databases=self.databases), []) + + def test_proxy_model_does_not_check_superclass_composite_pk_errors(self): + class Foo(models.Model): + pk = models.CompositePrimaryKey("a", "b") + a = models.SmallIntegerField() + + class Bar(Foo): + class Meta: + proxy = True + + self.assertEqual( + Foo.check(databases=self.databases), + [ + checks.Error( + "'b' cannot be included in the composite primary key.", + hint="'b' is not a valid field.", + obj=Foo, + id="models.E042", + ), + ], + ) + self.assertEqual(Bar.check(databases=self.databases), []) diff --git a/tests/migrations/test_loader.py b/tests/migrations/test_loader.py index 7cf11f7faa9c..efa7a9416661 100644 --- a/tests/migrations/test_loader.py +++ b/tests/migrations/test_loader.py @@ -1,7 +1,12 @@ import compileall import os +import subprocess +import sys +import tempfile from importlib import import_module +from pathlib import Path +from django.conf import settings from django.db import connection, connections from django.db.migrations.exceptions import ( AmbiguityError, @@ -649,6 +654,70 @@ def test_loading_package_without__file__(self): test_module.__spec__.origin = module_origin test_module.__spec__.has_location = module_has_location + def test_loading_order_does_not_create_circular_dependency(self): + """ + Before, for these migrations: + app1 + [ ] 0001_squashed_initial <- replaces app1.0001 + [ ] 0002_squashed_initial <- replaces app1.0001 + depends on app1.0001_squashed_initial & app2.0001_squashed_initial + app2 + [ ] 0001_squashed_initial <- replaces app2.0001 + + When loading app1's migrations, if 0002_squashed_initial was first: + {'0002_squashed_initial', '0001_initial', '0001_squashed_initial'} + Then CircularDependencyError was raised, but it's resolvable as: + {'0001_initial', '0001_squashed_initial', '0002_squashed_initial'} + """ + # Create a test settings file to provide to the subprocess. + MIGRATION_MODULES = { + "app1": "migrations.test_migrations_squashed_replaced_order.app1", + "app2": "migrations.test_migrations_squashed_replaced_order.app2", + } + INSTALLED_APPS = [ + "migrations.test_migrations_squashed_replaced_order.app1", + "migrations.test_migrations_squashed_replaced_order.app2", + ] + tests_dir = Path(__file__).parent.parent + with tempfile.NamedTemporaryFile( + mode="w", encoding="utf-8", suffix=".py", dir=tests_dir, delete=False + ) as test_settings: + for attr, value in settings._wrapped.__dict__.items(): + if attr.isupper(): + test_settings.write(f"{attr} = {value!r}\n") + # Provide overrides here, instead of via decorators. + test_settings.write(f"DATABASES = {settings.DATABASES}\n") + test_settings.write(f"MIGRATION_MODULES = {MIGRATION_MODULES}\n") + # Isolate away other test apps. + test_settings.write( + "INSTALLED_APPS=[a for a in INSTALLED_APPS if a.startswith('django')]\n" + ) + test_settings.write(f"INSTALLED_APPS += {INSTALLED_APPS}\n") + test_settings_name = test_settings.name + self.addCleanup(os.remove, test_settings_name) + + test_environ = os.environ.copy() + test_environ["PYTHONPATH"] = str(tests_dir) + # Ensure deterministic failures. + test_environ["PYTHONHASHSEED"] = "1" + + args = [ + sys.executable, + "-m", + "django", + "showmigrations", + "app1", + "--skip-checks", + "--settings", + Path(test_settings_name).stem, + ] + try: + subprocess.run( + args, capture_output=True, env=test_environ, check=True, text=True + ) + except subprocess.CalledProcessError as err: + self.fail(err.stderr) + class PycLoaderTests(MigrationTestBase): def test_valid(self): diff --git a/tests/migrations/test_migrations_squashed_replaced_order/__init__.py b/tests/migrations/test_migrations_squashed_replaced_order/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/tests/migrations/test_migrations_squashed_replaced_order/app1/0001_initial.py b/tests/migrations/test_migrations_squashed_replaced_order/app1/0001_initial.py new file mode 100644 index 000000000000..cf311d1f9309 --- /dev/null +++ b/tests/migrations/test_migrations_squashed_replaced_order/app1/0001_initial.py @@ -0,0 +1,9 @@ +from django.db import migrations + + +class Migration(migrations.Migration): + initial = True + + dependencies = [] + + operations = [] diff --git a/tests/migrations/test_migrations_squashed_replaced_order/app1/0001_squashed_initial.py b/tests/migrations/test_migrations_squashed_replaced_order/app1/0001_squashed_initial.py new file mode 100644 index 000000000000..7a77d339bc71 --- /dev/null +++ b/tests/migrations/test_migrations_squashed_replaced_order/app1/0001_squashed_initial.py @@ -0,0 +1,13 @@ +from django.db import migrations + + +class Migration(migrations.Migration): + initial = True + + replaces = [ + ("app1", "0001_initial"), + ] + + dependencies = [] + + operations = [] diff --git a/tests/migrations/test_migrations_squashed_replaced_order/app1/0002_squashed_initial.py b/tests/migrations/test_migrations_squashed_replaced_order/app1/0002_squashed_initial.py new file mode 100644 index 000000000000..fc16ed6d6eba --- /dev/null +++ b/tests/migrations/test_migrations_squashed_replaced_order/app1/0002_squashed_initial.py @@ -0,0 +1,16 @@ +from django.db import migrations + + +class Migration(migrations.Migration): + initial = True + + replaces = [ + ("app1", "0001_initial"), + ] + + dependencies = [ + ("app1", "0001_squashed_initial"), + ("app2", "0001_squashed_initial"), + ] + + operations = [] diff --git a/tests/migrations/test_migrations_squashed_replaced_order/app1/__init__.py b/tests/migrations/test_migrations_squashed_replaced_order/app1/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/tests/migrations/test_migrations_squashed_replaced_order/app2/0001_initial.py b/tests/migrations/test_migrations_squashed_replaced_order/app2/0001_initial.py new file mode 100644 index 000000000000..05d040949671 --- /dev/null +++ b/tests/migrations/test_migrations_squashed_replaced_order/app2/0001_initial.py @@ -0,0 +1,11 @@ +from django.db import migrations + + +class Migration(migrations.Migration): + initial = True + + dependencies = [ + ("app1", "0001_initial"), + ] + + operations = [] diff --git a/tests/migrations/test_migrations_squashed_replaced_order/app2/0001_squashed_initial.py b/tests/migrations/test_migrations_squashed_replaced_order/app2/0001_squashed_initial.py new file mode 100644 index 000000000000..de6bfe41352d --- /dev/null +++ b/tests/migrations/test_migrations_squashed_replaced_order/app2/0001_squashed_initial.py @@ -0,0 +1,13 @@ +from django.db import migrations + + +class Migration(migrations.Migration): + initial = True + + replaces = [ + ("app2", "0001_initial"), + ] + + dependencies = [] + + operations = [] diff --git a/tests/migrations/test_migrations_squashed_replaced_order/app2/__init__.py b/tests/migrations/test_migrations_squashed_replaced_order/app2/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1