Skip to content

Fix system condition handling #692

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

Closed
georgehristov wants to merge 8 commits into from
Closed

Fix system condition handling #692

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
030b5e0
[update] introduce and use Scope::create
georgehristov Aug 8, 2020
e9e6dfb
[update] introduce Condition::isTraversing
georgehristov Aug 8, 2020
0cc4be9
[fix] introduce AbstractScope::setSystem
georgehristov Aug 8, 2020
a332d86
[fix] use correct operator
georgehristov Aug 8, 2020
7db95b1
Revert "[update] introduce Condition::isTraversing"
georgehristov Aug 8, 2020
d12f1cc
Revert "[update] introduce and use Scope::create"
mvorisek Aug 8, 2020
cbdb1c9
Merge branch 'develop' into fix/introduce-system-condition
mvorisek Sep 1, 2023
50c8d75
fix merge
mvorisek Sep 1, 2023
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
9 changes: 9 additions & 0 deletions src/Model/Scope.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,15 @@ public function addCondition($field, $operator = null, $value = null)
return $this;
}

protected function setSystem($system = true)
{
foreach ($this->elements as $nestedCondition) {
$nestedCondition->setSystem($system && $this->isAnd());
}

return $this;
}

/**
* Return array of nested conditions.
*
Expand Down
5 changes: 5 additions & 0 deletions src/Model/Scope/AbstractScope.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,16 @@ public function init(): void

$this->_init();

// always set system flag if condition added to another condition
$this->setSystem($this->owner instanceof RootScope);

$this->onChangeModel();
}

abstract protected function onChangeModel();

abstract protected function setSystem($system = true);

/**
* Get the model this condition is associated with.
*/
Expand Down
19 changes: 18 additions & 1 deletion src/Model/Scope/Condition.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ class Condition extends AbstractScope
*/
public $value;

protected $system = false;

public const OPERATOR_EQUALS = '=';
public const OPERATOR_DOESNOT_EQUAL = '!=';
public const OPERATOR_GREATER = '>';
Expand Down Expand Up @@ -137,14 +139,21 @@ public function __construct($key, $operator = null, $value = null)
}
}

protected function setSystem($system = true)
{
$this->system = $system;

return $this;
}

protected function onChangeModel(): void
{
if ($model = $this->getModel()) {
// if we have a definitive scalar value for a field
// sets it as default value for field and locks it
// new records will automatically get this value assigned for the field
// @todo: consider this when condition is part of OR scope
if ($this->operator === self::OPERATOR_EQUALS && !is_object($this->value) && !is_array($this->value)) {
if ($this->system && $this->setsDefiniteValue()) {
Copy link
Member

@mvorisek mvorisek Aug 8, 2020

Choose a reason for hiding this comment

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

this whole code must be moved into Model under specific method

if this is currently used in refs etc., we must investigate, probably we even want to call this special method when traversing (thru refs)

Copy link
Collaborator Author

@georgehristov georgehristov Aug 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Model condition in ATK is integral part of the model as the data can be scoped down using Model::addCondition or Model::scope()->addCondition. So addCondition is the special method we use.
This present flag routine is necessary as it makes sure:

  • when Model/Scope are cloned the same defaults are set for the field. So whenever you add new record this value (if definite) is applied as default
  • when scope is created or cloned and added to another scope it makes sure the flag is set only when we have a definite value

It is not about reference records only. Consider a model People with field Gender. You can define a sub-scope model classMen when adding condition gender = 'male' in init method. So if you create new record in Men the Gender field will be set automatically to 'male' and having it as system field you cannot set it to anything else, which is correct.

The other not-defining way to apply condition is on the query directly after we merge my refactor on queries:
$people->toQuery('select')->where('gender', 'male');
or when I introduce the Model\QueryTrait:
$people->where('gender', 'male');

This does not change the scope on $people. It just returns an iterator for all men.

If you find it more descriptive I can change the setSystem to setSystemFieldIfDefinite and we merge this fix

Copy link
Member

@mvorisek mvorisek Aug 10, 2020

Choose a reason for hiding this comment

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

I know this. I will summarize shortly:

  • addCondition should never change Field property (thus under no condition set system flag without the user requests it)
  • I proposed special explicit method in Model thus - and you confirm it with the post above - it is about scoping Model, thus always topmost condition & and junction
  • this will work will cloning nicely too

Copy link
Collaborator Author

@georgehristov georgehristov Aug 10, 2020

Choose a reason for hiding this comment

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

I am explaining above that addCondition is this explicit method you are requesting.
It implicates that the model is being changed (not for querying only) as you addCondition to the model

The 'not explicit' method is Model::where

Copy link
Member

@mvorisek mvorisek Aug 10, 2020

Choose a reason for hiding this comment

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

Look at it differently - I use STANDARD addCondition for ex. for event, let say "party".

If want to restrict user to only one event - "party" - then I add condition "= 'party'".

With your proposal, the event field will behave absolutely differently vs. if I set "in('party')". That is unacceptable.

So maybe we mixed 2 things together:
a) there was some default/system behaviour when some definitive condition was set which was broken as we allow deeper nesting of not only and conditions now - this PR may solve this, even I think not optimally, as Condition can be made later made not always true
b) implicit change of Field with side effect to other code - described above, we MUST prevent this

Copy link
Collaborator Author

@georgehristov georgehristov Aug 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

So in this regard I would suggest to approach the other way:

  • if definite value set on condition - set value as field default
  • if no definite value set - validate new entries with model scope. So once you set event in ('party', 'gala') then no other values will be accepted for the event field and validation exception will be thrown.
    I have the validation routine for Scope but I removed it for the basic functionality.

We also need opinion and input on this from @romaninsh and @DarkSide666

Copy link
Member

@mvorisek mvorisek Aug 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

* if definite value set on condition - set value as field default

this may be leaking (like if we compare some secret) and not sure if you meant system as well

I propose to NOT set any Field param based on condition.

I think, that system&default is needed for refs or in definitive model scoping in general, but this should be fully intended by the user and the current approach is for = only (as default can have only one value), thus in in your later example can not be handled or even = when defined with expr.

* if no definite value set - validate new entries with model scope. So once you set `event in ('party', 'gala')` then no other values will be accepted for the event field and validation exception will be thrown.
  I have the validation routine for Scope but I removed it for the basic functionality.

If you do not have a full SQL parser like DSQL (which parses fully, but it quite limited on the other side), then you can not have a validation routine.

To move this thread forward I propose to implement:

so I stay very very firm to implement is using special method in Model - which I belive is clean, quite BC (100% in refs) and later, we might introduce extended concept - but probably never redefine Field from Condition/Scope

// key containing '/' means chained references and it is handled in toQueryArguments method
if (is_string($field = $this->key) && !str_contains($field, '/')) {
$field = $model->getField($field);
Expand Down Expand Up @@ -224,6 +233,14 @@ public function isEmpty(): bool
return array_filter([$this->key, $this->operator, $this->value]) ? false : true;
}

/**
* Checks if condition sets a definitive scalar value for a field.
*/
protected function setsDefiniteValue(): bool
{
return $this->operator === self::OPERATOR_EQUALS && !is_object($this->value) && !is_array($this->value);
}

public function clear()
{
$this->key = $this->operator = $this->value = null;
Expand Down
2 changes: 1 addition & 1 deletion tests/Model/Smbo/Transfer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ public function init(): void

// only used to create / destroy trasfer legs
if (!$this->detached) {
$this->addCondition('transfer_document_id', 'not', null);
$this->addCondition('transfer_document_id', 'is not', null);
}

$this->addField('destination_account_id', ['never_persist' => true]);
Expand Down