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

QueryBuilder created with new_from_builder() use the same references as the builder it is created from #682

Closed
JRubics opened this issue May 23, 2022 · 6 comments · Fixed by #716 or #738
Labels
bug An existing feature is not working as intended

Comments

@JRubics
Copy link
Contributor

JRubics commented May 23, 2022

Hi,
we are trying to do something like this:

website_query = Website.with_trashed()

# apply filter
website_query = apply_filter(website_query, filter)

# total items
total_items = website_query.new_from_builder().count()

# pagination
if limit and offset is not None:
    website_query = website_query.limit(limit).offset(offset)

# query execution
websites = website_query.get()

where apply_filter() besides some wheres, also adds a subquery to the builder:

operator, value = query._extract_operator_value(fil["op"], fil["value"])
subquery = type(query._model)
builder = getattr(subquery, f'filter_{fil["field"]}')(fil["op"], fil["value"] )
query._wheres += (
        (QueryExpression(None, operator, SubGroupExpression(builder), keyword=kw)),
)

The error that we have is that after doing count() (even though we use new_from_builder) the query goes from this:

SELECT * FROM `apps` WHERE ( EXISTS (SELECT * FROM `users` WHERE `users`.`id` = `apps`.`user_id` AND `users`.`username` = '<username>' AND `users`.`id` > '99')) AND `apps`.`archived_at` IS NULL

to this:

SELECT * FROM `apps` WHERE ( EXISTS (SELECT * FROM `users` WHERE `users`.`id` > '99')) AND `apps`.`archived_at` IS NULL

We think the problem is that new_from_builder() is creating a new query builder, but adding the data from the old one with reference, so when count executes get() (and get resets the query builder in the end), it also resets the _wheres part of the old query builder. We think it should somehow do a deep copy, but whatever we tried, we didn't manage to solve it.

Desktop:

OS: Linux
Version Ubuntu 21.10

What database are you using?

Type: [MySQL]
Version [14.14 Distrib 5.7.30, for Linux (x86_64) using EditLine wrapper]
Masonite ORM [2.4.2]
@JRubics JRubics added the bug An existing feature is not working as intended label May 23, 2022
@josephmancuso
Copy link
Member

josephmancuso commented Jun 5, 2022

Can you simplify this issue? Not really understanding the problem and i feel you left out a lot of code here. You said you are using new_from_builder but i don't see that in your code snippets here

@josephmancuso
Copy link
Member

Maybe a step by step way i can replicate the issue will help

@JRubics
Copy link
Contributor Author

JRubics commented Jun 6, 2022

Sure, sorry for the mess.

Let's begin with explaining what we want to do with this code:

website_query = Website.with_trashed()

# apply filter
website_query = apply_filter(website_query, filter)

# total items
new_query = website_query.new_from_builder()
total_items = new_query.count()

# pagination
if limit and offset is not None:
    website_query = website_query.limit(limit).offset(offset)

# query execution
websites = website_query.get()
  1. create Website query
  2. apply some filters to it like where('id', 1), etc in apply_filter() method
  3. call new_from_builder() and execute count() on the new QueryBuilder (we are using new_from_builder because count resets the query and we want to keep filters from step no. 2)
  4. add limit and offset before executing query. The idea is to be able to get count for all the websites, but return just some of them depending on limit and offset. That's why we can't count results in the end
  5. execute query

Addition to step no.2 is that we do this in apply_filter method:

website_query._wheres += (
        (QueryExpression(None, operator, SubGroupExpression(builder), keyword=kw)),
)

We do it to add a sub-query to our query and this is important because website_query now has _wheres attribute.

What we think is causing the problem

Problem is that new_from_builder() when coping values from old builder to new one, copy _wheres by reference on the line 1972 (so it is the same object, not a copy):

builder._columns = from_builder._columns
builder._creates = from_builder._creates
builder._sql = from_builder._sql = ""
builder._bindings = from_builder._bindings
builder._updates = from_builder._updates
builder._wheres = from_builder._wheres
builder._order_by = from_builder._order_by
builder._group_by = from_builder._group_by
builder._joins = from_builder._joins
builder._having = from_builder._having
builder._macros = from_builder._macros
builder._aggregates = from_builder._aggregates
builder._global_scopes = from_builder._global_scopes

This is causing us issues because when count resets a query in step no.3, it also resets _wheres part because it was pointing to the original query _wheres values.

Example 1

example showing that QueryBuilder objects have different id(because new one is copy from the old one), but their _wheres have same ids (so it's not a real copy, just referencing other object values):
tr8

Example 2

Here you can also see how it affects our raw query.
q1
When I print website_query.to_sql() I have all the filters needed.
q2
Two statements later, when I print same thing (you can check they are same by object id), query lost part of the filters even though we didn't make any change on website_query.

Possible solution

I think that the solution would be to make new_from_builder() create a deep copy instead of just creating references for _wheres and other attributes.

I hope I now added more details, but if anything else stayed unclear, I am willing to explain :)

@josephmancuso
Copy link
Member

@JRubics i'm not sure i was able to replicate this issue (I'm still kind of unclear what i'm looking for 😞) but i took your proposed solution and added it in #716

Can you install that pull request code and test and see if that fixes the issue?

pip uninstall masonite-orm
pip install https://github.com/masoniteframework/orm/archive/fix/682

@josephmancuso
Copy link
Member

josephmancuso commented Jun 9, 2022

Just added deep copy to these tuples

https://github.com/MasoniteFramework/orm/pull/716/files

@JRubics
Copy link
Contributor Author

JRubics commented Jun 10, 2022

@josephmancuso sorry, I didn't have time to test it, but I think we already tried this and it didn't solved our issue. We'll test it next week and if it doesn't work, we will try to create small project with this issue and share the code base with you. Thank you for the effort 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An existing feature is not working as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants