[Seasar-user:21656] Re: S2JDBC の AutoSelect への includes() と excludes() 実装

[Jun Futagawa]

ふたがわ (jfut) です。

丁寧なコメントありがとうございます。

On 2013/07/14 4:30, Koichi Kobayashi wrote:

> ご存じの通り、S2JDBC 含めて Seasar2 に関しては
> 
> http://d.hatena.ne.jp/higayasuo/20080714/1216021595
> 
> 「要望があれば小さな機能改善は行ないますが、大きな機能追加は行ないません。」
> 
> ということになっています。

そうでしたね。
今さら機能追加して使い方に混乱が生じそうであれば、
取り込まないという判断も当然あると思います。

以下、せっかく指摘していただいたので自分のためにも修正してみました。
修正したパッチも添付します。

> 取り込むかどうかは別として、パッチを見て気になった点:
> # 実際にパッチを適用して動かしたわけではなく、
> # 眺めただけで書いてます。
> 
> ・s2jdbc-it にもテストの追加が必要

これは足りていなかったですね。取り込まれるようでしたら作ります。

s2jdbc-it へはテストを追加していないですが、自分のアプリの DB で
実行してテストケースにある内容と同様のケースは動いています。

> ・ドキュメントの「注意点」

加筆しました。

> ・isTargetProperty()
> 
> @Id なプロパティを除外できちゃうのはまずいような。
> ちゃんと動作するのかな？ しない気がしないでもない。。。
> 特に対多な関連の場合とか。
> @Id なプロパティは (関連先も含めて) 無条件に
> includes 扱いにすべきかと思いますです。

@Id なプロパティを除外しても検索結果自体は取れるようですが、
更新時の整合性のために @Id は無条件で検索結果に含めるようにしました。
@Id のプロパティも含めていらない場合はフェッチしないように
innerJoin(), leftOuterJoin() の 第 2 引数に false を渡せば良いですね。

> あとこんなケース
> 
> List<Employee> results = 
>     jdbcManager
>         .from(Employee.class)
>         .includes("id", "name", "department")
>         .excludes("department.name")
>         .leftOuterJoin("department")
>         .leftOuterJoin("department.address")
>         .getResultList();
> 
> department 以下は全部取得するけど name だけは
> 除外する、というケースで name が除外されなさそう。

たしかに、この場合は department.name は除外されて欲しいですね。
上記が意図したとおり動くように、このようなケースでは、
関連のプロパティは除外するようにしました。

> パッチのテストケースでは excludes で関連を指定して
> includes で個別のプロパティを指定しているケースは
> ありますが、これはその逆のパターンでテストケースが
> 無いような気がします。
> 
> 似たケースで、
> 
> List<Employee> results = 
>     jdbcManager
>         .from(Employee.class)
>         .includes("id", "name", "department")
>         .excludes("department.address")
>         .leftOuterJoin("department")
>         .leftOuterJoin("department.address")
>         .getResultList();
...
> department.address.xxx があった場合、いきなり
> includes/excludes で指定された department.〜 で
> 始まるかチェックするのではなく、プロパティ名の
> 後ろから一つずつ要素を削って、まず department.address が
> includes/excludes に含まれるか、次に department が
> includes/excludes に含まれるか、というように、
> プロパティ名の側を基準にチェックしないとダメじゃ
> ないかと思いました (印象に過ぎませんが)。

関連の includes 条件にマッチした後に、excludes の条件に
除外指定がないかチェックすれば良さそうな気がしました。
添付のパッチはそのように修正してみました。

> 関連を指定する機能は仕様的に少々危うい感じが
> しないでもないです。
> 
> 
> と書いてみると、コード量の割に落とし穴を作る可能性が
> 少なくない感じがしますね。。。

そうですね、まだ考慮漏れがあるかもしれないです。

-- 
Jun Futagawa
-------------- next part --------------
テキスト形式以外の添付ファイルを保管しました...
ファイル名: s2tiger-s2jdbc-includes-excludes2.patch
型:         text/x-patch
サイズ:     7494 バイト
説明:       無し
URL:        <http://ml.seasar.org/archives/seasar-user/attachments/20130714/c72a78a4/attachment.bin>
-------------- next part --------------
テキスト形式以外の添付ファイルを保管しました...
ファイル名: seasar2-s2jdbc-includes-excludes2.patch
型:         text/x-patch
サイズ:     7479 バイト
説明:       無し
URL:        <http://ml.seasar.org/archives/seasar-user/attachments/20130714/c72a78a4/attachment-0001.bin>