Skip to content

Instantly share code, notes, and snippets.

@lyricallogical
Created April 23, 2012 12:49
Show Gist options
  • Star 9 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save lyricallogical/2470715 to your computer and use it in GitHub Desktop.
Save lyricallogical/2470715 to your computer and use it in GitHub Desktop.
play2.0 の anorm の問題点をまとめてみる。間違いがあったらコメントしてもらえると助かります。問題がなければ本家に対してまとまった形で報告するつもりです。最終的な目標は anorm の改善です。

play2.0 の anorm の問題点まとめ

実装の詳細が公開されている

Useful 等の公開すべきでないものが公開されている。 型が書かれていない public なメンバも多く、API に対する意識が低い。

不要なものが公開されている

Pk, Id, NoAssigned 等は Magic のために導入された型だが、 https://github.com/playframework/play-scala/commit/83b13b5a556a1cec45729eaf8009f63a0e39943d#L1R140 Magic は play2.0 では取り除かれた。 https://github.com/playframework/Play20/commit/f35fd305d98038ab0fe3f8076368ee7f8d90015b にも関わらず、未だに残っている。

更に不幸なことに、サンプルコードで利用されている。 https://github.com/playframework/Play20/blob/master/samples/scala/computer-database/app/models/Models.scala

不要なコードが残っている

Magic の実装に利用されていた TypeWranger クラスや Row#getType メソッド、Manifest の import 等、不要なコードがあちこちに残っている。

テストが存在しない

これは anorm に限らず、play2.0 全体にいえることだが…

ResultSetMetaData#getTableName が正しく扱われていない

getTableName は、空文字列を返すことが有る。現状は、これをまともに考慮した実装にはなっていない。 ColumnNotFound#toString 等に空文字列だった場合のための処理があるが、適切な処理とは言いがたい。 https://github.com/playframework/Play20/blob/master/framework/src/anorm/src/main/scala/Anorm.scala#L34

また、不幸にも null を返すドライバも存在する。これも同様に考慮した実装にはなっていない。 null を返す例。h2 や sqlite のドライバで null になる。

select name || "!" from users

ResultSetMetaData#getColumnName が正しく扱われていない

getColumnName は、カラム名を返すドライバと、エイリアス名を返すドライバが存在する。現状は、これを考慮した実装にはなっていない。 カラム名を返すドライバでは、エイリアス名による値の取得ができない。これは既に報告されている。 https://play.lighthouseapp.com/projects/82401-play-20/tickets/332

カラム名とエイリアス名が正しく扱われていない

  上記の問題に対する以下のような pull request がある。 playframework/playframework#241 この修正は適切ではない。何故なら、現状の実装では「テーブル名.カラム名」のような形式で値を保持しているからだ。 単に getColumnName を getColumnLabel に置き換えた場合、これが「テーブル名.カラムのエイリアス名」になってしまう。 その結果、以下のようなコードが動作してしまう。

SQL("select name as n from users").as(str("users.n") *)

更に、以下のようなコードで、ColumnNotFound#toString の結果がおかしくなってしまう。

SQL("select name as n from users").as(str("badname") *)

また、上述の通り getColumnName がエイリアス名を返すドライバが存在するため、現状の実装が既に正しくない。 本来なら getTableName, getColumnName, getColumnLabel それぞれの値は個別に保持すべきだ。

そして、前述の通り getColumnName がエイリアス名を返すドライバが存在するため、「テーブル名.カラム名」のような値を取得することは諦めるべきだ。postgres の場合、getBaseTableName 同様、getBaseColumnName メソッドが提供されているが、getColumnName がエイリアス名を返す他のドライバで、同様のメソッドが提供されているとは限らない。 getTableName や getColumnName は、ドライバによっては内部でクエリを発行することがあるため、パフォーマンスの上でも良くない。

ResultSet から値を取り出す際は、素直に ResultSet#findColumn を利用すべきだ。「テーブル名.カラム名」のような指定をサポートするのは諦めるべきだ。 エラー表示には getTableName は利用せず getColumnName の値のみを利用すべきだ。「テーブル名.カラム名」のような値を表示することは諦めるべきだ。

タイムゾーンを指定して Date を取り出すことができない

全てのカラムの値を getObject で取り出しているため、現状以下のメソッドを利用する手段が提供されていない。 http://docs.oracle.com/javase/6/docs/api/java/sql/ResultSet.html#getDate(java.lang.String, java.util.Calendar)

実装に無駄がある

https://github.com/playframework/Play20/blob/master/framework/src/anorm/src/main/scala/Anorm.scala#L268 二度も辞書を引くのは馬鹿げている。MetaDataItem がインデックスを持っていれば、二度も辞書を引く必要は無い。 https://github.com/playframework/Play20/blob/master/framework/src/anorm/src/main/scala/Anorm.scala#L504 不要かもしれないカラムの値も、常に全て生成されている。

Statement がリーク(long-lived)する

Sql#execute 等のメソッドは Statement を close しない。close するための手段も提供されていない。 https://github.com/playframework/Play20/blob/master/framework/src/anorm/src/main/scala/Anorm.scala#L443 そのため、ファイナライザスレッドで処理されるまで、リソースは解放されない。

DBApi#withConnection メソッドを利用する場合、発行した Statement を記憶しておき、close されるタイミングで、発行した全ての Statement を close する AutoCleanConnection クラスが利用されているため、比較的リソースは早く解放される。 https://github.com/playframework/Play20/blob/master/framework/src/play/src/main/scala/play/api/db/DB.scala#L74 https://github.com/playframework/Play20/blob/master/framework/src/play/src/main/scala/play/api/db/DB.scala#L410 しかし play2.0 が標準で提供する BoneCPPlugin 以外のユーザー定義の DBPlugin が利用された場合には、オーバーライドされている可能性がある。

RowParser を利用する場合、Row を全てなめた場合にのみ ResultSet ごと close される。 https://github.com/playframework/Play20/blob/master/framework/src/anorm/src/main/scala/Anorm.scala#L501

正確なリソース管理を行うべきだ。

RowParser でインデックスを指定することができない

現状、RowParser は必ず「名前」を指定しなければならない。

int("id") ~ str("name")

しかし、本質的に名前を持たないカラムもある。

SQL("select id, name || '!' from users").as(???)

as clause を用いることで、getColumnName がエイリアス名を返すドライバを利用していれば(詳細は前述の通り)、以下のように取り出すことができる。

SQL("select id, name || '!' as bang from users").as((int("id") ~ str("bang")).map(flatten) *)

しかし、インデックスを指定して値を取り出すことができれば、わざわざ as clause を用いる必要は無い。 何故以下のようにパーザを記述できないのか。

SQL("select id, name || '!' as bang from users").as((int(0) ~ str(1)).map(flatten) *)

RowParser の記述性が低い

何故以下のようにパーザを記述できないのか。

SQL("select id, name || '!' as bang from users").as((int ~ str).map(flatten) *)
@lyricallogical
Copy link
Author

わーなんか整形が。なんでコメント欄はプレビューできるのに gist 自体はプレビューできないのかなあ…

@tototoshi
Copy link

https://github.com/playframework/Play20/blob/master/samples/scala/zentasks/app/Global.scala
https://github.com/playframework/Play20/blob/master/samples/scala/zentasks/app/models/Task.scala#L150-177

こういうのを見る限り、Pkは意図的に残してあると思われます。
(個人的にはいらないと思いますが)

@lyricallogical
Copy link
Author

意図があるとしたらどのような意図でしょうか。
anorm はモデル層を提供しないとドキュメントで明記されてるので、Pk 群が残ってるのは単に消し忘れか、消したらサンプルコードが動かなくなるからとりあえず残しておいた、ぐらいのことだと思っています。

@tototoshi
Copy link

>意図
Task#create のようなメソッドで、IDを明示的に指定するのにもシーケンスから採番するのにも対応しつつ、Task => Task という型にするには、NotAssignedというプレースホルダ的なものが必要では?

> 消したらサンプルコードが動かなくなるからとりあえず残しておいた
でもぶっちゃけこれかな。。。

@syuta
Copy link

syuta commented Apr 23, 2012

Pk, Id, NoAssignedって不要だったのか。。代わりにOption[Long]とか使うんですかね

@lyricallogical
Copy link
Author

Pk, Id, NoAssignedって不要だったのか。。代わりにOption[Long]とか使うんですかね

不要というか、play2.0 の anorm の提供する他の機能とは一切関係ないですね。nullable なカラムの値を取得する場合は、ドキュメントにあるようにパーザの場合は getOption[T] とする必要があります。

不勉強でつい最近まで知らなかったのですが、ボクが確認した限りでは、JDBC 3.0 以降は Connection が close されるとき、Connection の発行した全ての Statement は close される、と仕様で規定されているようです。詳細は 13.1.3 参照。
勿論仕様はあくまで仕様で、現実には行儀の悪いドライバがいたりするわけですが、矢張りいて、oracle の jdbc ドライバなんかは仕様通りの実装にはなっていないみたいです。

あと「ファイナライザスレッドで処理されるまで、リソースは解放されない」も、ファイナライザメソッドを持つかどうかはドライバの実装依存(のはず)なので、ちょっと嘘ですね。

ということでリーク周りは色々嘘です。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment