cfpb / django-flags

Feature flags for Django projects
https://cfpb.github.io/django-flags/
Creative Commons Zero v1.0 Universal
260 stars 32 forks source link

Setting user based flag conditions prevents migrations running on a fresh database #96

Open moaxey opened 2 years ago

moaxey commented 2 years ago

My CI always starts with a fresh database. This user condition gave me the desired behaviour, and worked when adding the Django flags app and migrating from an existing database:

 FLAGS = {
     'FLAG_NAME': [
         {'condition': 'user', 'value': 'test_user'},
         {'condition': 'boolean', 'value': True, 'required': True}
     ]
 }

But the user condition would be tested before the user table was created when running migrations on a new database.

Traceback (most recent call last):
  File "/usr/local/lib/python3.9/site-packages/django/db/backends/utils.py", line 85, in _execute
    return self.cursor.execute(sql, params)
psycopg2.errors.UndefinedTable: relation "users_user" does not exist
LINE 1: ...ers_user"."date_joined", "users_user"."name" FROM "users_use...
                                                             ^

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/app/manage.py", line 31, in <module>
    execute_from_command_line(sys.argv)
  File "/usr/local/lib/python3.9/site-packages/django/core/management/__init__.py", line 425, in execute_from_command_line
    utility.execute()
  File "/usr/local/lib/python3.9/site-packages/django/core/management/__init__.py", line 419, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/usr/local/lib/python3.9/site-packages/django/core/management/base.py", line 373, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/usr/local/lib/python3.9/site-packages/django/core/management/base.py", line 417, in execute
    output = self.handle(*args, **options)
  File "/usr/local/lib/python3.9/site-packages/django/core/management/base.py", line 90, in wrapped
    res = handle_func(*args, **kwargs)
  File "/usr/local/lib/python3.9/site-packages/django/core/management/commands/migrate.py", line 75, in handle
    self.check(databases=[database])
  File "/usr/local/lib/python3.9/site-packages/django/core/management/base.py", line 438, in check
    all_issues = checks.run_checks(
  File "/usr/local/lib/python3.9/site-packages/django/core/checks/registry.py", line 77, in run_checks
    new_errors = check(app_configs=app_configs, databases=databases)
  File "/usr/local/lib/python3.9/site-packages/flags/checks.py", line 30, in flag_conditions_check
    condition.fn.validate(condition.value)
  File "/usr/local/lib/python3.9/site-packages/flags/conditions/validators.py", line 44, in validate_user
    UserModel.objects.get(**{UserModel.USERNAME_FIELD: value})
  File "/usr/local/lib/python3.9/site-packages/django/db/models/manager.py", line 85, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/usr/local/lib/python3.9/site-packages/django/db/models/query.py", line 435, in get
    num = len(clone)
  File "/usr/local/lib/python3.9/site-packages/django/db/models/query.py", line 262, in __len__
    self._fetch_all()
  File "/usr/local/lib/python3.9/site-packages/django/db/models/query.py", line 1354, in _fetch_all
    self._result_cache = list(self._iterable_class(self))
  File "/usr/local/lib/python3.9/site-packages/django/db/models/query.py", line 51, in __iter__
    results = compiler.execute_sql(chunked_fetch=self.chunked_fetch, chunk_size=self.chunk_size)
  File "/usr/local/lib/python3.9/site-packages/django/db/models/sql/compiler.py", line 1202, in execute_sql
    cursor.execute(sql, params)
  File "/usr/local/lib/python3.9/site-packages/django/db/backends/utils.py", line 67, in execute
    return self._execute_with_wrappers(sql, params, many=False, executor=self._execute)
  File "/usr/local/lib/python3.9/site-packages/django/db/backends/utils.py", line 76, in _execute_with_wrappers
    return executor(sql, params, many, context)
  File "/usr/local/lib/python3.9/site-packages/django/db/backends/utils.py", line 85, in _execute
    return self.cursor.execute(sql, params)
  File "/usr/local/lib/python3.9/site-packages/django/db/utils.py", line 90, in __exit__
    raise dj_exc_value.with_traceback(traceback) from exc_value
  File "/usr/local/lib/python3.9/site-packages/django/db/backends/utils.py", line 85, in _execute
    return self.cursor.execute(sql, params)
django.db.utils.ProgrammingError: relation "users_user" does not exist
LINE 1: ...ers_user"."date_joined", "users_user"."name" FROM "users_use...
                                                             ^

Do I need to add these flags in a migration themselves instead of in code?

willbarton commented 2 years ago

@moaxey Hmm, I don't think you should need to do anything in migrations. This looks like a bug in Django-Flags with the user condition validator being used in the Django checks.

The other observation I have is, with a boolean condition that is True, the user condition is unnecessary unless the user condition is also required.

But the traceback suggests to me that we should be handling model errors in the checks better than we are. I'll see if I can get a fix in.

moaxey commented 2 years ago

Thankyou @willbarton

cristobalmackenzie commented 1 year ago

We ran into the same issue trying to deploy a new user column. Luckily our django-flags usage is very sparse and we were able to delete them, migrate and then recreate them.

Traceback (most recent call last):
  File "/usr/local/lib/python3.10/site-packages/django/db/backends/utils.py", line 84, in _execute
    return self.cursor.execute(sql, params)
psycopg2.errors.UndefinedColumn: column users_user.mobile_number does not exist
LINE 1: ..._user"."is_active", "users_user"."is_accounting", "users_use...
                                                             ^

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/app/manage.py", line 39, in <module>
    execute_from_command_line(sys.argv)
  File "/usr/local/lib/python3.10/site-packages/django/core/management/__init__.py", line 419, in execute_from_command_line
    utility.execute()
  File "/usr/local/lib/python3.10/site-packages/django/core/management/__init__.py", line 413, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/usr/local/lib/python3.10/site-packages/django/core/management/base.py", line 354, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/usr/local/lib/python3.10/site-packages/django/core/management/base.py", line 398, in execute
    output = self.handle(*args, **options)
  File "/usr/local/lib/python3.10/site-packages/django/core/management/base.py", line 89, in wrapped
    res = handle_func(*args, **kwargs)
  File "/usr/local/lib/python3.10/site-packages/django/core/management/commands/migrate.py", line 75, in handle
    self.check(databases=[database])
  File "/usr/local/lib/python3.10/site-packages/django/core/management/base.py", line 419, in check
    all_issues = checks.run_checks(
  File "/usr/local/lib/python3.10/site-packages/django/core/checks/registry.py", line 76, in run_checks
    new_errors = check(app_configs=app_configs, databases=databases)
  File "/usr/local/lib/python3.10/site-packages/flags/checks.py", line 30, in flag_conditions_check
    condition.fn.validate(condition.value)
  File "/usr/local/lib/python3.10/site-packages/flags/conditions/validators.py", line 45, in validate_user
    UserModel.objects.get(**{UserModel.USERNAME_FIELD: value})
  File "/usr/local/lib/python3.10/site-packages/django/db/models/manager.py", line 85, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/usr/local/lib/python3.10/site-packages/django/db/models/query.py", line 431, in get
    num = len(clone)
  File "/usr/local/lib/python3.10/site-packages/django/db/models/query.py", line 262, in __len__
    self._fetch_all()
  File "/usr/local/lib/python3.10/site-packages/django/db/models/query.py", line 1324, in _fetch_all
    self._result_cache = list(self._iterable_class(self))
  File "/usr/local/lib/python3.10/site-packages/django/db/models/query.py", line 51, in __iter__
    results = compiler.execute_sql(chunked_fetch=self.chunked_fetch, chunk_size=self.chunk_size)
  File "/usr/local/lib/python3.10/site-packages/django/db/models/sql/compiler.py", line 1175, in execute_sql
    cursor.execute(sql, params)
  File "/usr/local/lib/python3.10/site-packages/django/db/backends/utils.py", line 98, in execute
    return super().execute(sql, params)
  File "/usr/local/lib/python3.10/site-packages/django/db/backends/utils.py", line 66, in execute
    return self._execute_with_wrappers(sql, params, many=False, executor=self._execute)
  File "/usr/local/lib/python3.10/site-packages/django/db/backends/utils.py", line 75, in _execute_with_wrappers
    return executor(sql, params, many, context)
  File "/usr/local/lib/python3.10/site-packages/django/db/backends/utils.py", line 79, in _execute
    with self.db.wrap_database_errors:
  File "/usr/local/lib/python3.10/site-packages/django/db/utils.py", line 90, in __exit__
    raise dj_exc_value.with_traceback(traceback) from exc_value
  File "/usr/local/lib/python3.10/site-packages/django/db/backends/utils.py", line 84, in _execute
    return self.cursor.execute(sql, params)
django.db.utils.ProgrammingError: column users_user.mobile_number does not exist
LINE 1: ..._user"."is_active", "users_user"."is_accounting", "users_use...
                                                             ^

I'll take a look to see if I can come up with a fix, though I'm not sure what the desired behaviour should be.

cristobalmackenzie commented 1 year ago

@willbarton I think I'd catch the django.db.utils.ProgrammingError inside the validate_user function and raise a ValidationError. In the flags_condition_check maybe use a new error code to mean that the validation couldn't run properly, and add a "There seems to be an unapplied migration in the User model" message of some sort.

If that seems like a decent solution I'll work on the PR