OpenPNE 3

下記チケットのコミットが邪魔なため revert する．

#2478 「プロフィール項目で「単一選択」の状態で保存したあとに「複数選択」の状態で保存を行おうとしても保存されていないように見える」

$ git revert f84facbf5ea0f7433881b5ccfd6735763ecf61bc

原因調査¶

mysql> select * from member_profile where profile_id = 5;
+----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+
| id | member_id | profile_id | profile_option_id | value | value_datetime | public_flag | tree_key | lft  | rgt  | level | created_at          | updated_at          |
+----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+
| 20 |         3 |          5 |              NULL |       | NULL           |        NULL |       20 |    1 |    4 |     0 | 2013-01-29 22:38:47 | 2013-01-29 22:38:47 |
| 21 |         3 |          5 |                 2 |       | NULL           |        NULL |       20 |    2 |    3 |     1 | 2013-01-29 22:38:47 | 2013-01-29 22:38:47 |
+----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+
2 rows in set (0.00 sec)

下記のような構造となっている．

1----4
 2--3

ここで proifle_option_id = 2 となるようなプロフィール項目を削除すると下記のようなデータになる．

mysql> select * from member_profile where profile_id = 5;
+----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+
| id | member_id | profile_id | profile_option_id | value | value_datetime | public_flag | tree_key | lft  | rgt  | level | created_at          | updated_at          |
+----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+
| 20 |         3 |          5 |              NULL |       | NULL           |        NULL |       20 |    1 |    4 |     0 | 2013-01-29 22:38:47 | 2013-01-29 22:38:47 |
+----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+
1 row in set (0.01 sec)

このとき，手順 5 を実行すると下記のような部分を通る．

lib/form/doctrine/MemberProfileForm.class.php
 57       if (is_null($value['value']))
 58       {
 59         if ($memberProfile)
 60         {
 61           if ($profile->isMultipleSelect())
 62           {
 63             $memberProfile->clearChildren();
 64           }

lib/model/doctrine/MemberProfile.class.php
202   public function clearChildren()
203   {
204     if ($this->getTreeKey() && $this->getNode()->hasChildren())
205     {
206       $children = $this->getNode()->getChildren();
207       $children->delete();
208     }
209   }

lib/vendor/symfony/lib/plugins/sfDoctrinePlugin/lib/vendor/doctrine/Doctrine/Node/NestedSet.php
  56     /**
  57      * test if node has children
  58      *
  59      * @return bool            
  60      */
  61     public function hasChildren()
  62     {
  63         return (($this->getRightValue() - $this->getLeftValue()) > 1);        
  64     }

ここで $this->getRightValue() - $this->getLeftValue() の値は 4 - 1 = 3 > 1 となり， hasChildren() は true を返すが，実際には子ノードは存在しない．そして getChildren() ではオブジェクトが返ってこないため例外が発生する．

この問題自体の懸念は https://redmine.openpne.jp/issues/2684#note-5 で述べていたが，具体的にどのような問題が発生するかはこの時点では不明だった．

取らなければならない対処としては以下の２つがある．

• 子ノードが削除された時に NestedSet における正しい削除を行う．
• すでに NestedSet の構造が壊れているデータに対して修正のマイグレーションを行う

管理画面から「プロフィール選択肢」を削除した場合の処理の流れ¶

apps/pc_backend/modules/profile/actions/actions.class.php
159   public function executeDeleteOption($request)
160   {
161     $this->profileOption = Doctrine::getTable('ProfileOption')->find($request->getParameter('id'));
162     $this->forward404Unless($this->profileOption);
163 
164     if ($request->isMethod('post'))
165     {
166       $request->checkCSRFProtection();
167       $this->profileOption->delete();
168     }
169     $this->redirect('profile/list');
170   }

ProfileOption 自体は単純にレコードを削除しているが，MemberProfile 側も on delete が cascade になっているため単純にをレコードを削除している．
これは NestedSet に対応した削除方法ではないため NestedSet として扱う場合に問題が生じる．

143 MemberProfile:
...
160     ProfileOption: { local: profile_option_id, foreign: id, onDelete: cascade }
<?pre>

修正例¶

diff --git a/apps/pc_backend/modules/profile/actions/actions.class.php b/apps/pc_backend/modules/profile/actions/actions.class.php
index f1f0003..6663243 100644
--- a/apps/pc_backend/modules/profile/actions/actions.class.php
+++ b/apps/pc_backend/modules/profile/actions/actions.class.php
@@ -158,12 +158,19 @@ class profileActions extends sfActions
   */
   public function executeDeleteOption($request)
   {
-    $this->profileOption = Doctrine::getTable('ProfileOption')->find($request->getParameter('id'));
+    $optionId = $request->getParameter('id');
+    $this->profileOption = Doctrine::getTable('ProfileOption')->find($optionId);
     $this->forward404Unless($this->profileOption);

     if ($request->isMethod('post'))
     {
       $request->checkCSRFProtection();
+      $memberProfiles = Doctrine::getTable('MemberProfile')->findByProfileOptionId($optionId);
+      foreach ($memberProfiles as $memberProfile)
+      {
+        $memberProfile->deleteNode();
+      }
+
       $this->profileOption->delete();
     }
     $this->redirect('profile/list');

実験１¶

「プロフィール選択肢」で deleteNode() を用いるようにした場合

初期状態

mysql> select * from member_profile where profile_id = 5;
+----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+------------
---------+
| id | member_id | profile_id | profile_option_id | value | value_datetime | public_flag | tree_key | lft  | rgt  | level | created_at          | updated_at 
         |
+----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+------------
---------+
| 22 |         3 |          5 |              NULL |       | NULL           |        NULL |       22 |    1 |    4 |     0 | 2013-01-30 19:18:36 | 2013-01-30 
19:18:36 |
| 23 |         3 |          5 |                 4 |       | NULL           |        NULL |       22 |    2 |    3 |     1 | 2013-01-30 19:18:36 | 2013-01-30 
19:18:36 |
+----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+------------
---------+

削除

mysql> select * from member_profile where profile_id = 5;
+----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+-----------$
---------+
| id | member_id | profile_id | profile_option_id | value | value_datetime | public_flag | tree_key | lft  | rgt  | level | created_at          | updated_at 
         |
+----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+-----------$
---------+
| 22 |         3 |          5 |              NULL |       | NULL           |        NULL |       22 |    1 |    2 |     0 | 2013-01-30 19:18:36 | 2013-01-30 
19:18:43 |
+----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+-----------$
---------+
1 row in set (0.00 sec)

一見成功しているように見える．

実験２¶

複数選択を複数回更新した場合．下記のような状態になりうる．

mysql> select * from member_profile where profile_id = 5;
+----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+-----------$
---------+
| id | member_id | profile_id | profile_option_id | value | value_datetime | public_flag | tree_key | lft  | rgt  | level | created_at          | updated_at 
         |
+----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+-----------$
---------+
| 22 |         3 |          5 |              NULL |       | NULL           |        NULL |       22 |    1 |    8 |     0 | 2013-01-30 19:18:36 | 2013-01-30 
19:19:23 |
| 26 |         3 |          5 |                 6 |       | NULL           |        NULL |       22 |    6 |    7 |     1 | 2013-01-30 19:19:23 | 2013-01-30 
19:19:23 |
+----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+------------
---------+
2 rows in set (0.00 sec)

削除

mysql> select * from member_profile where profile_id = 5;
+----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+------------
---------+
| id | member_id | profile_id | profile_option_id | value | value_datetime | public_flag | tree_key | lft  | rgt  | level | created_at          | updated_at 
         |
+----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+------------
---------+
| 22 |         3 |          5 |              NULL |       | NULL           |        NULL |       22 |    1 |    6 |     0 | 2013-01-30 19:18:36 | 2013-01-30 
19:19:29 |
+----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+------------
---------+
1 row in set (0.00 sec)

この挙動自体は NestedSet の仕様のため，対応しなければならない内容としては複数選択を複数回保存した場合でも正しいデータになっているようにすることと思われる．

修正例2¶

diff --git a/lib/model/doctrine/MemberProfile.class.php b/lib/model/doctrine/MemberProfile.class.php
index a085613..16bed25 100644
--- a/lib/model/doctrine/MemberProfile.class.php
+++ b/lib/model/doctrine/MemberProfile.class.php
@@ -204,7 +204,10 @@ class MemberProfile extends BaseMemberProfile implements opAccessControlRecordIn
     if ($this->getTreeKey() && $this->getNode()->hasChildren())
     {
       $children = $this->getNode()->getChildren();
-      $children->delete();
+      foreach ($children as $child)
+      {
+        $child->getNode()->delete();
+      }
     }
   }

実験¶

２つを選択して保存．

mysql> select * from member_profile where profile_id = 5;
+----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+
| id | member_id | profile_id | profile_option_id | value | value_datetime | public_flag | tree_key | lft  | rgt  | level | created_at          | updated_at          |
+----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+
| 82 |         3 |          5 |              NULL |       | NULL           |        NULL |       82 |    1 |    6 |     0 | 2013-01-30 20:13:07 | 2013-01-30 20:13:07 |
| 83 |         3 |          5 |                 7 |       | NULL           |        NULL |       82 |    2 |    3 |     1 | 2013-01-30 20:13:07 | 2013-01-30 20:13:07 |
| 84 |         3 |          5 |                 8 |       | NULL           |        NULL |       82 |    4 |    5 |     1 | 2013-01-30 20:13:07 | 2013-01-30 20:13:07 |
+----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+
3 rows in set (0.00 sec)

そのまま保存し直す．

mysql> select * from member_profile where profile_id = 5;
+----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+
| id | member_id | profile_id | profile_option_id | value | value_datetime | public_flag | tree_key | lft  | rgt  | level | created_at          | updated_at          |
+----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+
| 86 |         3 |          5 |              NULL |       | NULL           |        NULL |       86 |    1 |    6 |     0 | 2013-01-30 20:14:08 | 2013-01-30 20:14:28 |
| 88 |         3 |          5 |                 8 |       | NULL           |        NULL |       86 |    2 |    3 |     1 | 2013-01-30 20:14:08 | 2013-01-30 20:14:28 |
| 89 |         3 |          5 |                 7 |       | NULL           |        NULL |       86 |    8 |    9 |     1 | 2013-01-30 20:14:28 | 2013-01-30 20:14:28 |
| 90 |         3 |          5 |                 8 |       | NULL           |        NULL |       86 |    4 |    5 |     1 | 2013-01-30 20:14:28 | 2013-01-30 20:14:28 |
+----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+
4 rows in set (0.00 sec)

原因推測¶

最初に getChildren() をしたときには正しい値をそれぞれ持っているが， 子ノードが削除されるたびに DB 上の rgt, lft が更新されていくがメモリ上の子ノードが持つ rgt, lft の値が更新されず，期待された子ノードが削除されない．

解決策としては下記のようなことが考えられる

1. getChildren() を lft の降順で消してゆけば rgt, lft が変な値を持たずに削除される．
2. 根ノードごと消して作成し直す．

getChildren() は lft の昇順で取得されるため，単純にリストの逆順が得られれば何とかなる．

lib/vendor/symfony/lib/plugins/sfDoctrinePlugin/lib/vendor/doctrine/Doctrine/Node/NestedSet.php
 205     public function getChildren()
 206     { 
 207         return $this->getDescendants(1);
 208     }
...
 215     public function getDescendants($depth = null, $includeNode = false)
...
 224             $q->addWhere("$baseAlias.lft > ? AND $baseAlias.rgt < ?", $params)->addOrderBy("$baseAlias.lft asc");

修正例¶

逆順で子ノードを削除する．

diff --git a/lib/model/doctrine/MemberProfile.class.php b/lib/model/doctrine/MemberProfile.class.php
index a085613..9bdf498 100644
--- a/lib/model/doctrine/MemberProfile.class.php
+++ b/lib/model/doctrine/MemberProfile.class.php
@@ -204,7 +204,9 @@ class MemberProfile extends BaseMemberProfile implements opAccessControlRecordIn
     if ($this->getTreeKey() && $this->getNode()->hasChildren())
     {
       $children = $this->getNode()->getChildren();
-      $children->delete();
+      for ($i = $children->count(); $i-- > 0;){
+        $children[$i]->getNode()->delete();
+      }
     }
   }

実験¶

２つを選択して保存．

mysql> select * from member_profile where profile_id = 5;
+----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+
| id | member_id | profile_id | profile_option_id | value | value_datetime | public_flag | tree_key | lft  | rgt  | level | created_at          | updated_at          |
+----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+
| 97 |         3 |          5 |              NULL |       | NULL           |        NULL |       97 |    1 |    6 |     0 | 2013-01-30 20:55:35 | 2013-01-30 20:55:35 |
| 98 |         3 |          5 |                 7 |       | NULL           |        NULL |       97 |    2 |    3 |     1 | 2013-01-30 20:55:35 | 2013-01-30 20:55:35 |
| 99 |         3 |          5 |                 8 |       | NULL           |        NULL |       97 |    4 |    5 |     1 | 2013-01-30 20:55:35 | 2013-01-30 20:55:35 |
+----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+
3 rows in set (0.00 sec)

保存し直す．

mysql> select * from member_profile where profile_id = 5;
+-----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+
| id  | member_id | profile_id | profile_option_id | value | value_datetime | public_flag | tree_key | lft  | rgt  | level | created_at          | updated_at          |
+-----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+
|  97 |         3 |          5 |              NULL |       | NULL           |        NULL |       97 |    1 |    4 |     0 | 2013-01-30 20:55:35 | 2013-01-30 20:55:56 |
| 100 |         3 |          5 |                 7 |       | NULL           |        NULL |       97 |    8 |    9 |     1 | 2013-01-30 20:55:56 | 2013-01-30 20:55:56 |
| 101 |         3 |          5 |                 8 |       | NULL           |        NULL |       97 |    2 |    3 |     1 | 2013-01-30 20:55:56 | 2013-01-30 20:55:56 |
+-----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+
3 rows in set (0.00 sec)

子ノードは正しく削除されているが， lft, rgt が変な値になっている．

原因推測¶

$parent->cleaerChildren() で正しく削除されているが， $parent の lft, rgt の値自体が新しい状態に更新されていないため，それを起点にしたデータが作成される．

diff --git a/lib/model/doctrine/MemberProfile.class.php b/lib/model/doctrine/MemberProfile.class.php
index a085613..3af9436 100644
--- a/lib/model/doctrine/MemberProfile.class.php
+++ b/lib/model/doctrine/MemberProfile.class.php
@@ -204,7 +204,10 @@ class MemberProfile extends BaseMemberProfile implements opAccessControlRecordIn
     if ($this->getTreeKey() && $this->getNode()->hasChildren())
     {
       $children = $this->getNode()->getChildren();
-      $children->delete();
+      for ($i = $children->count(); $i-- > 0;){
+        $children[$i]->getNode()->delete();
+      }
+      $this->refresh();
     }
   }

実験¶

２つを選択して保存．

mysql> select * from member_profile where profile_id = 5;
+-----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+
| id  | member_id | profile_id | profile_option_id | value | value_datetime | public_flag | tree_key | lft  | rgt  | level | created_at          | updated_at          |
+-----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+
| 114 |         3 |          5 |              NULL |       | NULL           |        NULL |      114 |    1 |    6 |     0 | 2013-01-30 21:06:34 | 2013-01-30 21:12:21 |
| 121 |         3 |          5 |                 7 |       | NULL           |        NULL |      114 |    2 |    3 |     1 | 2013-01-30 21:12:21 | 2013-01-30 21:12:21 |
| 122 |         3 |          5 |                 8 |       | NULL           |        NULL |      114 |    4 |    5 |     1 | 2013-01-30 21:12:21 | 2013-01-30 21:12:21 |
+-----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+
3 rows in set (0.01 sec)

そのまま保存し直す．

mysql> select * from member_profile where profile_id = 5;
+-----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+
| id  | member_id | profile_id | profile_option_id | value | value_datetime | public_flag | tree_key | lft  | rgt  | level | created_at          | updated_at          |
+-----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+
| 114 |         3 |          5 |              NULL |       | NULL           |        NULL |      114 |    1 |    6 |     0 | 2013-01-30 21:06:34 | 2013-01-30 21:12:33 |
| 123 |         3 |          5 |                 7 |       | NULL           |        NULL |      114 |    2 |    3 |     1 | 2013-01-30 21:12:33 | 2013-01-30 21:12:33 |
| 124 |         3 |          5 |                 8 |       | NULL           |        NULL |      114 |    4 |    5 |     1 | 2013-01-30 21:12:33 | 2013-01-30 21:12:33 |
+-----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+
3 rows in set (0.00 sec)

元々の子ノードが削除され，新しい状態のノードの状態も正しいため，正しい挙動をしていると思われる．

note-12 のマイグレーションについては #3321 で対応するため，本チケットでは note-11 の内容をコミットします．

逆順 node->delete() + refresh の修正で問題ないことは確認しました。

別解を2つ考えたのでコメントしておきます。

node の delete 時に lft, rgt が最新の状態である必要があることを明示する方針で、
以下のように毎回 refresh するのもありかなと思います。（以下のコードはループ初回が冗長ですが）

--- a/lib/model/doctrine/MemberProfile.class.php
+++ b/lib/model/doctrine/MemberProfile.class.php
@@ -204,7 +204,12 @@ class MemberProfile extends BaseMemberProfile implements opAccessControlRecordIn
     if ($this->getTreeKey() && $this->getNode()->hasChildren())
     {
       $children = $this->getNode()->getChildren();
-      $children->delete();
+      foreach ($children as $child)
+      {
+        $child->refresh();
+        $child->getNode()->delete();
+      }
+      $this->refresh();
     }
   }

ただ、refresh() は内部でクエリ発行するためループの中で使うのは効率がよいとは言えず、
効率重視でいくのであればいっそ node->delete() はまったく呼ばずに Collection->delete() 後に rgt を上書きしてしまうのも手です。以下

--- a/lib/model/doctrine/MemberProfile.class.php
+++ b/lib/model/doctrine/MemberProfile.class.php
@@ -204,7 +204,13 @@ class MemberProfile extends BaseMemberProfile implements opAccessControlRecordIn
     if ($this->getTreeKey() && $this->getNode()->hasChildren())
     {
       $children = $this->getNode()->getChildren();
-      $children->delete();
+      if ($children)
+      {
+        $children->delete();
+      }
+
+      $this->getNode()->setRightValue($this->getNode()->getLeftValue() + 1);
+      $this->save();
     }
   }

Doctrine_Collection::delete() もループで1レコードずつ削除するので、効率という点ではそこも改善の余地はあります。（preDelete() や postDelete() が呼ばれるメリットはありますが、MemberProfile では使ってなさそう）