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

Fix gateway_error when no order is defined #4156

Merged
merged 3 commits into from
Sep 8, 2021
Merged

Fix gateway_error when no order is defined #4156

merged 3 commits into from
Sep 8, 2021

Conversation

alexblackie
Copy link
Contributor

If the current controller raises a gateway error, but does not have
@order defined, this error handler will raise a NoMethodError when it
tries to access nil.errors.

We can resolve this by falling back to an empty Order instance if none
is defined. This maintains the existing behaviour, and as the response
only serializes the errors hash, there shouldn't be any surprises with
null fields, etc.

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have added tests to cover this change (if needed)

We use the `controller` helper that RSpec provides, and so this class
was never used.
If the current controller raises a gateway error, but does not have
`@order` defined, this error handler will raise a NoMethodError when it
tries to access `nil.errors`.

We can resolve this by falling back to an empty Order instance if none
is defined. This maintains the existing behaviour, and as the response
only serializes the errors hash, there shouldn't be any surprises with
null fields, etc.

Signed-off-by: Alex Blackie <alex@alexblackie.com>
These contexts were flipped accidentally.
@kennyadsl
Copy link
Member

Thanks, @alexblackie! Just to add some context, in which circumstances can this happen? I saw the specs but with a standard checkout flow it's very unlikely that we'll have an exception related to the gateway in a controller that is not related to the checkout where, I guess, having an order is mandatory.

@alexblackie
Copy link
Contributor Author

@kennyadsl Ah yeah, for context, this was raised in a custom reimbursements API controller, which interacts with payments (refunds) but does not have an @order.

Since the :authorize_for_order filter is conditional on params, there is no guarantee @order will be set even in Core controllers, unless the child controller assigns it. Also from an architecture/abstraction point of view it seems wrong to have a rescue_from in a base controller make assumptions about pre-existing internal state, especially if that state came from a child.

Copy link
Member

@jarednorman jarednorman left a comment

Choose a reason for hiding this comment

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

Makes sense to me. Thanks, Alex!

@kennyadsl
Copy link
Member

Also from an architecture/abstraction point of view it seems wrong to have a rescue_from in a base controller make assumptions about pre-existing internal state, especially if that state came from a child.

Agree, thanks for sharing the context!

@kennyadsl kennyadsl merged commit 0d61702 into solidusio:master Sep 8, 2021
@alexblackie alexblackie deleted the bug/gateway_error_errors branch September 8, 2021 15:22
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 this pull request may close these issues.

4 participants