Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NEW: Add method to DB to check if a class is ready for db queries. #10041

Closed
wants to merge 1 commit into from
Closed

NEW: Add method to DB to check if a class is ready for db queries. #10041

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 105 additions & 1 deletion src/ORM/DB.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
use InvalidArgumentException;
use SilverStripe\Control\Director;
use SilverStripe\Control\HTTPRequest;
use SilverStripe\Core\ClassInfo;
use SilverStripe\Core\Config\Config;
use SilverStripe\Core\Convert;
use SilverStripe\Core\Environment;
Expand Down Expand Up @@ -60,7 +61,14 @@ class DB
*/
protected static $configs = [];


/**
* Array of classes that have been confirmed ready for database queries.
* When the database has once been verified as ready, it will not do the
* checks again.
*
* @var boolean[]
*/
protected static $databaseReadyClasses = [];

/**
* The last SQL query run.
Expand Down Expand Up @@ -696,4 +704,100 @@ public static function alteration_message($message, $type = "")
{
self::get_schema()->alterationMessage($message, $type);
}

/**
* Check if all tables and field columns for a class exist in the database.
*
* @param string $class
* @return boolean
*/
public static function databaseIsReady(string $class): bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit of a misleading method name since it's for a specific dataobject, rather than the entire database. Maybe rename it to something like tablesCreated(string $class): bool. Also update the protected static property to match

Copy link
Member Author

@GuySartorelli GuySartorelli Apr 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a fair point. tablesCreated isn't quite right, since it's checking more than just if the tables are created (e.g. the table may be created but missing columns), but you're right that the current name isn't right either. I'll think of something more accurately descriptive.

{
if (!is_subclass_of($class, DataObject::class)) {
throw new InvalidArgumentException("$class is not a subclass of " . DataObject::class);
}

// Don't check again if we already know the db is ready for this class.
// Necessary here before the loop to catch situations where a subclass
// is forced as ready without having to check all the superclasses.
if (!empty(self::$databaseReadyClasses[$class])) {
return true;
}

// Check if all tables and fields required for the class exist in the database.
$requiredClasses = ClassInfo::dataClassesFor($class);
GuySartorelli marked this conversation as resolved.
Show resolved Hide resolved
$schema = DataObject::getSchema();
foreach ($requiredClasses as $required) {
// Skip test classes, as not all test classes are scaffolded at once
if (is_a($required, TestOnly::class, true)) {
continue;
}

// Don't check again if we already know the db is ready for this class.
if (!empty(self::$databaseReadyClasses[$class])) {
continue;
}

// if any of the tables aren't created in the database
$table = $schema->tableName($required);
if (!ClassInfo::hasTable($table)) {
return false;
}

// HACK: DataExtensions aren't applied until a class is instantiated for
// the first time, so create an instance here.
singleton($required);

// if any of the tables don't have all fields mapped as table columns
$dbFields = DB::field_list($table);
if (!$dbFields) {
return false;
}

$objFields = $schema->databaseFields($required, false);
$missingFields = array_diff_key($objFields, $dbFields);

if ($missingFields) {
return false;
}

// Add each ready class to the cached array.
self::$databaseReadyClasses[$required] = true;
}

return true;
}

/**
* Resets the databaseReadyClasses cache.
*
* @param string|null $class The specific class to be cleared.
* If not passed, the cache for all classes is cleared.
* @param bool $clearFullHeirarchy Whether to clear the full class hierarchy or only the given class.
*/
public static function clearDatabaseIsReady(?string $class = null, bool $clearFullHierarchy = true)
{
if ($class) {
$clearClasses = [$class];
if ($clearFullHierarchy) {
$clearClasses = ClassInfo::dataClassesFor($class);
}
foreach ($clearClasses as $clear) {
unset(self::$databaseReadyClasses[$clear]);
}
} else {
self::$databaseReadyClasses = [];
}
}

/**
* For the databaseIsReady call to return a certain value for the given class - used for testing
*
* @param string $class The class to be forced as ready/not ready.
* @param boolean $isReady The value to force.
*/
public static function forceDatabaseIsReady(string $class, bool $isReady)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is method is pretty unecessary IMO and it just clutters up the API. I think you're better off just update the visibility of the property to public via Reflection in the unit test and then updating the property directly

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security::force_database_is_ready() is a public method, which is what's called by the test. I agree that ideally the method wouldn't exist in the new location... should I replace it with reflection in Security::force_database_is_ready() (so that method still does what it has always done)?
And then any new tests that might need that functionality can use reflection directly in the tests?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave the existing Security method as is and use reflection within the new unit test. That will minimise the amount of API changes

Copy link
Member Author

@GuySartorelli GuySartorelli Apr 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm I realised a much cleaner way to handle this, which goes in line with #10041 (comment)
Edit: Posted this without seeing your latest reply - yup that's exactly the thinking.

{
self::$databaseReadyClasses[$class] = $isReady;
}
}
70 changes: 27 additions & 43 deletions src/Security/Security.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,12 +174,16 @@ class Security extends Controller implements TemplateGlobalProvider
private static $login_recording = false;

/**
* @deprecated 5.0 use {@link DB::$databaseReadyClasses} instead
*
* @var boolean If set to TRUE or FALSE, {@link database_is_ready()}
* will always return FALSE. Used for unit testing.
*/
protected static $force_database_is_ready;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an API deletion, would require a new major

Just leave it in and deprecate it. Along with $database_is_ready below

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For deprecating this, do you also want it to be populated when the method is called, and to affect the outcome of the checks? Or just leave the variable here unused? Same with $database_is_ready

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably best to keep them functioning as they currently are if it's not too much extra work


/**
* @deprecated 5.0 use {@link DB::$databaseReadyClasses} instead
*
* When the database has once been verified as ready, it will not do the
* checks again.
*
Expand Down Expand Up @@ -1229,49 +1233,16 @@ public static function encrypt_password($password, $salt = null, $algorithm = nu
*/
public static function database_is_ready()
{
// Used for unit tests
if (self::$force_database_is_ready !== null) {
return self::$force_database_is_ready;
}

if (self::$database_is_ready) {
return self::$database_is_ready;
}

$requiredClasses = ClassInfo::dataClassesFor(Member::class);
$requiredClasses[] = Group::class;
$requiredClasses[] = Permission::class;
$schema = DataObject::getSchema();
foreach ($requiredClasses as $class) {
// Skip test classes, as not all test classes are scaffolded at once
if (is_a($class, TestOnly::class, true)) {
continue;
}

// if any of the tables aren't created in the database
$table = $schema->tableName($class);
if (!ClassInfo::hasTable($table)) {
return false;
}

// HACK: DataExtensions aren't applied until a class is instantiated for
// the first time, so create an instance here.
singleton($class);

// if any of the tables don't have all fields mapped as table columns
$dbFields = DB::field_list($table);
if (!$dbFields) {
return false;
}

$objFields = $schema->databaseFields($class, false);
$missingFields = array_diff_key($objFields, $dbFields);

if ($missingFields) {
$toCheck = [
Member::class,
Group::class,
Permission::class,
];
foreach ($toCheck as $class) {
if (!DB::databaseIsReady($class)) {
return false;
}
}
self::$database_is_ready = true;

return true;
}
Expand All @@ -1281,8 +1252,14 @@ public static function database_is_ready()
*/
public static function clear_database_is_ready()
{
self::$database_is_ready = null;
self::$force_database_is_ready = null;
$toClear = [
Member::class,
Group::class,
Permission::class,
];
foreach ($toClear as $class) {
DB::clearDatabaseIsReady($class);
}
}

/**
Expand All @@ -1292,7 +1269,14 @@ public static function clear_database_is_ready()
*/
public static function force_database_is_ready($isReady)
{
self::$force_database_is_ready = $isReady;
$toForce = [
Member::class,
Group::class,
Permission::class,
];
foreach ($toForce as $class) {
DB::forceDatabaseIsReady($class, $isReady);
}
}

/**
Expand Down