Skip to content

Commit

Permalink
Assert if reference type is the same by default (#1091)
Browse files Browse the repository at this point in the history
  • Loading branch information
mvorisek authored Feb 9, 2023
1 parent e4cb17b commit 856f742
Show file tree
Hide file tree
Showing 12 changed files with 100 additions and 45 deletions.
2 changes: 1 addition & 1 deletion docs/joins.rst
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ For example it can be used to pull country information based on user.country_id
but you wouldn't want that adding a new user would create a new country::

$user->addField('username');
$user->addField('country_id');
$user->addField('country_id', ['type' => 'integer']);
$jCountry = $user->join('country', ['weak' => true, 'prefix' => 'country_']);
$jCountry->addField('code');
$jCountry->addField('name');
Expand Down
2 changes: 1 addition & 1 deletion docs/persistence.rst
Original file line number Diff line number Diff line change
Expand Up @@ -628,7 +628,7 @@ some other database (for archive purposes) you can implement it like this::
$arc = $this->withPersistence($m->getApp()->archive_db);

// add some audit fields
$arc->addField('original_id')->set($this->getId());
$arc->addField('original_id', ['type' => 'integer'])->set($this->getId());
$arc->addField('saved_by')->set($this->getApp()->user);

$arc->saveAndUnload();
Expand Down
2 changes: 1 addition & 1 deletion docs/references.rst
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@ process and the loadAny() will look like this:
By passing options to hasOne() you can also differentiate field name::

$o->addField('user_id');
$o->addField('user_id', ['type' => 'integer']);
$o->hasOne('User', ['model' => $u, 'ourField' => 'user_id']);

$o->load(1)->ref('User')['name'];
Expand Down
2 changes: 1 addition & 1 deletion src/Persistence.php
Original file line number Diff line number Diff line change
Expand Up @@ -439,7 +439,7 @@ protected function _typecastSaveField(Field $field, $value)
* This is the actual field typecasting, which you can override in your
* persistence to implement necessary typecasting.
*
* @param scalar|null $value
* @param scalar $value
*
* @return mixed
*/
Expand Down
2 changes: 1 addition & 1 deletion src/Persistence/Sql/Join.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ protected function init(): void
if (!$this->reverse && !$this->getOwner()->hasField($this->masterField)) {
$owner = $this->hasJoin() ? $this->getJoin() : $this->getOwner();

$field = $owner->addField($this->masterField, ['system' => true, 'readOnly' => true]);
$field = $owner->addField($this->masterField, ['type' => 'integer', 'system' => true, 'readOnly' => true]);

$this->masterField = $field->shortName;
}
Expand Down
16 changes: 16 additions & 0 deletions src/Reference.php
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,12 @@ class Reference
*/
protected ?string $theirField = null;

/**
* Database our/their field types must always match, but DBAL types can be different in theory,
* set this to false when the DBAL types are intentionally different.
*/
public bool $checkTheirType = true;

/**
* Caption of the referenced model. Can be used in UI components, for example.
* Should be in plain English and ready for proper localization.
Expand Down Expand Up @@ -227,6 +233,16 @@ public function createTheirModel(array $defaults = []): Model

$this->addToPersistence($theirModel, $defaults);

if ($this->checkTheirType) {
$ourField = $this->getOurField();
$theirField = $theirModel->getField($this->getTheirFieldName($theirModel));
if ($theirField->type !== $ourField->type) {
throw (new Exception('Reference type mismatch'))
->addMoreInfo('ourField', $ourField)
->addMoreInfo('theirField', $theirField);
}
}

return $theirModel;
}

Expand Down
2 changes: 2 additions & 0 deletions src/Reference/ContainsBase.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ abstract class ContainsBase extends Reference
{
use ContainsSeedHackTrait;

public bool $checkTheirType = false;

/** Field type. */
public string $type = 'json';

Expand Down
34 changes: 17 additions & 17 deletions tests/JoinSqlTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public function testJoinSaving1(): void
],
]);

$user->addField('contact_id');
$user->addField('contact_id', ['type' => 'integer']);
$user->addField('name');
$j = $user->join('contact');
$this->createMigrator()->createForeignKey($j);
Expand Down Expand Up @@ -226,12 +226,12 @@ public function testJoinLoading(): void

$user2 = $user->load(1);
static::assertSame([
'id' => 1, 'name' => 'John', 'contact_id' => '1', 'contact_phone' => '+123',
'id' => 1, 'name' => 'John', 'contact_id' => 1, 'contact_phone' => '+123',
], $user2->get());

$user2 = $user->load(3);
static::assertSame([
'id' => 3, 'name' => 'Joe', 'contact_id' => '2', 'contact_phone' => '+321',
'id' => 3, 'name' => 'Joe', 'contact_id' => 2, 'contact_phone' => '+321',
], $user2->get());

$user2 = $user2->unload();
Expand All @@ -257,7 +257,7 @@ public function testJoinUpdate(): void
]);

$user = new Model($this->db, ['table' => 'user']);
$user->addField('contact_id');
$user->addField('contact_id', ['type' => 'integer']);
$user->addField('name');
$j = $user->join('contact');
$this->createMigrator()->createForeignKey($j);
Expand Down Expand Up @@ -351,7 +351,7 @@ public function testJoinDelete(): void
]);

$user = new Model($this->db, ['table' => 'user']);
$user->addField('contact_id');
$user->addField('contact_id', ['type' => 'integer']);
$user->addField('name');
$j = $user->join('contact');
// TODO persist order is broken $this->createMigrator()->createForeignKey($j);
Expand Down Expand Up @@ -432,7 +432,7 @@ public function testDoubleJoin(): void
]);

$user = new Model($this->db, ['table' => 'user']);
$user->addField('contact_id');
$user->addField('contact_id', ['type' => 'integer']);
$user->addField('name');
$jContact = $user->join('contact');
// TODO persist order is broken $this->createMigrator()->createForeignKey($jContact);
Expand Down Expand Up @@ -498,7 +498,7 @@ public function testDoubleReverseJoin(): void
]);

$user = new Model($this->db, ['table' => 'user']);
$user->addField('contact_id');
$user->addField('contact_id', ['type' => 'integer']);
$user->addField('name');
$j = $user->join('contact');
// TODO persist order is broken $this->createMigrator()->createForeignKey($j);
Expand Down Expand Up @@ -556,13 +556,13 @@ public function testJoinHasOneHasMany(): void
// main user model joined to contact table
$user = new Model($this->db, ['table' => 'user']);
$user->addField('name');
$user->addField('contact_id');
$user->addField('contact_id', ['type' => 'integer']);
$j = $user->join('contact');
$this->createMigrator()->createForeignKey($j);

$user2 = $user->load(1);
static::assertSame([
'id' => 1, 'name' => 'John', 'contact_id' => '10',
'id' => 1, 'name' => 'John', 'contact_id' => 10,
], $user2->get());

// hasOne phone model
Expand All @@ -574,35 +574,35 @@ public function testJoinHasOneHasMany(): void

$user2 = $user->load(1);
static::assertSame([
'id' => 1, 'name' => 'John', 'contact_id' => '10', 'phone_id' => 20, 'number' => '+123',
'id' => 1, 'name' => 'John', 'contact_id' => 10, 'phone_id' => 20, 'number' => '+123',
], $user2->get());

// hasMany token model (uses default ourField, theirField)
$token = new Model($this->db, ['table' => 'token']);
$token->addField('user_id');
$token->addField('user_id', ['type' => 'integer']);
$token->addField('token');
$refMany = $j->hasMany('Token', ['model' => $token]); // hasMany on JOIN (use default ourField, theirField)
$this->createMigrator()->createForeignKey($refMany);

$user2 = $user->load(1);
static::assertSameExportUnordered([
['id' => 30, 'user_id' => '1', 'token' => 'ABC'],
['id' => 31, 'user_id' => '1', 'token' => 'DEF'],
['id' => 30, 'user_id' => 1, 'token' => 'ABC'],
['id' => 31, 'user_id' => 1, 'token' => 'DEF'],
], $user2->ref('Token')->export());

$this->markTestIncompleteWhenCreateUniqueIndexIsNotSupportedByPlatform();

// hasMany email model (uses custom ourField, theirField)
$email = new Model($this->db, ['table' => 'email']);
$email->addField('contact_id');
$email->addField('contact_id', ['type' => 'integer']);
$email->addField('address');
$refMany = $j->hasMany('Email', ['model' => $email, 'ourField' => 'contact_id', 'theirField' => 'contact_id']); // hasMany on JOIN (use custom ourField, theirField)
$this->createMigrator()->createForeignKey($refMany);

$user2 = $user->load(1);
static::assertSameExportUnordered([
['id' => 40, 'contact_id' => '10', 'address' => 'john@foo.net'],
['id' => 41, 'contact_id' => '10', 'address' => 'johnny@foo.net'],
['id' => 40, 'contact_id' => 10, 'address' => 'john@foo.net'],
['id' => 41, 'contact_id' => 10, 'address' => 'johnny@foo.net'],
], $user2->ref('Email')->export());
}

Expand Down Expand Up @@ -704,7 +704,7 @@ public function testJoinActualFieldNamesAndPrefix(): void
]);

$user = new Model($this->db, ['table' => 'user']);
$user->addField('contact_id', ['actual' => $contactForeignIdFieldName]);
$user->addField('contact_id', ['type' => 'integer', 'actual' => $contactForeignIdFieldName]);
$user->addField('name', ['actual' => 'first_name']);
// normal join
$j = $user->join('contact', ['prefix' => 'j1_']);
Expand Down
2 changes: 1 addition & 1 deletion tests/Model/Smbo/Transfer.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ protected function init(): void
$this->addCondition('transfer_document_id', '!=', null);
}

$this->addField('destination_account_id', ['neverPersist' => true]);
$this->addField('destination_account_id', ['type' => 'integer', 'neverPersist' => true]);

$this->onHookShort(self::HOOK_BEFORE_SAVE, function () {
// only for new records and when destination_account_id is set
Expand Down
26 changes: 13 additions & 13 deletions tests/ReferenceSqlTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public function testBasic(): void

$o = new Model($this->db, ['table' => 'order']);
$o->addField('amount', ['type' => 'integer']);
$o->addField('user_id');
$o->addField('user_id', ['type' => 'integer']);

$u->hasMany('Orders', ['model' => $o]);

Expand Down Expand Up @@ -80,7 +80,7 @@ public function testLink(): void

$o = new Model($this->db, ['table' => 'order']);
$o->addField('amount');
$o->addField('user_id');
$o->addField('user_id', ['type' => 'integer']);

$u->hasMany('Orders', ['model' => $o]);

Expand Down Expand Up @@ -268,7 +268,7 @@ public function testRelatedExpression(): void
$i->addField('ref_no');

$l = new Model($this->db, ['table' => 'invoice_line']);
$l->addField('invoice_id');
$l->addField('invoice_id', ['type' => 'integer']);
$l->addField('total_net');
$l->addField('total_vat');
$l->addField('total_gross');
Expand Down Expand Up @@ -350,7 +350,7 @@ public function getValue(): int
$file->addField('name');
$file->hasOne('parentDirectory', [
'model' => $file,
'type' => $integerWrappedTypeName, // TODO should be implied from their model
'type' => $integerWrappedTypeName,
'ourField' => 'parentDirectoryId',
]);
$file->hasMany('childFiles', [
Expand Down Expand Up @@ -433,7 +433,7 @@ public function testAggregateHasMany(): void
$i->addField('ref_no');

$l = new Model($this->db, ['table' => 'invoice_line']);
$l->addField('invoice_id');
$l->addField('invoice_id', ['type' => 'integer']);
$l->addField('total_net', ['type' => 'atk4_money']);
$l->addField('total_vat', ['type' => 'atk4_money']);
$l->addField('total_gross', ['type' => 'atk4_money']);
Expand Down Expand Up @@ -511,7 +511,7 @@ public function testOtherAggregates(): void
$l->addField('name');

$i = new Model($this->db, ['table' => 'item']);
$i->addField('list_id');
$i->addField('list_id', ['type' => 'integer']);
$i->addField('name');
$i->addField('code');

Expand Down Expand Up @@ -567,15 +567,15 @@ protected function setupDbForTraversing(): Model

$user = new Model($this->db, ['table' => 'user']);
$user->addField('name');
$user->addField('company_id');
$user->addField('company_id', ['type' => 'integer']);

$company = new Model($this->db, ['table' => 'company']);
$company->addField('name');

$user->hasOne('Company', ['model' => $company, 'ourField' => 'company_id', 'theirField' => 'id']);

$order = new Model($this->db, ['table' => 'order']);
$order->addField('company_id');
$order->addField('company_id', ['type' => 'integer']);
$order->addField('description');
$order->addField('amount', ['default' => 20, 'type' => 'float']);

Expand All @@ -590,14 +590,14 @@ public function testReferenceHasOneTraversing(): void
$userEntity = $user->load(1);

static::assertSameExportUnordered([
['id' => 1, 'company_id' => '1', 'description' => 'Vinny Company Order 1', 'amount' => 50.0],
['id' => 3, 'company_id' => '1', 'description' => 'Vinny Company Order 2', 'amount' => 15.0],
['id' => 1, 'company_id' => 1, 'description' => 'Vinny Company Order 1', 'amount' => 50.0],
['id' => 3, 'company_id' => 1, 'description' => 'Vinny Company Order 2', 'amount' => 15.0],
], $userEntity->ref('Company')->ref('Orders')->export());

static::assertSameExportUnordered([
['id' => 1, 'company_id' => '1', 'description' => 'Vinny Company Order 1', 'amount' => 50.0],
['id' => 2, 'company_id' => '2', 'description' => 'Zoe Company Order', 'amount' => 10.0],
['id' => 3, 'company_id' => '1', 'description' => 'Vinny Company Order 2', 'amount' => 15.0],
['id' => 1, 'company_id' => 1, 'description' => 'Vinny Company Order 1', 'amount' => 50.0],
['id' => 2, 'company_id' => 2, 'description' => 'Zoe Company Order', 'amount' => 10.0],
['id' => 3, 'company_id' => 1, 'description' => 'Vinny Company Order 2', 'amount' => 15.0],
], $userEntity->getModel()->ref('Company')->ref('Orders')->export());
}

Expand Down
Loading

0 comments on commit 856f742

Please sign in to comment.