developer.jelix.org is not used any more and exists only for history. Post new tickets on the Github account.
developer.jelix.org n'est plus utilisée, et existe uniquement pour son historique. Postez les nouveaux tickets sur le compte github.

Opened 12 years ago

Closed 12 years ago

#724 closed bug (fixed)

jDao: autoremove of empty groups in jDaoConditions

Reported by: foxmask Owned by: Julien
Priority: normal Milestone: jelix 1.1
Component: jelix:dao Version: 1.1 beta 1
Severity: normal Keywords:
Cc: Blocked By:
Blocking: Documentation needed: no
Hosting Provider: Php version:

Description

With the following code

class searchwikiListener extends jEventListener{
    
   function onSearchThat ($event) {
      
        $q = $event->getParam('q');
        
        $dao = jDao::get('wiki~wiki');
        
        $wiki = array();
        
        $conditions = jDao::createConditions();
        $conditions->startGroup('OR');
            for ($i=0;$i<count($q);$i++) {
                $conditions->addCondition('text','like','%'.$q[$i].'%');
                $conditions->addCondition('comment','like','%'.$q[$i].'%');
            }
        $conditions->endGroup();
        $conditions->addItemOrder('time','desc');
        $conditions->addItemGroup('name');
        
        echo "<pre>";
        print_r($conditions);
        echo "</pre>";
        $rs = $dao->findBy($conditions);
...
   }
}

the SQL query failed and display :

[exception 403] Erreur dans la requête (You have an error in your SQL syntax; check the manual that corresponds to your 
MySQL server version for the right syntax to use near 'GROUP BY name ORDER BY time desc' at line 1
(SELECT `wiki`.`name`, `wiki`.`version`, `wiki`.`time`, `wiki`.`author`, `wiki`.`ipnr`, `wiki`.`text`, `wiki`.`comment`, `wiki`.`readonly`
 FROM `wiki` AS `wiki` WHERE GROUP BY name ORDER BY time desc)) 
E:\wamp\www_bug\lib\jelix\plugins\db\mysql\mysql.dbconnection.php 72

We can cleary see there is nothing after the WHERE clause as expected the print_r display this :

jDaoConditions Object
(
    [condition] => jDaoCondition Object
        (
            [parent] => 
            [conditions] => Array
                (
                )

            [group] => Array
                (
                    [0] => jDaoCondition Object
                        (
                            [parent] => jDaoCondition Object
 *RECURSION*
                            [conditions] => Array
                                (
                                )

                            [group] => Array
                                (
                                )

                            [glueOp] => OR
                        )

                )

            [glueOp] => AND
        )

    [order] => Array
        (
            [time] => desc
        )

    [group] => Array
        (
            [0] => name
        )

    [_currentCondition:private] => jDaoCondition Object
        (
            [parent] => 
            [conditions] => Array
                (
                )

            [group] => Array
                (
                    [0] => jDaoCondition Object
                        (
                            [parent] => jDaoCondition Object
 *RECURSION*
                            [conditions] => Array
                                (
                                )

                            [group] => Array
                                (
                                )

                            [glueOp] => OR
                        )

                )

            [glueOp] => AND
        )

)

Attachments (2)

724-jDao-createConditions-emptiness-recursive-check.diff (3.7 KB) - added by Julien 12 years ago.
patch + unit tests
724-jDao-createConditions-emptiness-recursive-check-2.diff (3.3 KB) - added by Julien 12 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 12 years ago by Julien

  • Owner set to Julien
  • Status changed from new to assigned

I found the bug, will fix it.

You can bypass it like this (i think, didn't test):

if(count($q)){
    $conditions->startGroup('OR');
        for ($i=0;$i<count($q);$i++) {
            $conditions->addCondition('text','like','%'.$q[$i].'%');
            $conditions->addCondition('comment','like','%'.$q[$i].'%');
        }
    $conditions->endGroup();
}

comment:2 Changed 12 years ago by Julien

jDaoConditions::hasConditions() has to be improved to check empty groups, but I think it will bring performance a little bit down.

We may also consider that the developper should'nt create such groups.

I'll write a patch and we'll decide.

Changed 12 years ago by Julien

patch + unit tests

comment:3 Changed 12 years ago by Julien

  • review set to review?

Here's the patch + unit tests.

Recursive testing emptiness for groups works well, I don't think it will bring performance really down, as there is usually not a lot of neted groups, and recursive test stop as soon as it meets 1 criterion.

comment:4 follow-up: Changed 12 years ago by laurentj

We may also consider that the developper should'nt create such groups.

Yes, it doesn't make sens to create a group if you don't pu anything in it. Clearly, he should have test $q as you indicate

Recursive testing emptiness for groups works well, I don't think it will bring performance really down, as there is usually not a lot of neted groups, and recursive test stop as soon as it meets 1 criterion.

you're right. But perhaps we can remove this recursivity by doing something in endGroup(). In this method, we could test if the current group is empty or not. If it is, we remove it from its parent. And no need to check child groups of this group, since they are already deleted if they were empty (during the call of their own endGroup() method :-) ). What do you think about it ?

comment:5 in reply to: ↑ 4 Changed 12 years ago by Julien

Replying to laurentj:

we could test if the current group is empty or not. If it is, we remove it from its parent. What do you think about it ?

seems fine to me, good idea ;)

should I write such a patch, or you do ?

comment:6 Changed 12 years ago by laurentj

  • review review? deleted

write it :-)

comment:7 Changed 12 years ago by Julien

  • review set to review?

Here it is. I kept the same unit tests, and verified they pass well.

comment:8 Changed 12 years ago by laurentj

  • review changed from review? to review+

It's better, thank you :-)

comment:9 Changed 12 years ago by Julien

  • Resolution set to fixed
  • Status changed from assigned to closed
  • Summary changed from jDao::createConditions failed in a Listener to jDao: autoremove of empty groups in jDaoConditions

commit done in the trunk and 1.0.x branch

Note: See TracTickets for help on using tickets.