[Database] modify the _implodeWithKeys() function to allow IS NULL in query #3551

PapillonMcGill · 2018-03-16T15:11:30Z

This pull request let IS NULL value in the WHERE clause. It convert null value in $param array to IS NULL in the SQL query.

required for an upcoming PR

Step to test.
create a test file to call implodeWithKeys directly with an associative array containing NULL as a value as no code currently in Loris do it and ensure that the query generated by the prepared statement contain and "IS NULL" for that value. For that, you will need to rename the _implodeWithKeys to implodeWithKeys and make it public.

driusan · 2018-03-16T19:38:57Z

php/libraries/Database.class.inc

-            if ($item===null || $item==='') {
+            if ($item===null ) {
+                $output[] = "`$key` IS NULL";
+            } elseif ( $item==='') {


This seems like it deserves unit tests to make sure it doesn't cause any regressions

ridz1208 · 2018-03-19T21:11:00Z

php/libraries/Database.class.inc

@@ -978,7 +978,9 @@ class Database
        }

        foreach ($dataArray as $key => $item ) {
-            if ($item===null || $item==='') {
+            if ($item===null) {


@driusan correct me if i'm wrong but isn't this function also used to build the $set array for UPDATE statements ?

in that case it would generate UPDATE table_name SET variable1=5, variable 2 IS NULL, variable3...

In which case this is highly non-backwards compatible and can lead to some errors

@ridz1208 @driusan @xlecours You are right about that. It should not be the case now as it was just not permitted before.
Does adding an optional parameter for NULL value, link = null with possible value 'null' default, 'IS' and 'IS NOT'.
This should be an array of the same size of $param as it is possible that some parameter need to be IS NULL while other migth be IS NOT NULL.

This way we will fix it for every possible situation.

@PapillonMcGill think @ridz1208 is talking about the SET part an UPDATE query, not the WHERE part. The SET part never needs "IS NULL/IS NOT NULL" while the WHERE part always does.. shouldn't a single extra bool parameter be sufficient?

@driusan Yes, I could add a parameter $clause = 'set' by default and allow 'where', but if in a where clause we later need IS NOT NULL instead of IS NULL, we will still be stuck. So with an array (NULL by default), we could add for each parameter where we need a comparison with null to be either IS NULL or IS NOT NULL as required and for those parameter where it is not a comparison with null or it is in a SET clause, leave that parameter as null and '=' will be used.

I think we should have an _implodeSetWithKeys() and an _implodeWhereWithKeys()

There's already 2 calls (one for set, and one for where), so you still don't need an array (but @ridz1208's proposal of 2 private functions with different names also makes more sense than an extra param to me..)... but now that I'm looking at the code, the update path is only using implodeWithKeys for printing the query. It uses _implodeAsPrepared for the actual query that it runs.

(Delete should also be using the prepared variant, I'm not sure why it isn't.)

@driusan using _implodeAsPrepared() will be hard for "IS NULL" or "IS NOT NULL" as PDO do not support that in prepared statements.
So I will have to stick with _implodeWithKeys(). But adding a variable $clause = 'set' in the function definition will not solve the future possibility of "IS NOT NULL". Unless we use a dirty trick like using PHP_EOL constant to convert to IS NOT NULL but that might induce other problem.

nicolasbrossard · 2018-04-16T21:14:57Z

php/libraries/Database.class.inc

     *
     * @return string the string containing the imploded array
     * @access private
     */
-    function _implodeWithKeys($glue, $dataArray)
+    function _implodeWithKeys($glue, $dataArray, $clause = 'set_')


Since $clause seems to be used as a boolean, I'd be in favour of naming it $isSetClause with value true for set clauses and false otherwise.

There is already a variable used as "set_" elsewhere in that module. So I used it the same way. It is also more clear to a humain than 0 or 1, no need to put comment. Of course binary would be (a little bit) faster at execution

nicolasbrossard · 2018-04-16T21:18:02Z

php/libraries/Database.class.inc

-            if ($exec_vals !== null) {
-                $exec_vals["$prefix$varname"] = $item;
+            $varname = str_replace("%", "_percent_", $key);
+            if (is_null($item) && $prefix == "where_") {


Nitpick: maybe rewrite this as

if (is_null($item)) { $output[] = $prefix == 'where_' ? "`$key` IS NULL" : "`$key` = NULL"; } else { ...

would effectively be more concise

stale

PapillonMcGill · 2018-06-26T18:31:03Z

@nicolasbrossard Since I can't implement the $clause variable as you requested without lots of modification to existing code, Could you re-review or dismiss your request to change $clause for a boolean variable?

PapillonMcGill · 2018-08-06T11:15:02Z

@nicolasbrossard Could you re-review? see previous comment.

in IS NULL in the SQL query minor lint fix addition of unit test modify to show it ONLY delete where field IS NULL modified _implodeAsPrepared to support 'IS NULL' and '= NULL' as _implodeWithKeys modified _implodeWithKeys to correctly handle '= NULL' in SET clause added 'set_' or 'where_' to existing call bug fix change if structure as requested lint fix restore deleted lines by some mistake corrected varname

…_allow_delete_where_IS_NULL

zaliqarosli · 2018-08-27T16:57:55Z

@PapillonMcGill could you update the PR description to include steps for testing?

PapillonMcGill · 2018-08-27T17:24:11Z

@zaliqarosli see the steps in the description.

Here is the test script I made. Unit test would fail I it require to temporarily make a private function public.

<?php

require_once __DIR__ . "/Database.class.inc";
require_once __DIR__ . "/testConfig.inc";

$array = array('VisitID' => '12', 'Label' => null, 'LegacyLabel' => 'mouain');

$db = new Database();
$db = Database::singleton($database, $username, $password, $host);

$setkey = $db->implodeWithKeys(',', $array, 'set_');
$wherekey = $db->implodeWithKeys(' AND ', $array, 'where_');

$empty = array();
$setPrep = $db->implodeAsPrepared(',', $array, $empty, 'set_');
$wherePrep = $db->implodeAsPrepared(' AND ', $array, $empty, 'where_');

    $prep   = $db->_PDO->prepare("UPDATE visit SET $setPrep WHERE $wherePrep");
    $result = $prep->execute($empty);
    $query = $prep->debugDumpParams();
    print($query."\n");

print("set:\n$setkey\n$setPrep\n\n");
print("where:\n$wherekey\n$wherePrep\n");
?>

Just need to put the DB credential in testConfig.inc file.

Stale

johnsaigle · 2018-11-28T18:07:35Z

Tested using the supplied script and IS NULL appeared in the final print statements.

samirdas · 2019-01-11T21:01:06Z

@xlecours Please review this one more carefully than usual.

xlecours

The unittest need to be changed but the code is working.

@PapillonMcGill could you add test for insert and replace as well?

test/unittests/Database_Test.php

driusan · 2019-01-14T19:27:34Z

@xlecours When you say the code is working, did you test it in all of:

the update/insert case for columns
the where case for select/update
the showDatabaseQueries case for printing?

added test to update data

PapillonMcGill · 2019-01-15T18:32:19Z

@xlecours I implemented your request for update but insert you just don't provide data for an attribute and the regular insert function just won't have to call this code.

PapillonMcGill · 2019-01-18T18:48:07Z

@xlecours Could you re-review?

xlecours · 2019-01-21T14:58:58Z

@PapillonMcGill

... but insert you just don't provide data for an attribute and the regular insert function just won't have to call this code.

I think you are right only when default value is null.

xlecours · 2019-01-21T16:03:18Z

@xlecours When you say the code is working, did you test it in all of:

the update/insert case for columns

the where case for select/update

the showDatabaseQueries case for printing?

@driusan Yes.

<?php
require_once __DIR__ . "/vendor/autoload.php";

class DatabaseTest
{
    private $database;

    public function __construct()
    {
        $this->database = \Database::singleton(
            'dbname',
            'username',
            'password',
            'hostname',
            true
        );
    }

    public function testInsert() {
        $insert_result = $this->database->insert(
            'Project',
            array(
                'Name' => 'Bob 1',
                'recruitmentTarget' => null
            )
        );

        var_dump($insert_result);

        $select_result = $this->database->pselect('select * from Project', array());
        var_dump($select_result);
    }

    public function testUpdate() 
    {
        $update_result = $this->database->update(
            'Project',
            array(
                'Name' => null,
                'recruitmentTarget' => 100
            ),
            array(
                'recruitmentTarget' => null,
                'Name' => 'Bob 1'
            )
        );
        $select_result = $this->database->pselect('select * from Project', array());
        var_dump($select_result);
    }

    public function testReplace()
    {
        $projectid = $this->database->pselectOne('SELECT MAX(ProjectID) FROM Project', array());
        $replace_result = $this->database->replace(
            'Project',
            array(
                'ProjectID' => $projectid,
                'Name' => null,
                'recruitmentTarget' => null
            ),
        );
        $select_result = $this->database->pselect('select * from Project', array());
        var_dump($select_result);
    }

    public function testDelete()
    {
        $delete_result = $this->database->delete(
            'Project',
            array(
                'Name' => null,
                'recruitmentTarget' => null,
            )
        );
        $select_result = $this->database->pselect('select * from Project', array());
        var_dump($select_result);
    }
}

$t = new DatabaseTest();
$t->testInsert();
$t->testUpdate();
$t->testReplace();
$t->testDelete();

PapillonMcGill · 2019-01-21T17:56:14Z

@xlecours rigth, I had not saw that realInsert function is using SET in stead of VALUES()

xlecours

Could you add a test for replace as well?
Even if the new tests are not specifically testing _implodeWithKeys and _implodeAsPrepared behaviors, I think that we should at least have tests for higher level functions. Thank you

test/unittests/Database_Test.php

… query (aces#3551) This modifies the Database class to allow null values to be used in WHERE clauses or as SET values.

driusan reviewed Mar 16, 2018

View reviewed changes

ZainVirani added the Passed manual tests PR has been successfully tested by at least one peer label Mar 19, 2018

ridz1208 previously requested changes Mar 19, 2018

View reviewed changes

ridz1208 removed the Passed manual tests PR has been successfully tested by at least one peer label Mar 20, 2018

PapillonMcGill added State: Needs work PR awaiting additional work by the author to proceed and removed State: Needs work PR awaiting additional work by the author to proceed labels Apr 9, 2018

PapillonMcGill mentioned this pull request Apr 11, 2018

introduce the visit and visit_projet_subproject_rel tables for #3579

Closed

ridz1208 added the [branch] minor label Apr 16, 2018

nicolasbrossard previously requested changes Apr 16, 2018

View reviewed changes

ridz1208 added the State: Needs work PR awaiting additional work by the author to proceed label Apr 25, 2018

PapillonMcGill removed the State: Needs work PR awaiting additional work by the author to proceed label Apr 30, 2018

PapillonMcGill added 2 commits August 6, 2018 07:35

Merge branch 'major' of https://github.com/aces/Loris into 2018-03-16…

8e59f02

…_allow_delete_where_IS_NULL

zaliqarosli added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Aug 13, 2018

PapillonMcGill force-pushed the 2018-03-16_allow_delete_where_IS_NULL branch from 95c9bec to 8e59f02 Compare August 14, 2018 11:41

PapillonMcGill removed the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Aug 14, 2018

PapillonMcGill mentioned this pull request Aug 21, 2018

[core Visit] Normalization of Visit #3887

Merged

johnsaigle added the Passed manual tests PR has been successfully tested by at least one peer label Nov 28, 2018

ridz1208 self-assigned this Dec 18, 2018

zaliqarosli previously approved these changes Jan 7, 2019

View reviewed changes

samirdas assigned xlecours Jan 11, 2019

xlecours requested changes Jan 14, 2019

View reviewed changes

test/unittests/Database_Test.php Outdated Show resolved Hide resolved

test/unittests/Database_Test.php Show resolved Hide resolved

xlecours added the State: Needs work PR awaiting additional work by the author to proceed label Jan 14, 2019

moved insertion of fake data to proper test

2cbdc5a

added test to update data

PapillonMcGill dismissed zaliqarosli’s stale review via 2cbdc5a January 15, 2019 18:25

clear temporary data between test.

cf5eb64

PapillonMcGill removed the State: Needs work PR awaiting additional work by the author to proceed label Jan 17, 2019

inserted test for insert with null value

ad53c43

PapillonMcGill added 4 commits January 21, 2019 14:24

typo

94f1baa

change clearing of temp data

617bd68

change FakeDB insert and clearing

3bad84f

typo

4362139

xlecours requested changes Jan 22, 2019

View reviewed changes

test/unittests/Database_Test.php Outdated Show resolved Hide resolved

added test for REPLACE with null value

090c575

xlecours approved these changes Jan 24, 2019

View reviewed changes

ridz1208 removed their assignment Jan 24, 2019

driusan merged commit 373a8fe into aces:minor Jan 28, 2019

ridz1208 added this to the 20.3.0 milestone Feb 2, 2019

kchatpar pushed a commit to kchatpar/Loris that referenced this pull request Apr 15, 2019

[Database] modify the _implodeWithKeys() function to allow IS NULL in…

9f52b84

… query (aces#3551) This modifies the Database class to allow null values to be used in WHERE clauses or as SET values.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Database] modify the _implodeWithKeys() function to allow IS NULL in query #3551

[Database] modify the _implodeWithKeys() function to allow IS NULL in query #3551

PapillonMcGill commented Mar 16, 2018 •

edited

Loading

driusan Mar 16, 2018

ridz1208 Mar 19, 2018 •

edited

Loading

PapillonMcGill Mar 20, 2018 •

edited

Loading

driusan Mar 20, 2018 •

edited

Loading

PapillonMcGill Mar 21, 2018

ridz1208 Mar 21, 2018

driusan Mar 21, 2018 •

edited

Loading

PapillonMcGill Mar 26, 2018

nicolasbrossard Apr 16, 2018

PapillonMcGill Apr 17, 2018

nicolasbrossard Apr 16, 2018

PapillonMcGill Apr 17, 2018

PapillonMcGill commented Jun 26, 2018

PapillonMcGill commented Aug 6, 2018

zaliqarosli commented Aug 27, 2018

PapillonMcGill commented Aug 27, 2018 •

edited by johnsaigle

Loading

johnsaigle commented Nov 28, 2018

samirdas commented Jan 11, 2019 •

edited by ridz1208

Loading

xlecours left a comment

driusan commented Jan 14, 2019

PapillonMcGill commented Jan 15, 2019

PapillonMcGill commented Jan 18, 2019

xlecours commented Jan 21, 2019

xlecours commented Jan 21, 2019

PapillonMcGill commented Jan 21, 2019

xlecours left a comment

[Database] modify the _implodeWithKeys() function to allow IS NULL in query #3551

[Database] modify the _implodeWithKeys() function to allow IS NULL in query #3551

Conversation

PapillonMcGill commented Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

ridz1208 Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

PapillonMcGill Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

driusan Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

driusan Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PapillonMcGill commented Jun 26, 2018

PapillonMcGill commented Aug 6, 2018

zaliqarosli commented Aug 27, 2018

PapillonMcGill commented Aug 27, 2018 • edited by johnsaigle Loading

johnsaigle commented Nov 28, 2018

samirdas commented Jan 11, 2019 • edited by ridz1208 Loading

xlecours left a comment

Choose a reason for hiding this comment

driusan commented Jan 14, 2019

PapillonMcGill commented Jan 15, 2019

PapillonMcGill commented Jan 18, 2019

xlecours commented Jan 21, 2019

xlecours commented Jan 21, 2019

PapillonMcGill commented Jan 21, 2019

xlecours left a comment

Choose a reason for hiding this comment

PapillonMcGill commented Mar 16, 2018 •

edited

Loading

ridz1208 Mar 19, 2018 •

edited

Loading

PapillonMcGill Mar 20, 2018 •

edited

Loading

driusan Mar 20, 2018 •

edited

Loading

driusan Mar 21, 2018 •

edited

Loading

PapillonMcGill commented Aug 27, 2018 •

edited by johnsaigle

Loading

samirdas commented Jan 11, 2019 •

edited by ridz1208

Loading