Fix Item->search() to resolve issues with attributes

[objecttothis]

Background information

IMPORTANT: If you choose to ignore this issue report template, your issue will be closed as we cannot help without the requested information.

Please make sure you tick (add an x between the square brackets with no spaces) the following check boxes:

• Reporting an issue of an unmodified OSPOS installation
• Checked open and closed issues and no similar issue was already reported (please make sure you searched!)
• Read README, WHATS_NEW, INSTALL.md and UPGRADE
• Read the FAQ for any known install and/or upgrade gotchas (in specific PHP extensions installed)
• Read the wiki
• Executed any database upgrade scripts if an upgrade pre 3.0.0 (e.g. database/2.4_to_3.0.sql)
• Aware the installation code is in bintray (see README), and GitHub master is for developers only and therefore not complete nor stable

Installation information

OSPOS Installation Info: 3.3.2 - 9d2fd1

Language Code: en-US

Extensions & Modules:
» GD: Enabled ✓
» BC Math: Enabled ✓
» INTL: Enabled ✓
» OpenSSL: Enabled ✓
» MBString: Enabled ✓
» Curl: Enabled ✓

User Configuration:
.Browser: Firefox
.Server Software: Server
.PHP Version: 7.4.4
.DB Version: 5.7.29-log
.Server Port: 80
.OS: FreeBSD 11.3-RELEASE-p7

File Permissions:
» [application/logs:] - 0770 | Writable ✓
» [public/uploads:] - 0770 | Writable ✓
» [public/uploads/item_pics:] - 0770 | Writable ✓
» [import_customers.csv:] - 0640 | Not Writable ✗

Bug

In items, if you enable "Search Attributes" then type a search phrase, the pagination breaks at the bottom and becomes:

[objecttothis]

@jekkos I assigned this to 3.3.2, since its a bug related to a recent change, but feel free to reassign it to 3.4 if you want to just release 3.3.2 and deal with it later. Due to the quarantine, I desperately need to get our eCommerce site up and finish the integration between OSPOS and it, so I can't mess with this now. Feel free to tackle it if you want... I think you were the main developer on the attributes stuff, so it might be a straight forward resolution.

[jekkos]

Ok, fair enough. I'll have a look.

[objecttothis]

@jekkos do you have time to look at this so that we can get 3.3.2 out the door? Other options include punting until 3.4.0 or me taking a look.

[objecttothis]

I've narrowed it down to Models->Item->search(). When the $count_only variable is set to TRUE and you have search attributes enabled it produces the following SQL:

SELECT COUNT(DISTINCT items.item_id) AS count, GROUP_CONCAT(DISTINCT CONCAT_WS('_', `definition_id`, attribute_value) ORDER BY definition_id SEPARATOR '|') AS attribute_values, GROUP_CONCAT(DISTINCT CONCAT_WS('_', `definition_id`, DATE_FORMAT(attribute_date, '%d.%m.%Y')) SEPARATOR '|') AS attribute_dtvalues, GROUP_CONCAT(DISTINCT CONCAT_WS('_', `definition_id`, attribute_decimal) SEPARATOR '|') AS attribute_dvalues
FROM `ospos_items` AS `items`
LEFT JOIN `ospos_suppliers` AS `suppliers` ON `suppliers`.`person_id` = `items`.`supplier_id`
JOIN `ospos_inventory` AS `inventory` ON `inventory`.`trans_items` = `items`.`item_id`
JOIN `ospos_item_quantities` AS `item_quantities` ON `item_quantities`.`item_id` = `items`.`item_id`
LEFT JOIN `ospos_attribute_links` ON `ospos_attribute_links`.`item_id` = `items`.`item_id` AND `ospos_attribute_links`.`receiving_id` IS NULL AND `ospos_attribute_links`.`sale_id` IS NULL AND `definition_id` IN (1,3,4,7)
LEFT JOIN `ospos_attribute_values` ON `ospos_attribute_values`.`attribute_id` = `ospos_attribute_links`.`attribute_id`
WHERE `location_id` = '1'
AND DATE_FORMAT(trans_date, "%Y-%m-%d") BETWEEN '2010-01-01' AND '2020-06-27'
AND `items`.`deleted` = 0
AND `items`.`item_type` IN(0, 1, 2)
HAVING `attribute_values` LIKE '%bible%'
OR `attribute_dtvalues` LIKE '%bible%'
OR `attribute_dvalues` LIKE '%bible%'

where "bible" is my search criteria and you get an empty result set with a warning "Warning: #1260 Row 49 was cut by GROUP_CONCAT()". If I do the same search without search attributes it generates the SQL

SELECT COUNT(DISTINCT items.item_id) AS count, GROUP_CONCAT(DISTINCT CONCAT_WS('_', `definition_id`, attribute_value) ORDER BY definition_id SEPARATOR '|') AS attribute_values, GROUP_CONCAT(DISTINCT CONCAT_WS('_', `definition_id`, DATE_FORMAT(attribute_date, '%d.%m.%Y')) SEPARATOR '|') AS attribute_dtvalues, GROUP_CONCAT(DISTINCT CONCAT_WS('_', `definition_id`, attribute_decimal) SEPARATOR '|') AS attribute_dvalues
FROM `ospos_items` AS `items`
LEFT JOIN `ospos_suppliers` AS `suppliers` ON `suppliers`.`person_id` = `items`.`supplier_id`
JOIN `ospos_inventory` AS `inventory` ON `inventory`.`trans_items` = `items`.`item_id`
JOIN `ospos_item_quantities` AS `item_quantities` ON `item_quantities`.`item_id` = `items`.`item_id`
LEFT JOIN `ospos_attribute_links` ON `ospos_attribute_links`.`item_id` = `items`.`item_id` AND `ospos_attribute_links`.`receiving_id` IS NULL AND `ospos_attribute_links`.`sale_id` IS NULL AND `definition_id` IN (1,3,4,7)
LEFT JOIN `ospos_attribute_values` ON `ospos_attribute_values`.`attribute_id` = `ospos_attribute_links`.`attribute_id`
WHERE `location_id` = '1'
AND DATE_FORMAT(trans_date, "%Y-%m-%d") BETWEEN '2010-01-01' AND '2020-06-27'
AND   (
`name` LIKE '%bible%' ESCAPE '!'
OR  `item_number` LIKE '%bible%' ESCAPE '!'
OR  `items`.`item_id` LIKE '%bible%' ESCAPE '!'
OR  `company_name` LIKE '%bible%' ESCAPE '!'
OR  `items`.`category` LIKE '%bible%' ESCAPE '!'
 )
AND `items`.`deleted` = 0
AND `items`.`item_type` IN(0, 1, 2) 

which gives me a proper count result. So here's the thing. The results of the search attributes do actually include the additional rows where their attributes are within the search criteria, but when $count_only is set to TRUE, it's just getting the total number of rows and that fails.

[objecttothis]

The other thing to note is that the search in attributes should not be either search in items OR search in attributes. When search in attributes is enabled it should search in both items for the search phrase AND search in attributes.

[objecttothis]

Maybe the warning has something to do with it? https://stackoverflow.com/questions/7208773/mysql-row-30153-was-cut-by-group-concat-error

[objecttothis]

@jekkos, I just created a single text attribute on the demo and one item with that attribute. It does not exhibit the same behavior, but it's still off. It shows the result rows of the search, but instead of the error in the OP it says showing 1 to 7 of 7 rows. The problem is that there is only one result row. It should read 1 to 1 of 1 row. The error I'm experiencing on my dev box is likely because the result concat is too long for the field. (I have over 16000 item rows in the database). The error on the demo is related to the same function not returning proper results.

[objecttothis]

@jekkos Per the stack overflow article, if I change my.cnf to group_concat_max_len = 49152 (1024 is the default) then the null value issue caused by the error in group_concat goes away, but it still returns all the rows in the database rather than the actual number of rows in the result. I wonder if we might have to resort to breaking these queries up and doing some of the legwork in PHP rather than all the legwork in MySQL. I don't think everyone will have access to modify their my.cnf files.

[objecttothis]

It appears you can set group_concat_max_len at the session level in the PHP with $this->db->simple_query('SET SESSION group_concat_max_len=49152'); I don't know if this is the best way to resolve that issue, because it will come up again if someone needs a concat longer than that.

Edit: I ran the query in phpmyadmin and had to bump it up to 262144 before it stopped giving the warning.

[jekkos]

Ok great thanks for sorting this out already. We might be able to set this variable for now to solve this although it feels a little hacky.

I agree that it's not very clean the way I used group_concat to stitch together mysql resultsets. Maybe doing two queries and then merging those resultset in memory is a better option indeed, but this means it will need to be reworked and that will take some time to validate and test.

So I'd say let's go for this patch for now and create a technical debt ticket to resolve it later on. It might have other advantages as I remember that sorting does not work either for the attributes as the valeus are inside the concat and thus sort cannot be applied directly.

[objecttothis]

I've been reading Robert C. Martin's "Clean Code" and I'm regretting it 😉 because it's making me want to rewrite everything I come across. I think you're right that we will need to rework the function. Right now it's doing two things (returning row count of the results but no results or returning results), so one easy change is to refactor out the count. The other change would likely need to get rid of our group_concats and merge the results from two separate queries. I'll add the hack for the 3.3.2 release and then we can provide a proper fix in 3.4.0.

[objecttothis]

@jekkos the other reason to yank the group_concat would be that it would resolve the issues caused by #2538 (having to disable only_full_group_by). It seems that it's more trouble than it's worth. 😬

[objecttothis]

For future reference, the issues to be resolved for 3.4.0 are as follows:

• Item search with "Attribute search" enabled produces pagination problems
• Sort items tables by attributes not working.
• group_concat causing problems with only_full_group_by is enabled.
• Attribute search yields counts that are incorrect (both count of results, and total rows)

The proposed resolution now is to change the session variable group_concat_max_len and then for 3.4.0 have a technical debt that needs to be repaid by likely breaking apart search() and replacing group_concat with multiple queries and let PHP do the business logic.