From 03519c53bbcc9f6d1b3528cced44eb03628a0f26 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 1 Oct 2014 07:59:44 -0700 Subject: [PATCH] Mark questionable column nullability for later Summary: Ref T1191. Ref T6203. While generating expected schemata, I ran into these columns which seem to have sketchy nullability. - Mark most of them for later resolution (T6203). They work fine today and don't need to block T1191. Changing them can break the application, so we can't autofix them. - Forgive a couple of them that are sort-of reasonable or going to get wiped out. Test Plan: Saw 94 remaining warnings. Reviewers: btrahan Reviewed By: btrahan Subscribers: hach-que, epriestley Maniphest Tasks: T1191, T6203 Differential Revision: https://secure.phabricator.com/D10593 --- .../config/schema/PhabricatorConfigStorageSchema.php | 2 +- .../differential/storage/DifferentialChangeset.php | 8 ++++++++ .../differential/storage/DifferentialDiff.php | 8 ++++++-- .../differential/storage/DifferentialRevision.php | 2 +- .../harbormaster/storage/HarbormasterObject.php | 2 +- .../harbormaster/storage/HarbormasterSchemaSpec.php | 6 +++++- .../storage/build/HarbormasterBuildLog.php | 10 +++++++--- .../storage/build/HarbormasterBuildTarget.php | 5 ++++- .../storage/configuration/HarbormasterBuildStep.php | 7 ++++++- src/applications/herald/storage/HeraldRule.php | 5 ++++- src/applications/maniphest/storage/ManiphestTask.php | 5 +++++ .../metamta/storage/PhabricatorMetaMTAMail.php | 5 ++++- src/applications/paste/storage/PhabricatorPaste.php | 4 ++++ src/applications/phame/storage/PhameBlog.php | 7 ++++++- src/applications/phame/storage/PhamePost.php | 12 ++++++++++-- .../phriction/storage/PhrictionContent.php | 5 ++++- src/applications/ponder/storage/PonderAnswer.php | 5 ++++- src/applications/ponder/storage/PonderQuestion.php | 5 ++++- .../project/storage/PhabricatorProject.php | 7 ++++++- src/applications/releeph/storage/ReleephRequest.php | 6 ++++-- .../storage/PhabricatorRepositoryRefCursor.php | 5 ++++- .../settings/storage/PhabricatorUserSSHKey.php | 9 ++++++--- .../workers/storage/PhabricatorWorkerActiveTask.php | 7 +++++++ 23 files changed, 111 insertions(+), 26 deletions(-) diff --git a/src/applications/config/schema/PhabricatorConfigStorageSchema.php b/src/applications/config/schema/PhabricatorConfigStorageSchema.php index e1c8bda4cd..61475ae350 100644 --- a/src/applications/config/schema/PhabricatorConfigStorageSchema.php +++ b/src/applications/config/schema/PhabricatorConfigStorageSchema.php @@ -171,10 +171,10 @@ abstract class PhabricatorConfigStorageSchema extends Phobject { case self::ISSUE_MISSING: case self::ISSUE_SURPLUS: case self::ISSUE_SUBFAIL: + case self::ISSUE_NULLABLE: return self::STATUS_FAIL; case self::ISSUE_SUBWARN: case self::ISSUE_COLUMNTYPE: - case self::ISSUE_NULLABLE: return self::STATUS_WARN; case self::ISSUE_SUBNOTE: case self::ISSUE_CHARSET: diff --git a/src/applications/differential/storage/DifferentialChangeset.php b/src/applications/differential/storage/DifferentialChangeset.php index 5ae22d8a2c..8e5c3af6b2 100644 --- a/src/applications/differential/storage/DifferentialChangeset.php +++ b/src/applications/differential/storage/DifferentialChangeset.php @@ -36,6 +36,14 @@ final class DifferentialChangeset extends DifferentialDAO 'fileType' => 'uint32', 'addLines' => 'uint32', 'delLines' => 'uint32', + + // T6203/NULLABILITY + // These should all be non-nullable, and store reasonable default + // JSON values if empty. + 'awayPaths' => 'text?', + 'metadata' => 'text?', + 'oldProperties' => 'text?', + 'newProperties' => 'text?', ), self::CONFIG_KEY_SCHEMA => array( 'diffID' => array( diff --git a/src/applications/differential/storage/DifferentialDiff.php b/src/applications/differential/storage/DifferentialDiff.php index 7828510f80..7e74ad0929 100644 --- a/src/applications/differential/storage/DifferentialDiff.php +++ b/src/applications/differential/storage/DifferentialDiff.php @@ -57,9 +57,13 @@ final class DifferentialDiff 'branch' => 'text255?', 'bookmark' => 'text255?', 'arcanistProjectPHID' => 'phid?', - 'creationMethod' => 'text255', - 'description' => 'text255', 'repositoryUUID' => 'text64?', + + // T6203/NULLABILITY + // These should be non-null; all diffs should have a creation method + // and the description should just be empty. + 'creationMethod' => 'text255?', + 'description' => 'text255?', ), self::CONFIG_KEY_SCHEMA => array( 'revisionID' => array( diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index c8b3fa17af..f33ba47526 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -84,7 +84,7 @@ final class DifferentialRevision extends DifferentialDAO 'lastReviewerPHID' => 'phid?', 'lineCount' => 'uint32?', 'mailKey' => 'bytes40', - 'branchName' => 'text255', + 'branchName' => 'text255?', 'arcanistProjectPHID' => 'phid?', 'repositoryPHID' => 'phid?', ), diff --git a/src/applications/harbormaster/storage/HarbormasterObject.php b/src/applications/harbormaster/storage/HarbormasterObject.php index f83b41a4b9..648c04c3dc 100644 --- a/src/applications/harbormaster/storage/HarbormasterObject.php +++ b/src/applications/harbormaster/storage/HarbormasterObject.php @@ -8,7 +8,7 @@ final class HarbormasterObject extends HarbormasterDAO { return array( self::CONFIG_AUX_PHID => true, self::CONFIG_COLUMN_SCHEMA => array( - 'name' => 'text255', + 'name' => 'text255?', ), ) + parent::getConfiguration(); } diff --git a/src/applications/harbormaster/storage/HarbormasterSchemaSpec.php b/src/applications/harbormaster/storage/HarbormasterSchemaSpec.php index 11ca19c5c5..da3ab69d14 100644 --- a/src/applications/harbormaster/storage/HarbormasterSchemaSpec.php +++ b/src/applications/harbormaster/storage/HarbormasterSchemaSpec.php @@ -27,7 +27,11 @@ final class HarbormasterSchemaSpec extends PhabricatorConfigSchemaSpec { 'id' => 'id', 'logID' => 'id', 'encoding' => 'text32', - 'size' => 'uint32', + + // T6203/NULLABILITY + // Both the type and nullability of this column are crazily wrong. + 'size' => 'uint32?', + 'chunk' => 'bytes', ), array( diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php b/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php index 8ac71f6a25..6702a18fe7 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php @@ -31,9 +31,13 @@ final class HarbormasterBuildLog extends HarbormasterDAO return array( self::CONFIG_AUX_PHID => true, self::CONFIG_COLUMN_SCHEMA => array( - 'logSource' => 'text255', - 'logType' => 'text255', - 'duration' => 'uint32', + // T6203/NULLABILITY + // It seems like these should be non-nullable? All logs should have a + // source, etc. + 'logSource' => 'text255?', + 'logType' => 'text255?', + 'duration' => 'uint32?', + 'live' => 'bool', ), self::CONFIG_KEY_SCHEMA => array( diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuildTarget.php b/src/applications/harbormaster/storage/build/HarbormasterBuildTarget.php index c64db741ea..c043d137e2 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuildTarget.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuildTarget.php @@ -103,10 +103,13 @@ final class HarbormasterBuildTarget extends HarbormasterDAO self::CONFIG_COLUMN_SCHEMA => array( 'className' => 'text255', 'targetStatus' => 'text64', - 'name' => 'text255', 'dateStarted' => 'epoch?', 'dateCompleted' => 'epoch?', 'buildGeneration' => 'uint32', + + // T6203/NULLABILITY + // This should not be nullable. + 'name' => 'text255?', ), self::CONFIG_KEY_SCHEMA => array( 'key_build' => array( diff --git a/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php b/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php index 557e17ab34..82f61ad9e0 100644 --- a/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php +++ b/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php @@ -29,8 +29,13 @@ final class HarbormasterBuildStep extends HarbormasterDAO self::CONFIG_COLUMN_SCHEMA => array( 'className' => 'text255', 'sequence' => 'uint32', - 'name' => 'text255', 'description' => 'text', + + // T6203/NULLABILITY + // This should not be nullable. Current `null` values indicate steps + // which predated editable names. These should be backfilled with + // default names, then the code for handling `null` shoudl be removed. + 'name' => 'text255?', ), self::CONFIG_KEY_SCHEMA => array( 'key_plan' => array( diff --git a/src/applications/herald/storage/HeraldRule.php b/src/applications/herald/storage/HeraldRule.php index 9f921b705d..938edfb707 100644 --- a/src/applications/herald/storage/HeraldRule.php +++ b/src/applications/herald/storage/HeraldRule.php @@ -36,10 +36,13 @@ final class HeraldRule extends HeraldDAO 'contentType' => 'text255', 'mustMatchAll' => 'bool', 'configVersion' => 'uint32', - 'repetitionPolicy' => 'uint32', 'ruleType' => 'text255', 'isDisabled' => 'uint32', 'triggerObjectPHID' => 'phid?', + + // T6203/NULLABILITY + // This should not be nullable. + 'repetitionPolicy' => 'uint32?', ), self::CONFIG_KEY_SCHEMA => array( 'key_phid' => null, diff --git a/src/applications/maniphest/storage/ManiphestTask.php b/src/applications/maniphest/storage/ManiphestTask.php index 1174bd68ff..d85c2155f5 100644 --- a/src/applications/maniphest/storage/ManiphestTask.php +++ b/src/applications/maniphest/storage/ManiphestTask.php @@ -78,6 +78,11 @@ final class ManiphestTask extends ManiphestDAO 'ownerOrdering' => 'text64?', 'originalEmailSource' => 'text255?', 'subpriority' => 'double', + + // T6203/NULLABILITY + // This should not be nullable. It's going away soon anyway. + 'ccPHIDs' => 'text?', + ), self::CONFIG_KEY_SCHEMA => array( 'key_phid' => null, diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php index d7882231ec..e5cf6f46dd 100644 --- a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php @@ -34,8 +34,11 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { ), self::CONFIG_COLUMN_SCHEMA => array( 'status' => 'text255', - 'message' => 'text', 'relatedPHID' => 'phid?', + + // T6203/NULLABILITY + // This should just be empty if there's no body. + 'message' => 'text?', ), self::CONFIG_KEY_SCHEMA => array( 'status' => array( diff --git a/src/applications/paste/storage/PhabricatorPaste.php b/src/applications/paste/storage/PhabricatorPaste.php index 9191f316a2..02f37853af 100644 --- a/src/applications/paste/storage/PhabricatorPaste.php +++ b/src/applications/paste/storage/PhabricatorPaste.php @@ -48,6 +48,10 @@ final class PhabricatorPaste extends PhabricatorPasteDAO 'language' => 'text64', 'mailKey' => 'bytes20', 'parentPHID' => 'phid?', + + // T6203/NULLABILITY + // Pastes should always have a view policy. + 'viewPolicy' => 'policy?', ), self::CONFIG_KEY_SCHEMA => array( 'parentPHID' => array( diff --git a/src/applications/phame/storage/PhameBlog.php b/src/applications/phame/storage/PhameBlog.php index beccfdc329..d094359449 100644 --- a/src/applications/phame/storage/PhameBlog.php +++ b/src/applications/phame/storage/PhameBlog.php @@ -31,7 +31,12 @@ final class PhameBlog extends PhameDAO 'name' => 'text64', 'description' => 'text', 'domain' => 'text255?', - 'joinPolicy' => 'policy', + + // T6203/NULLABILITY + // These policies should always be non-null. + 'joinPolicy' => 'policy?', + 'editPolicy' => 'policy?', + 'viewPolicy' => 'policy?', ), self::CONFIG_KEY_SCHEMA => array( 'key_phid' => null, diff --git a/src/applications/phame/storage/PhamePost.php b/src/applications/phame/storage/PhamePost.php index ee98628e70..8c65ff26a3 100644 --- a/src/applications/phame/storage/PhamePost.php +++ b/src/applications/phame/storage/PhamePost.php @@ -91,9 +91,17 @@ final class PhamePost extends PhameDAO self::CONFIG_COLUMN_SCHEMA => array( 'title' => 'text255', 'phameTitle' => 'text64', - 'body' => 'text', 'visibility' => 'uint32', - 'datePublished' => 'epoch?', + + // T6203/NULLABILITY + // These seem like they should always be non-null? + 'blogPHID' => 'phid?', + 'body' => 'text?', + 'configData' => 'text?', + + // T6203/NULLABILITY + // This one probably should be nullable? + 'datePublished' => 'epoch', ), self::CONFIG_KEY_SCHEMA => array( 'key_phid' => null, diff --git a/src/applications/phriction/storage/PhrictionContent.php b/src/applications/phriction/storage/PhrictionContent.php index f7b58f07eb..07ffc3347a 100644 --- a/src/applications/phriction/storage/PhrictionContent.php +++ b/src/applications/phriction/storage/PhrictionContent.php @@ -35,9 +35,12 @@ final class PhrictionContent extends PhrictionDAO 'title' => 'text', 'slug' => 'text128', 'content' => 'text', - 'description' => 'text', 'changeType' => 'uint32', 'changeRef' => 'uint32?', + + // T6203/NULLABILITY + // This should just be empty if not provided? + 'description' => 'text?', ), self::CONFIG_KEY_SCHEMA => array( 'documentID' => array( diff --git a/src/applications/ponder/storage/PonderAnswer.php b/src/applications/ponder/storage/PonderAnswer.php index 4757dfbb28..a85aa6d3ad 100644 --- a/src/applications/ponder/storage/PonderAnswer.php +++ b/src/applications/ponder/storage/PonderAnswer.php @@ -70,7 +70,10 @@ final class PonderAnswer extends PonderDAO self::CONFIG_COLUMN_SCHEMA => array( 'voteCount' => 'sint32', 'content' => 'text', - 'contentSource' => 'text', + + // T6203/NULLABILITY + // This should always exist. + 'contentSource' => 'text?', ), self::CONFIG_KEY_SCHEMA => array( 'key_phid' => null, diff --git a/src/applications/ponder/storage/PonderQuestion.php b/src/applications/ponder/storage/PonderQuestion.php index 29f0da91ca..4db9118867 100644 --- a/src/applications/ponder/storage/PonderQuestion.php +++ b/src/applications/ponder/storage/PonderQuestion.php @@ -38,10 +38,13 @@ final class PonderQuestion extends PonderDAO 'voteCount' => 'sint32', 'status' => 'uint32', 'content' => 'text', - 'contentSource' => 'text', 'heat' => 'double', 'answerCount' => 'uint32', 'mailKey' => 'bytes20', + + // T6203/NULLABILITY + // This should always exist. + 'contentSource' => 'text?', ), self::CONFIG_KEY_SCHEMA => array( 'key_phid' => null, diff --git a/src/applications/project/storage/PhabricatorProject.php b/src/applications/project/storage/PhabricatorProject.php index afacd65fcf..3617ad12e0 100644 --- a/src/applications/project/storage/PhabricatorProject.php +++ b/src/applications/project/storage/PhabricatorProject.php @@ -125,11 +125,16 @@ final class PhabricatorProject extends PhabricatorProjectDAO 'name' => 'text255', 'status' => 'text32', 'phrictionSlug' => 'text128?', - 'joinPolicy' => 'policy', 'isMembershipLocked' => 'bool', 'profileImagePHID' => 'phid?', 'icon' => 'text32', 'color' => 'text32', + + // T6203/NULLABILITY + // These are definitely wrong and should always exist. + 'editPolicy' => 'policy?', + 'viewPolicy' => 'policy?', + 'joinPolicy' => 'policy?', ), self::CONFIG_KEY_SCHEMA => array( 'key_phid' => null, diff --git a/src/applications/releeph/storage/ReleephRequest.php b/src/applications/releeph/storage/ReleephRequest.php index 8159af0479..54976ae451 100644 --- a/src/applications/releeph/storage/ReleephRequest.php +++ b/src/applications/releeph/storage/ReleephRequest.php @@ -155,10 +155,12 @@ final class ReleephRequest extends ReleephDAO ), self::CONFIG_COLUMN_SCHEMA => array( 'requestCommitPHID' => 'phid?', - 'commitIdentifier' => 'text40', - 'pickStatus' => 'uint32', + 'commitIdentifier' => 'text40?', + 'commitPHID' => 'phid?', + 'pickStatus' => 'uint32?', 'inBranch' => 'bool', 'mailKey' => 'bytes20', + 'userIntents' => 'text?', ), self::CONFIG_KEY_SCHEMA => array( 'key_phid' => null, diff --git a/src/applications/repository/storage/PhabricatorRepositoryRefCursor.php b/src/applications/repository/storage/PhabricatorRepositoryRefCursor.php index 7a9acb9912..699108f7b8 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryRefCursor.php +++ b/src/applications/repository/storage/PhabricatorRepositoryRefCursor.php @@ -30,8 +30,11 @@ final class PhabricatorRepositoryRefCursor extends PhabricatorRepositoryDAO self::CONFIG_COLUMN_SCHEMA => array( 'refType' => 'text32', 'refNameHash' => 'bytes12', - 'refNameEncoding' => 'text16', 'commitIdentifier' => 'text40', + + // T6203/NULLABILITY + // This probably should not be nullable; refNameRaw is not nullable. + 'refNameEncoding' => 'text16?', ), self::CONFIG_KEY_SCHEMA => array( 'key_cursor' => array( diff --git a/src/applications/settings/storage/PhabricatorUserSSHKey.php b/src/applications/settings/storage/PhabricatorUserSSHKey.php index 2720ecb19f..250250a84f 100644 --- a/src/applications/settings/storage/PhabricatorUserSSHKey.php +++ b/src/applications/settings/storage/PhabricatorUserSSHKey.php @@ -12,11 +12,14 @@ final class PhabricatorUserSSHKey extends PhabricatorUserDAO { public function getConfiguration() { return array( self::CONFIG_COLUMN_SCHEMA => array( - 'name' => 'text255', - 'keyType' => 'text255', - 'keyBody' => 'text', 'keyHash' => 'bytes32', 'keyComment' => 'text255?', + + // T6203/NULLABILITY + // These seem like they should not be nullable. + 'name' => 'text255?', + 'keyType' => 'text255?', + 'keyBody' => 'text?', ), self::CONFIG_KEY_SCHEMA => array( 'userPHID' => array( diff --git a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php index 20e3c0d109..8870d9e203 100644 --- a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php +++ b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php @@ -36,6 +36,13 @@ final class PhabricatorWorkerActiveTask extends PhabricatorWorkerTask { ), ); + $config[self::CONFIG_COLUMN_SCHEMA] = array( + // T6203/NULLABILITY + // This isn't nullable in the archive table, so at a minimum these + // should be the same. + 'dataID' => 'uint32?', + ) + $parent[self::CONFIG_COLUMN_SCHEMA]; + return $config + $parent; }