Go to Qiita Advent Calendar 2024 Top
LoginSignup
171
153

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?

More than 5 years have passed since last update.

Ruby on RailsAdvent Calendar 2018

Day 17
@yuyasat

現場から学ぶ、RailsのControllerアンチパターン

Last updated at Posted at 2018-12-16

0. はじめに

　最近、新規のRailsプロジェクトで一から実装されていて、Controllerの実装の中でこれは分かりにくいなと思ったコードがあり、Controllerのアンチパターンと言えるのではないかと感じたので、改めてここに記述したいと思います。
　RailsのControllerのアンチパターンは検索するといくつか出てきますが、今回はそれとは関係なく、私が出会った実体験を元に紹介したいと思います。
　今回のControllerは、フォームでユーザーを作成したり、申込みを行ったりするControllerに注目しています。APIとしてクライアントサイドから呼びされるControllerではまた状況が異なると思いますので、ご留意ください。

1. POSTしたあとにrenderしている

　私は、例えば次のようなコードをみたことがあります。このコードでは、userの作成に成功した後にshowをrenderしています。

def create
  user = User.new(user_params)
  if user.save
    render 'show'
  else
    render 'new'
  end
end

1-1. アンチパターンである理由

フォームにおいて、POSTしたあとはredirectするのがWeb開発においては定石といえます。Rails Tutorialには、文章上で明記されていないように思いますが、Djangoのチュートリアルには以下の文の記載があります。1

POST データが成功した後は常に HttpResponseRedirect を返す必要があります。これは Django 固有のものではなく、 Web 開発における良いプラクティスです。

　DjangoにおいてHttpResponseRedirectを返すとはRailsでいえばredirect_toを返すということに相当します。
　POSTが成功した後にredirectする理由の一つは二重投稿防止です。これは、一般にPRGパターンと呼ばれるもので、二重投稿防止については「さいきょうの二重サブミット対策」が詳しいのでぜひご覧ください。

図1. リロードすると二重投稿されることがある。Chromeではこのような確認が出ることもある。

1-2. アンチパターンを回避する方法

　PRGパターンに従い、userの保存に成功した場合は適切なページへリダイレクトします。

def create
  user = User.new(user_params)
  if user.save
    redirect_to user_url(user)
  else
    render 'new' # こちらについては後述
  end
end

2. 同一アクション内で条件によってrender/redirectの挙動が異なる

例えば次の例1では、saveに成功した時はroot_urlにredirectし、失敗した時はrenderする挙動になっています。この例は、私もRailsを一番最初に勉強する際に学んだRails TutorialのUsersControllerから拝借していますが、今思えばこれはアンチパターンだと思っています。

def create
  @user = User.new(user_params)
  if @user.save # => Validation
    @user.send_activation_email
    flash[:info] = "Please check your email to activate your account."
    redirect_to root_url
  else
    # Failure
    render 'new'      
  end
end

2-1. アンチパターンである理由

　今回のようなバリデーションに成功/失敗であれば、挙動がわかりやすいですが、実運用されているコードでは、こんなにシンプルなロジックであることは稀です。そのこと自体、FatControllerとして避けるべきですが、実運用では必ずしも安全にリファクタリングができる時間が確保されるとは限りません。したがって、Fat化したControllerにおいて、状況によってrenderとredirectの挙動が変わるのは、（１００歩譲って最初の実装者にとっては分かりやすくても）後から参画した開発者の頭を悩ませることになります。
　こうしたバリデーションに失敗した後の挙動の期待としては、入力フォームに戻ってきてユーザーに入力を再度行わせ、エラーを解消してもらうことです。Rails Tutorialのここの実装の場合、URLが/signupから/usersに変わっており、まずこの時点で違和感がありますが、これだけでは機能上の問題とは言えません。しかし、この実装は機能としても問題であると言えます。
　ユーザーはこのあとどのような操作をするかを考えた時に、もちろん素直に入力し直してユーザー作成を行ってくれることもありますが、ブラウザのリロードを行なうこともあると想定されます。Rails Tutorialでは/usersでリロードを行なうとindexアクションが呼ばれ、before_actionのlogged_in_userが実行され、/loginにリダイレクトされます。これは申込みサイトにおいて、大変困った挙動といえます。フォームの数がたくさんある申込み機能において、一生懸命入力した内容がなんとなくF5ボタンを押しただけで消去されてしまうのです。
image.png
図2. エラー時にはURLが変わってしまう

2-2. アンチパターンを回避する方法

　いくつか追加で実装しなければいけません。
　まず一つ目はエラーメッセージの表示です。Railsであればflashを活用するのがよいです。次のようにflashに@user.errors.full_messagesを入れておき、view側でflashから取り出します。

def create
  @user = User.new(user_params)
  if @user.save # => Validation
    # Sucess
    @user.send_activation_email
    flash[:info] = "Please check your email to activate your account."
    redirect_to root_url
  else
    # Failure
    flash[:danger] = @user.errors.full_messages
    redirect_to signup_url
  end
end

<% flash.each do |message_type, messages| %>
  <% messages.each do |message| %>
    <div class="alert alert-<%= message_type %>"><%= message %></div>
  <% end %>
<% end %>

　最近では、サーバーサイドのバリデーションとフロントエンドを連携し、サーバーサイドでエラーになった箇所は赤く目立たせるなどの処理を行うことがあります。そういう場合は、flashに入れる形式をHashにし、key, value形式で持たせると良いです。view側ではkeyでエラーとなった属性値を取得できるのでフロント側と連動する実装を行います。

flash[:danger] = @user.errors.keys.map { |key| [key, @user.errors.full_messages_for(key)] }.to_h
# => {:email=>["Email can't be blank", "Email is invalid"], :error=>[]}

　二つ目は、POSTした値の保持です。redirectしてしまうとPOSTの値を保持できなくなります。これではユーザビリティを著しく損なってしまいます。一番簡単なのはsessionに保持しておく方法です。createで失敗した際にはsessionに保存し、redirect先のnewではsessionからUserインスタンスを復元するようにします。

  def create
    @user = User.new(user_params)
    if @user.save # => Validation
      # Sucess
      @user.send_activation_email
      session[:user] = nil # *1 成功した際は速やかに削除する
      flash[:info] = "Please check your email to activate your account."
      redirect_to root_url
    else
      # Failure
      session[:user] = @user.attributes.slice(*user_params.keys) # *2 フォームで渡された値のみ保存
      # *3 session[:user] = user_params としないこと！
      flash[:danger] = @user.errors.full_messages
      redirect_to signup_url
    end
  end

def new
  @user = User.new(session[:user] || {}) # sessionからUserインスタンスを生成
end

この実装の際のポイントを上記コードにコメントしましたが、もう少し補足を行わせてください。
*1: sessionに長らく情報を保持しておくと、色々とsessionの値のハンドリングが面倒なことがあるので、使用し終えたら極力速やかに削除しておいたほうが良いです。
*2: sessionに保存する情報ですが、@user.attributesとしてしまうと、idやcreated_atなど必要でない値まで保存してしまうので、フォームで渡された値のみを保持するようします。場合によってはパスワードなどはもう一度入力してもらうため、sessionに保存しないようにする必要があるかもしれません。
*3: 保存するときにuser_paramsをそのまま保存しないようにします。これは、RailsのバージョンによってActionController::Parametersの仕様が変わる可能性があるからです。現に私はRails4から5にあげる過程でこの問題に直面しました。できる限り、純粋なHash型で保存しておくべきだと思います。

2-3. sessionに保存する際の注意点

　サーバーサイドのsessionはブラウザのどのタブで操作が行われたかを判別することができません。というのも、sessionはCookieの_session_idなどと紐づけられて管理されますが、Cookieはタブごとに別れておらず、ブラウザを通じて一つです。これはブラウザの仕様なのでサーバーサイドでどうにかできる問題ではありません。
　別タブでも同じsession情報として保存されてしまうということは複数タブで同じフォームを開きサブミットすると、後から開いたフォームの値がsessionに保存され、先に開いたフォームでサブミットしても後のフォームの値が送信されてしまうという事象を引き起こします。俗に複数タブ問題と呼ばれる事象です。これは、システムによってはクリティカルな問題となり、できればサーバーサイドのsessionで保存すべきではありません。2
　ここで、何度か「サーバーサイドの」という言葉を使用しました。つまり、サーバーサイドでなければ回避する方法があります。現在のブラウザには、sessionStorageという機能3（WebStorageの一つ）が備えられておりJavaScriptなどを用いてsessionStorageに保存すれば別タブは別のsessionStorageに保存されるので複数タブ問題が回避できます。例えば、Vue.jsではvuejs-storage4などの便利なパッケージが開発されており、簡単に値をsessionStorageに格納することができます。

3. アクション、before_action以外でrenderやredirectを行なう

3-1. アンチパターンである理由

　いうまでもなく、Controllerでは、アクションメソッド以外にも通常のメソッドも利用できます。例えば、メールアドレスにexampleという文字列が含まれている場合はroot_urlにredirectする処理を行いたいとします。次のコードは正しく動くでしょうか。

Wrong
def create
  @user = User.new(user_params)
  redirect_if_example_email
  
  if @user.save # => Validation
    # Success
    @user.send_activation_email
    session[:user] = nil
    flash[:info] = "Please check your email to activate your account."
    redirect_to user_url(@user)
  else
    # Failure
    session[:user] = @user.attributes.slice(*user_params.keys)
    flash[:danger] = @user.errors.full_messages
    redirect_to signup_url
  end
end

def redirect_if_example_email
  redirect_to root_url if @user.email.include?('example')
end

　これは正しく動きません。redirect_if_example_emailの中でredirect_toを使った後、さらにifの中でもredirect_toを使っているので、AbstractController::DoubleRenderErrorが発生してしまいます。それを回避するには、

redirect_if_example_email and return

のようにand returnを追加してあげる5か、

redirect_if_example_email
return if performed?

のようにperfomed?メソッドで判断してあげる必要があります。perfomed?メソッドは、すでにそのアクション内でrenderかredirectがされたかを確認するメソッドです6が、一つのアクションの中でrenderやredirectが色々な箇所で行われると、そのアクションのどこでどうrenderやredirectをされているか分からなくなってしまいます。今回の例ではロジックが比較的シンプルなので大きな混乱はないと思います。しかし、これが肥大化するとコードの見通しが非常に悪くなりメンテナンス性を低下させます。私は、アクションの中ではrenderやredirectは局所化することが望ましいと考えています。

3-2. アンチパターンを回避する方法

　今回のケースでは、emailにexampleという文字列が含まれているかがredirectするかしないかの判断条件になっていました。これは、本来ならば、モデルのバリデーションで行うべき実装です。
　次は、emailにexampleという文字列が含まれているかをのチェックをモデルに移した例です。

user.rb
class User < ApplicationRecord
  VALID_EMAIL_REGEX = /\A[\w+\-.]+@[a-z\d\-.]+\.[a-z]+\z/i
  validates :email, presence: true, length: { maximum: 255 },
                    format: { with: VALID_EMAIL_REGEX },
                    uniqueness: { case_sensitive: false }
  validate :validate_email # <= 追加
  # メソッド化したくない場合は、ラムダで渡しても良い
  # validate -> {
  #   errors.add(:email, "should not contain 'example'") if email.include?('example')
  # }
  
  private

  def validate_email
    errors.add(:email, "should not contain 'example'") if email.include?('example')
  end
end

　こうすることで、Controller内に必要以上にロジックを増やすことなくアクションをシンプルにできます。Controller内で異様な量のバリデーションが記載されているプロダクションをいくつか見てきました。これではテストも書きにくいですが、ロジックをモデルに移すことでテストも書きやすくなります。
　ActiveRecordのモデルではない場合は、FormObjectを利用することを検討しても良いでしょう。

4. もう少し複雑な例

　これまで紹介した例は比較的シンプルでしたが、私が実際に出会った例を紹介します。少し複雑になるので、興味のある方のみお読みください。

　以下の例では、updateアクション内で二つのことを行なっています。この呼び出し元のビューには送信ボタンが二つあり、どちらの送信ボタンが押されたかによって挙動が変わります。nameにcommit_apiと指定されている送信ボタンを押した場合は、APIを呼ぶ。APIにはAPI1とAPI2の二つがあり、API2を呼ぶ前にAPI1を呼ばなければいけないようです。

def update
  # 送信ボタンが二つあり、送信ボタンのnameによって挙動を変える
  return send_to_api_2 if params.key?(:commit_api)

  update_order
end

send_to_api_2メソッドとupdate_orderはControllerのprivateメソッドとして定義されてあります。（実際には、同様の処理を別のControllerでも用いるので、concernにありました。）
update_orderメソッドでは、フォームから渡されたパラメータを元に@orderを更新する処理を行います。saveに失敗した時にcommon_showというメソッドが呼び出されていますが、のちほど出てきます。このメソッド名を見るとコードのにおいというのを感じます。

private

def update_order
  @order.assign_attributes(order_params)
  @order.status = :confirmed unless @order.status_changed?
  if @order.save
    redirect_to request.referer
  else
    common_show
    @error_messages = ['エラーがあります。']
    render :show
  end
end

　次は、updateアクションから呼び出されるsend_to_api_2というメソッドです。　
　まず［1］では@orderのステータスがconfirmではない場合はrender_errorメソッドを呼び出しています。呼び出し元のupdateアクションはPOSTであり、送信する前は/orders/1などというURLですが/updateというURLになります。エラーになった後ブラウザをリロードすると、POSTが実行され、何度もAPI連携されてしまいます。POSTしたあとにredirectしないことが無駄なAPI連携を誘発させています。
　このメソッドの中では、API1とAPI2を読んでいます。API1は、send_to_api_1というメソッドを呼び出すことでAPI1がコールされています。send_to_api_1の中でもrenderが実行されることがあり、［2］でperformed?を用いることでAbstractController::DoubleRenderErrorを回避しています。同じメソッドなのに、APIコールに失敗した時と成功した時で返り値の型が異なるようになっています。Rubyは返り値の型が条件により異なるような実装も可能ですが、どんな条件でも極力同じ型で返すのが良いでしょう。

def send_to_api_2
  return render_error('ステータスが確認済みではありません') unless @order.status_confirmed? # ・・・[1]

  send_to_api_1 # API2を呼ぶ前に、API1を呼ぶ 
  return if performed? # ・・・[2]

  # Api2クラスはAPI2をコールするために実装したクラス
  response = Api2.post("/api2_url", params: { order: @order })
  unless response.success? 
    @error_messages = response.errors # 詳細な実装は、GET先のインターフェスによる
    raise # ・・・[3]
  end

  @order.status = :linked
  @order.save!

  @notice = 'API2に送信しました。'
  redirect_to request.referer
rescue StandardError => e # ・・・[4]
  @notice = nil
  @error_messages = ['API2に失敗しました。'].concat(@error_messages || [])
  render_show # ・・・[５]
end

　そして、API1を呼び出すメソッドであるsend_to_api_1です。　send_to_api_1の中では、呼び出しに失敗した場合、意図的にエラーを起こし（［6］）、rescueのなかでrenderしています（［7］）。send_to_api_1はAPI1の呼び出しに成功した場合はrenderもredirectも行いませんが、エラーとなった場合にのみrenderするといういびつなメソッドになってしまっています。

def send_to_api_1
  # Api1クラスはAPI1をコールするために実装したクラス
  response = Api1.get("/ap1_url", params: { order: @order })
  unless response.success?
    @error_messages = response.errors # 詳細な実装は、GET先のインターフェスによる
    raise  # ・・・[６]
  end

  @notice = 'API1に送信しました。'
rescue StandardError => e
  @notice = nil 
  @error_messages = ['API1に失敗しました。'].concat(@error_messages || [])
  render_show # ・・・[７]
end

　残りは、これまでの関数の中で呼び出されたメソッドが実装されています。render_showやcommon_showといった関数は2.で紹介したように、成功した時も失敗した時もPOSTのときはredirectするようにすれば、必要のないメソッドです。うまく共通化されたように感じるかもしれませんが、共通化の前に考えることは共通化せずに実装できないかということです。今回の場合は、send_to_api_1やsend_to_api_2のエラー時にrenderしてしまっているのですが、エラー時もredirectするようにすれば、この処理はすべてshowに集約することができます。

def render_show
  common_show
  render :show
end

def render_error
  @error_messages ||= []
  @error_messages << message
  render_show
end

def common_show
  # 省略
  # インスタンス変数などをセット
end

　今回のケースの場合、send_to_api_1やsend_to_api_2の返り値はシンプルにAPIに失敗したか成功したかをtrue, falseで返し、エラーメッセージは返り値の二つ目で返すようにすると見通しが良くなると思いました。そして、redirect先は、すべて詳細ページに統一します。エラーメッセージはインスタンス変数ではなく、flashで渡すようにします。
　以下に、上記のコードよりも見通しを良くしたと感じているコードを次に示します。この例では、アクション以外のメソッドの返り値を配列にし、一つ目に成功したかどうか、二つ目にメッセージを入れるようにしています。より汎用的に用いられるメソッドで分かりやすくするには、Hash型にして{ success: false, error_code: 'ERR001', messages: ['メッセージ'] }などとしても分かりやすいでしょう。

def update
  success, messages = if params.key?(:commit) 
                        send_to_api_2
                      else
                        update_order
                      end
  flash[success ? :notice : :error] = messages

  redirect_to show_url(@order),
end

def update_order
  @order.assign_attributes(order_params)
  @order.status = :confirmed unless @order.status_changed?
  if @order.save
    [true, ['保存しました']]
  else
    [false, ['エラーがあります。各項目を確認してください。'] + @order.errors.full_messages]
  end
end

def send_to_api_2
  # この条件をOrderモデル内でcontextつきのvalidationにしても良い。
  return [false, ['ステータスが確認済みではありません']] unless @order.status_confirmed?

  success, messages = send_to_api_1 # API2を呼ぶ前に、API1を呼ぶ
  return [false, ['API1に失敗しました'] + messages] unless success

  response = Api2.post("/api2_url", params: { order: @order })
  if response.success?
    @order.status = :linked
    @order.save!
    [true, ['API2に送信しました']]
  else
    [false, ['API2に失敗しました'] + response.errors]
  end
rescue StandardError => e
  [false, ['API連携に失敗しました']]
end

def send_to_api_1
  response = Api1.get("/ap1_url", params: { order: @order })
  if response.success?
    [true, ['API1に送信しました']]
  else
    [false, response.errors]
  end
end

　現場からは以上です。
　3年間のRailsの現場での経験を踏まえて、改めてRails　Tutorialを見てみると、必ずしもベストプラクティスな実装にはなっていない箇所があります。それは、Rails TutorialがRails初心者をターゲットにしており、細かな実装にこだわると本筋から離れてしまい、焦点がぶれたチュートリアルになってしまうことを懸念したものだともいえます。Rails TutorialでRailsの概要を理解した上で、開発現場でどういう実装がベストなのかを議論し、各々のエンジニアの経験も踏まえた上で実装を決めて行くのが良いでしょう。
　本記事は以上ですが、あくまで自分の実感としてすので、「いや違うよ」、「こうしたほうがいいのではないか」という意見もあると思います。私自身、模索しながら実装ですので、間違っている点も多々あると思います。ご指摘いただければ幸いです。また、議論のきっかけとなり、より良い実装が浸透していけば嬉しいです。

  1. https://docs.djangoproject.com/ja/2.1/intro/tutorial04/ ↩ ↩2

  2. 今回の場合、sessionに格納された情報はエラー時のみ使用しているので複数タブ問題は発生しない。createアクション内では、sessionからではなくフォームに入力された値からUserインスタンスを保存していることに注意。 ↩

  3. https://developer.mozilla.org/ja/docs/Web/API/Window/sessionStorage ↩

  4. https://www.npmjs.com/package/vuejs-storage ↩

  5. redirectの返り値が"<html><body>You are being <a href=\"http://localhost:3000/\">redirected</a>.</body></html>"のような値になっており、これは真偽値ではtrueを意味することを利用している。 ↩

  6. https://apidock.com/rails/ActionController/Metal/performed%3f ↩

171
153
2

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
  3. You can use dark theme
What you can do with signing up
Sign upLogin
171
153

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?