Skip to content

Fix query lookup when using Money object - #13

Merged
VictorTan93 merged 9 commits into
masterfrom
victor/fix-custom-manager-bug
Dec 23, 2025
Merged

VictorTan93 merged 9 commits into
masterfrom
victor/fix-custom-manager-bug

Conversation

@VictorTan93

@VictorTan93 VictorTan93 commented Dec 22, 2025

Copy link
Copy Markdown

📝 Description

Fix query lookup when using Money object

This PR fixes an issue with Django ORM query lookups when using Money objects directly in filters.

Previously, querying with a Money object (e.g. Model.objects.filter(price=Money(100, 'USD'))) would only compare the amount without checking the currency, leading to incorrect results when multiple currencies are present in the database.


📝 Changes

Core Fix:

Implemented custom lookup classes (MoneyExactLookup, MoneyLtLookup, MoneyLteLookup, MoneyGtLookup, MoneyGteLookup):

Moving away from MoneyManager approach, because it is easily overwritten when a custom manager is used in a Django Model (i.e. objects = CustomManager()).

Code Cleanup:

  • Removed MoneyManager class
  • Extracted MoneyFieldProxy into its own module (proxy.py) for better organization
  • Extracted utility functions (currency_field_name, currency_field_db_column) into utils.py

Testing:

Added comprehensive test suite in test_django_lookups.py covering:

  • Exact lookups with Money objects
  • Greater than/less than comparisons
  • Greater than or equal to / less than or equal to comparisons
  • Edge cases and currency validation

Behavior

When filtering with a Money object:

Product.objects.filter(price=Money(100, 'USD'))

The generated SQL now checks both the amount column AND the currency column:

WHERE (price = 100 AND price_currency = 'USD')

This ensures that only records with matching currency are returned, preventing incorrect comparisons between amounts in different currencies.


✅ Pre-review checklist for the PR author

  • I have tested my code locally on my machine
  • I have written tests (if appropriate)
  • I have added appropriate label to PR

@VictorTan93 VictorTan93 changed the title Remove MoneyManager and disable lookups using Money Fix query lookup using Money object Dec 23, 2025
@VictorTan93 VictorTan93 changed the title Fix query lookup using Money object Fix query lookup when using Money object Dec 23, 2025
ent = MoneyModelDefaults.objects.get(pk=ent.id)
assert ent.price == Money(100, "USD")

def test_lookup(self) -> None:

@VictorTan93 VictorTan93 Dec 23, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Moved to test_django_lookups.py

class MoneyField(InfiniteDecimalField):
description = gettext_lazy("An amount and type of currency")

add_currency_field: bool

@ewendlick ewendlick Dec 23, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

NAB: Very minor, but what do you think about calling this something a little different, like contains_currency_field, has_currency_field, with_currency_field, or using_currency_field?

"add_currency_field" feels a little ambiguous to me, whether it is for adding a currency field to an existing instance, for positive amounts, or if it is a method

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ahhh this is not introduced by me, this line was added for mypy typing.
Changing this will affect everyone using it

@VictorTan93
VictorTan93 merged commit 4707a77 into master Dec 23, 2025
6 checks passed
@VictorTan93
VictorTan93 deleted the victor/fix-custom-manager-bug branch December 24, 2025 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants