Skip to content

Fixed #29177: FKs accessible in subsequent migrations for unmanaged models#19452

Draft
michalnik wants to merge 3 commits intodjango:mainfrom
michalnik:ticket_29177
Draft

Fixed #29177: FKs accessible in subsequent migrations for unmanaged models#19452
michalnik wants to merge 3 commits intodjango:mainfrom
michalnik:ticket_29177

Conversation

@michalnik
Copy link
Copy Markdown
Contributor

@michalnik michalnik commented May 8, 2025

Work done

  1. I added regression tests for the scenario described in ticket #29177 in the test_autodetector_unmanaged module.
  2. I implemented a fix in django.db.migrations.autodetector.
  3. Finally, I added also unittests for Autodetector covering problems with unmanaged models and foreign keys

Trac ticket number

ticket-29177

Branch description

Provide a concise overview of the issue or rationale behind the proposed changes.

Checklist

  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • I have checked the "Has patch" ticket flag in the Trac system.
  • I have added or updated relevant tests.

@github-actions github-actions bot added the no ticket Based on PR title, no linked Trac ticket label May 8, 2025
@michalnik michalnik force-pushed the ticket_29177 branch 4 times, most recently from 0aeec5c to 2b75fd6 Compare May 11, 2025 17:30
@michalnik michalnik changed the title added regression tests WIP: Ticket #29177 May 12, 2025
@github-actions github-actions bot removed the no ticket Based on PR title, no linked Trac ticket label May 12, 2025
@michalnik michalnik changed the title WIP: Ticket #29177 WIP: Bug fix from ticket #29177 May 12, 2025
@michalnik michalnik force-pushed the ticket_29177 branch 10 times, most recently from 19d2f91 to 8508370 Compare May 13, 2025 14:32
@michalnik michalnik changed the title WIP: Bug fix from ticket #29177 FKs accessible in subsequent migrations for unmanaged models #29177 May 13, 2025
@michalnik michalnik changed the title FKs accessible in subsequent migrations for unmanaged models #29177 Ticket #29177: FKs accessible in subsequent migrations for unmanaged models May 13, 2025
@michalnik michalnik changed the title Ticket #29177: FKs accessible in subsequent migrations for unmanaged models Fixed #29177: FKs accessible in subsequent migrations for unmanaged models May 13, 2025
@michalnik michalnik marked this pull request as draft May 18, 2025 07:44
@michalnik michalnik marked this pull request as ready for review May 18, 2025 16:31
@michalnik
Copy link
Copy Markdown
Contributor Author

Regarding merge conflicts, they are caused by unrelated changes introduced after this fix.
Please advise if you want me to resolve them, or if maintainers prefer handling it.

@coolbootscoder
Copy link
Copy Markdown

@michalnik I'm not a maintainer but I think you will need to resolve the conflicts before a reviewer looks at the pull request. After a successful review and the ticket is marked for checkin, then a merger will look at the pull request.

@michalnik
Copy link
Copy Markdown
Contributor Author

michalnik commented Dec 2, 2025

@michalnik I'm not a maintainer but I think you will need to resolve the conflicts before a reviewer looks at the pull request. After a successful review and the ticket is marked for checkin, then a merger will look at the pull request.

Ok, currently I will start working on it

@coolbootscoder Thanks for the note! I double-checked the Django docs and you're correct — conflicts need to be resolved before a reviewer can look at the PR:
https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/working-with-git/#after-upstream-has-changed

@michalnik michalnik force-pushed the ticket_29177 branch 3 times, most recently from dec247a to 057ed67 Compare December 2, 2025 17:08
@michalnik michalnik marked this pull request as draft December 2, 2025 17:41
@michalnik michalnik closed this Dec 3, 2025
@michalnik
Copy link
Copy Markdown
Contributor Author

michalnik commented Dec 3, 2025

Sorry, currently I am struggling with anything from my previous commits did not work as expected.

@michalnik michalnik reopened this Dec 4, 2025
@michalnik
Copy link
Copy Markdown
Contributor Author

michalnik commented Dec 4, 2025

At my localhost, runtests ended up with success! 💯

I made changes in the core of the autodetector tool used while migrations run and It has potential to fix the bug.

So, I hope, that we can discuss what i am not sure what to do with it or anything you want.

I wrote 2 kinds of tests:

  1. integration (regression tests) - module test_autodetector_unmanaged
  2. unit tests after the change - module test_autodector (currently in the same commit as the first)

Questions:

  1. I think that integration tests should be somewhere else but I dont know where ... ?
  2. I think that unit tests cover the main problem, but maybe it is not cover all cases ... ?

Please, add the label "needs discussion" if it makes sense to you, thanx 👍

@michalnik michalnik force-pushed the ticket_29177 branch 2 times, most recently from a2172f6 to 89d264a Compare December 6, 2025 10:59
@github-actions
Copy link
Copy Markdown

github-actions bot commented Feb 5, 2026

📊 Coverage Report for Changed Files

-------------
Diff Coverage
Diff: origin/main...HEAD, staged and unstaged changes
-------------
No lines with coverage information in this diff.
-------------


Note: Missing lines are warnings only. Some lines may not be covered by SQLite tests as they are database-specific.

For more information about code coverage on pull requests, see the contributing documentation.

Integration tests part (test_autodetector_unmanaged)
1) On creation of a new managed=False model in migration with FK.
2) On addition of a FK field to already created managed=False model.
3) On removing of a FK field from already created managed=False model.

Unit tests part (test_autodetector)
1) add FK to unmanged model
2) remove FK from unmanaged model
3) rename FK within unmanaged model
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants