Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace log_process_action to append_info_to_payload #4375

Merged
merged 2 commits into from
Nov 22, 2017

Conversation

fbbergamo
Copy link
Contributor

@fbbergamo fbbergamo commented Dec 10, 2016

This implementation replace log_process_action to append_info_to_payload that way the status should be present in ActiveSupport::Notifications's payload and at the log

It's based on what AR is doing:
https://github.com/rails/rails/blob/92703a9ea5d8b96f30e0b706b801c9185ef14f0e/activerecord/lib/active_record/railties/controller_runtime.rb#L35

No test change was needed but one test was include.

@benlovell
Copy link

Is there any appetite for landing this change? Happy to help get it across the line if anything outstanding.

@@ -543,6 +543,18 @@ class AuthenticationOthersTest < Devise::IntegrationTest
refute warden.authenticated?(:user)
end
end

test 'not signed in should returns notification payload with 401 status' do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo, returns => return

@jonpaul
Copy link

jonpaul commented Aug 8, 2017

Would love to see this re-reviewed.

@myriky
Copy link

myriky commented Sep 4, 2017

I hope this PR will be merge.

@fbbergamo
Copy link
Contributor Author

@rafaelfranca @lucasmazza
Is there anything that I may help to merge this PR?

@rafaelfranca rafaelfranca merged commit 701d492 into heartcombo:master Nov 22, 2017
@fbbergamo fbbergamo deleted the change-status-append branch November 22, 2017 21:19
@rafaelsales
Copy link

rafaelsales commented Feb 28, 2018

This broke my logging when I upgraded from 4.3.0 to 4.4.0, and 4.4.1 is still broken. I'm on Rails 5.1.4

I noticed that logs in my app started reporting Completed 401 Unauthorized when the action failed with an unhandled exception. The client still receives 500 Internal Error, but the log reports 401.

I'm checking how to fix it

@rafaelfranca @lucasmazza @fbbergamo

@tegon
Copy link
Member

tegon commented Feb 28, 2018

@rafaelsales I'm not sure it's related, we released this on v4.4.0

@rafaelsales
Copy link

@tegon sorry, I had a typo in the version numbers of my comment. Just fixed.

If I revert this particular commit, the previous behavior is established. I'm building a test case to reproduce.

@rafaelsales
Copy link

rafaelsales commented Feb 28, 2018

@tegon Here's the test case to reproduce - this commit was made on top of the latest master:
rafaelsales@302c53e

I tried to dig in a little bit and I found out that in the following snippet of this PR, the payload[:exception] always returns nil even if a public action raises an error, so the 401 status is added to the payload.

        def append_info_to_payload(payload)
          super
          payload[:status] ||= 401 unless payload[:exception]
        end

@tegon
Copy link
Member

tegon commented Feb 28, 2018

@rafaelsales I see. Can you open a PR for this? If you don't know how to solve it, just add a failing test example so that other person can work on it.

@rafaelsales
Copy link

Sure, thanks

@stanhu
Copy link
Contributor

stanhu commented Mar 20, 2020

I did some investigation on this. Cross-linking my thoughts in the Lograge issue tracker: roidrage/lograge#67 (comment)

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

Successfully merging this pull request may close these issues.

9 participants