@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.)
hq.recaptime.dev/wiki/Phorge
phorge
phabricator
Remove PHID database, add Harbormaster database
Summary:
- We currently write every PHID we generate to a table. This was motivated by two concerns:
- **Understanding Data**: At Facebook, the data was sometimes kind of a mess. You could look at a random user in the ID tool and see 9000 assocs with random binary data attached to them, pointing at a zillion other objects with no idea how any of it got there. I originally created this table to have a canonical source of truth about PHID basics, at least. In practice, our data model has been really tidy and consistent, and we don't use any of the auxiliary data in this table (or even write it). The handle abstraction is powerful and covers essentially all of the useful data in the app, and we have human-readable types in the keys. So I don't think we have a real need here, and this table isn't serving it if we do.
- **Uniqueness**: With a unique key, we can be sure they're unique, even if we get astronomically unlucky and get a collision. But every table we use them in has a unique key anyway. So we actually get pretty much nothing here, except maybe some vague guarantee that we won't reallocate a key later if the original object is deleted. But it's hard to imagine any install will ever have a collision, given that the key space is 36^20 per object type.
- We also currently use PHIDs and Users in tests sometimes. This is silly and can break (see D2461).
- Drop the PHID database.
- Introduce a "Harbormaster" database (the eventual CI tool, after Drydock).
- Add a scratch table to the Harbormaster database for doing unit test meta-tests.
- Now, PHID generation does no writes, and unit tests are isolated from the application.
- @csilvers: This should slightly improve the performance of the large query-bound tail in D2457.
Test Plan: Ran unit tests. Ran storage upgrade.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: csilvers, aran, nh, edward
Differential Revision: https://secure.phabricator.com/D2466
+109
-77
+1
resources/sql/patches/phiddrop.sql
+1
resources/sql/patches/phiddrop.sql
···
1
+
DROP DATABASE IF EXISTS {$NAMESPACE}_phid;
+7
resources/sql/patches/testdatabase.sql
+7
resources/sql/patches/testdatabase.sql
···
1
+
CREATE TABLE {$NAMESPACE}_harbormaster.harbormaster_scratchtable (
2
+
`id` int unsigned NOT NULL AUTO_INCREMENT PRIMARY KEY,
3
+
`data` varchar(64) NOT NULL collate utf8_bin,
4
+
`dateCreated` int unsigned NOT NULL,
5
+
`dateModified` int unsigned NOT NULL,
6
+
KEY (data)
7
+
) ENGINE=InnoDB DEFAULT CHARSET=utf8;
+4
-3
src/__phutil_library_map__.php
+4
-3
src/__phutil_library_map__.php
···
423
423
'DrydockResourceStatus' => 'applications/drydock/constants/resourcestatus',
424
424
'DrydockSSHCommandInterface' => 'applications/drydock/interface/command/ssh',
425
425
'DrydockWebrootInterface' => 'applications/drydock/interface/webroot/base',
426
+
'HarbormasterDAO' => 'applications/harbormaster/storage/base',
427
+
'HarbormasterScratchTable' => 'applications/harbormaster/storage/scratchtable',
426
428
'HeraldAction' => 'applications/herald/storage/action',
427
429
'HeraldActionConfig' => 'applications/herald/config/action',
428
430
'HeraldApplyTranscript' => 'applications/herald/storage/transcript/apply',
···
783
785
'PhabricatorPHID' => 'applications/phid/storage/phid',
784
786
'PhabricatorPHIDConstants' => 'applications/phid/constants',
785
787
'PhabricatorPHIDController' => 'applications/phid/controller/base',
786
-
'PhabricatorPHIDDAO' => 'applications/phid/storage/base',
787
788
'PhabricatorPHIDLookupController' => 'applications/phid/controller/lookup',
788
789
'PhabricatorPaste' => 'applications/paste/storage/paste',
789
790
'PhabricatorPasteController' => 'applications/paste/controller/base',
···
1411
1412
'DrydockResourceStatus' => 'DrydockConstants',
1412
1413
'DrydockSSHCommandInterface' => 'DrydockCommandInterface',
1413
1414
'DrydockWebrootInterface' => 'DrydockInterface',
1415
+
'HarbormasterDAO' => 'PhabricatorLiskDAO',
1416
+
'HarbormasterScratchTable' => 'HarbormasterDAO',
1414
1417
'HeraldAction' => 'HeraldDAO',
1415
1418
'HeraldApplyTranscript' => 'HeraldDAO',
1416
1419
'HeraldCommitAdapter' => 'HeraldObjectAdapter',
···
1689
1692
'PhabricatorOwnersOwner' => 'PhabricatorOwnersDAO',
1690
1693
'PhabricatorOwnersPackage' => 'PhabricatorOwnersDAO',
1691
1694
'PhabricatorOwnersPath' => 'PhabricatorOwnersDAO',
1692
-
'PhabricatorPHID' => 'PhabricatorPHIDDAO',
1693
1695
'PhabricatorPHIDController' => 'PhabricatorController',
1694
-
'PhabricatorPHIDDAO' => 'PhabricatorLiskDAO',
1695
1696
'PhabricatorPHIDLookupController' => 'PhabricatorPHIDController',
1696
1697
'PhabricatorPaste' => 'PhabricatorPasteDAO',
1697
1698
'PhabricatorPasteController' => 'PhabricatorController',
+29
src/applications/harbormaster/storage/scratchtable/HarbormasterScratchTable.php
+29
src/applications/harbormaster/storage/scratchtable/HarbormasterScratchTable.php
···
1
+
<?php
2
+
3
+
/*
4
+
* Copyright 2012 Facebook, Inc.
5
+
*
6
+
* Licensed under the Apache License, Version 2.0 (the "License");
7
+
* you may not use this file except in compliance with the License.
8
+
* You may obtain a copy of the License at
9
+
*
10
+
* http://www.apache.org/licenses/LICENSE-2.0
11
+
*
12
+
* Unless required by applicable law or agreed to in writing, software
13
+
* distributed under the License is distributed on an "AS IS" BASIS,
14
+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15
+
* See the License for the specific language governing permissions and
16
+
* limitations under the License.
17
+
*/
18
+
19
+
/**
20
+
* This is just a test table that unit tests can use if they need to test
21
+
* generic database operations. It won't change and break tests and stuff, and
22
+
* mistakes in test construction or isolation won't impact the application in
23
+
* any way.
24
+
*/
25
+
final class HarbormasterScratchTable extends HarbormasterDAO {
26
+
27
+
protected $data;
28
+
29
+
}
+12
src/applications/harbormaster/storage/scratchtable/__init__.php
+12
src/applications/harbormaster/storage/scratchtable/__init__.php
+2
-2
src/applications/phid/storage/base/PhabricatorPHIDDAO.php
src/applications/harbormaster/storage/base/HarbormasterDAO.php
+2
-2
src/applications/phid/storage/base/PhabricatorPHIDDAO.php
src/applications/harbormaster/storage/base/HarbormasterDAO.php
···
16
16
* limitations under the License.
17
17
*/
18
18
19
-
abstract class PhabricatorPHIDDAO extends PhabricatorLiskDAO {
19
+
abstract class HarbormasterDAO extends PhabricatorLiskDAO {
20
20
21
21
public function getApplicationName() {
22
-
return 'phid';
22
+
return 'harbormaster';
23
23
}
24
24
25
25
}
+1
-1
src/applications/phid/storage/base/__init__.php
src/applications/harbormaster/storage/base/__init__.php
+1
-1
src/applications/phid/storage/base/__init__.php
src/applications/harbormaster/storage/base/__init__.php
+3
-15
src/applications/phid/storage/phid/PhabricatorPHID.php
+3
-15
src/applications/phid/storage/phid/PhabricatorPHID.php
···
16
16
* limitations under the License.
17
17
*/
18
18
19
-
final class PhabricatorPHID extends PhabricatorPHIDDAO {
19
+
final class PhabricatorPHID {
20
20
21
21
protected $phid;
22
22
protected $phidType;
23
23
protected $ownerPHID;
24
24
protected $parentPHID;
25
25
26
-
public static function generateNewPHID($type, array $config = array()) {
27
-
$owner = idx($config, 'owner');
28
-
$parent = idx($config, 'parent');
29
-
26
+
public static function generateNewPHID($type) {
30
27
if (!$type) {
31
28
throw new Exception("Can not generate PHID with no type.");
32
29
}
33
30
34
31
$uniq = Filesystem::readRandomCharacters(20);
35
-
$phid = 'PHID-'.$type.'-'.$uniq;
36
-
37
-
$phid_rec = new PhabricatorPHID();
38
-
$phid_rec->setPHIDType($type);
39
-
$phid_rec->setOwnerPHID($owner);
40
-
$phid_rec->setParentPHID($parent);
41
-
$phid_rec->setPHID($phid);
42
-
$phid_rec->save();
43
-
44
-
return $phid;
32
+
return 'PHID-'.$type.'-'.$uniq;
45
33
}
46
34
47
35
}
-3
src/applications/phid/storage/phid/__init__.php
-3
src/applications/phid/storage/phid/__init__.php
+12
-4
src/infrastructure/setup/sql/phabricator/PhabricatorBuiltinPatchList.php
+12
-4
src/infrastructure/setup/sql/phabricator/PhabricatorBuiltinPatchList.php
···
83
83
'type' => 'db',
84
84
'name' => 'flag',
85
85
),
86
+
'db.harbormaster' => array(
87
+
'type' => 'db',
88
+
'name' => 'harbormaster',
89
+
),
86
90
'db.herald' => array(
87
91
'type' => 'db',
88
92
'name' => 'herald',
···
114
118
'db.phame' => array(
115
119
'type' => 'db',
116
120
'name' => 'phame',
117
-
),
118
-
'db.phid' => array(
119
-
'type' => 'db',
120
-
'name' => 'phid',
121
121
),
122
122
'db.phriction' => array(
123
123
'type' => 'db',
···
870
870
'emailtableremove.sql' => array(
871
871
'type' => 'sql',
872
872
'name' => $this->getPatchPath('emailtableremove.sql'),
873
+
),
874
+
'phiddrop.sql' => array(
875
+
'type' => 'sql',
876
+
'name' => $this->getPatchPath('phiddrop.sql'),
877
+
),
878
+
'testdatabase.sql' => array(
879
+
'type' => 'sql',
880
+
'name' => $this->getPatchPath('testdatabase.sql'),
873
881
),
874
882
);
875
883
}
+18
-25
src/storage/connection/isolated/__tests__/AphrontIsolatedDatabaseConnectionTestCase.php
+18
-25
src/storage/connection/isolated/__tests__/AphrontIsolatedDatabaseConnectionTestCase.php
···
29
29
}
30
30
31
31
public function testIsolation() {
32
-
$conn = $this->newIsolatedConnection();
33
-
$test_phid = $this->generateTestPHID();
34
-
32
+
// This will fail if the connection isn't isolated.
35
33
queryfx(
36
-
$conn,
37
-
'INSERT INTO phabricator_phid.phid (phid) VALUES (%s)',
38
-
$test_phid);
39
-
40
-
$this->assertNoSuchPHID($test_phid);
34
+
$this->newIsolatedConnection(),
35
+
'INSERT INVALID SYNTAX');
41
36
}
42
37
43
38
public function testInsertGeneratesID() {
···
116
111
public function testTransactionRollback() {
117
112
$check = array();
118
113
119
-
$phid = new PhabricatorPHID();
114
+
$phid = new HarbormasterScratchTable();
120
115
$phid->openTransaction();
121
116
for ($ii = 0; $ii < 3; $ii++) {
122
-
$test_phid = $this->generateTestPHID();
117
+
$key = $this->generateTestData();
123
118
124
-
$obj = new PhabricatorPHID();
125
-
$obj->setPHID($test_phid);
126
-
$obj->setPHIDType('TEST');
127
-
$obj->setOwnerPHID('PHID-UNIT-!!!!');
119
+
$obj = new HarbormasterScratchTable();
120
+
$obj->setData($key);
128
121
$obj->save();
129
122
130
-
$check[] = $test_phid;
123
+
$check[] = $key;
131
124
}
132
125
$phid->killTransaction();
133
126
134
-
foreach ($check as $test_phid) {
135
-
$this->assertNoSuchPHID($test_phid);
127
+
foreach ($check as $key) {
128
+
$this->assertNoSuchRow($key);
136
129
}
137
130
}
138
131
···
141
134
return new AphrontIsolatedDatabaseConnection($config);
142
135
}
143
136
144
-
private function generateTestPHID() {
145
-
return 'PHID-TEST-'.Filesystem::readRandomCharacters(20);
137
+
private function generateTestData() {
138
+
return Filesystem::readRandomCharacters(20);
146
139
}
147
140
148
-
private function assertNoSuchPHID($phid) {
141
+
private function assertNoSuchRow($data) {
149
142
try {
150
-
$real_phid = id(new PhabricatorPHID())->loadOneWhere(
151
-
'phid = %s',
152
-
$phid);
143
+
$row = id(new HarbormasterScratchTable())->loadOneWhere(
144
+
'data = %s',
145
+
$data);
153
146
$this->assertEqual(
154
147
null,
155
-
$real_phid,
156
-
'Expect fake PHID to exist only in isolation.');
148
+
$row,
149
+
'Expect fake row to exist only in isolation.');
157
150
} catch (AphrontQueryConnectionException $ex) {
158
151
// If we can't connect to the database, conclude that the isolated
159
152
// connection actually is isolated. Philosophically, this perhaps allows
+1
-1
src/storage/connection/isolated/__tests__/__init__.php
+1
-1
src/storage/connection/isolated/__tests__/__init__.php
···
6
6
7
7
8
8
9
-
phutil_require_module('phabricator', 'applications/phid/storage/phid');
9
+
phutil_require_module('phabricator', 'applications/harbormaster/storage/scratchtable');
10
10
phutil_require_module('phabricator', 'infrastructure/testing/testcase');
11
11
phutil_require_module('phabricator', 'storage/connection/isolated');
12
12
phutil_require_module('phabricator', 'storage/queryfx');
+1
-1
src/storage/connection/mysql/__tests__/AphrontMySQLDatabaseConnectionTestCase.php
+1
-1
src/storage/connection/mysql/__tests__/AphrontMySQLDatabaseConnectionTestCase.php
+1
-1
src/storage/connection/mysql/__tests__/__init__.php
+1
-1
src/storage/connection/mysql/__tests__/__init__.php
···
6
6
7
7
8
8
9
-
phutil_require_module('phabricator', 'applications/phid/storage/phid');
9
+
phutil_require_module('phabricator', 'applications/harbormaster/storage/scratchtable');
10
10
phutil_require_module('phabricator', 'infrastructure/testing/testcase');
11
11
phutil_require_module('phabricator', 'storage/queryfx');
12
12
+16
-20
src/storage/lisk/dao/__tests__/LiskFixtureTestCase.php
+16
-20
src/storage/lisk/dao/__tests__/LiskFixtureTestCase.php
···
29
29
// If the user from either test persists, the other test will fail.
30
30
$this->assertEqual(
31
31
0,
32
-
count(id(new PhabricatorUser())->loadAll()));
32
+
count(id(new HarbormasterScratchTable())->loadAll()));
33
33
34
-
id(new PhabricatorUser())
35
-
->setUserName('alincoln')
36
-
->setRealName('Abraham Lincoln')
34
+
id(new HarbormasterScratchTable())
35
+
->setData('alincoln')
37
36
->save();
38
37
}
39
38
40
39
public function testTransactionalIsolation2of2() {
41
40
$this->assertEqual(
42
41
0,
43
-
count(id(new PhabricatorUser())->loadAll()));
42
+
count(id(new HarbormasterScratchTable())->loadAll()));
44
43
45
-
id(new PhabricatorUser())
46
-
->setUserName('ugrant')
47
-
->setRealName('Ulysses S. Grant')
44
+
id(new HarbormasterScratchTable())
45
+
->setData('ugrant')
48
46
->save();
49
47
}
50
48
51
49
public function testFixturesBasicallyWork() {
52
50
$this->assertEqual(
53
51
0,
54
-
count(id(new PhabricatorUser())->loadAll()));
52
+
count(id(new HarbormasterScratchTable())->loadAll()));
55
53
56
-
id(new PhabricatorUser())
57
-
->setUserName('gwashington')
58
-
->setRealName('George Washington')
54
+
id(new HarbormasterScratchTable())
55
+
->setData('gwashington')
59
56
->save();
60
57
61
58
$this->assertEqual(
62
59
1,
63
-
count(id(new PhabricatorUser())->loadAll()));
60
+
count(id(new HarbormasterScratchTable())->loadAll()));
64
61
}
65
62
66
63
public function testReadableTransactions() {
···
70
67
LiskDAO::endIsolateAllLiskEffectsToTransactions();
71
68
try {
72
69
73
-
$phid = 'PHID-TEST-'.Filesystem::readRandomCharacters(32);
70
+
$data = Filesystem::readRandomCharacters(32);
74
71
75
-
$obj = new PhabricatorPHID();
72
+
$obj = new HarbormasterScratchTable();
76
73
$obj->openTransaction();
77
74
78
-
$obj->setPHID($phid);
79
-
$obj->setPHIDType('TEST');
75
+
$obj->setData($data);
80
76
$obj->save();
81
77
82
-
$loaded = id(new PhabricatorPHID())->loadOneWhere(
83
-
'phid = %s',
84
-
$phid);
78
+
$loaded = id(new HarbormasterScratchTable())->loadOneWhere(
79
+
'data = %s',
80
+
$data);
85
81
86
82
$obj->killTransaction();
87
83
+1
-1
src/storage/lisk/dao/__tests__/__init__.php
+1
-1
src/storage/lisk/dao/__tests__/__init__.php
···
6
6
7
7
8
8
9
-
phutil_require_module('phabricator', 'applications/people/storage/user');
9
+
phutil_require_module('phabricator', 'applications/harbormaster/storage/scratchtable');
10
10
phutil_require_module('phabricator', 'applications/phid/storage/phid');
11
11
phutil_require_module('phabricator', 'infrastructure/testing/testcase');
12
12
phutil_require_module('phabricator', 'storage/lisk/dao');