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

Only use #original_message in Api::BaseController#parameter_missing_error if defined #3940

Merged
merged 1 commit into from
Feb 18, 2021
Merged

Only use #original_message in Api::BaseController#parameter_missing_error if defined #3940

merged 1 commit into from
Feb 18, 2021

Conversation

dividedharmony
Copy link
Contributor

@dividedharmony dividedharmony commented Feb 17, 2021

While working on solidusio-contrib/solidus_related_products#88 for solidus_related_products, I came across a bug that has caused every branch on that repository to fail its specs.

Sometimes, for reasons that I don't completely understand, the ParameterMissing error class does not define #original_message, causing a NoMethodError to be raised, which obscures the original execption and returns a 500 status response.

Further background is detailed in #3941

As error handling should not raise its own obscuring error, use the #try method to obtain #original_message, if defined.

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have updated Guides and README accordingly to this change (if needed)
  • I have added tests to cover this change (if needed)
  • I have attached screenshots to this PR for visual changes (if needed)

Sometimes, either because of older Rails versions or weird load
order bugs, #original_message is not defined for the
ParameterMissing error class.

As error handling should not raise its own obscuring error, use
the #try method to obtain #original_message, if defined.
@dividedharmony dividedharmony marked this pull request as ready for review February 17, 2021 19:20
@dividedharmony dividedharmony changed the title Use #try(:original_message) for exceptions in Api::BaseController Only use #original_message in Api::BaseController#parameter_missing_error if defined Feb 17, 2021
@kennyadsl
Copy link
Member

Last week I was working on trying to fix solidus_related_products spec suite and I was literally going mad on this issue. I also couldn't understand why this happens but I'm glad you find a working solution, thanks!

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Thanks!

@kennyadsl kennyadsl added Needs Backport changelog:solidus_api Changes to the solidus_api gem changelog:solidus_core Changes to the solidus_core gem and removed changelog:solidus_core Changes to the solidus_core gem labels Feb 17, 2021
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Nice work, Larry!

@kennyadsl kennyadsl merged commit 61864f8 into solidusio:master Feb 18, 2021
@dividedharmony dividedharmony deleted the fix-no_method_error-original_message branch February 19, 2021 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:solidus_api Changes to the solidus_api gem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants