Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

findAll() ignore 'group' & 'distinct' when used with calculated properties #89

Closed
chapmandu opened this Issue Sep 20, 2012 · 7 comments

Comments

4 participants
Owner

chapmandu commented Sep 20, 2012

http://code.google.com/p/cfwheels/issues/detail?id=672

For example:

<cfcomponent extends="Model" output="false">
        <cffunction name="init">
                <cfset property(name="first_letter", sql="SubStr(name, 1, 1)")>
        </cffunction>

        <cffunction name="first_letters">
                <cfreturn this.findAll(select='first_letter', group='first_letter', order='first_letter')>
        </cffunction>
</cfcomponent>

findAll() doesn't group. The same with:
findAll(select='first_letter', distinct=true, order='first_letter')

Reported by soe...@gmail.com, Dec 9, 2010

@ghost ghost assigned rip747 Dec 13, 2012

@perdjurner perdjurner added this to the 1.1.9 milestone Jul 14, 2014

@perdjurner perdjurner modified the milestones: 1.2.0, 1.1.9, 1.3.0, 1.4.0 Jul 15, 2014

@perdjurner perdjurner modified the milestones: 1.3.1, 1.4.0 Jul 24, 2014

@perdjurner perdjurner modified the milestones: 1.3.1, 1.3.2 Aug 13, 2014

Owner

perdjurner commented Aug 19, 2014

I think I have a fix for this, just need to write some tests.

@perdjurner perdjurner modified the milestones: 1.3.2, 1.3.1 Aug 19, 2014

@perdjurner perdjurner self-assigned this Aug 19, 2014

@perdjurner perdjurner added a commit that referenced this issue Aug 19, 2014

@perdjurner perdjurner Closes #89. b036610
Owner

perdjurner commented Aug 19, 2014

This should work now although it can be made better in the future...

To group with a calculated property you need to do add something like this to the model:

property(name="firstLetter", sql="SUBSTRING(users.username, 1, 1)")
property(name="groupCount", sql="COUNT(users.id)")

Then call like this:
model("user").findAll(select="firstLetter, groupCount", group="firstLetter")

One important thing to note is that the groupCount will cause an error when used in any finder call that does not group (so with this in place, make sure to always use the select argument to all finders rather than having Wheels try to return all of them).

@perdjurner perdjurner closed this Aug 19, 2014

@perdjurner perdjurner added a commit that referenced this issue Aug 19, 2014

@perdjurner perdjurner Closes #89. 99eecfb

@perdjurner perdjurner added a commit that referenced this issue Aug 19, 2014

@perdjurner perdjurner Reverts code for #89. 07e38e1
Contributor

scahyono commented Aug 19, 2014

Unfortunately, the commits breaks $addColumnsToSelectAndGroupBy in wheels/model/adapters/Base.cfc
The "count()" found in ORDER BY is got added into GROUP BY, somehow and breaks three db adapters out of five.

Owner

perdjurner commented Aug 19, 2014

Postponing for later, when we can do a better fix.

@perdjurner perdjurner reopened this Aug 19, 2014

@perdjurner perdjurner removed this from the 1.3.1 milestone Aug 19, 2014

@perdjurner perdjurner added a commit that referenced this issue Aug 20, 2014

@perdjurner perdjurner Reverts code for #89. 26f827b

@scahyono scahyono added a commit that referenced this issue Sep 29, 2014

@scahyono scahyono Adapt Oracle test #89 #366 #364 c85eaa4

@scahyono scahyono added a commit that referenced this issue Sep 29, 2014

@scahyono scahyono Repair Oracle test #89 #366 #364 517141b
Contributor

scahyono commented Oct 2, 2014

Hi Per, this "findAll() ignore 'group' & 'distinct'" issue was originally for 1.3.2 milestone.
Should we back port the fix there?

Owner

perdjurner commented Oct 2, 2014

Yes, I think so.

@perdjurner perdjurner added this to the 1.3.2 milestone Oct 2, 2014

@perdjurner perdjurner assigned scahyono and unassigned perdjurner Oct 2, 2014

Contributor

scahyono commented Oct 2, 2014

fixes back ported to 1.3.2; change log updated

@scahyono scahyono closed this Oct 2, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment