Information Required ? Pending

User tests: Successful: Unsuccessful:

avatar softarius
softarius
6 Jun 2019

Summary of Changes

Method _getListCount of class BaseDatabaseModel select query with keyword "distinct"

Testing Instructions

  • insert into any table repeated values
  • create a model extends ListModel
  • in method getListQuery write SQL query for getting "dictionary" like "select distinct field1 from table1"
  • write view and layout with pagination for getting a table this dictionary
    I wrote a component (only PostgreSQL supported) for demonstrating (attached).
    Use URL joomlasite/index.php?option=com_person&view=names

com_person.zip

Expected result

Correct pagination
espected

Actual result

Pagination with the wrong page counting, because distinct practically ignored
actual

Documentation Changes Required

Nothing

avatar softarius softarius - open - 6 Jun 2019
avatar softarius softarius - change - 6 Jun 2019
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 6 Jun 2019
Category Libraries
avatar softarius softarius - change - 6 Jun 2019
Labels Added: ?
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 6 Jun 2019

If this is a new Feature please rebaso on J4 as J3 is only Bug-Fixing.

avatar softarius softarius - change - 6 Jun 2019
Labels Added: ?
avatar alikon
alikon - comment - 6 Jun 2019

@franz-wohlkoenig
sounds like a bug to me

avatar HLeithner
HLeithner - comment - 20 Aug 2019

It's a bug but should test all core components list views.

avatar softarius softarius - change - 18 Sep 2019
Labels Removed: ?
avatar SharkyKZ
SharkyKZ - comment - 18 Sep 2019

Did anyone read the note?

* Note: Current implementation of this method assumes that getListQuery() returns a set of unique rows,
* thus it uses SELECT COUNT(*) to count the rows. In cases that getListQuery() uses DISTINCT
* then either this method must be overriden by a custom implementation at the derived Model Class
* or a GROUP BY clause should be used to make the set unique.

avatar softarius
softarius - comment - 19 Sep 2019

Did anyone read the note?

* Note: Current implementation of this method assumes that getListQuery() returns a set of unique rows,
* thus it uses SELECT COUNT(*) to count the rows. In cases that getListQuery() uses DISTINCT
* then either this method must be overriden by a custom implementation at the derived Model Class
* or a GROUP BY clause should be used to make the set unique.

I have read this note. The phrase about "must be overridden" similar to the description of the bug. Let's just fix it.
The main idea of this fix - not use count(*) if distinct exists. It's all.

avatar SharkyKZ
SharkyKZ - comment - 22 Sep 2019

There is no bug if you follow the documentation. You have to override the method in your model. And you should do that not just to get the correct result count but also to improve performance. That fallback method is extremely slow and should be avoided.

Furthermore, simple string manipulation like this is going to cause the slow query to run more often than it should, i.e. whenever anything contains the string distinct. This could be a table name, a column, a search keyword or anything else. So at least, if maintainers don't mind, we should add distinct capability to Database API to correctly detect where distinct is actually used or not.

avatar richard67
richard67 - comment - 27 Dec 2019

@SharkyKZ So should this PR be closed according to your previous comment?
@softarius If @SharkyKZ 's answer is yes, would you agree?

avatar SharkyKZ
SharkyKZ - comment - 30 Dec 2019

It's up to maintainers to decide.

avatar HLeithner
HLeithner - comment - 30 Dec 2019

I think you are right it brings more harm and the dev should do it right as in the description.

@softarius thanks anyway for your contribution.

avatar HLeithner HLeithner - close - 30 Dec 2019
avatar HLeithner HLeithner - change - 30 Dec 2019
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2019-12-30 11:50:47
Closed_By HLeithner
Labels Added: Information Required

Add a Comment

Login with GitHub to post a comment