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.

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

[WIP] Redis ActiveRecord #905

Merged
merged 62 commits into from
Nov 23, 2013
Merged

[WIP] Redis ActiveRecord #905

merged 62 commits into from
Nov 23, 2013

Conversation

cebe
Copy link
Member

@cebe cebe commented Sep 25, 2013

This is WIP redis ActiveRecord support.
It has the following features:

  • insert, update, delete interface is exactly the same as db\ActiveRecord
  • relation, relations via (except relations viaTable because there are no tables in a key value store ;) )
  • optimistic locking
  • dirty attributes
  • find() has the same interface as db\ActiveRecord except a few limitations. It does not support select(), from() join(), group(), having(), union()
  • complex where conditions via Lua Script which is executed on redis server and will return matching records.

TODO

  • support for orderBy(). should be possible to implement via Lua script on server side.
  • support LIKE conditions. also possible via pattern matching in Lua script.
  • Refactor ActiveRelation in db\AR
    • currently ActiveRelation class is hardcoded in db\AR. redis uses its own implementation which is nearly copy&paste from the db ActiveRelation class but it extends from the custom ActiveQuery.
    • One possible soution could be an interface for ActiveRelation
    • Another solution would be to define the ActiveRelation class via static property in AR class.
    • 6133133 adds a dependency db\AR -> redis\AR which makes it all work but needs to be refactored to remove the dependency.

A redis AR can be created like this:

class Customer extends yii\redis\ActiveRecord
{
    // relation declaration is the same as in db\AR
    /**
     * @return \yii\redis\ActiveRelation
     */
    public function getOrders()
    {
        return $this->hasMany('Order', array('customer_id' => 'id'));
    }

    // also scopes work the same as in db\AR but with limited query options (where is fully supported)
    public static function active($query)
    {
        $query->andWhere(array('status' => 1));
    }
    // this is the only new thing. you have to define attributes and pk yourself as there is no schema in redis
    public static function attributes()
    {
        return ['id', 'email', 'name', 'address', 'status']:
    }

       // primaryKey is "id" by default, can be customized by overriding primaryKey method (can be composite key)
}

cebe and others added 30 commits February 1, 2013 23:29
* master: (117 commits)
  More Model tests
  Better validation rules validity check
  Some tests for Model
  A bit more friendly behavior for unsetting a model attribute
  User WIP.
  Fixed Model::getFirstErrors()
  Finished AccessControl.
  Finished HttpCache.
  User WIP
  line ending fix.
  refactoring cache and db references.
  Fixed session bug.
  Finishes flash feature.
  refactored url creation shortcut method.
  HttpCache WIP.
  Finished AccessControl and HttpCache.
  Finished PageCache.
  finished fragment caching.
  fragment cache WIP
  bug fixes.
  ...
* origin/master: (104 commits)
  Fixed typo in query builder and reverted changes to DbCache.php
  added note about enabling APC cache for CLI
  more assertions for cache test
  fixed dbcache multiget
  fixed Html test under Windows (line endings)
  fixed DB quoting typo
  create a webapp in test bootstrap
  updated expected exception message
  Added a basic app.
  initial implementation of asset command.
  script WIP
  script command WIP
  Fixed bug in yiic.php. Refactoring AssetConverter.
  adjusted default app layout
  refactoring and documentation for asset/script management.
  Finished asset converter.
  removed unnecessary code, added comment about displaying errors
  better handling of errors during rendering an error
  safer exception rendering
  turn asset manager into a getter.
  ...
* master: (27 commits)
  form wip
  form wip
  form WIP
  script WIP
  Minor change about twig alias.
  Added Controller::populate().
  Added bootstrap. form WIP
  form wip
  form wip
  Refactored UrlManager.
  refactored AccessControl
  activeform WIP
  Removed the $value parameter from radio and checkbox.
  Support alternative URL rule syntax.
  Support default standard component class.
  updated view renderers docs
  activeform wip
  Refactored view renderers.
  updated view renderers docs
  refactoring activeform.
  ...
testExpire is failing on PHP 5.3.10
* 'master' of github.com:yiisoft/yii2: (87 commits)
  Added dirty attribute description.
  Fixed doc about renderers.
  Finished the initial draft of upgrading instructions
  Fix attaching behavior via config
  Changed default value of View::renderers.
  Fix attaching behavior via config
  Menu WIP
  upgrading instructions WIP
  Refactored Breadcrumbs.
  Fixes issue #54: Implemented Breadcrumbs.
  Added Breadcrumbs.php
  Fixes issue #134
  Rollback word consistencty over entire codebase (ref. #139).
  Add ensureBehaviors() to detachBehavior*()
  Fixes issue #124.
  code reorganization fix.
  reorganized app code. removed app template from framework folder.
  Fixes issue #128.
  Fixes issue #124.
  Add Newlines
  ...

Conflicts:
	tests/unit/framework/caching/ApcCacheTest.php
* master: (108 commits)
  fixed indentation in test files
  cleanup tests
  Travis-CI build status.
  CS fixes.
  CS fixes.
  Change magic property to normal method. (Thanks to @maxlapko.)
  CS fixes.
  Fixed duplicated merge.
  Fixes issue #50: implemented pager widgets.
  fix: remove pointless unset
  remove fix for unset timezone from bootstrap app
  mod: don't use ArrayHelper to avoid loading another class during every request
  better have timezone fix in application
  Add exception classes hierarchy UML diagram.
  Initial Gettext support.
  RBAC sql files identation fix
  RBAC sql files
  DbManager some Db fields renaming
  DbManager code style fix + right Exception type fix
  Unit tests fix
  ...

Conflicts:
	tests/unit/framework/caching/ApcCacheTest.php
	tests/unit/framework/caching/CacheTest.php
* master: (179 commits)
  Fixes #312. Additional docs on IDN in EmailValidator and UrlValidator.
  bug fix.
  Added composer extension.
  Added psr-0.
  Fixed breaking test.
  Fixed iii include path.
  reorganized the main repo to satisfy PSR-0.
  Fixed bootstrap asset registration issue.
  Widget::$transition removed, bundles depency fixed
  'yii/bootstrap/popover' bundle depency optimization
  Bundle names fixes, base widget fixes, other widgets fixes
  Fixed join query for AR.
  Responsive bundle depency fix
  Bundle names and depency fixes
  YiiBase move all after class definition
  YiiBase
  Reloaded
  Additional bootstrap packages
  New approach
  minor refactoring.
  ...

Conflicts:
	tests/unit/framework/caching/ApcCacheTest.php
	tests/unit/framework/caching/CacheTest.php
* master: (806 commits)
  avoid confusing docs about autoload return value
  doc fix.
  Fixed doc.
  Fixed autoloader behavior according to leading \
  Fix in RequestPanel
  changed to trace from info.
  Removed yii\debug\Module::enabled.
  minor enhancement of debugger.
  Refactored the basic app.
  Added ErrorAction.
  Use `hasAttribute()` instead to avoid code duplication
  refactored NavBar and basic app navbar.
  test break fix.
  test break fix.
  Fixed test breaks.
  #735: removed hiddenInput to avoid confusion.
  ResizableAsset typo fix!
  Update radio and checkbox due to e3801fb
  css fix.
  Fixed the basic app template.
  ...

Conflicts:
	.travis.yml
	tests/unit/data/config.php
* master: (131 commits)
  css fix.
  Finished model generator.
  more tests for FileHelper
  Added note about theme asset bundle to bootstrap widgets guide
  fixed typos in ArrayHelperBase phpdoc
  More tests for ArrayHelper and Inflector
  Fixed typo and code style
  Fixes #21: implemented jQueryUI Slider
  Fixes #790: added visible for Nav and Dropdown
  finished rule generation for model generator.
  bug fix of form generator.
  minor fixes of debugger.
  Added SafeValidator.
  fix dataProvider getSort()
  cleanup MemCache timeout API after #804
  Added failureCallback, reduced timeouts to 1s
  Mentioning that timeoutms is available in memcache only.
  set default format for gridview to text
  Removed extra line
  Added timeoutms parameter in MemCache
  ...

Conflicts:
	tests/unit/data/config.php
- introduced RecordSchema class for schema definition
This was referenced Sep 25, 2013
@@ -386,7 +386,7 @@ public function __get($name)
return $this->_related[$t];
}
$value = parent::__get($name);
if ($value instanceof ActiveRelation) {
if ($value instanceof ActiveRelation || $value instanceof \yii\redis\ActiveRelation) { // TODO this should be done differently remove dep on redis
Copy link
Member

Choose a reason for hiding this comment

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

ActiveRelationInterface?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, needs to be defined. WIll discuss this with qiang when he has time.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.18%) when pulling b95c056 on redis into 7d2d925 on master.

@qiangxue
Copy link
Member

From the test cases, the usage design looks good to me. One suggestion about ActiveRecord::getRecordSchema(): perhaps this can be replaced by introducing an abstract static method columns() and making both primaryKey() and tableName() static.

Also, why do we need to store the table schema cache in ActiveRecord instead of Connection?

I need more time to think about the class/interface designs.

@cebe
Copy link
Member Author

cebe commented Sep 26, 2013

One suggestion about ActiveRecord::getRecordSchema(): perhaps this can be replaced by introducing an abstract static method columns() and making both primaryKey() and tableName() static.

I tried that once but failed with that. Currently don't remember why. The main reason for recordSchema is that AR internals rely on getTableSchema() in many places. Will try again and see how it works.

Also, why do we need to store the table schema cache in ActiveRecord instead of Connection?

You can use the Connection without having AR. When you use redis itself there is no such thing as a table as redis is a key-value-store so I decided to leave this in AR.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 179618c on redis into 7d2d925 on master.

* master: (613 commits)
  fixed typo
  Fixes #1222: refactored jui/Widget, intorduced jui/Slider and jui/SliderInput
  Correct Nav.php comments/PHP doc to reflect BS3 dropdown support.
  Correct Nav.php comments/PHP doc to reflect BS3 dropdown support.
  Correct Nav.php comments/PHP doc to reflect BS3 dropdown support.
  Added example for dividers to bootstrap Nav
  Checkboxlist documentation fix
  Added "Using controller action to render errors"
  Fix doc
  Renamed DetailView attribute type to format
  encode email in Formatter
  Added default status code setting.
  "yii\swiftmailer\Mailer" transport setup has been advanced to support constructor arguments and plugins.
  fix rbac select statement
  Comments cleanup.
  Reverted closeButton
  Nomenclature and code realignment.
  better nginx config
  guide about using bootstrap less files
  Include Schema in new migrations by default
  ...

Conflicts:
	framework/yii/db/ActiveRecord.php
	framework/yii/db/ActiveRelation.php
	tests/unit/data/config.php
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling bc4324c on redis into 704f910 on master.

- make use of traits
- short array
- better implementation of query findByPk
@coveralls
Copy link

Coverage Status

Coverage increased (+0.46%) when pulling 8542448 on redis into 704f910 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling cb4504a on redis into 704f910 on master.

@cebe
Copy link
Member Author

cebe commented Nov 22, 2013

Reworked the complete redis implementation. From my point it is ready for merge. We can add support for orderBy and Like conditions later if we find need and time for it.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.78%) when pulling 6995e8d on redis into 704f910 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.8%) when pulling 6995e8d on redis into 704f910 on master.

@qiangxue
Copy link
Member

I have reviewed the code. It looks great to me!

So will you still abstract out common part of ActiveRecord into interface and trait?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.46%) when pulling ed98df5 on redis into 704f910 on master.

@cebe
Copy link
Member Author

cebe commented Nov 22, 2013

Will do that when I have an idea of how eleasticsearch will work. Elasticsearch does not store the primarykey among the attributes but in an extra _id field. This currently messes up my implementation and is not the only point that is different.
Imo it's better to extract common parts when we have multiple systems that have something in common to see what that common parts actually are.

qiangxue added a commit that referenced this pull request Nov 23, 2013
[WIP] Redis ActiveRecord
@qiangxue qiangxue merged commit a95d54c into master Nov 23, 2013
@qiangxue qiangxue deleted the redis branch November 23, 2013 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants