Skip to content

Commit

Permalink
NEW: Static validation for relationships. (#9874)
Browse files Browse the repository at this point in the history
* NEW: Static validation for relationships.

* Unit tests added.

* PR fixes

* PR feedback: Execute validation on flush.

* PR fixes.

* PR fixes.
mfendeksilverstripe authored Feb 4, 2022

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
1 parent 8f1c68d commit 89c87dd
Showing 10 changed files with 1,168 additions and 3 deletions.
5 changes: 4 additions & 1 deletion _config/extensions.yml
Original file line number Diff line number Diff line change
@@ -6,4 +6,7 @@ SilverStripe\Security\Member:
- SilverStripe\Security\InheritedPermissionFlusher
SilverStripe\Security\Group:
extensions:
- SilverStripe\Security\InheritedPermissionFlusher
- SilverStripe\Security\InheritedPermissionFlusher
SilverStripe\ORM\DatabaseAdmin:
extensions:
- SilverStripe\Dev\Validation\DatabaseAdminExtension
16 changes: 14 additions & 2 deletions docs/en/02_Developer_Guides/00_Model/02_Relations.md
Original file line number Diff line number Diff line change
@@ -201,7 +201,9 @@ class Company extends DataObject
```

Multiple `$has_one` relationships are okay if they aren't linking to the same object type. Otherwise, they have to be
named.
named. With that said, naming is recommended in all cases as it makes your code more resilient to change. Adding new relationships is easier when you don't need to review and update existing ones.

You can use `RelationValidationService` for validation of relationships. This tool will point out the relationships which may need a review.

If you're using the default scaffolded form fields with multiple `has_one` relationships, you will end up with a CMS field for each relation. If you don't want these you can remove them by their IDs:

@@ -224,6 +226,8 @@ declaring the `$belongs_to`.
Similarly with `$has_many`, dot notation can be used to explicitly specify the `$has_one` which refers to this relation.
This is not mandatory unless the relationship would be otherwise ambiguous.

You can use `RelationValidationService` for validation of relationships. This tool will point out the relationships which may need a review.

```php
use SilverStripe\ORM\DataObject;

@@ -251,7 +255,15 @@ how the developer wishes to manage this join table.

[warning]
Please specify a $belongs_many_many-relationship on the related class as well, in order
to have the necessary accessors available on both ends.
to have the necessary accessors available on both ends. You can use `RelationValidationService` for validation of relationships. This tool will point out the relationships which may need a review.

Example configuration:

```yaml
SilverStripe\Dev\Validation\RelationValidationService:
output_enabled: true
```
[/warning]
Much like the `has_one` relationship, `many_many` can be navigated through the `ORM` as well.
34 changes: 34 additions & 0 deletions src/Dev/Validation/DatabaseAdminExtension.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<?php

namespace SilverStripe\Dev\Validation;

use ReflectionException;
use SilverStripe\Core\Extension;
use SilverStripe\ORM\DatabaseAdmin;

/**
* Hook up static validation to the deb/build process
*
* @method DatabaseAdmin getOwner()
*/
class DatabaseAdminExtension extends Extension
{
/**
* Extension point in @see DatabaseAdmin::doBuild()
*
* @param bool $quiet
* @param bool $populate
* @param bool $testMode
* @throws ReflectionException
*/
public function onAfterBuild(bool $quiet, bool $populate, bool $testMode): void
{
$service = RelationValidationService::singleton();

if (!$service->config()->get('output_enabled')) {
return;
}

$service->executeValidation();
}
}
635 changes: 635 additions & 0 deletions src/Dev/Validation/RelationValidationService.php

Large diffs are not rendered by default.

4 changes: 4 additions & 0 deletions src/ORM/DatabaseAdmin.php
Original file line number Diff line number Diff line change
@@ -227,6 +227,8 @@ public static function lastBuilt()
*/
public function doBuild($quiet = false, $populate = true, $testMode = false)
{
$this->extend('onBeforeBuild', $quiet, $populate, $testMode);

if ($quiet) {
DB::quiet();
} else {
@@ -400,6 +402,8 @@ public function doBuild($quiet = false, $populate = true, $testMode = false)
}

ClassInfo::reset_db_cache();

$this->extend('onAfterBuild', $quiet, $populate, $testMode);
}

/**
36 changes: 36 additions & 0 deletions tests/php/Dev/Validation/Freelancer.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<?php

namespace SilverStripe\Dev\Tests\Validation;

use SilverStripe\Dev\TestOnly;
use SilverStripe\ORM\DataObject;

/**
* Class Freelancer
*
* @property string $Title
* @method Team TemporaryTeam()
* @method Member TemporaryMember()
*/
class Freelancer extends DataObject implements TestOnly
{
/**
* @var string
*/
private static $table_name = 'RelationValidationTest_Freelancer';

/**
* @var array
*/
private static $db = [
'Title' => 'Varchar(255)',
];

/**
* @var array
*/
private static $has_one = [
'TemporaryTeam' => Team::class,
'TemporaryMember' => Member::class,
];
}
43 changes: 43 additions & 0 deletions tests/php/Dev/Validation/Hat.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
<?php

namespace SilverStripe\Dev\Tests\Validation;

use SilverStripe\Dev\TestOnly;
use SilverStripe\ORM\DataObject;
use SilverStripe\ORM\ManyManyList;

/**
* Class Hat
*
* @property string $Title
* @method Member Hatter()
* @method ManyManyList|Team[] TeamHats()
*/
class Hat extends DataObject implements TestOnly
{
/**
* @var string
*/
private static $table_name = 'RelationValidationTest_Hat';

/**
* @var array
*/
private static $db = [
'Title' => 'Varchar(255)',
];

/**
* @var array
*/
private static $belongs_to = [
'Hatter' => Member::class . '.Hat',
];

/**
* @var array
*/
private static $belongs_many_many = [
'TeamHats' => Team::class . '.ReserveHats',
];
}
54 changes: 54 additions & 0 deletions tests/php/Dev/Validation/Member.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
<?php

namespace SilverStripe\Dev\Tests\Validation;

use SilverStripe\Dev\TestOnly;
use SilverStripe\ORM\DataObject;
use SilverStripe\ORM\HasManyList;
use SilverStripe\ORM\ManyManyThroughList;

/**
* Class Member
*
* @property string $Title
* @method Team HomeTeam()
* @method Hat Hat()
* @method HasManyList|Freelancer[] TemporaryMembers()
* @method ManyManyThroughList|Member[] FreelancerTeams()
*/
class Member extends DataObject implements TestOnly
{
/**
* @var string
*/
private static $table_name = 'RelationValidationTest_Member';

/**
* @var array
*/
private static $db = [
'Title' => 'Varchar(255)',
];

/**
* @var array
*/
private static $has_one = [
'HomeTeam' => Team::class,
'Hat' => Hat::class,
];

/**
* @var array
*/
private static $has_many = [
'TemporaryMembers' => Freelancer::class . '.TemporaryMember',
];

/**
* @var array
*/
private static $belongs_many_many = [
'FreelancerTeams' => Team::class . '.Freelancers',
];
}
291 changes: 291 additions & 0 deletions tests/php/Dev/Validation/RelationValidationTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,291 @@
<?php

namespace SilverStripe\Dev\Tests\Validation;

use Page;
use SilverStripe\Core\Config\Config;
use SilverStripe\Dev\SapphireTest;
use SilverStripe\Dev\Validation\RelationValidationService;

class RelationValidationTest extends SapphireTest
{
/**
* @var array
*/
protected static $extra_dataobjects = [
Team::class,
Member::class,
Hat::class,
Freelancer::class,
];

/**
* @param string|null $class
* @param string|null $field
* @param array $value
* @param array $expected
* @dataProvider validateCasesProvider
*/
public function testValidation(?string $class, ?string $field, array $value, array $expected): void
{
if ($class && $field) {
Config::modify()->set($class, $field, $value);
}

$data = RelationValidationService::singleton()->inspectClasses([
Team::class,
Member::class,
Hat::class,
Freelancer::class,
]);

$this->assertSame($expected, $data);
}

/**
* @param string $class
* @param string|null $relation
* @param array $config
* @param bool $expected
* @dataProvider ignoredClassesProvider
*/
public function testIgnoredClass(string $class, ?string $relation, array $config, bool $expected): void
{
$service = RelationValidationService::singleton();

foreach ($config as $name => $value) {
$service->config()->set($name, $value);
}

$result = $service->isIgnored($class, $relation);

$this->assertEquals($expected, $result);
}

public function validateCasesProvider(): array
{
return [
'correct setup' => [
null,
null,
[],
[],
],
'ambiguous has_one - no relation name' => [
Hat::class,
'belongs_to',
[
'Hatter' => Member::class,
],
[
'SilverStripe\Dev\Tests\Validation\Member / Hat : Back relation not found or ambiguous (needs class.relation format)',
'SilverStripe\Dev\Tests\Validation\Hat / Hatter : Relation is not in the expected format (needs class.relation format)',
],
],
'ambiguous has_one - incorrect relation name' => [
Hat::class,
'belongs_to',
[
'Hatter' => Member::class . '.ObviouslyWrong',
],
[
'SilverStripe\Dev\Tests\Validation\Member / Hat : Back relation not found or ambiguous (needs class.relation format)',
'SilverStripe\Dev\Tests\Validation\Hat / Hatter : Back relation not found',
],
],
'ambiguous has_one - too many relations' => [
Hat::class,
'belongs_to',
[
'Hatter' => Member::class . '.Hat',
'HatterCopy' => Member::class . '.Hat',
],
[
'SilverStripe\Dev\Tests\Validation\Member / Hat : Back relation is ambiguous',
],
],
'invalid has one' => [
Member::class,
'has_one',
[
'HomeTeam' => Team::class . '.UnnecessaryRelation',
'Hat' => Hat::class,
],
[
'SilverStripe\Dev\Tests\Validation\Member / HomeTeam : Relation SilverStripe\Dev\Tests\Validation\Team.UnnecessaryRelation is not in the expected format (needs class only format).'
],
],
'ambiguous has_many - no relation name' => [
Team::class,
'has_many',
[
'Members' => Member::class,
'FreelancerMembers' => Freelancer::class . '.TemporaryTeam',
],
[
'SilverStripe\Dev\Tests\Validation\Team / Members : Relation is not in the expected format (needs class.relation format)',
'SilverStripe\Dev\Tests\Validation\Member / HomeTeam : Back relation not found or ambiguous (needs class.relation format)',
],
],
'ambiguous has_many - incorrect relation name' => [
Team::class,
'has_many',
[
'Members' => Member::class . '.ObviouslyWrong',
'FreelancerMembers' => Freelancer::class . '.TemporaryTeam',
],
[
'SilverStripe\Dev\Tests\Validation\Team / Members : Back relation not found or ambiguous (needs class.relation format)',
'SilverStripe\Dev\Tests\Validation\Member / HomeTeam : Back relation not found or ambiguous (needs class.relation format)',
],
],
'ambiguous has_many - too many relations' => [
Team::class,
'has_many',
[
'Members' => Member::class . '.HomeTeam',
'MembersCopy' => Member::class . '.HomeTeam',
'FreelancerMembers' => Freelancer::class . '.TemporaryTeam',
],
[
'SilverStripe\Dev\Tests\Validation\Member / HomeTeam : Back relation is ambiguous',
],
],
'ambiguous many_many - no relation name' => [
Hat::class,
'belongs_many_many',
[
'TeamHats' => Team::class,
],
[
'SilverStripe\Dev\Tests\Validation\Team / ReserveHats : Back relation not found or ambiguous (needs class.relation format)',
'SilverStripe\Dev\Tests\Validation\Hat / TeamHats : Relation is not in the expected format (needs class.relation format)',
],
],
'ambiguous many_many - incorrect relation name' => [
Hat::class,
'belongs_many_many',
[
'TeamHats' => Team::class . '.ObviouslyWrong',
],
[
'SilverStripe\Dev\Tests\Validation\Team / ReserveHats : Back relation not found or ambiguous (needs class.relation format)',
'SilverStripe\Dev\Tests\Validation\Hat / TeamHats : Back relation not found',
],
],
'ambiguous many_many - too many relations' => [
Hat::class,
'belongs_many_many',
[
'TeamHats' => Team::class . '.ReserveHats',
'TeamHatsCopy' => Team::class . '.ReserveHats',
],
[
'SilverStripe\Dev\Tests\Validation\Team / ReserveHats : Back relation is ambiguous',
],
],
'ambiguous many_many through - no relation name' => [
Member::class,
'belongs_many_many',
[
'FreelancerTeams' => Team::class,
],
[
'SilverStripe\Dev\Tests\Validation\Team / Freelancers : Back relation not found or ambiguous (needs class.relation format)',
'SilverStripe\Dev\Tests\Validation\Member / FreelancerTeams : Relation is not in the expected format (needs class.relation format)',
],
],
'ambiguous many_many through - incorrect relation name' => [
Member::class,
'belongs_many_many',
[
'FreelancerTeams' => Team::class . '.ObviouslyWrong',
],
[
'SilverStripe\Dev\Tests\Validation\Team / Freelancers : Back relation not found or ambiguous (needs class.relation format)',
'SilverStripe\Dev\Tests\Validation\Member / FreelancerTeams : Back relation not found',
],
],
'ambiguous many_many through - too many relations' => [
Member::class,
'belongs_many_many',
[
'FreelancerTeams' => Team::class . '.Freelancers',
'FreelancerTeamsCopy' => Team::class . '.Freelancers',
],
[
'SilverStripe\Dev\Tests\Validation\Team / Freelancers : Back relation is ambiguous',
],
],
];
}

public function ignoredClassesProvider(): array
{
return [
'class default' => [
Team::class,
null,
[],
true,
],
'class relation default' => [
Team::class,
'Members',
[],
true,
],
'page should by included by default (empty namespace)' => [
Page::class,
null,
[],
false,
],
'class relation via allow rules' => [
Team::class,
'Members',
[
'allow_rules' => ['app' => 'SilverStripe\Dev\Tests\Validation'],
],
false,
],
'class included via allow rules but overwritten by deny rules' => [
Team::class,
null,
[
'allow_rules' => ['app' => 'SilverStripe\Dev\Tests\Validation'],
'deny_rules' => [Team::class],
],
true,
],
'class relation included via allow rules but overwritten by deny rules' => [
Team::class,
'Members',
[
'allow_rules' => ['app' => 'SilverStripe\Dev\Tests\Validation'],
'deny_rules' => [Team::class],
],
true,
],
'class relation included via allow rules but overwritten by deny relations' => [
Team::class,
'Members',
[
'allow_rules' => ['app' => 'SilverStripe\Dev\Tests\Validation'],
'deny_relations' => [Team::class . '.Members'],
],
true,
],
'class relation included via allow rules and not overwritten by deny relations of other class' => [
Member::class,
'HomeTeam',
[
'allow_rules' => ['app' => 'SilverStripe\Dev\Tests\Validation'],
'deny_rules' => [Team::class],
'deny_relations' => [Team::class . '.Members'],
],
false,
],
];
}
}
53 changes: 53 additions & 0 deletions tests/php/Dev/Validation/Team.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
<?php

namespace SilverStripe\Dev\Tests\Validation;

use SilverStripe\Dev\TestOnly;
use SilverStripe\ORM\DataObject;
use SilverStripe\ORM\HasManyList;
use SilverStripe\ORM\ManyManyList;
use SilverStripe\ORM\ManyManyThroughList;

/**
* Class Team
*
* @property string $Title
* @method HasManyList|Member[] Members()
* @method HasManyList|Freelancer[] FreelancerMembers()
* @method ManyManyThroughList|Member[] Freelancers()
* @method ManyManyList|Hat[] ReserveHats()
*/
class Team extends DataObject implements TestOnly
{
/**
* @var string
*/
private static $table_name = 'RelationValidationTest_Team';

/**
* @var array
*/
private static $db = [
'Title' => 'Varchar(255)',
];

/**
* @var array
*/
private static $has_many = [
'Members' => Member::class . '.HomeTeam',
'FreelancerMembers' => Freelancer::class . '.TemporaryTeam',
];

/**
* @var array
*/
private static $many_many = [
'ReserveHats' => Hat::class,
'Freelancers' => [
'through' => Freelancer::class,
'from' => 'TemporaryTeam',
'to' => 'TemporaryMember',
],
];
}

0 comments on commit 89c87dd

Please sign in to comment.