第14回/Issues Rest APIで、複数のtracker_idを簡単に指定できるようにして欲しい
メンバー: @sanaksanak.icon , @rbt, @tohosaku, @mbasa
GitHub:
はじめに
現状、ドキュメントに未記載の方法でAPI呼び出しは可能だが、もっと簡単にならないか?
現状: /issues.json?f%5B%5D=tracker_id&op%5Btracker_id%5D=%3D&v%5Btracker_id%5D%5B%5D=18&v%5Btracker_id%5D%5B%5D=21
-> /issues.json?f[]=tracker_id&op[tracker_id]==&v[tracker_id][]=18&v[tracker_id][]=21
理想: /issues.json?tracker_id=18,21
Issuesリストのtracker_id複数指定で生成されるURLが上記となる。
Redmine Rest API - Issues
tracker_id=1,2 だと 500 エラーが返る
issue_idはカンマ区切りをサポートしている
コードを見ていく。
tracker=id=1,2 を指定した場合のエラー箇所
code:rb
Query::StatementInvalid: PG::InvalidTextRepresentation: ERROR: invalid input syntax for type integer: "1,2"
LINE 1: ...HERE is_closed=FALSE)) AND (issues.tracker_id IN ('1,2')) AN...
^
Completed 500 Internal Server Error in 211ms (ActiveRecord: 16.0ms | Allocations: 11402)
IssueControllerのretrieve_queryの@query.build_from_paramsメソッド中で以下が生成される
code:rb
#<IssueQuery id: nil, project_id: 1, name: "_", filters: {"status_id"=>{:operator=>"o", :values=>""}, "tracker_id"=>{:operator=>"=", :values=>"1,2"}}, user_id: 0, column_names: nil, sort_criteria: "id", "desc", group_by: nil, type: "IssueQuery", visibility: 0, options: {:totalable_names=>[], :display_type=>"list", :draw_relations=>nil, :draw_progress_line=>nil, :draw_selected_columns=>nil}> SQLは以下
code:sql
SELECT COUNT(*) FROM "issues"
INNER JOIN "projects" ON "projects"."id" = "issues"."project_id"
INNER JOIN "issue_statuses" ON "issue_statuses"."id" = "issues"."status_id"
WHERE (((projects.status <> 9 AND EXISTS (SELECT 1 AS one FROM enabled_modules em WHERE em.project_id = projects.id AND em.name='issue_tracking')) AND (((projects.is_public = TRUE AND projects.id NOT IN (SELECT project_id FROM members WHERE user_id IN (4,2))) AND ((issues.is_private = FALSE)))))) AND ((issues.status_id IN (SELECT id FROM issue_statuses WHERE is_closed=FALSE))
AND (issues.tracker_id IN ('1,2')) AND projects.id = 1)
IN ('1,2')) がだめ。IN (1,2)) じゃなきゃ。
issue_id=2,3の場合は、以下が生成される
code:rb
#<IssueQuery id: nil, project_id: 1, name: "_", filters: {"status_id"=>{:operator=>"o", :values=>""}, "issue_id"=>{:operator=>"=", :values=>"2,3"}}, user_id: 0, column_names: nil, sort_criteria: "id", "desc", group_by: nil, type: "IssueQuery", visibility: 0, options: {:totalable_names=>[], :display_type=>"list", :draw_relations=>nil, :draw_progress_line=>nil, :draw_selected_columns=>nil}> こちらのSQLは
code:sql
SELECT COUNT(*) FROM "issues"
INNER JOIN "projects" ON "projects"."id" = "issues"."project_id"
INNER JOIN "issue_statuses" ON "issue_statuses"."id" = "issues"."status_id"
WHERE (((projects.status <> 9 AND EXISTS (SELECT 1 AS one FROM enabled_modules em WHERE em.project_id = projects.id AND em.name='issue_tracking')) AND (((projects.is_public = TRUE AND projects.id NOT IN (SELECT project_id FROM members WHERE user_id IN (4,2))) AND ((issues.is_private = FALSE)))))) AND ((issues.status_id IN (SELECT id FROM issue_statuses WHERE is_closed=FALSE))
AND issues.id IN (2,3) AND projects.id = 1)
500エラーの原因は
models/issue_query.rb の以下の statement で間違ったSQLが生成される
code:rb
def base_scope
Issue.visible.joins(:status, :project).where(statement)
end
models/query.rb の中の def statement 中のfiltersに {"status_id"=>{:operator=>"o", :values=>[""]}, "tracker_id"=>{:operator=>"=", :values=>["1,2"]}} が入ってくる
現状、regular_field扱いになっている
code:rb
# regular field
filters_clauses << '(' + sql_for_field(field, operator, v, queried_table_name, field) + ')'
models/issue_query.rb 内で、既存の sql_for_issue_id_field を参考に以下を追加することで、 tracker_id=1,2 のリクエストが通ることを確認
code:rb
def sql_for_tracker_id_field(field, operator, value)
if operator == "="
# accepts a comma separated list of ids
ids = value.first.to_s.scan(/\d+/).map(&:to_i)
if ids.present?
"tracker_id IN (#{ids.join(",")})"
else
"1=0"
end
else
# TODO:
sql_for_field("id", operator, value, Tracker.table_name, "id")
end
end
以下より、operatorが"="の他に"!"もあるようだが、流れてこない。
code:rb
add_available_filter(
"tracker_id",
)
~~ 昼休憩 ~~
Rest APIでは"!"サポートは不要のよう
既存の"等しくない"フィルタ時に流れるSQLは以下
code:sql
引数を調整
=> 上記SQLを流す形に変更
テスト
フィルターのテストを下記で追加
test/functional/issues_controller_test.rb:211
以下の1行を追加して、パッチなし状態だとエラー、パッチあり状態で通ることを確認
code:diff
'tracker_id' => {
'3' => {:op => '=', :values => '3'}, - '=3' => {:op => '=', :values => '3'} + '=3' => {:op => '=', :values => '3'}, + '=1,2,3' => {:op => '=', :values => '1,2,3'} },
Issueの検索のテストがないので、追加が必要。
test/unit/query_test.rb:772 付近
以下はサンプル
code:rb
def test_filter_on_tracker_id
query =
IssueQuery.new(
:name => '_',
:filters => {
'tracker_id' => {
:operator => '=',
}
}
)
assert_equal 14, find_issues_with_query(query).size
end
気になったこと(加藤)
そもそも想定していないパラメータが入力されたときにSQLまで走ってしまって 500を返すのはまずくないか?
issues.json?issue_id=%212 (issues.json?issue_id=!2) を入力すると
{"errors":["チケット は不正な値です"]}というJSONが返ってくる。これが本来期待されている挙動のはず。
TODO(このissueとは別件)
Rails側でURLパラメータがパースされた後、Redmine で Query としてパースされる過程の理解を深める。
formからsubmitした場合と、Rest API で許容されているクエリの違いはどのあたりで定義されているのか。
Rails では、controller に許容するパラメータの型を定義する仕組みがある(StrongParameter Rails4系から)。ただし、そうした仕組みが取り入れられたのは、Redmine のQueryまわりの仕様が固まった後。Railsの機能とRedmineに元からある仕組みを比較してみて、500を防ぐ方法があるか探る。
Redmine のコントローラの中にも、StrongParameter を取り入れているものはある(多分、入れやすいところから入れている)。
やり残し:
Issueの検索のテストを追加する
本家の方にまだIssueを作成していないので、それをまず作成する
Defect扱いにするか、 Feature扱いにするか迷い中
まずはFeatureでパッチを送ってみて、本家側の反応を見てみる。
issue_id は Entityそのもののidだが、tracker_idはEntityそのものでないのでfilter表現になるのも妥当だ、と言う意見もありそう。その意見に対してどう主張する?
500じゃねーだろというDefectならいいかも