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

Bug: shared BaseBuilder instance and Model #4461

Open
iRedds opened this issue Mar 21, 2021 · 11 comments
Open

Bug: shared BaseBuilder instance and Model #4461

iRedds opened this issue Mar 21, 2021 · 11 comments
Labels
database Issues or pull requests that affect the database layer

Comments

@iRedds
Copy link
Collaborator

iRedds commented Mar 21, 2021

Describe the bug
Wrong approach to implementing a generic BaseBuilder instance in the model and post processing queries to the database.
When using a subquery, the work of the model / builder breaks.

$t = new Test(); //table test

$t->from('(SELECT * FROM test_1) t', true)->paginate(1);
d($t->db->getLastQuery()); // "SELECT * FROM (SELECT * FROM test_1) t LIMIT 1"

$t->findAll();
d($t->db->getLastQuery()); // "SELECT * FROM (SELECT * FROM test_1) t"  expected SELECT * FROM test

$t->from('(SELECT id, name FROM test_1) t', true)->paginate(1);
d($t->db->getLastQuery()); // "SELECT * FROM (SELECT id, name FROM test_1) t LIMIT 1"

$t->findAll(); // SQL error -  SELECT * FROM (SELECT id

The fact is that after the query is executed, the table is reassigned.

// Reset QBFrom part
if (! empty($this->QBFrom))
{
$this->from(array_shift($this->QBFrom), true);
}

For the first case, the value will be: (SELECT * FROM test_1) t
For the second case: (SELECT id, because the from() method can accept as a table name a string consisting of several tables separated by commas: from('table_1, table_2, table_3)

Affected module(s)
Model, BaseBuilder

You can fix this with crutches, but I think it's time to change the concept of the model and query builder.

@iRedds iRedds added the bug Verified issues on the current code behavior or pull requests that will fix them label Mar 21, 2021
@kenjis
Copy link
Member

kenjis commented Mar 21, 2021

It seems the table reassignment is come from #2800

Related: #4457

@lonnieezell
Copy link
Member

Are you trying to override the table given in the model? I'm assuming that's what you're doing. And that goes against how the Models are designed to work. The problem is I don't see what it is you're trying to have happen - you just point out how it doesn't do what you're expecting.

You can always grab a new QueryBuilder with either $this->db->table('test_1')->paginate() or $this->builder('test_1)->paginate()`. Both of these assume you're within the model.

If you're outside of the model trying to use that query, then you can get it using something like db_connect()->table('test_1')->paginate().

@iRedds
Copy link
Collaborator Author

iRedds commented Mar 22, 2021

BaseBuilder has no paginate() method. It is only defined in BaseModel()

kenjis added a commit to kenjis/CodeIgniter4 that referenced this issue Mar 22, 2021
@kenjis
Copy link
Member

kenjis commented Mar 22, 2021

@iRedds How about PR #4463 ?

@iRedds
Copy link
Collaborator Author

iRedds commented Mar 22, 2021

@kenjis Good idea, but for some reason I am plagued by doubts. It seems that some nuance is slipping out of sight.

@kenjis kenjis added the database Issues or pull requests that affect the database layer label Sep 23, 2021
@kenjis
Copy link
Member

kenjis commented Nov 24, 2021

@iRedds
Specifying sub query in from() is not documented in the User Guide in CI3/CI4.

$builder->from('(SELECT id, name FROM test_1) t', true);

$from (mixed) – Table name(s); string or array
https://codeigniter4.github.io/CodeIgniter4/database/query_builder.html#from

'(SELECT id, name FROM test_1) t' is not table names.

So it seems this is simply misuse and it is not a bug.
Is it a common use case around you?

@iRedds
Copy link
Collaborator Author

iRedds commented Nov 24, 2021

@kenjis
I am not using CI4.
Yes, this is not a bug.
But I believe this cuts off the functionality.
In the case I described, in order not to break the work of the model, you will have to write a raw SQL query.

@kenjis kenjis removed the bug Verified issues on the current code behavior or pull requests that will fix them label Nov 24, 2021
@kenjis
Copy link
Member

kenjis commented Jun 18, 2022

@iRedds Is this issue still there?

@iRedds
Copy link
Collaborator Author

iRedds commented Jun 18, 2022

Yes. Nothing changed.
Is it relevant for me? No.
As I wrote earlier, I don't use CI4.

I won't mind if this issue is closed.

@kenjis
Copy link
Member

kenjis commented Jun 18, 2022

Thanks. I forgot the details of the issue, but I thought recent development of Query Builder might change this.

As I sent a PR, I thought there was a problem.
So you don't need to close this.
I would like to fix someday.

@iRedds
Copy link
Collaborator Author

iRedds commented Jun 18, 2022

A solution may emerge from a discussion where all parties have an understanding of the situation or a desire to understand it. Because a possible solution may be at the level of the concept of working with a model and a database.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database Issues or pull requests that affect the database layer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants