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: Incorrect Query Prefixing for Embedded Models in Query Construction #657

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bilal02arbisoft
Copy link

@bilal02arbisoft bilal02arbisoft commented Sep 22, 2024

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 the ExpressionProxy 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
Copy link
Author

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

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.

Incorrect Query Mapping for Embedded Model
1 participant