Skip to content

Commit 4299819

Browse files
DerkyDerky
authored
Merge pull request #17 from Derky/feature/efficient-garbage-collect
Improve garbage collect query (with db testing)
2 parents 8075d00 + 96f2e3d commit 4299819

File tree

3 files changed

+149
-27
lines changed

3 files changed

+149
-27
lines changed

‎captcha/sortables.php‎

Lines changed: 15 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -240,36 +240,24 @@ public function get_hidden_fields()
240240
}
241241

242242
/**
243-
* API function, just the same from captcha_qa but with other table names
243+
* API function, if sessions are pruned also remove related sortables_confirm rows
244244
*/
245245
public function garbage_collect($type = 0)
246246
{
247-
$sql = 'SELECT c.confirm_id
248-
FROM ' . $this->table_sortables_confirm . ' c
249-
LEFT JOIN ' . SESSIONS_TABLE . ' s
250-
ON (c.session_id = s.session_id)
251-
WHERE s.session_id IS NULL' .
252-
((empty($type)) ? '' : ' AND c.confirm_type = ' . (int) $type);
253-
$result = $this->db->sql_query($sql);
254-
255-
if ($row = $this->db->sql_fetchrow($result))
256-
{
257-
$sql_in = array();
258-
259-
do
260-
{
261-
$sql_in[] = (string) $row['confirm_id'];
262-
}
263-
while ($row = $this->db->sql_fetchrow($result));
264-
265-
if (count($sql_in))
266-
{
267-
$sql = 'DELETE FROM ' . $this->table_sortables_confirm . '
268-
WHERE ' . $this->db->sql_in_set('confirm_id', $sql_in);
269-
$this->db->sql_query($sql);
270-
}
271-
}
272-
$this->db->sql_freeresult($result);
247+
// Using subquery for SQLite support (instead of using DELETE with LEFT JOIN directly) this however causes the following
248+
// problem in MySQL "You can't specify target table for update in FROM clause", workaround by adding a derived table on the subquery result
249+
$sql = 'DELETE FROM ' . $this->table_sortables_confirm . '
250+
WHERE confirm_id IN (
251+
SELECT derived.confirm_id
252+
FROM (
253+
SELECT c.confirm_id
254+
FROM ' . $this->table_sortables_confirm . ' c
255+
LEFT JOIN ' . SESSIONS_TABLE . ' s
256+
ON (c.session_id = s.session_id)
257+
WHERE s.session_id IS NULL' .
258+
((empty($type)) ? '' : ' AND c.confirm_type = ' . (int) $type) .
259+
') derived)';
260+
$this->db->sql_query($sql);
273261
}
274262

275263
/**

‎tests/feature/fixtures/garbage_collection.xml‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
<?xml version="1.0" encoding="UTF-8" ?>
2+
<dataset>
3+
<table name="phpbb_sortables_confirm">
4+
<column>session_id</column>
5+
<column>confirm_id</column>
6+
<row>
7+
<value>session1</value>
8+
<value>confirm1</value>
9+
</row>
10+
<row>
11+
<value>session2</value>
12+
<value>confirm2</value>
13+
</row>
14+
<row>
15+
<value>session3</value>
16+
<value>confirm3</value>
17+
</row>
18+
</table>
19+
<table name="phpbb_sessions">
20+
<column>session_id</column>
21+
<row>
22+
<value>session1</value>
23+
</row>
24+
<row>
25+
<value>session4</value>
26+
</row>
27+
</table>
28+
</dataset>

‎tests/feature/garbage_collection_test.php‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
<?php
2+
/**
3+
*
4+
* @copyright (c) Derky <http://www.derky.nl>
5+
* @license GNU General Public License, version 2 (GPL-2.0)
6+
*
7+
*/
8+
9+
namespace derky\sortablescaptcha\captcha\tests\feature;
10+
11+
class garbage_collection_test extends \phpbb_database_test_case
12+
{
13+
/** @var \phpbb\db\tools */
14+
protected $db_tools;
15+
16+
/** @var string */
17+
protected $table_prefix;
18+
19+
/** @var \phpbb\db\driver\driver_interface */
20+
private $db;
21+
22+
static protected function setup_extensions()
23+
{
24+
return array('derky/sortablescaptcha');
25+
}
26+
27+
public function getDataSet()
28+
{
29+
return $this->createXMLDataSet(dirname(__FILE__) . '/fixtures/garbage_collection.xml');
30+
}
31+
32+
public function setUp()
33+
{
34+
parent::setUp();
35+
36+
global $table_prefix;
37+
38+
$this->table_prefix = $table_prefix;
39+
$this->db = $this->new_dbal();
40+
$this->db_tools = new \phpbb\db\tools($this->db);
41+
}
42+
43+
/**
44+
* Construct sortables class with mocked services except database driver
45+
*
46+
* @param array $input_post_array Variables like submitted with $_POST
47+
* @return \derky\sortablescaptcha\captcha\sortables
48+
*/
49+
public function get_sortables($input_post_array = array())
50+
{
51+
global $phpbb_root_path, $phpEx;
52+
53+
$request = new \phpbb_mock_request(array(), $input_post_array);
54+
55+
$this->cache = $this->getMockBuilder('\phpbb\cache\driver\driver_interface')
56+
->disableOriginalConstructor()
57+
->getMock();
58+
59+
$this->config = new \phpbb\config\config(array());
60+
61+
$this->language = new \phpbb\language\language(new \phpbb\language\language_file_loader($phpbb_root_path, $phpEx));
62+
63+
$this->user = $this->getMockBuilder('\phpbb\user')
64+
->setConstructorArgs(array(
65+
$this->language,
66+
'\phpbb\datetime'
67+
))
68+
->getMock();
69+
70+
$this->template = $this->getMockBuilder('\phpbb\template\template')
71+
->disableOriginalConstructor()
72+
->getMock();
73+
74+
return new \derky\sortablescaptcha\captcha\sortables(
75+
$this->db,
76+
$this->cache,
77+
new \phpbb\config\config(array()),
78+
new \phpbb\log\dummy(),
79+
$request,
80+
$this->template,
81+
$this->user,
82+
'phpbb_sortables_questions',
83+
'phpbb_sortables_answers',
84+
'phpbb_sortables_confirm');
85+
}
86+
87+
public function test_sortables_confirm_table_exists()
88+
{
89+
$this->assertTrue($this->db_tools->sql_table_exists($this->table_prefix . 'sortables_confirm'), 'Asserting that table "' . $this->table_prefix . 'sortables_confirm" exist');
90+
}
91+
92+
public function test_sortables_garbage_collection()
93+
{
94+
// Garbage collection should delete 2 from the 3 rows because related sessions no longer exists
95+
$expected_confirm_rows = 1;
96+
$sortables = $this->get_sortables();
97+
$sortables->garbage_collect();
98+
99+
$sql = 'SELECT COUNT(confirm_id) AS count
100+
FROM ' . $this->table_prefix . 'sortables_confirm';
101+
$this->db->sql_query($sql);
102+
$count = (int) $this->db->sql_fetchfield('count');
103+
104+
$this->assertEquals($expected_confirm_rows, $count);
105+
}
106+
}

0 commit comments

Comments
 (0)