Skip to content

Fixed #36533 -- Fix startapp to allow empty directories as valid targets. #19708

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jefferies917
Copy link

@jefferies917 jefferies917 commented Aug 6, 2025

Trac ticket number

ticket-36533

Branch description

manage.py startapp <name> <directory> incorrectly raised a CommandError when the target directory already existed, even if it was empty and had a valid name.

This prevented valid workflows such as creating an app inside an intentionally pre-created directory (e.g. manage.py startapp blog apps/blog).

Changes:

  • Updated startapp handling to:

    • Allow creating an app inside an existing empty directory, provided the directory name itself is valid.
    • Still raise a CommandError if the directory is non-empty (to avoid silent overwrites).
    • Always validate the app name against existing Python modules (so names like os remain disallowed).
    • Validate the target directory name only when the directory does not exist yet (so os is disallowed as a fresh target, but an empty destination/ directory is valid).
  • Unified behavior of django-admin startapp and manage.py startapp.

Tests:

  • Added tests confirming that empty directories (e.g. destination) do not raise false errors.
  • Added tests confirming that os (a Python module) is always disallowed.
  • Added tests confirming that non-empty directories still raise an overlaying error.

Expected Behavior:

  • django-admin startapp example os -> disallowed
  • manage.py startapp example os -> disallowed
  • django-admin startapp example destination -> allowed (even if destination/ exists and is empty)
  • manage.py startapp example destination -> allowed (even if destination/ exists and is empty)
  • Overlaying into a non-empty directory -> disallowed

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.
  • I have added or updated relevant docs, including release notes if applicable.
  • I have attached screenshots in both light and dark modes for any UI changes.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hello! Thank you for your contribution 💪

As it's your first contribution be sure to check out the patch review checklist.

If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!

If you have any design or process questions then you can ask in the Django forum.

Welcome aboard ⛵️!

Copy link
Contributor

@sarahboyce sarahboyce left a comment

Choose a reason for hiding this comment

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

Thank you for the PR 👍

Comment on lines 3079 to 3100
def test_invalid_target_name(self):
for bad_target in (
"invalid.dir_name",
"7invalid_dir_name",
".invalid_dir_name",
):
with self.subTest(bad_target):
_, err = self.run_django_admin(["startapp", "app", bad_target])
self.assertOutput(
err,
"CommandError: '%s' is not a valid app directory. Please "
"make sure the directory is a valid identifier." % bad_target,
)

def test_importable_target_name(self):
_, err = self.run_django_admin(["startapp", "app", "os"])
self.assertOutput(
err,
"CommandError: 'os' conflicts with the name of an existing Python "
"module and cannot be used as an app directory. Please try "
"another directory.",
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that django-admin startapp example os results in CommandError: 'os' conflicts with the name of an existing Python module and cannot be used as an app directory. Please try another directory.

But after this change it is allowed.
This is reverting much of ticket-30393, which is done because if you were to then add the app created in the os directory, this would break the project with ModuleNotFoundError: No module named 'os.apps'; 'os' is not a package

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for this! I've had a look more closely and the issue might be how manage.py and django-admin handles import checks. I think manage.py startapp causes a false CommandError if the target directory exists and sys.path thinks it's an importable module even if it is empty.

I'll change the patch to adjust this behaviour and correct the changes to the tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that django-admin startapp example os results in CommandError: 'os' conflicts with the name of an existing Python module and cannot be used as an app directory. Please try another directory.

But after this change it is allowed.

This is still true after the recent changes. What I would expect is:

  • doing django-admin startapp example os or manage.py startapp example os should both be disallowed, even when there is an empty os directory in the project
  • doing django-admin startapp example destination or manage.py startapp example destination should be allowed, creating the app in destination

Copy link
Author

Choose a reason for hiding this comment

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

I can't get this to ever quite work right. I think the core issue is with find_spec() treating an empty directory as an importable module. And since django-admin runs outside of any specific project it doesn't see it and allows the creation if it already exists but manage.py detects the empty directory and thinks its a module.

So i either have to run find_spec and its thinks an empty directory is a module or ignore the directory which skips the validation. Neither of which are good solutions because they cause a different problem with the command.

@jefferies917 jefferies917 changed the title Fixed #36533 -- manage.py startapp <name> <directory> failed to create new app. Fixed #36533 -- Refactored startapp handle method to simplify empty directory handling. Aug 7, 2025
@jefferies917 jefferies917 force-pushed the ticket_36533 branch 2 times, most recently from 703a4e7 to d8ccaef Compare August 7, 2025 14:08
@jefferies917 jefferies917 changed the title Fixed #36533 -- Refactored startapp handle method to simplify empty directory handling. Fixed #36533 -- Fix startapp to allow empty directories as valid targets. Aug 18, 2025
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