Skip to content

Disable signal when running the remove_farm_perms migration#649

Merged
vincent-olivert-riera merged 1 commit intoline:masterfrom
hoangpn:chores/skip_signal_in_migration
Sep 24, 2025
Merged

Disable signal when running the remove_farm_perms migration#649
vincent-olivert-riera merged 1 commit intoline:masterfrom
hoangpn:chores/skip_signal_in_migration

Conversation

@hoangpn
Copy link
Contributor

@hoangpn hoangpn commented Sep 23, 2025

Promgen has a signal that creates Audit Logs every time a user removes the permission of another user. This is not necessary in the case where we run a migration script to clean up permission objects because they are automatically deleted by Promgen itself. This signal can even cause some unexpected errors if it is triggered in this case. Therefore, we temporarily disable this signal when running the migration script.

@vincent-olivert-riera
Copy link
Contributor

@hoangpn ,

This signal can even cause some unexpected errors if it is triggered in this case.

Please provide more details about that. Under which circumstances it happens, and which error message do you see?

@vincent-olivert-riera vincent-olivert-riera dismissed their stale review September 23, 2025 23:25

I would like a more detailed explanation.

@vincent-olivert-riera
Copy link
Contributor

@hoangpn ,

This signal can even cause some unexpected errors if it is triggered in this case.

Please provide more details about that. Under which circumstances it happens, and which error message do you see?

After our private discussion between me and @hoangpn , we have decided that we will go with this commit message:

Disconnect signal while running the remove_farm_perms migration

At this moment, the real relationship between a Project and a Farm is one-to-one. To honor that, in
a future PR we plan to define the relationship between Project and Farm as a OneToOneField [1]. When
that happens, the project_id field on the Farm model will not exist anymore.

If the remove_farm_perms migration runs with that new code in place, the delete_log signal that is
triggered when the permissions are removed from a Farm instance will fail because at some point (in
order to construct a log) will try to access the project_id field of that Farm instance.

Taking into account that we do not care about Farm permissions anymore (because they will be
inherited from the Project to which it belongs), we also do not care about those logs.

Therefore, to allow that migration to run without any issues in the future, we are disconnecting the
offending signal while it runs.

1: https://docs.djangoproject.com/en/5.2/topics/db/examples/one_to_one/#one-to-one-relationships

@hoangpn
Copy link
Contributor Author

hoangpn commented Sep 24, 2025

@hoangpn ,

This signal can even cause some unexpected errors if it is triggered in this case.

Please provide more details about that. Under which circumstances it happens, and which error message do you see?

The error message is django.db.utils.OperationalError: (1054, "Unknown column 'promgen_farm.project_id' in 'field list'")
This happend when I tried add a new OneToOneField named "project" to the Farm model. (I was preparing for the future changes about Farm-Project relationship). Here is how the error happened:

  1. When the 0033_remove_farm_perms migration script was ran, it removed the unneccesary permissions related to the Farm.
  2. Currently, we are recording the deletion of the permissions and saving message to the database. We are also using the str() method of the deleted object to build the logging message.
  3. We are deleting the Django-Guardian's UserObjectPermission model. The str() method of this model was trying to access to the related object of the permission to get more information, which was the Farm model.
  4. The OneToOneField "project" was added after the No. 0033 migration script. Therefore, Django ORM saw the different between the model in source code and the table in the database at the moment the script was ran, so it throw the exception "Unknown column..."

You can see that this error occurs indirectly. It cannot represent the entire reason I skipped the signal in this PR. However, I found that this signal is unnecessary when running the migration script, and skipping it also helps us fix the bug in a simple way, so I decided to skip it.

At this moment, the real relationship between a Project and a Farm is one-to-one. To honor that, in
a future PR we plan to define the relationship between Project and Farm as a OneToOneField [1]. When
that happens, the project_id field on the Farm model will not exist anymore.

If the remove_farm_perms migration runs with that new code in place, the delete_log signal that is
triggered when the permissions are removed from a Farm instance will fail because at some point (in
order to construct a log) will try to access the project_id field of that Farm instance.

Taking into account that we do not care about Farm permissions anymore (because they will be
inherited from the Project to which it belongs), we also do not care about those logs.

Therefore, to allow that migration to run without any issues in the future, we are disconnecting the
offending signal while it runs.

1: https://docs.djangoproject.com/en/5.2/topics/db/examples/one_to_one/#one-to-one-relationships
@hoangpn hoangpn force-pushed the chores/skip_signal_in_migration branch from f42f954 to 4d3bc8c Compare September 24, 2025 03:11
@vincent-olivert-riera vincent-olivert-riera merged commit 27298db into line:master Sep 24, 2025
5 checks passed
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