Project

General

Profile

Bug(バグ) #2106

部分一致検索を行う箇所でワイルドカード検索ができてしまう

Added by Minoru Takai over 8 years ago. Updated almost 4 years ago.

Status:
Fixed(完了)
Priority:
High(高め)
Assignee:
Target version:
Start date:
2011-05-17
Due date:
% Done:

100%

3.6 で発生するか:
Yes
3.8 で発生するか:
Unknown (未調査)

Description

Overview (現象)

メンバー画面の「メンバー検索」や「コミュニティ検索」、あるいは管理画面の「メンバー管理」などでは、ニックネームやキーワード等で部分一致検索が行える。ここで、アンダースコア("_")やパーセント("%")を含めて検索すると、文字としてではなくメタ文字(ワイルドカード)として検索が行なわれてしまう。

これは仕様と考えれば問題ではないと言えるが、この動作は意図した仕様ではなく、不具合として扱うべきであると考えられる。

Causes (原因)

LIKE検索を行っている箇所では

$query->addWhere('name LIKE ?', '%'.$value.'%');

のように(部分一致)検索文字列として入力された値を直接渡しているためメタ文字がそのままメタ文字として扱われてしまっている。

Way to fix (修正内容)

LIKE検索は関数化されていない(LIKEメソッドがあるわけではない)ため、LIKE文を用いている箇所全てで入力文字列中のメタ文字をエスケープするか、最終的にSQLが生成される時点でエスケープを施すという対応が考えられる。

opDoctrineQuery クラスに LIKE 検索用のメソッドを作成し、それを各所で使用するように書き換える。このチケットではコアのみを扱う。

なお、このチケットで「意図しないワイルドカード検索ができてしまう問題」を解消するのは MySQL を使っている場合とする(仮に他のDBで問題が解消されていなくても MySQL でさえ解消されていれば良いものとする)。

Note (補足)

バックスラッシュを含む文字列での検索についての挙動も気になる。例えば "foo\bar" というニックネームのメンバーがいる場合、メンバー検索で "foo" と検索すればヒットするが、 "foo\" と検索してもヒットせず、 "foo\\" と検索するとヒットするような動作を確認している。


Related issues

Related to OpenPNE 3 - Bug(バグ) #1840: バックスラッシュの検索 Invalid(無効) 2010-12-10
Related to OpenPNE 3 - Backport(バックポート) #2108: 部分一致検索を行う箇所でワイルドカード検索ができてしまう Fixed(完了) 2011-05-17
Related to OpenPNE 3 - Backport(バックポート) #2109: 部分一致検索を行う箇所でワイルドカード検索ができてしまう Fixed(完了) 2011-05-17 2011-10-06
Related to opAlbumPlugin - Bug(バグ) #2335: 部分一致検索を行う箇所でワイルドカード検索ができてしまう Fixed(完了) 2011-08-03
Related to opCommunityTopicPlugin - Bug(バグ) #2336: 部分一致検索を行う箇所でワイルドカード検索ができてしまう Fixed(完了) 2011-08-03
Related to opDiaryPlugin - Bug(バグ) #2337: 部分一致検索を行う箇所でワイルドカード検索ができてしまう Fixed(完了) 2011-08-03
Related to opOpenSocialPlugin - Bug(バグ) #2338: [PATCH][etc] 部分一致検索を行う箇所でワイルドカード検索ができてしまう Fixed(完了) 2011-08-03
Related to opWebAPIPlugin - Bug(バグ) #2339: 部分一致検索を行う箇所でワイルドカード検索ができてしまう Fixed(完了) 2011-08-03
Related to opDiaryPlugin - Bug(バグ) #2600: 管理画面の日記コメントリストで日記ID以外で検索実行した場合結果がヒットしない Accepted(着手) 2011-11-15

Associated revisions

Revision b3ac7f2e (diff)
Added by Hiroshi Kato about 8 years ago

overridden the constructor method in Doctrine_Formatter.
added escape function to like query. (fixes #2106)

Revision da02cb21 (diff)
Added by Hiroshi Kato about 8 years ago

Revert "overridden the constructor method in Doctrine_Formatter.
added escape function to like query. (fixes #2106)"

This reverts commit b3ac7f2ea6761311afa7c267605c40d2eef78c18.

Revision 95d9ef33 (diff)
Added by Hiroshi Kato about 8 years ago

added function for 'where like' query in opDoctrineQuery class. (fixes #2106)

Revision 7598001a (diff)
Added by Hiroshi Kato about 8 years ago

added function whereLike() to like query.(fixes #2106)

Revision bf00466d (diff)
Added by Hiroshi Kato about 8 years ago

fixed for code review.(fixes #2106) * changed class constants value in opDoctrineQuery. * fixed for coding standard. * fixed for exact match.

History

#1 Updated by Shingo Yamada about 8 years ago

  • 3.6 で発生するか set to Yes

#2 Updated by Kousuke Ebihara about 8 years ago

Doctrine_Formatter::escapePattern() でワイルドカードに用いられる文字をエスケープできるようです。

ワイルドカードに用いられる文字はコネクションのプロパティで定義できるようになっており、デフォルト値は lib/vendor/symfony/lib/plugins/sfDoctrinePlugin/lib/vendor/doctrine/Doctrine/Connection.php の 153 行目で定義されている '%' と '_' です。

また、 LIKE 等のパターンマッチングをおこなう SQL の表現を生成するメソッドとして Doctrine_Expression_Mysql::matchPattern()
が用意されているようです。

#3 Updated by Minoru Takai about 8 years ago

メモ

Doctrine_Formatter::escapePattern()
lib/vendor/symfony/lib/plugins/sfDoctrinePlugin/lib/vendor/doctrine/Doctrine/Connection.php L153
'wildcards'           => array('%', '_'),
Doctrine_Expression_Mysql::matchPattern()
$ ack -l --ignore-dir=vendor -w "LIKE" plugins lib apps
plugins/opAlbumPlugin/lib/model/doctrine/PluginAlbumImageTable.class.php
plugins/opAlbumPlugin/lib/model/doctrine/PluginAlbumTable.class.php
plugins/opCommunityTopicPlugin/apps/pc_backend/modules/communityTopic/lib/CommunityEventCommentSearchForm.class.php
plugins/opCommunityTopicPlugin/apps/pc_backend/modules/communityTopic/lib/CommunityEventMemberSearchForm.class.php
plugins/opCommunityTopicPlugin/apps/pc_backend/modules/communityTopic/lib/CommunityTopicCommentSearchForm.class.php
plugins/opCommunityTopicPlugin/lib/model/doctrine/PluginCommunityEventTable.class.php
plugins/opCommunityTopicPlugin/lib/model/doctrine/PluginCommunityTopicTable.class.php
plugins/opDiaryPlugin/lib/model/doctrine/PluginDiaryCommentTable.class.php
plugins/opDiaryPlugin/lib/model/doctrine/PluginDiaryTable.class.php
plugins/opOpenSocialPlugin/lib/form/ApplicationSearchForm.class.php
plugins/opWebAPIPlugin/lib/api/opAPI.class.php
lib/filter/doctrine/CommunityFormFilter.class.php
lib/form/searchForm/opMemberProfileSearchForm.class.php
lib/model/doctrine/FileTable.class.php
lib/model/doctrine/MemberTable.class.php
lib/util/opFormItemGenerator.class.php

#4 Updated by Mutsumi Imamura about 8 years ago

  • Target version set to OpenPNE 3.7.0

#5 Updated by Shingo Yamada about 8 years ago

  • Assignee set to Hiroshi Kato
  • Priority changed from Normal(通常) to High(高め)

#6 Updated by Hiroshi Kato about 8 years ago

  • Status changed from New(新規) to Pending Review(レビュー待ち)
  • % Done changed from 0 to 50

更新履歴 b3ac7f2ea6761311afa7c267605c40d2eef78c18 で適用されました。

#7 Updated by Hiroshi Kato about 8 years ago

  • Status changed from Pending Review(レビュー待ち) to Accepted(着手)

#8 Updated by Hiroshi Kato about 8 years ago

  • Status changed from Accepted(着手) to Pending Review(レビュー待ち)

#9 Updated by Hiroshi Kato about 8 years ago

プラグイン側追加修正のため、各プラグインへのpull requestを一旦closeしました。
各プラグインへの対応はプラグイン毎のチケットにてすすめます。

#10 Updated by Minoru Takai about 8 years ago

  • Status changed from Pending Review(レビュー待ち) to Rejected(差し戻し)

この修正を見て、より利便性のよい修正が可能ではないかと思ったのでコメントします。

LIKE 検索を行う側で、

$q->andWhere($column.' LIKE ?', '%'.Doctrine_Manager::connection()->formatter->escapePattern($value).'%');

のようにメソッドをコールするような修正となっていますが、コアにしてもプラグインにしても、LIKE検索を行う箇所で、わざわざこの長い記述を行なわせることは好ましくないのではないかと思いました。

opToolkit クラスなどに以下のように汎用的なメソッドを定義しておき、

diff --git a/lib/util/opToolkit.class.php b/lib/util/opToolkit.class.php
index d0db038..e5e0fe7 100644
--- a/lib/util/opToolkit.class.php
+++ b/lib/util/opToolkit.class.php
@@ -17,6 +17,16 @@
  */
 class opToolkit
 {
+  /**
+   * Returns the escaped string for a LIKE operator of DQL.
+   *
+   * @return string
+   */
+  public static function likeQuery($value)
+  {
+    return '%'.Doctrine_Manager::connection()->formatter->escapePattern($value).'%';
+  }
+
  /**
   * Returns the list of mobile e-mail address domains.
   *

LIKE 検索を行う側では以下のようにメソッドを呼ばせるようにするのはどうでしょうか。

$q->andWhere($column.' LIKE ?', opToolkit::likeQuery($value));
  • likeQuery というメソッド名が適切かどうかは分かりませんが、 get*** や escape*** のようなメソッド名は似合わないように感じたので暫定的に likeQuery としています。
  • opToolkit::likeQuery() は将来的に、引数を付けることで呼び出し元からエスケープ対象文字を変更したり、あるいは部分一致検索ではなく前方一致検索・後方一致検索を可能にしたりする拡張が可能かもしれません。
  • 余談ですが、 opToolkit クラス内の /** から始まる既存のコメントは何故かインデントがずれているようです。

また、プラグイン側での修正を見ましたが、

      if (defined('OPENPNE_VERSION') && version_compare(OPENPNE_VERSION, '3.6beta13-dev', '>='))
      {
        $keyword = Doctrine_Manager::connection()->formatter->escapePattern($keyword);
      }

のような記述が各所に追記されていますが、同じ目的で記述しているのであれば、なるべく記述を抽象化した方が保守しやすいのではないかと思います。各所に version_compare(OPENPNE_VERSION, '3.6beta13-dev', '>=') のような記述が書かれているのが好ましくないように思います。

likeQuery() を

  public static function likeQuery($value)
  {
    if (defined('OPENPNE_VERSION') && version_compare(OPENPNE_VERSION, '3.6beta13-dev', '>='))
    {
      $value = Doctrine_Manager::connection()->formatter->escapePattern($value);
    }

    return '%'.$value.'%';
  }

のように定義し、これを各所でコールした方が、後に本件の仕様を変更したい場合にも、このメソッドを書き換えるだけで済むため好ましいように思います。


実装不備ではありませんが、提案について確認していただきたいのでステータスを差し戻しに変更します。

プラグイン側の実装も鑑みて、現状のままでも良いのではないかと判断されるのであれば、その旨をコメントして改めてレビュー待ちにしていただければと思います。

8/5(金)追記

このコメント内容について実装者と検討を行いました。検討内容とは別ですが、上記のコメントに若干誤りがあるので補足しておきます。

likeQuery() を

  public static function likeQuery($value)
  {
    if (defined('OPENPNE_VERSION') && version_compare(OPENPNE_VERSION, '3.6beta13-dev', '>='))
    {
      $value = Doctrine_Manager::connection()->formatter->escapePattern($value);
    }

    return '%'.$value.'%';
  }

のように定義し、これを各所でコールした方が、
...

$q->andWhere($column.' LIKE ?', opToolkit::likeQuery($value));

と示しましたが、各所でいきなり likeQuery() をコールしても、このメソッドが定義されるのはコアのマイナーバージョンが最新版のもののみなので、新しいプラグインと古いコアを使う場合に未定義のメソッドを呼ぶことによる問題が生じます。

後半の内容については以下のようにすることになるかもしれません。

  • メソッドの定義
      public static function likeQuery($value)
      {
        return '%'.Doctrine_Manager::connection()->formatter->escapePattern($value).'%';
      }
    
    • 実際に内部でどのようにエスケープをするか(Doctrine が用意している escapePattern() を用いるか、あるいは使うにしても Doctrine_Manager::connection()->formatter 以外を経由して escapePattern() を用いるなど)を変更したい時は、このメソッドの実装を変更することで対応できるようにする。
    • このメソッドで % を付加する目的は、LIKE検索を行いたい箇所で、「部分一致検索のためには前後に % を付加する」という事実を隠蔽(抽象化)するためです。LIKE検索(部分一致検索)したいときは、とにかく opToolkit::likeQuery() に値を渡して、返り値を andWhere() などに渡せばよいようにするという発想に基づいています。
  • 呼び出し側の記述例(1)
    $q->andWhere($column.' LIKE ?', method_exists('opToolkit', 'likeQuery') ? opToolkit::likeQuery($value) : '%'.$value.'%');
    
    • この例では likeQuery() が 検索文字列 $value の前後に(部分一致検索とするためのワイルドカード) % を付加した文字列を返す仕様を前提としています。
  • 呼び出し側の記述例(2)
    if (method_exists('opToolkit', 'likeQuery'))
    {
      $value = opToolkit::likeQuery($value);
    }
    $q->andWhere($column.' LIKE ?', '%'.$value.'%');
    
    • この(2)の例では、 likeQuery() が '%'.(検索文字列).'%' を返すようにしていても、部分位置検索を前提としていれば andWhere() の第2引数の時点で '%'.('%'.(検索文字列).'%').'%' となるだけで、検索結果には影響は無いと思います( % が何個連続してもワイルドカードとしての意味は変わらない)。

#11 Updated by Shingo Yamada about 8 years ago

  • 360対象 set to beta13

#12 Updated by Hiroshi Kato about 8 years ago

  • Status changed from Rejected(差し戻し) to Pending Review(レビュー待ち)

更新履歴 7598001aa03369606ec2c9ba26a78930fdede272 で適用されました。

#13 Updated by Hiroshi Kato about 8 years ago

更新履歴 95d9ef3386809ab4aeefa446a516b1da2bcb82df で適用されました。

#14 Updated by Hiroshi Kato about 8 years ago

更新履歴 da02cb21c8b288fbe913eb5f569840e09a8dc3b8 で適用されました。

#15 Updated by Youichi Kimura about 8 years ago

SQLiteでのLIKEパターンのエスケープにDoctrineが対応していないため、DBMSにSQLiteを使用している環境では 95d9ef3 によるエスケープは行われませんでした。
サポート外環境での話ですが参考までに。

#16 Updated by Minoru Takai about 8 years ago

  • Status changed from Pending Review(レビュー待ち) to Rejected(差し戻し)

opDoctrineQuery クラスにメソッドを追加して対応する方針としたようですが、チケット作成時の問題に対する対応としてこの方針での修正を適用することはよいと思います。 note-15 のような話もあるようですが、これまでワイルドカードに対して何の考慮もしていなかったことに対して、少なくとも MySQL を使用した環境でこれが対応されていればよく、他のDBでも対応したいとなればこのチケットでの改善を基に(別チケットで)対応すればよいと考えています。

指摘点

note-12 から note-14 での修正内容は概ねよいと思っています。細かい点ですが以下について確認お願いします。

  • (1) {where|andWhere}Like($column, $value) への書き換えの対応漏れ
    • コア側で 'LIKE' が使われている箇所( plugins 下のコアに含まれるプラグインは該当なし)
    • (1-1) FileTable クラス内で直接 LIKE 文が使われている箇所は、書き換える必要はないでしょうか。
    • (1-2) opFormItemGenerator クラス内は書き換えるべきだと思いますが、書き換える必要はないでしょうか。
      $ ack 'LIKE' -w --ignore-dir=vendor lib apps -C1
      
      lib/model/doctrine/FileTable.class.php
      46-    $q = $this->getImageOrderdQuery()
      47:      ->where('type NOT LIKE ?', 'image%');
      48-    return $this->getPager($q, $page, $this->getValidPagerSize($size));
      --
      53-    $q = $this->getImageOrderdQuery()
      54:      ->where('type LIKE ?', 'image%');
      55-    return $this->getPager($q, $page, $this->getValidPagerSize($size));
      
      lib/util/opFormItemGenerator.class.php
      364-      case 'date':
      365:        $q->andWhere($column.' LIKE ?', $value);
      366-        break;
      
      lib/util/opDoctrineQuery.class.php
      316-    {
      317:      $this->andWhere($expr.' NOT LIKE ?', $match);
      318-    }
      --
      320-    {
      321:      $this->andWhere($expr.' LIKE ?', $match);
      322-    }
      
  • (2) MATCH_BROAD 定数の順序と値
    • opDoctrineQuery では以下のように記述されていますが、定数の記述順序と、 case 文の記述順序が一致していないのが気になります。
      23:  const MATCH_EXACT = 0;
      :
      28:  const MATCH_BROAD = 1;
      :
      33:  const MATCH_LEFT = 2;
      :
      38:  const MATCH_RIGHT = 3;
      --
      298-  public function andWhereLike($expr, $param, $matchType = self::MATCH_BROAD, $not = false)
      299-  {
      300-    $match = $this->escapePattern($param);
      301-
      302-    switch ($matchType)
      303-    {
      304-      case self::MATCH_LEFT:
      305-        $match = $match.'%';
      306-        break;
      307-      case self::MATCH_RIGHT:
      308-        $match = '%'.$match;
      309-        break;
      310-      case self::MATCH_BROAD:
      311-        $match = '%'.$match.'%';
      312-        break;
      313-    }
      
    • 順序の不一致を直すのに加え、改善が可能かもしれません。例えば $matchType = opDoctrineQuery::MATCH_LEFT & opDoctrineQuery::MATCH_RIGHT; とした場合に結果として opDoctrineQuery::MATCH_BROAD となるように値を設定すると、この定数の利便性が上がります。
      • 訂正: opDoctrineQuery::MATCH_LEFT & opDoctrineQuery::MATCH_RIGHT と書きましたが、 & ではなく | ですね。
    • 修正案
      const MATCH_EXACT = 0;
      const MATCH_BROAD = 1;
      const MATCH_LEFT = 2;
      const MATCH_RIGHT = 3;
      
      これを以下のように書き換えます。
      
      const MATCH_EXACT = 0;
      const MATCH_LEFT = 1;
      const MATCH_RIGHT = 2;
      const MATCH_BROAD = 3;
      
  • (3) opDoctrineQuery::escapePattern() がコーディング規約に違反しています
    • 既存のコードをコピーペーストしたのかもしれませんが、インデント等が一部不適切です。
    • (3-1) 333 行目:インデントが多いです
    • (3-2) 340 行目:インデントが多いです
    • (3-3) 340 行目:文字列結合演算子の前後にスペースがあります
      327:  public function escapePattern($text)
      328-  {
      329-    $conn = $this->getConnection();
      330-
      331:    if (!$conn->string_quoting['escape_pattern'])
      332-    {
      333-        return $text;
      334-    }
      335-    $tmp = $conn->string_quoting;
      336-
      337:    $text = str_replace($tmp['escape_pattern'], $tmp['escape_pattern'].$tmp['escape_pattern'], $text);
      338-
      339-    foreach ($conn->wildcards as $wildcard) {
      340:        $text = str_replace($wildcard, $tmp['escape_pattern'] . $wildcard, $text);
      341-    }
      342-
      343-    return $text;
      344-  }
      

ちなみに、 matchType の定数名が **LEFT, **RIGHT, **BROAD でよいかなども考えましたがこれはこれで悪くないと思いました。上記の指摘項目以外には問題はないと思います。

以上について、確認した上で修正する必要がない箇所があればそのままでもよいと思います。修正すべきであればコミットをお願いします。

#17 Updated by Minoru Takai about 8 years ago

  • Description updated (diff)

#18 Updated by Hiroshi Kato about 8 years ago

Minoru Takai は書きました:

  • (1) {where|andWhere}Like($column, $value) への書き換えの対応漏れ
    • コア側で 'LIKE' が使われている箇所( plugins 下のコアに含まれるプラグインは該当なし)
    • (1-1) FileTable クラス内で直接 LIKE 文が使われている箇所は、書き換える必要はないでしょうか。
    • (1-2) opFormItemGenerator クラス内は書き換えるべきだと思いますが、書き換える必要はないでしょうか。

当該箇所について、LIKE検索は行っていますが、特に問題が起こっている箇所ではありません。
修正を行った場合、テストを行う必要があることを考えると、今回は最低限の実装にとどめ、
当該箇所の修正を行わなくてもいいのではないかと小川さんと話し、修正を行いませんでした。

  • (2) MATCH_BROAD 定数の順序と値
    • opDoctrineQuery では以下のように記述されていますが、定数の記述順序と、 case 文の記述順序が一致していないのが気になります。
      [...]
    • 順序の不一致を直すのに加え、改善が可能かもしれません。例えば $matchType = opDoctrineQuery::MATCH_LEFT & opDoctrineQuery::MATCH_RIGHT; とした場合に結果として opDoctrineQuery::MATCH_BROAD となるように値を設定すると、この定数の利便性が上がります。
    • 修正案
      [...]

$matchType = opDoctrineQuery::MATCH_LEFT | opDoctrineQuery::MATCH_RIGHT; でしょうか。(+でも可)
修正案の通りに修正しまっす。

  • (3) opDoctrineQuery::escapePattern() がコーディング規約に違反しています

修正します。

#19 Updated by Hiroshi Kato about 8 years ago

  • Status changed from Rejected(差し戻し) to Pending Review(レビュー待ち)

更新履歴 bf00466d085647955e6f5b27371343ee03838b74 で適用されました。

#20 Updated by Minoru Takai about 8 years ago

  • Status changed from Pending Review(レビュー待ち) to Pending Testing(テスト待ち)
  • % Done changed from 50 to 70

note-19 をレビューしました。レビュー OK です。

  • lib/model/doctrine/FileTable.class.php と lib/util/opFormItemGenerator.class.php を修正していない点については、今回の修正での変更箇所を最小限にするという意図があるとのことなので、このままで OK とします。
  • note-16 での他の指摘内容が修正されており、コア側については note-19 までの修正で「意図しないワイルドカードを含んだ検索がされてしまう問題」が解消できていると思われるので OK とします。

このチケットでの対応

LIKE 検索のためのメソッドを用意する

  • andWhere(opToolkit::likeQuery($value)) のようなラッパではなく、 opDoctrineQuery に andWhereLike() のようなメソッドを直接追加し、それを用いるようにした。
  • 今回は、検索で問題が生じている箇所に対応できればよいため、必要最低限なメソッドしか追加していない。
    • whereLike() と andWhereLike() しか追加しておらず、 addWhereLike(), orWhereLike(), whereNotLike() などは定義していない。
    • 用意したほうがよい状況になった場合に追加する可能性があるが、今回のチケットではバグ対応に重点をおき、メソッドの追加は扱わないこととする。
  • andWhereLike() の実装についても必要最低限にすべきかもしれないが、必ずエスケープを行う部分一致検索(MATCH_BROAD)の場合のみの実装だと、部分一致以外の検索をしたい場合や、呼び出し側の値が意図してワイルドカードを値に含めることができず、 andWhereLike() というメソッドを使える場面が限定されてしまうことを懸念して、ある程度柔軟な実装にしておくこととした。

用意したメソッドを使うように書き換える

コア側に含まれる「意図しないワイルドカードを含んだ検索がされてしまう問題」が生じてしまう箇所のみを修正することとした。そのため lib/model/doctrine/FileTable.class.php と lib/util/opFormItemGenerator.class.php は意図的に修正しないままにしている。このチケットでは修正しないままにしているだけであり、修正しないことが好ましいという意味ではない。

#21 Updated by Fumie Toyooka almost 8 years ago

  • Status changed from Pending Testing(テスト待ち) to Fixed(完了)
  • % Done changed from 70 to 100

#22 Updated by Fumie Toyooka almost 8 years ago

  • Status changed from Fixed(完了) to Pending Testing(テスト待ち)
  • % Done changed from 100 to 70

#23 Updated by Fumie Toyooka almost 8 years ago

  • Status changed from Pending Testing(テスト待ち) to Fixed(完了)
  • % Done changed from 70 to 100

テストOKです

#24 Updated by kaoru n almost 4 years ago

  • 3.8 で発生するか set to Unknown (未調査)

Also available in: Atom PDF