Skip to content
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

Fix issue with non-string values in get_db_prep_save for a bulk_update operation #35

Merged
merged 2 commits into from
Sep 27, 2024

Conversation

jeroendottech
Copy link
Contributor

@jeroendottech jeroendottech commented Sep 26, 2024

This pull request addresses an issue in the BaseEncryptedField class where objects with an as_sql method are not checked and are incorrectly processed in the get_db_prep_save method during a bulk_update operation.

Problem

During a bulk_update, the get_db_prep_save method is passed query expressions. However, the method incorrectly attempts to encrypt these query expressions directly, leading to exponential growth in the data size within the database during repeated bulk_update operations. This behavior occurs because query expressions, like Cast <django.db.models.functions.comparison.Cast>, are not handled correctly within the encryption process.

Solution

The new implementation checks if the value is a query expression (i.e., it has an as_sql() method), and skips encryption in such cases. This ensures that query expressions are handled properly without being mistakenly converted into bytes.

The encryption still occurs for standard field values (e.g., Decimal, Integer, String) that require encryption, while query expressions are correctly processed by the database.

Changes

  • Updated get_db_prep_save:
    The get_db_prep_save method now checks if the value is a query expression (as_sql()) and returns the value without encryption if it is. This allows SQL expressions to be processed properly during database operations, and ensures encryption occurs later for the actual values.
  • New Test for bulk_update:
    A new test has been added to verify the correct behavior of bulk_update operations. The test ensures that the get_db_prep_save method processes encrypted fields correctly while handling SQL expressions appropriately during bulk_update.

Cheers,
Jeroen Weustink

@jeroendottech jeroendottech changed the title Fix issue with non-string values in get_db_prep_save for bulk operations Fix issue with non-string values in get_db_prep_save for a bulk_update operation Sep 26, 2024
@jeroendottech jeroendottech changed the title Fix issue with non-string values in get_db_prep_save for a bulk_update operation Fix issue with non-string values in get_db_prep_save for a bulk_update operation Sep 26, 2024
@dcwatson
Copy link
Owner

Looks the parameter is named prepared, not is_prepared. If possible, could you also add a test case doing a bulk_update?

…` test

- Updated `get_db_prep_save` to skip encryption for SQL expressions like `Cast` during `bulk_update`.
- Added a test to ensure correct handling of values in bulk updates while maintaining encryption for regular fields.
@jeroendottech
Copy link
Contributor Author

Looks the parameter is named prepared, not is_prepared. If possible, could you also add a test case doing a bulk_update?

I was wrong. Although this fixes part of the problem, it creates a few new ones. The correct solution has been committed. Please have a look; this is the same solution as implemented in django.db.models.fields.Field.get_db_prep_save()

I've also added test_bulk_update.

@dcwatson dcwatson merged commit 6c3cb2c into dcwatson:main Sep 27, 2024
9 checks passed
@dcwatson
Copy link
Owner

dcwatson commented Sep 27, 2024

Thank you! I'll get a 3.0.1 release out soon.

Update: just released it.

@jeroendottech jeroendottech deleted the fix-bug-get_db_prep_save branch September 27, 2024 13:58
@asmedes
Copy link

asmedes commented Sep 28, 2024

Well done @jeroendottech and @dcwatson.

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.

3 participants