Enh: Introduce FindInstanceInterface - Part 1.a: Creating base classes

[martin-rueegg]

Introduce FindInstanceInterface

Part 1.a Creating base classes

FindInstanceInterface series:

• Enh: Introduce FindInstanceInterface - Part 1.a: Creating base classes #6503
• Enh: Introduce FindInstanceInterface - Part 1.b: TagDependency  #6528
• Enh: Introduce FindInstanceInterface - Part 2: Implementing throughout the code #6463

Functionality

$model::FindInstance($identifier, ?iterable $simpleCondition = null) is the main new method in this PR. It provides a simplified way to get a single instance of a model that is implementing FindInstanceInterface. It allows:

• Retrieval of a single $model::class instance by
  • providing
    • the instance itself (which is then simply returned)
    • the primary key
    • any other unique identifier, eg. ['object_model' => some::class. 'object_id' => 123]
    • another string identifier, e.g. guid, if it is defined in the model's implementation (and if the primary key is not a string itself)
    • an additional simple condition (key-value-pairs), e.g. for status or category columns
  • returning
    • a model instance, or
    • null if record does not exist (or does not pass the "simple condition")
• Caching the instance in the RuntimeCache (RTC) after first retrieval using the key variants from
  • UniqueIdentifiersInterface::getUniqueIdVariants(), if available, or
  • RuntimeCacheHelper::buildUniqueIDs() as a fallback with some sane defaults: using
    • the $model->getPrimaryKey() (if a BaseActiveRecord),
    • the properties $model->id and $model->guid (if they exist), and
    • the result from $model->getUniqueId() (if exists).
• Retrieving the instance from the RTC, rather than the DB, if
  • it has been retrieved before
  • the provided keys correspond to one of the $model::getUniqueIdVariants() (see previous point for details) once normalised.

Implementation

A model has to either

• derive from CachedActiveRecord, or must
• implement FindInstanceInterface directly.

Deriving from CachedActiveRecord

Just deriving from CachedActiveRecord should cover the basic functionality. However, some adjustments can be considered by overriding the following methods:

• findInstance(): Useful if you want do define a default value (e.g. for the current user in Users) or to allow a special key combination (e.g. [$space_id, $user_id] for Membership) or something along those lines.
• validateInstanceIdentifier(): Useful to specify a default key for string identifiers, e.g. guid as used in Space
• getUniqueIdVariants(): Useful to provide additional key variants used to store the record in the cache, see Content
• afterSave() and afterDelete(): Useful to delete the cache before performing additional steps, as done in Content
  1. Delete the cache: RuntimeCacheHelper::deleteVariants($this);
  2. Perform additinal steps
  3. Run the parent's afterSave, but skipping deleting the cache again: ActiveRecord::afterSave($insert, $changedAttributes);
     Please note, that this works if you directly inherit from CachedActiveRecord or any intermediate class does not override the afterSave() method. So please double check.

Please also see notes on Implementing FindInstanceInterface below.

Implementing FindInstanceInterface

For this, one may use the FindInstanceTrait as a reference implementation or include it directly. In the latter case, the following functions may have to be "redirected" to the trait if additional functionality needs to be added (ContentContainer may serve as an example here):

• find() should return an instance of CacheableActiveQuery
• afterSave() should delete the current record from cache
• afterDelete() should delete the current record from cache
• updateAll() should delete all affected records from cache, or the entire cache
• deleteAll() should delete all affected records from cache, or the entire cache

Please also see notes on Deriving from CachedActiveRecord above.

Background

Since #6457 was only a partial solution for #6375 as it

1. uses a non-standard name for findMembership and
2. deletes the cache only when memberships are updated, and
3. does the latter in an insufficient way (ignores both deleteAll() and saveAll() and linked content),

there is an improved attempt in #6463 to address this in a broader way, and ultimately #6482, which addresses some more issues.

To break #6463 down, I've created this PR for review, and I've submitted towards the statable branch, as #6430 also bilds on top of #6463. Once, #6463 is merged in statable too, both can be merged int develop, which I recommend to happen before v1.15 is released.

PR Admin

What kind of change does this PR introduce?

• Bugfix
• Feature
• Refactor

Does this PR introduce a breaking change?

• No. However, module developers SHOULD adapt to use the new implementation of findInstance, rather than findOne where the searched object is only identified by either id or guid (see additional examples in Enh: Introduce FindInstanceInterface - Part 2: Implementing throughout the code #6463).

The PR fulfills these requirements:

• It's submitted to the develop branch, not the master branch if no hotfix
• When resolving a specific issue, it's referenced in the PR's description (e.g. Fix #xxx[,#xxx], where "xxx" is the Github issue number)
• All tests are passing
• New/updated tests are included
• Changelog was modified

[martin-rueegg]

@luke- could you please update statable with develop? Thank you!

@gevorgmansuryan this is the first step towards #6482, hence I'm including you here, too!

[martin-rueegg]

Originally posted by @gevorgmansuryan in #6457 (comment):

@luke- Regarding runtimeCache potential issues we need to discuss. Adding cache delete in all model's updating lifecycle methods is not a good solution. Maybe we create some behavior that will handle it all automatically.

I've been looking a bit into the possibility of using a behavior here. It looks like an elegant solution at first, specially with the idea that behaviors can be attached and detached to a component dynamically without requiring modification of the component class.

However, I have not found a way to actually do attach a behavior to let's say a ActiveRecord instance without modifying the thing. The reason being, that there is no event called when objects are instantiated so that I could listen to that event and then attach the behaviour. So we would have to

• find a reliable event to be able to do that
• create our own version of \Yii that does fire an event e.g \Yii::EVENT_BEFORE_CREATE_OBJECT and \Yii::EVENT_AFTER_CREATE_OBJECT (which would be a great thing to introduce anyway!)

But with that behavior we would have to listen to events again (after save and update) and we cannot add static functions (as findInstance()) AFAIK.

So we could do the cache cleaning using a behavior, but we would still need to change the classes in order to implement findInstance() method.

Or is there anything I've missed here?

[luke-]

@martin-rueegg

So we could do the cache cleaning using a behavior, but we would still need to change the classes in order to implement findInstance() method.

Or is there anything I've missed here?

Yes, I don't think we can add e.g. a SomeAR::findInstance(1) method just using a behavior.

---

In general, I frankly don't know if the current solution, which is really very nicely worked out, doesn't introduce too much new complexity and non standard (Yii2) behavior for me.

I basically like the idea of a Model::findInstance($pkOrInstance) apprach.

But I would have liked to implement this with 2-3 new methods.

This PR really covers a lot of cases: Relations, ::deleteAll(), Conditions, Multi PKs, Options like (exceptionMessageSuffix, exception type) etc.

When I look at the "Membership" model as an example, I really have trouble understanding right away what exactly is going on in the ::findInstance() method for example. Also the introduction of new relations hasOne() to hasOneCached() really requires deeper understanding.

Currently, I think it's good that we rely very heavily on well-documented Yii 2 standard components. If we now start to change or introduce other approaches, we have to document it somehow but also maintain it in the long run.

So I would prefer to introduce the simplest/clearest possible caching, which may only covers 80% of the cases, rather than something complex to achieve a bit higher coverage.

For example, it is probably clear for most Yii 2 developers that deleteAll() or updateAll() do not execute events/before/after/validate and should therefore only be executed very selectively. So if no runtime cache is cleared here, I would be perfectly fine with that.

I'm not sure if I'm being too critical here... maybe we should discuss this with the team...

[martin-rueegg]

@luke- Thank you for your review and comments!

I'm not sure if I'm being too critical here... maybe we should discuss this with the team...

I guess that's your job and I'm grateful you are a critical reviewer!

This PR really covers a lot of cases: Relations, ::deleteAll(), Conditions, Multi PKs, Options like (exceptionMessageSuffix, exception type) etc.

Yes, that's true. Some explanatory thoughts:

• Relations:
  Yep, a creative apprach. Motivation was that a lot of record lookups happen through this standard way of getting objects, particularly to get Content or a ContentActiveRecord. However, it could be probably reduced in the sense that only the result of the lookup is cached, not the ::getMyActiveRecord() itself modified.
• deleteAll/updateAll:
  The thing is, that this is the same as updateAll() (in terms of what needs to be done cache-wise). And updateAll() is used by \yii\db\BaseActiveRecord::updateAttributes() which in turn is used in different places (e.g. (un)pinning content, but first and foremost in updating the status [currently softDelete()]). This would have to be discouraged.
• Conditions:
  came along since quite often, a very specific item is looked for, but only if a given status or visibility is given. Now, since these two criteria are looked up repeatedly it still does make sense to cache the object to avoid further lookups. Hence the specifically called $simpleCondition, as it only matches properties and does not support string conditions or ConditionExpressions as in eg findByCondition() would.
• Multi-PK:
  Yes, I don't see this as a big issue as it draws on standard Yii capabilities. The Membership is an exception here, as it
  1. uses a unique key (not in an db-enforced sense) that is NOT the PK.
  2. the identifiers themselves vary accross the code and are sometimes User objects or IDs. This could be simplified by requiring the calling code to do something like
     Membership::findInstance([Space::findInstanceAsId($space), User::findInstanceAsId($user])) rather than just
     Membership::findInstance([$space, $user]).
     However, I hope to have made the code more understandable now by adding some comments
• Option exception type:
  The idea was to be able to throw a HTTP 404/403 exception, if a record is not available. But this is an edge case. Code has been removed.
• Option exceptionMessageSuffix:
  This is just to be kind with developers, showing them what they've missed. The suffix is alredy supported by the given Exception and so I added it. Can be removed obviously, or gently ignored. :-)

When I look at the "Membership" model as an example, I really have trouble understanding right away what exactly is going on in the ::findInstance() method for example.

I hope, my new comments in the code help. See also above.

Also the introduction of new relations hasOne() to hasOneCached() really requires deeper understanding.

Agree. hasOneCached() can be discarded! See new commits.

Currently, I think it's good that we rely very heavily on well-documented Yii 2 standard components. If we now start to change or introduce other approaches, we have to document it somehow but also maintain it in the long run.

Sure.

So I would prefer to introduce the simplest/clearest possible caching, which may only covers 80% of the cases, rather than something complex to achieve a bit higher coverage.

From: #6463 (comment)
However, for me arises the question: why not using the default db-caching mechanism from Yii2 and just deal with it's clearing as we do in the runtimeCache?

Was that looked into? I mean, why not using the built-in db-query cache?

For example, it is probably clear for most Yii 2 developers that deleteAll() or updateAll() do not execute events/before/after/validate and should therefore only be executed very selectively. So if no runtime cache is cleared here, I would be perfectly fine with that.

See my comments above. I don't think we can ignore the updateAll unless some code is re-manufactured to avoid it. But changing a state (soft-delete so far) can change the result that has to be returned from an ajax call for example. Having that one record in the cache, that has been updated by the updateAll via the updateAttributes, will completely ignore this update (that happened in the same request).

maybe we should discuss this with the team...

Sure. Appreciate any feedback!

[luke-]

@martin-rueegg Thanks for the response. I'm soon in a meeting and need some time to reply. In the meantime an untested/buggy but totally simple/stupid example of a findInstance() implementation which would I prefer.

64f0fcc

Of course there is (or it need) much room for improvements. (Interface, Behavior, Trait, Multi PK,..........)

[martin-rueegg]

@luke- Thanks! I had a look into the dependency stuff but did not get my head around. That's helpful in your example. Well, helpful in the sense of understanding the usage of dependencies. However, since we do not currently serialize the cached values, the dependency would simply be ignored: $dependency "This parameter is ignored if $serializer is false." We would either have to change that and serialize stuff (creating overhead in serialization and unserialization and potential sideeffects that would have to be addressed in __sleep() and __wakeup() methods), or create an implementation that still deals with dependencies. (My own implementation in #6482 does that in a different way.)

But, yes, I get your point. And it could be a reasonable solution to avoid invalid objects in the store (if we can solve this with the dependency).

Things I see unresolved:

1. Same object is retrieved by different keys:

   • id
   • guid
   • object_model + object_id
   • space_id + user_id.

   One could easily argue, that

   1. we only store PKs. Which would mean disabling cache for Content, ContentContainer and Membership for sure as they are usually retrieved using a combination of fields that do not constitute the PK.
      While Content is cached already in the PolymorphicRelation, the latter two record classes are widely used repeatedly for the same values (hence caching makes most sense).
   2. the record is then retrieved two or three times, at most. Yes, that would be ok. But again, the clearing is the issue here. If we could get dependency work, we could always add two tags, one for the class name and one for the unique identifier using the PK. However, here we could no longer use the getOrSet() method, as we would know the PK only once we have the record in hand. This would require coding the logic of getOrSet into our function, which is of course not difficult, but easily adding a few lines.

2. Related record retrieval is not cached. Although, caching it is actually quite simple.

3. Related records are not cleared (e.g. if an Content is soft deleted, then the Post record needs to be removed from the cache, as otherwise the Post:getInstance() would return the thing although it should not.

4. If you add a bit of type and other validity checks, the complexity of the function will soon reach the one of findInstanceHelper.

I'm getting the impression, we're trying to reduce complexity so much but missing the targeting by doing so: If for the classes where the most repeated calls are expected, we do not have a cache, then for the others, we need to cache the objects often twice, since it's first accessed by one key, and then the other, then we create more overhead than we solve.

Also, your draft is 60 lines long, and it will also require some other 10 lines for the missing findOne() method and some other 20-30 lines for validation.. My version is roughly 150 Lines plus roughly another 110 lines in CacheableActiveQuery. Making the ratio being somewhere around 100:260. We're not talking about an exorbitant complexity here. :-) However, the coverage of cases where the cache becomes useful seems to be much better, given the limitations laid out above ...

Of course, we could gain some lines by removing some options, or the exception suffix, or some other small stuff. But while saving a few lines, we have to add back those checks in the originating code. In the end, that's making the change for the 181 lines we save with this PR. If you deduct those from my 260 lines above, I might still end up better with only about 80 net plus. :-)

[luke-]

@martin-rueegg Thanks for the points, it definitely makes things clearer to me. Unfortunately, I have not enough time right now, come back to you in the next days... have a nice weekend!

[luke-]

@martin-rueegg Thank you for your patience.

In my example, it's not just about the sheer number of lines but also about complexity.

Some thoughts:

• If a feature is already quite complex, I'd prefer to omit certain options, even if it means adding a few more lines of code elsewhere. The key is to maintain readability and understandability.
• RuntimeCacheHelper::cacheProcessVariants is quite extensive, with multiple $actions, etc. (I haven't had time to understand it in detail yet.)
• e.g. if I want to use this in a custom model, I'd need to add FindInstanceInterface, FindInstanceTrait, possibly pass afterSave() methods to the trait, and also implement a MyRecord::findInstance() with options. If a custom ActiveQuery is used, it must also be derived from CacheableActiveQuery. We probably need documentation here.

However, I understand that it makes sense to create a solution that covers as many cases as possible, even if it makes it more extensive.

To ensure I understand all the challenges, here's a list:

• Instance retrieval based on:
  • Primary Key (PK),
  • Object,
  • Multi PK (e.g., Membership user_id, space_id),
  • (Multi) Custom Attributes (e.g., Content object_id, object_model, or just guid),
  • simpleCondition - is it actually used? (Side node: e.g. deleteAll(condition) not always uses simple Conditions)
  • Additional configurations (Default Value, Nullable, with Caching, String vs. Int Lookup Fields)
• Cleaning up cache entries based on multiple cache keys and dependencies.
• Automatic caching of related records (relations).

Here are some suggestions:

1. Currently, the solution is spread across many classes. Do you think it's possible to create a humhub\components\CachedActiveRecord alongside humhub\components\ActiveRecord, which bundles the trait, helper, interface, and query? I know too many layers of inheritance can be problematic, but in this case, it might work well. Additionally, methods like afterSave() could simply run via parent::afterSave().
2. For me, there are too many options and minor additional features that bloat the code and make it challenging to review.
3. Perhaps it makes sense to be more pragmatic in some places. The runtime cache is only used during a request. For instance, if we invalidate the entire cache while deleting a record and avoid complex cache dependencies, that would be fine for me.

[martin-rueegg]

@luke- Thank you for your time to review this and for your comments.

Some thoughts:

• If a feature is already quite complex, I'd prefer to omit certain options, even if it means adding a few more lines of code elsewhere. The key is to maintain readability and understandability.

I understand.

• RuntimeCacheHelper::cacheProcessVariants is quite extensive, with multiple $actions, etc. (I haven't had time to understand it in detail yet.)

You are apparently referring to the follow-up PR

• Enh: Introduce FindInstanceInterface - Part 1.b: TagDependency  #6528

I'm happy to add some explanatory comments there.

In the version of this PR (#6503), the $action is either set or delete (in most cases, the latter). So in order to apply the above principle, the code could be split into two functions, e.g. cacheSetVariants and cacheDeleteVariants, which may it make less confusing.

PR #6528 on the other hand uses tag dependencies as suggested by your example. I find this also a bit too complicated and yii's TagDependency implementation seems to be too sophisticated for our purpose (which should also be super performant, given it's a runtime cache). I would opt for the middle way of the current PR and in case the deleteAll() or updateAll() function uses a condition that is not only the PK, then flush the entire cache as suggested by you.

• e.g. if I want to use this in a custom model, I'd need to add FindInstanceInterface, FindInstanceTrait, possibly pass afterSave() methods to the trait, and also implement a MyRecord::findInstance() with options. If a custom ActiveQuery is used, it must also be derived from CacheableActiveQuery. We probably need documentation here.

Yes, documentation has been left aside so far. I appreciate that documentation would be helpful for reviewing. On the other hand, writing a lot of documentation for something that is not yet in principle approved, is also very time consuming. I'll do my best to be more verbose in future PRs to make reviewing easier. Otherwise maybe a review on a video call might also safe some time?

To ensure I understand all the challenges, here's a list:

• Instance retrieval based on:
  • Primary Key (PK),
  • Object,
  • Multi PK (e.g., Membership user_id, space_id),

Actually, this is not the PK, but a non-enforced unique key. (Side note: on DB level this is not enforced. There isn't even a index on this combination in neither direction.)

• (Multi) Custom Attributes (e.g., Content object_id, object_model, or just guid),
• simpleCondition - is it actually used? (Side node: e.g. deleteAll(condition) not always uses simple Conditions)

Yes, it is used. However, it is only used in follow-up PRs, where the implementation of findInstance() is applied throughout the code.

The simpleCondition is only used for retrieval. A condition is considered "simple" if it is an array of simple key => value-pairs, where key must be a string and value a scalar. This is now validated and if this condition for simpleCondition is not met

• findeInstance() throws an error
• deleteAll() does delete the entire cache

• Additional configurations (Default Value, Nullable, with Caching, String vs. Int Lookup Fields)

• Default Value (onEmpty): is used, if the value is not found in the db or does not match the simpleCondition. Defaults to null.
• nullable has been removed. It is now only used for the Exception message, but this could be dropped, as the only place where it is used it for User, where a null value for $identifier would return the current user.
• with Caching (cached): is never used. Maybe we could remove it? If the calling code need to do this, it could either
  • first clear the key from the cache, or
  • get it with find() and then update the cache
• String vs. Int Lookup Fields: is the same as the PK/Custom Attributes. See below for further explanation.

In the current implementation there is a slight distinction between "record retrieval" (i.e. findInstance()) and "cache management".

record retrieval (findInstance())
The current implementation allows the retrieval based on

• an instance of the model (which is simply returned. This is for convenience only and helps avoiding redundant checks in the calling code)
• an integer key (defaulting to id)
• optionally a string key (if defined by $config['stringKey'])
• a composite-field: an array of arbitrary field => $value-pairs, where it is assumed that the concatenation of the values form a unique identifier within the table (without including the field names and without colliding with the integer field or string field mentioned above). However, it is not required to be a PK. This is used for e.g., Membership (space_id-user_id) and Content (object_model-object_id) (in both cases it's not the PK, but just a unique key [UK]).

cache management
In order to make caching as efficient as possible, records are saved under different keys, once retrieved from the database. This is because records are often retrieved by their ID or GUID/composite-field. Through this mechanism, the record is retrieved once, rather than two or three times.

Now this mechanism is somewhat independent of the configuration that is used to retrieve the record. Maybe this could be streamlined by using a function like cacheKeys() (similar to tableName() or attributes()) to define those fields. But for now the record is automatically saved under the keys using the values from the

• properties id and guid (if they exist),
• the PK (if the object is an instance of BaseActiveRecord)

In case the record is removed from the cache, some additional steps are performed:

• if the record is an instance of ContentActiveRecord, the Content is also removed from cache
• if the record has a polymorphic relation, the related record is also removed from cache

• Cleaning up cache entries based on multiple cache keys and dependencies.

yes.

• Automatic caching of related records (relations).

correct.

Here are some suggestions:

1. Currently, the solution is spread across many classes. Do you think it's possible to create a humhub\components\CachedActiveRecord alongside humhub\components\ActiveRecord, which bundles the trait, helper, interface, and query? I know too many layers of inheritance can be problematic, but in this case, it might work well. Additionally, methods like afterSave() could simply run via parent::afterSave().

This could be a good solution, yes. There is one implication: \humhub\modules\content\models\ContentContainer extends \yii\db\ActiveRecord, rather than \humhub\components\ActiveRecord. Is there a reason for this? We have two options:

1. We keep the the code in FindInstanceTrait and link this both to ContentContainer and CachedActiveRecord. From the latter we then derive the other classes like ContentActiveRecord, Space, User, etc.
2. We derive ContentContainer also from CachedActiveRecord (and hence humhub\components\ActiveRecord) but would have to check if there are some methods we have to direct to \yii\db\ActiveRecord (jumping humhub\components\ActiveRecord).

2. For me, there are too many options and minor additional features that bloat the code and make it challenging to review.

Which ones would that be?

I could see the following simplifications:

• $config['cached'] could be removed (see above).
• $config['onEmpty'] could be removed and we always return null if the record is not found.
• $config['intKey'] and $config['stringKey'] could also be removed and replaced by the following logic:
  • if $identifier is a scalar (or an integer-indexed array) it must match the PK
  • otherwise it must be a [column => value]-array.
  • Exceptions can still be implemented as it is in Membership::findInstance(), where the integer-indexed array [$space, $user] is valid, although it's not the PK.
  • Down-side: determining if a supplied $identifier is an interger (e.g. for id-field) or a string (e.g. for guid-field) has to be done in the implementing findInstance() eg. like so (simplified):

    public static function findInstance($identifier, iterable $simpleCondition = []): ?self
    {
        switch (self::validateInstanceIdentifier($identifier)) {
            case 1: // instance of self
                return $identifier;
    
            case 2: // int
            case 4: // array
                return parent::findInstance($identifier, $simpleCondition);
    
            case 3: // string
                return parent::findInstance(['guid' => $identifier], $simpleCondition);
    
            case 0: // null
                throw new \Exception('Null is not allowed.');
    
            default:
                throw new \Exception('Invalid argument');
        }
    }
    
    protected static function validateInstanceIdentifier(&$identifier)
    {
        if ($identifier === null) {
            return 0;
        }
    
        if ($identifier instanceof static) {
            return 1;
        }
    
        if (is_int($identifier)) {
            return 2;
        }
    
        if (is_string($identifier)) {
            $identifier = trim($identifier);
    
            if ($identifier === '') {
                $identifier = null;
                return 0;
            }
    
            $id = filter_var($identifier, FILTER_VALIDATE_INT, FILTER_NULL_ON_FAILURE);
    
            if ($id !== null) {
                $identifier = $id;
                return 2;
            }
    
            return 3;
        }
    
        if (is_int($identifier)) {
            return 2;
        }
    
        if (is_array($identifier)) {
            if (count($identifier) === 0) {
                $identifier = null;
                return 0;
            }
    
            return 4;
        }
    
        return -1;
    }

• as such, the entire $config-parameter could be omitted. Maybe that is more readable?

3. Perhaps it makes sense to be more pragmatic in some places. The runtime cache is only used during a request. For instance, if we invalidate the entire cache while deleting a record and avoid complex cache dependencies, that would be fine for me.

The deletion of the cache would also have to happen on every update, though. Since we have flags that would allow the return of the record (e.g. soft delete) or it's related records.

Given that we currently store the record under different keys, the deletion of those keys is not really a big issue. Also, the deletion of the related records are just a few lines of code in cacheProcessVariants, which I could maybe rewrite in a less compact manner so it gets more readable. If we do not want to store the records under any variant, but rather retrieve it several times, then, of course, this code could be omitted.

I guess the biggest simplification is done by omitting the TagDependency that is implemented in #6528 (Part 1 b). Either we can identify the record, or we delete the entire cache. In most cases, deleteAll() and updateAll() use the PK as identifier anyway. So it should not be a big deal.

[martin-rueegg]

@luke- @gevorgmansuryan

Before we can really continue with this,

• Enable RuntimeCache during tests #6527 needs to be addressed
• develop should be merged into statable