Opened 10 months ago

Closed 5 months ago

#28913 closed Bug (fixed)

If MIGRATIONS_MODULES has a missing top-level package, proper error message is not displayed

Reported by: oTree-org Owned by: Sanket Saurav
Component: Migrations Version: 1.11
Severity: Normal Keywords:
Cc: Sanket Saurav Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The following code in django.db.migrations.writer doesn't handle the case where the top-level package doesn't exist. When existing_dirs only has one element left, it will be popped, so existing_dirs is empty. Then import_module() will be passed an empty string, which doesn't raise ImportError, but rather "ValueError: Empty module name". Because ValueError is not caught, the "raise ValueError" with the helpful error message is not displayed.

        # In case of using MIGRATION_MODULES setting and the custom package
        # doesn't exist, create one, starting from an existing package
        existing_dirs, missing_dirs = migrations_package_name.split("."), []
        while existing_dirs:
            missing_dirs.insert(0, existing_dirs.pop(-1))
            try:
                base_module = import_module(".".join(existing_dirs))
            except ImportError:
                continue
            else:
                try:
                    base_dir = module_dir(base_module)
                except ValueError:
                    continue
                else:
                    break
        else:
            raise ValueError(
                "Could not locate an appropriate location to create "
                "migrations package %s. Make sure the toplevel "
                "package exists and can be imported." %
                migrations_package_name)

Attachments (1)

bug28913.zip (13.2 KB) - added by oTree-org 9 months ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 9 months ago by Tim Graham

Component: UncategorizedMigrations

Could you give a concrete example of the problem? I'm having difficulty translating your description into steps to reproduce.

Changed 9 months ago by oTree-org

Attachment: bug28913.zip added

comment:2 Changed 9 months ago by oTree-org

Sure, I attached a sample project. Try running "python manage.py makemigrations foo" to reproduce the error.

Repro steps, from scratch:

  • python manage.py startapp "foo"
  • Add "foo" to INSTALLED_APPS
  • Add a simple model in foo/models.py
  • Set:
    MIGRATION_MODULES = {'foo': 'nonexisting_dir'}
    
  • Run "python manage.py makemigrations foo".

Expected: this should trigger the following error explaining that the 'nonexisting_dir' package is missing and needs to be created first (this error message already exists in the source code but is unreachable because of faulty exception handling):

                "Could not locate an appropriate location to create "
                "migrations package %s. Make sure the toplevel "
                "package exists and can be imported."

Actual result: uninformative error message

Traceback (most recent call last):
  File "C:\oTree\ve_dj11\lib\site-packages\django\core\management\commands\makemigrations.py", line 212, in write_migration_files
    migration_string = os.path.relpath(writer.path)
  File "C:\oTree\ve_dj11\lib\site-packages\django\db\migrations\writer.py", line 289, in path
    return os.path.join(self.basedir, self.filename)
  File "C:\oTree\ve_dj11\lib\site-packages\django\db\migrations\writer.py", line 256, in basedir
    base_module = import_module(".".join(existing_dirs))
  File "C:\Users\wi\AppData\Local\Programs\Python\Python36-32\Lib\importlib\__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 973, in _gcd_import
  File "<frozen importlib._bootstrap>", line 925, in _sanity_check
ValueError: Empty module name

Without the proper error message, it's unclear how to fix the problem. I would not have guessed that the top-level package needs to exist; I would have expected "makemigrations foo" to create it for me.

comment:3 Changed 9 months ago by Tim Graham

Triage Stage: UnreviewedAccepted

comment:4 Changed 5 months ago by Sanket Saurav

Owner: changed from nobody to Sanket Saurav
Status: newassigned

comment:5 Changed 5 months ago by Sanket Saurav

Cc: Sanket Saurav added
Has patch: set

comment:6 Changed 5 months ago by Tim Graham

Needs tests: set

comment:7 Changed 5 months ago by Sanket Saurav

Needs tests: unset

Added tests and updated PR.

comment:8 Changed 5 months ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 079f3243:

Fixed #28913 -- Fixed error handling when MIGRATIONS_MODULES specifies a nonexistent top-level package.

Note: See TracTickets for help on using tickets.
Back to Top