Skip to content

Commit 6c37f36

Browse files
jasdeepkhalsajasdeepkhalsa
committed
fix(bug-2,bug-9): handle all DiffOp types and null values in UpdateDataSQL
getDown() previously called getOldValue() unconditionally, which crashes with a fatal error when the diff contains DiffOpAdd objects (which lack that method). Also, getDown() did not null-guard getOldValue(), causing NULL column values to be written as empty strings instead of SQL NULL. Both getUp() and getDown() now: - Check for DiffOpAdd/DiffOpRemove to avoid calling missing methods - Null-guard return values to emit col = NULL instead of col = '' Added comprehensive unit tests covering DiffOpChange, DiffOpAdd, DiffOpRemove, null values, and mixed DiffOp types in a single UPDATE. Related contributor PR: #93
1 parent 5a3b81e commit 6c37f36

2 files changed

Lines changed: 164 additions & 5 deletions

File tree

‎src/SQLGen/DiffToSQL/UpdateDataSQL.php‎

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33
use DBDiff\SQLGen\SQLGenInterface;
44
use DBDiff\SQLGen\Dialect\DialectRegistry;
55
use DBDiff\SQLGen\Dialect\SQLDialectInterface;
6+
use Diff\DiffOp\DiffOpAdd;
7+
use Diff\DiffOp\DiffOpRemove;
68

79

810
class UpdateDataSQL implements SQLGenInterface {
@@ -20,10 +22,14 @@ public function getUp(): string {
2022
$d = $this->dialect;
2123
$values = $this->obj->diff['diff'];
2224
array_walk($values, function (&$diff, $column) use ($d) {
23-
$q = $d->quote($column);
24-
$diff = is_null($diff->getNewValue())
25-
? "$q = NULL"
26-
: "$q = '" . addslashes($diff->getNewValue()) . "'";
25+
$q = $d->quote($column);
26+
if ($diff instanceof DiffOpRemove) {
27+
$diff = "$q = NULL";
28+
} elseif (!method_exists($diff, 'getNewValue') || is_null($diff->getNewValue())) {
29+
$diff = "$q = NULL";
30+
} else {
31+
$diff = "$q = '" . addslashes($diff->getNewValue()) . "'";
32+
}
2733
});
2834
$keys = $this->obj->diff['keys'];
2935
array_walk($keys, function (&$value, $column) use ($d) {
@@ -37,7 +43,14 @@ public function getDown(): string {
3743
$d = $this->dialect;
3844
$values = $this->obj->diff['diff'];
3945
array_walk($values, function (&$diff, $column) use ($d) {
40-
$diff = $d->quote($column) . " = '" . addslashes($diff->getOldValue()) . "'";
46+
$q = $d->quote($column);
47+
if ($diff instanceof DiffOpAdd) {
48+
$diff = "$q = NULL";
49+
} elseif (!method_exists($diff, 'getOldValue') || is_null($diff->getOldValue())) {
50+
$diff = "$q = NULL";
51+
} else {
52+
$diff = "$q = '" . addslashes($diff->getOldValue()) . "'";
53+
}
4154
});
4255
$keys = $this->obj->diff['keys'];
4356
array_walk($keys, function (&$value, $column) use ($d) {

‎tests/Unit/UpdateDataSQLTest.php‎

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
<?php
2+
3+
namespace DBDiff\Tests\Unit;
4+
5+
use PHPUnit\Framework\TestCase;
6+
use DBDiff\SQLGen\DiffToSQL\UpdateDataSQL;
7+
use DBDiff\SQLGen\Dialect\MySQLDialect;
8+
use Diff\DiffOp\DiffOpAdd;
9+
use Diff\DiffOp\DiffOpRemove;
10+
use Diff\DiffOp\DiffOpChange;
11+
12+
class UpdateDataSQLTest extends TestCase
13+
{
14+
private function makeObj(string $table, array $diffMap, array $keys): object
15+
{
16+
return (object) [
17+
'table' => $table,
18+
'diff' => ['diff' => $diffMap, 'keys' => $keys],
19+
];
20+
}
21+
22+
// ── getUp ─────────────────────────────────────────────────────────────
23+
24+
public function testGetUpWithDiffOpChange(): void
25+
{
26+
$obj = $this->makeObj('users', [
27+
'name' => new DiffOpChange('old_name', 'new_name'),
28+
], ['id' => '1']);
29+
30+
$sql = new UpdateDataSQL($obj, new MySQLDialect());
31+
$result = $sql->getUp();
32+
33+
$this->assertSame("UPDATE `users` SET `name` = 'new_name' WHERE `id` = '1';", $result);
34+
}
35+
36+
public function testGetUpWithDiffOpChangeNullNewValue(): void
37+
{
38+
$obj = $this->makeObj('users', [
39+
'email' => new DiffOpChange('old@test.com', null),
40+
], ['id' => '2']);
41+
42+
$sql = new UpdateDataSQL($obj, new MySQLDialect());
43+
$result = $sql->getUp();
44+
45+
$this->assertSame("UPDATE `users` SET `email` = NULL WHERE `id` = '2';", $result);
46+
}
47+
48+
public function testGetUpWithDiffOpRemoveDoesNotCrash(): void
49+
{
50+
$obj = $this->makeObj('users', [
51+
'old_col' => new DiffOpRemove('removed_value'),
52+
], ['id' => '3']);
53+
54+
$sql = new UpdateDataSQL($obj, new MySQLDialect());
55+
$result = $sql->getUp();
56+
57+
// DiffOpRemove has no getNewValue(), so should produce NULL
58+
$this->assertSame("UPDATE `users` SET `old_col` = NULL WHERE `id` = '3';", $result);
59+
}
60+
61+
public function testGetUpWithDiffOpAddDoesNotCrash(): void
62+
{
63+
$obj = $this->makeObj('users', [
64+
'new_col' => new DiffOpAdd('added_value'),
65+
], ['id' => '4']);
66+
67+
$sql = new UpdateDataSQL($obj, new MySQLDialect());
68+
$result = $sql->getUp();
69+
70+
$this->assertSame("UPDATE `users` SET `new_col` = 'added_value' WHERE `id` = '4';", $result);
71+
}
72+
73+
// ── getDown ───────────────────────────────────────────────────────────
74+
75+
public function testGetDownWithDiffOpChange(): void
76+
{
77+
$obj = $this->makeObj('users', [
78+
'name' => new DiffOpChange('old_name', 'new_name'),
79+
], ['id' => '1']);
80+
81+
$sql = new UpdateDataSQL($obj, new MySQLDialect());
82+
$result = $sql->getDown();
83+
84+
$this->assertSame("UPDATE `users` SET `name` = 'old_name' WHERE `id` = '1';", $result);
85+
}
86+
87+
public function testGetDownWithDiffOpChangeNullOldValue(): void
88+
{
89+
$obj = $this->makeObj('users', [
90+
'email' => new DiffOpChange(null, 'new@test.com'),
91+
], ['id' => '2']);
92+
93+
$sql = new UpdateDataSQL($obj, new MySQLDialect());
94+
$result = $sql->getDown();
95+
96+
$this->assertSame("UPDATE `users` SET `email` = NULL WHERE `id` = '2';", $result);
97+
}
98+
99+
public function testGetDownWithDiffOpAddDoesNotCrash(): void
100+
{
101+
// DiffOpAdd has no getOldValue() — this was the crash in Bug #2
102+
$obj = $this->makeObj('users', [
103+
'new_col' => new DiffOpAdd('added_value'),
104+
], ['id' => '3']);
105+
106+
$sql = new UpdateDataSQL($obj, new MySQLDialect());
107+
$result = $sql->getDown();
108+
109+
$this->assertSame("UPDATE `users` SET `new_col` = NULL WHERE `id` = '3';", $result);
110+
}
111+
112+
public function testGetDownWithDiffOpRemoveDoesNotCrash(): void
113+
{
114+
$obj = $this->makeObj('users', [
115+
'old_col' => new DiffOpRemove('removed_value'),
116+
], ['id' => '4']);
117+
118+
$sql = new UpdateDataSQL($obj, new MySQLDialect());
119+
$result = $sql->getDown();
120+
121+
$this->assertSame("UPDATE `users` SET `old_col` = 'removed_value' WHERE `id` = '4';", $result);
122+
}
123+
124+
// ── Mixed DiffOp types in a single UPDATE ─────────────────────────────
125+
126+
public function testMixedDiffOpTypes(): void
127+
{
128+
$obj = $this->makeObj('users', [
129+
'name' => new DiffOpChange('old', 'new'),
130+
'bio' => new DiffOpAdd('new_bio'),
131+
'phone' => new DiffOpRemove('old_phone'),
132+
], ['id' => '5']);
133+
134+
$sql = new UpdateDataSQL($obj, new MySQLDialect());
135+
136+
$up = $sql->getUp();
137+
$this->assertStringContainsString("`name` = 'new'", $up);
138+
$this->assertStringContainsString("`bio` = 'new_bio'", $up);
139+
$this->assertStringContainsString('`phone` = NULL', $up);
140+
141+
$down = $sql->getDown();
142+
$this->assertStringContainsString("`name` = 'old'", $down);
143+
$this->assertStringContainsString('`bio` = NULL', $down);
144+
$this->assertStringContainsString("`phone` = 'old_phone'", $down);
145+
}
146+
}

0 commit comments

Comments
 (0)