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

GroceryCrud search fails if ->where() or ->set_relation() are used - FIXED #341

Open
Doctor-Paul opened this issue Dec 5, 2015 · 30 comments

Comments

@Doctor-Paul
Copy link

Using GC 1.5.2 and CI 3.0

As others have noted, "search all" fails completely if a ->where() statement has been used.

This is because GC is chaining calls to ->or_like() in addition to ->where() and the resulting SQL is simply wrong.

Search also fails for the same reason if made against a single column which is the subject of a call to ->set_relation()

I have produced a modified version of the set_ajax_list_queries() function which fixes this problem.

If a "where" exists, it generates a query string of chained OR LIKE statements and then feeds it to the ->where() function, which CI automatically ANDs with the existing where statement.

Happy to share this code, work in progress shown below:

@Doctor-Paul
Copy link
Author

protected function set_ajax_list_queries($state_info = null)
{
    if(!empty($state_info->per_page))
    {
        if(empty($state_info->page) || !is_numeric($state_info->page) )
            $this->limit($state_info->per_page);
        else
        {
            $limit_page = ( ($state_info->page-1) * $state_info->per_page );
            $this->limit($state_info->per_page, $limit_page);
        }
    }

    if(!empty($state_info->order_by))
    {
        $this->order_by($state_info->order_by[0],$state_info->order_by[1]);
    }

    if(!empty($state_info->search))
    {
        if(!empty($this->relation))

            foreach($this->relation as $relation_name => $relation_values)
                $temp_relation[$this->_unique_field_name($relation_name)] = $this->_get_field_names_to_search($relation_values);

        if($state_info->search->field !== null)
        {
            if(isset($temp_relation[$state_info->search->field]))
            {
                if(is_array($temp_relation[$state_info->search->field])) {

                    if(!empty($this->where)) { // we already have a where(), so chaining or_like won't work
                        $like_where = '(';
                        $escaped_text = $this->basic_model->escape_str($state_info->search->text);

                        foreach($temp_relation[$state_info->search->field] as $search_field) {
                            $like_where .= '`'.$search_field."` LIKE '%".$escaped_text."%' OR ";
                        }

                        if ($like_where != '(') {
                            $like_where = substr($like_where, 0, -4).')'; // trim off the trailing  OR and add closing bracket
                            $this->where($like_where);
                        }

                    } else { // there isn't a where(), so just chain the or_likes

                        foreach($temp_relation[$state_info->search->field] as $search_field) {
                           $this->or_like($search_field, $state_info->search->text);
                        }

                    }

                } else {
                    $this->like($temp_relation[$state_info->search->field] , $state_info->search->text);
                }

            } elseif(isset($this->relation_n_n[$state_info->search->field])) {
                $escaped_text = $this->basic_model->escape_str($state_info->search->text);
                $this->having($state_info->search->field." LIKE '%".$escaped_text."%'");
            } else {
                $this->like($state_info->search->field , $state_info->search->text);
            }
        }
        else // search against all columns
        {
            $columns = $this->get_columns();
            $search_text = $state_info->search->text;

            if(!empty($this->where))
                foreach($this->where as $where)
                    $this->basic_model->having($where[0],$where[1],$where[2]);

            //Get the list of actual columns and then before adding it to search ..
            //compare it with the field ... does it exists? .. if yes.. great ..
            //go ahead and add it to search list.. if not.. just ignore it                
            $field_types = $this->get_field_types();
            $actual_columns = array();
            $srch = array();

            foreach($field_types as $field) {
                if( !isset($field->db_extra) || $field->db_extra != 'auto_increment' )
                    $actual_columns[] = $field->name;
            }

            foreach($columns as $column)
            {
                if(isset($temp_relation[$column->field_name]))
                {
                    if(is_array($temp_relation[$column->field_name]))
                    {
                        foreach($temp_relation[$column->field_name] as $search_field)
                        {
                            //$this->or_like($search_field, $search_text);
                            $srch[] = $search_field;
                        }

                    }
                    else
                    {
                        //$this->or_like($temp_relation[$column->field_name], $search_text);
                        $srch[] = $temp_relation[$column->field_name];
                    }
                }
                elseif(isset($this->relation_n_n[$column->field_name]))
                {
                    //@todo have a where for the relation_n_n statement
                }
                else
                {
                    if(array_search($column->field_name, $actual_columns) === false) { // amit mod - bail out if column not in db
                        continue;
                    }
                    //$this->or_like($column->field_name, $search_text);
                    $srch[] = $column->field_name;
                }
            }

            //die(var_dump($srch));
            if(!empty($this->where)) { // we already have a where(), so chaining or_like won't work
                $like_where = '(';
                $escaped_text = $this->basic_model->escape_str($search_text);

                foreach($srch as $search_field) {
                    $like_where .= '`'.$search_field."` LIKE '%".$escaped_text."%' OR ";
                }

                if ($like_where != '(') {
                    $like_where = substr($like_where, 0, -4).')'; // trim off the trailing  OR and add closing bracket
                    $this->where($like_where);
                }

            } else { // there isn't a where(), so just chain the or_likes

                foreach($srch as $search_field) {
                    $this->or_like($search_field, $search_text);
                }

            }

        }
    }
}

@buoncri
Copy link
Contributor

buoncri commented Dec 9, 2015

👍

@multimulti
Copy link

👍 Perfect, please include this in the next version of GC!

@moritzgloeckl
Copy link

Hi, this problem still persists when you select a table which has a self referencing field. For example I have a table named Company, with a field name headquarter which is a foreign key to Company. Error is: Column in where clause is ambiguous.

@multimulti
Copy link

Only for clarification: The code above fixes two things for me which are broken in stock GC 1.5.3:

  • Filtering now works as advertised when using $crud->where
  • Filtering now works as advertised when having a relation to a table with same column name (that is, having a join)

@moritzgloeckl
Copy link

@multimulti I'm also using GC 1.5.3 and ajax_list_info throws this error for me, with the updated function:

Column 'name' in where clause is ambiguous

SELECT * FROM company LEFT JOIN company as j56bb8bb3 ON j56bb8bb3.company_id = company.headquarter WHERE company.deleted != '1' AND name LIKE '%test%' ESCAPE '!'

Maybe I did something wrong? I just replaced the original function with the updated one from above...

@Doctor-Paul
Copy link
Author

GC does throw errors if you use set_relation() and two tables contain the same field/column name.

Can you add the table name to your query so that the SQL comes out as "... AND company.name LIKE....."?

Otherwise, I work round it by making sure I use unique field names - so change the 'name' field in the 'company' table to 'company_name' or similar.

Something at the back of my mind is telling me that this "ambiguous clause" problem has been fixed in another thread, either here on Github or on the GC forum. If I'm right and I can find it I will post it here.

A further clarification: My function also includes code from Amit which stops "dummy columns" causing search errors.

@moritzgloeckl
Copy link

What do you mean with add the table name? The error is in ajax_list_info, and I'm using your function. I can't use unique field names, because the table is referencing itself... The company has a headquarter, which is also a company!

@Doctor-Paul
Copy link
Author

My code doesn't address the self-referencing problem, which I think is a known issue with GC. I suggest that you check out the GC forum - for example this post addresses the problem and proposes changes to two GC functions to fix it (you need to scroll down a bit to find the proposed code):

http://www.grocerycrud.com/forums/topic/921-ajax-list-info-ambiguous-field-in-search/

Here is the key code:

1- Modify the get_columns method.
2- Modify the showList method.

1.1- Add these lines before the first foreach:

$ci =& get_instance();
$base_table_fields = $ci->db->list_fields($this->basic_db_table);

1.2- Add the next code inner the second foreach:

else
{
$fields = $base_table_fields;
foreach ($fields as $field_name)
{
if ($field_name == $column)
{
$new_column = $this->basic_db_table.'.'.$column;
$column = $new_column;
$this->columns[$col_num] = $new_column;
$this->display_as[$column] = ucfirst(str_replace('_',' ', $field_name));
}
}
}
2.1 - Add this foreach after the $data->columns = $this->get_columns();

foreach ($data->columns as $column)
{
$new_column_name = substr(strstr($column->field_name, "."), 1);

if ( ! empty($new_column_name))
{
$column->field_name = $new_column_name;
}
}

This work for me, Try with a test environment or separate example.

@moritzgloeckl
Copy link

I already tried this... It doesn't work unfortunately. Since you wrote the new ajax_list_info, maybe you know where the LEFT JOIN in the select comes from? It isn't even needed in the query for the search, so maybe the easiest solution is to just remove it. Where does this get added or how do I remove it? Thank you

@Doctor-Paul
Copy link
Author

I believe this is how GC implements the set_relation() calls - this is to make search work with the "target" table of the set_relation() call. e.g. "company" is stored as an id in another table, but you can get search results for, say, the company name when searching that first table thanks to the JOIN.

GC is built on top of CodeIgniter and makes calls to CI's Query Builder Class, which is where the actual SQL will be generated.

Good luck with drilling down into that!

PS Have you actually tried using unique column names for all the tables in the database? (Or at least for the .name field in different tables) It may just fix the ambiguity problem.

@moritzgloeckl
Copy link

Hmm okay, well my other approach would be to give the company where I select from a name. Any guess where this comes from?

@Doctor-Paul
Copy link
Author

Don't quite follow I'm afraid, but the answer is likely to be "deep inside CodeIgniter" :-(

@scoumbourdis
Copy link
Owner

Hello @Doctor-Paul do you mind to send me a pull request of your code change in order to review the changes and maybe merge it with the grocery CRUD master code? If you don't know how to do it please let me know so I can help you with that :)

I think this bug mentioned is fixed at the version 1.5.3 but not sure though!

Thanks
Johnny

@Doctor-Paul
Copy link
Author

Haven't done this before but I can see the pull request button - I'll give it a go and shout if I need help.

@Doctor-Paul
Copy link
Author

OK, I'm shouting :-)

Pull request asks me to compare two versions of the code - can you talk me through the process?

@moritzgloeckl
Copy link

@Doctor-Paul I fixed the problem myself, I modified your code and added a $this->get_table() in front of the $search_field variable. Maybe you want to update your code? It works perfectly now for me. (I actually changed it just in the if after the //die(var_dump($srch)); So the new statement in the foreach now looks like this:

$like_where .= '`'.$this->get_table().'`.`'.$search_field."` LIKE '%".$escaped_text."%' OR ";

@razorlegacy
Copy link

This message was created automatically by mail delivery software.

A message that you sent could not be delivered to one or more of its
recipients. This is a temporary error. The following address(es) deferred:

[email protected]
Domain michaelmohammed.com has exceeded the max emails per hour (5/5 (100%)) allowed. Message will be reattempted later

------- This is a copy of the message, including all the headers. ------
Received: from github-smtp2-ext3.iad.github.net ([192.30.252.194]:54650 helo=github-smtp2b-ext-cp1-prd.iad.github.net)
by vps.michaelmohammed.com with esmtps (TLSv1.2:DHE-RSA-AES256-GCM-SHA384:256)
(Exim 4.86)
(envelope-from [email protected])
id 1aG8SU-000298-QX
for [email protected]; Mon, 04 Jan 2016 11:58:19 -0500
Date: Mon, 04 Jan 2016 08:57:34 -0800
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=github.com;
s=pf2014; t=1451926654;
bh=l8Y1GUheAAYlljwcinXeAdg1/lwKxQaa9hNIwq8jLiA=;
h=From:Reply-To:To:In-Reply-To:References:Subject:List-ID:
List-Archive:List-Post:List-Unsubscribe:From;
b=Q5+AuJUeqkH36PZxFd7EKt7Tb3+inE0SP6LnA5/DGWBtUnAez+/5TKxWGmi13IOpG
1kMVJc0VfGxOCb6p/awjoRwRDYSAk7XW8Ah8SOeNecIG1ZkQ0kxbkhWz6/Zb2xmsYV
DrTyRpvOvk1fEB2nWpU4b1f/aI0cdzBtss7myv2I=
From: Moe [email protected]
Reply-To: scoumbourdis/grocery-crud [email protected]
To: scoumbourdis/grocery-crud [email protected]
Message-ID: scoumbourdis/grocery-crud/issues/341/[email protected]
In-Reply-To: scoumbourdis/grocery-crud/issues/[email protected]
References: scoumbourdis/grocery-crud/issues/[email protected]
Subject: Re: [grocery-crud] GroceryCrud search fails if ->where() or
->set_relation() are used - FIXED (#341)
Mime-Version: 1.0
Content-Type: multipart/alternative;
boundary="--==_mimepart_568aa47e72738_a3a3f95c48432a0270948";
charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: list
X-GitHub-Sender: m1s73r
X-GitHub-Recipient: razorlegacy
List-ID: scoumbourdis/grocery-crud <grocery-crud.scoumbourdis.github.com>
List-Archive: https://github.com/scoumbourdis/grocery-crud
List-Post: mailto:[email protected]
List-Unsubscribe: mailto:unsub+0000f6993bfa3713d41d7827570ba70a893db7b5f6adcb3592cf0000000112a2667e92a169ce07300f7e@reply.github.com,
https://github.com/notifications/unsubscribe/AAD2mShkpnf7ZsbV2Ux8mPgIlSABpUqDks5pWpv-gaJpZM4GvhZd
X-Auto-Response-Suppress: All
X-GitHub-Recipient-Address: [email protected]
X-Spam-Status: No, score=-4.1
X-Spam-Score: -40
X-Spam-Bar: ----
X-Ham-Report: Spam detection software, running on the system "vps.michaelmohammed.com",
has NOT identified this incoming email as spam. The original
message has been attached to this so you can view it or label
similar future email. If you have any questions, see
root@localhost for details.

Content preview: @Doctor-Paul I fixed the problem myself, I modified your code
and added a $this->get_table() in front of the $search_field variable.
Maybe you want to update your code? It works perfectly now for me. (I actually
changed it just in the if after the //die(var_dump($srch)); So the new
statement in the foreach now looks like this: [...]

Content analysis details: (-4.1 points, 5.0 required)

pts rule name description


-5.0 RCVD_IN_DNSWL_HI RBL: Sender listed at http://www.dnswl.org/, high
trust
[192.30.252.194 listed in list.dnswl.org]
-0.0 RCVD_IN_MSPIKE_H4 RBL: Very Good reputation (+4)
[192.30.252.194 listed in wl.mailspike.net]
0.0 HTML_MESSAGE BODY: HTML included in message
1.0 HTML_IMAGE_ONLY_16 BODY: HTML: images with 1200-1600 bytes of words
-0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's
domain
0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid
-0.1 DKIM_VALID Message has at least one valid DKIM or DK signature
-0.0 RCVD_IN_MSPIKE_WL Mailspike good senders
X-Spam-Flag: NO

----==_mimepart_568aa47e72738_a3a3f95c48432a0270948
Content-Type: text/plain;
charset=UTF-8
Content-Transfer-Encoding: 7bit

@Doctor-Paul I fixed the problem myself, I modified your code and added a $this->get_table() in front of the $search_field variable. Maybe you want to update your code? It works perfectly now for me. (I actually changed it just in the if after the //die(var_dump($srch)); So the new statement in the foreach now looks like this:

$like_where .= '`'.$this->get_table().'`.`'.$search_field."` LIKE '%".$escaped_text."%' OR ";

Reply to this email directly or view it on GitHub:
#341 (comment)
----==_mimepart_568aa47e72738_a3a3f95c48432a0270948
Content-Type: text/html;
charset=UTF-8
Content-Transfer-Encoding: 7bit

@Doctor-Paul I fixed the problem myself, I modified your code and added a $this->get_table() in front of the $search_field variable. Maybe you want to update your code? It works perfectly now for me. (I actually changed it just in the if after the //die(var_dump($srch)); So the new statement in the foreach now looks like this:

$like_where .= '`'.$this->get_table().'`.`'.$search_field."` LIKE '%".$escaped_text."%' OR ";


Reply to this email directly or view it on GitHub.

----==_mimepart_568aa47e72738_a3a3f95c48432a0270948--

@Doctor-Paul
Copy link
Author

Excellent!

I will certainly add that to my code, many thanks.

EDIT: This may solve your problem, but I think it will break the search if set_relation() is used to link to another table.

@moritzgloeckl
Copy link

Do you have an example where it might break? It seems to work for me!

@Doctor-Paul
Copy link
Author

It's just a suspicion at the moment, will try to look into it further although it will be a few days before I can do that.

@multimulti
Copy link

@Doctor-Paul I have found one bug in the code in your second post here: When no explicit column ("Search all") is selected in the drop-down right to the search text, the search always returns no results. The expected behavior is to then search in ALL columns and return the results found.

But, again, thank you a lot for your code, apart from that it seems to work perfectly.

@darkbe
Copy link

darkbe commented Mar 10, 2016

Is there workaround for this "Search all" problem?

@darkbe
Copy link

darkbe commented Mar 17, 2016

I am using the version 1.5.4 of GC and ci 3.0.4. with original "employees_management" example.
I set $crud->where('employees.officeCode', '1');

I use "Search all" option to "mu" string, i get mysql error:
Unknown column 'jd29d42cf.city' in 'where clause' - Invalid query

Sql is:
SELECT *
FROM employees
LEFT JOIN offices as jd29d42cf ON jd29d42cf.officeCode = employees.officeCode
WHERE employees.officeCode = '1'
AND (lastName LIKE '%mu%' OR firstName LIKE '%mu%' OR extension LIKE '%mu%' OR email LIKE '%mu%' OR jd29d42cf.city LIKE '%mu%' OR file_url LIKE '%mu%' OR jobTitle LIKE '%mu%')
HAVING employees.officeCode = '1'

@Blair2004
Copy link

Hi, i may have a clue for this. Actually, when you have many table connected together (relation), proceeding a search, i'll use temp relation table and all col name of the main table defined with "set_table". So to have this fixed, it's required for all thoses col name to be prefixed with the main table name like this "exemple.ID" if ID is the ambiguous field.

First thing to have it fixed, create a new method on Grocery_Crud_Model.php

public function get_table_name() { return $this->table_name; }

Then, edit the Grocery_Crud.php library file and reach the method "set_ajax_list_queries". Within this method, to the last "else" statement, change this :

                    if(array_search($column->field_name, $actual_columns) === false) {
                        continue;
                    }
                    $this->or_like( $column->field_name, $search_text);

Into this :

$table_name = $this->basic_model->get_table_name();
if(array_search($column->field_name, $actual_columns) === false) {
continue;
}
$this->or_like( $table_name . '.' . $column->field_name, $search_text);

Hope it will help.

@mckaygerhard
Copy link

the title are market as "FIXED" well this break the preview of the images! this patch are incomplete and buggy

@mnadeemijaz
Copy link

grocery crude not working with search all at the time of using where
like
$crud->set_table('members');
$crud->set_relation('session_id','club_sessions','title',array('club_id' => $this->session->userdata('club_id')));
$crud->where('members.club_id',$this->session->userdata('club_id'));
if i select any field name then it is working well but if i use search filter then it is not working
i also change the above code of GC but.....
please help me to solve this problem

@sephinah
Copy link

Hi pips! im having the same problem with i would like to know how to fix for the search box / and search filter for columns
im having set->relation_n_n table...

where should i start

please reply.

@mckaygerhard
Copy link

the patch provided in this issue works, but break the image previews!

@scoumbourdis
Copy link
Owner

scoumbourdis commented Jul 19, 2017 via email

mckaygerhard added a commit to codeigniterpower/codeigniter-gastos that referenced this issue Dec 18, 2017
mckaygerhard added a commit to codeigniterpower/codeigniter-gastos that referenced this issue Dec 18, 2017
… busqueda ajax

* scoumbourdis/grocery-crud#341
* el codigo original esta comentado, para si en futuro hay problemas alternar
mckaygerhard added a commit to codeigniterpower/codeigniter-gastos that referenced this issue Dec 18, 2017
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

No branches or pull requests