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

Could not log "process_action.action_controller" event. ArgumentError: wrong number of arguments (0 for 1) when validation error occured #126

Closed
a-know opened this issue May 27, 2015 · 10 comments · Fixed by #134
Assignees

Comments

@a-know
Copy link

a-know commented May 27, 2015

Issue

In bug fix of the issue #110 , you had modified following this.

https://github.com/roidrage/lograge/pull/112/files#diff-d3a7c7717409a1c1353805f9579b980bR64

But, in get_error_status_code method, there is exception_object = exception.constantize.new .

https://github.com/roidrage/lograge/pull/112/files#diff-d3a7c7717409a1c1353805f9579b980bR71

If the exception requires arguments (for example, as of ActiveRecord::RecordInvalid), exception_object = exception.constantize.new throw ArgumentError: wrong number of arguments.
Such errors cause Could not log "process_action.action_controller" event. ArgumentError: wrong number of arguments (0 for 1) in 85 line of activesupport-4.2.0/lib/active_support/log_subscriber.rb .

Environments

  • Lograge version : 0.3.2
  • Rails version : 4.2.0
  • RVM/rbenv/chruby/etc are not used
  • Ruby version : 2.2.2p95 (2015-04-13 revision 50295)
  • OS : Cent 7
@sakuro
Copy link

sakuro commented May 27, 2015

Should use ActionDispatch::ExceptionWrapper.status_code_for_exception instead of instantiating a new exception from the class name and use its name?

@pxlpnk pxlpnk self-assigned this Jun 2, 2015
@spscream
Copy link

spscream commented Jun 5, 2015

Any workarounds? I have the same issue with logger.

@pxlpnk
Copy link
Collaborator

pxlpnk commented Jun 5, 2015

I will look into this the coming weekend. Thanks for the input @sakuro

@pxlpnk
Copy link
Collaborator

pxlpnk commented Jun 6, 2015

I looked into it,
using ActionDispatch::ExceptionWrapper.status_code_for_exception would work for a number of errors:
https://github.com/rails/rails/blob/1ca13b47da46247345a0d4547edc495f0cc83bf6/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb#L10-L20

But it would completely ignore the ActiveRecord::NotFound and return a 500 for this.
Maybe there could be a combination of both approaches to catch more of the status codes.

@sakuro
Copy link

sakuro commented Jun 7, 2015

AR seems that it registers AR specific exceptions to the mappings, right?

https://github.com/rails/rails/blob/1ca13b47da46247345a0d4547edc495f0cc83bf6/activerecord/lib/active_record/railtie.rb#L25

@pxlpnk
Copy link
Collaborator

pxlpnk commented Jun 7, 2015

I expected that too after reading the code, but when executing the code with tests there is no AR specific mapping in the exception wrapper. Maybe this just gets loaded when you run rails as an "actual" instance?

@andywenk
Copy link

We have the same issue in our production system. Can I help with any info? Any progress here? Thanks a lot!!!

Example stacktrace can be found here: https://gist.githubusercontent.com/andywenk/2aa6e974e637ad93bde9/raw/ab0fdf9c718f5626666252c90df3db9fbcb9e680/gistfile1.txt

@pxlpnk
Copy link
Collaborator

pxlpnk commented Jun 15, 2015

Sorry I am currently a bit down under. We are happy about a pull request that would fix this.

@benlovell benlovell assigned benlovell and unassigned pxlpnk Jun 15, 2015
@benlovell
Copy link
Collaborator

I'm looking into this now. Hold tight.

benlovell added a commit that referenced this issue Jun 15, 2015
This resolves #126 by not attempting to create exception classes. A
couple of points no less:

1. The `ActiveRecord` specific exceptions _are_ loaded and merged when
its railtie is loaded. Our tests do not load `ActiveRecord` so while
they don't exist in our context, they _will_ be around in your garden
variety rails application.
2. We essentially replicate what the `ExceptionWrapper` was doing for
us, but by leveraging `Rack::Utils.status_code` we avoid the need to
create an instance of the actual exception class. Thus avoiding the
problems experienced creating exceptions that expect initializer
arguments.
@benlovell
Copy link
Collaborator

@pxlpnk Ideally we'd have at least a few integration specs that load rails in its entirety to cover this end to end, but for now at least I think #134 should resolve this issue.

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

Successfully merging a pull request may close this issue.

6 participants