redis / redis-om-python

Object mapping, and more, for Redis and Python
MIT License
1.13k stars 114 forks source link

Fix: Incorrect Query Prefixing for Embedded Models in Query Construction #657

Open bilal02arbisoft opened 1 month ago

bilal02arbisoft commented 1 month ago

Background: In the current implementation, constructing queries involving embedded models results in incorrect field prefixing. Specifically, when combining multiple conditions using logical operators (e.g., OR |), the parents list—which tracks the hierarchy of embedded fields—is unintentionally merged. This leads to improperly concatenated field names, causing queries like @player1_player2_username:{username}instead of the intended@player1_username:{username} | @player2_username:{username}. as mentioned in #636 .

Problem: When executing queries with embedded models, the shared parents list across different expressions causes incorrect prefix concatenation. This results in invalid RediSearch queries that do not accurately target the intended fields within embedded models.

class Player(EmbeddedJsonModel):
    username: str = Field(index=True)

class Game(JsonModel):
    player1: Optional[Player]
    player2: Optional[Player]

q = Game.find((Game.player1.username=='username') | (Game.player2.username=='username'))
print(q.query)
# Output: (@player1_player2_username:{username}) | (@player1_player2_username:{username})
# Expected: (@player1_username:{username}) | (@player2_username:{username})

Solution: To resolve this issue, the management of the parents list within theExpressionProxy and FindQuery classes has been revised to ensure that each expression maintains its own isolated parents hierarchy. This prevents the unintended merging of prefixes when combining multiple query conditions.

Key Changes:

Cloning the parents List in ExpressionProxy: __init__ Method: Cloned the parents list to ensure each ExpressionProxy instance operates with its own copy, preventing shared state across expressions. __getattr__ Method: Cloned the parents list before appending a new parent, maintaining isolation between different query expressions. Adjusting FindQuery.resolve_redisearch_query: Ensured that the parents list specific to each expression is used independently when constructing the query string. Prevented the merging of prefixes by correctly prefixing field names based on their respective parents hierarchies. Ensuring Correct Prefixing in resolve_value: Confirmed that the resolve_value method receives and utilizes the correctly prefixed field_name, eliminating the possibility of incorrect query segments.

Impact: Correct Query Construction: Queries involving embedded models will now correctly reflect the intended field hierarchies without erroneous prefix concatenations. Enhanced Reliability: Prevents future issues related to query inaccuracies when dealing with complex embedded model structures. Backward Compatibility: Existing functionalities remain unaffected except for the correction of the query construction logic. This resolves #636 and also #542

bilal02arbisoft commented 1 month ago

Hi @banker @simonprickett @chayim can you guys pls take a look at this?