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 webhook requests for headers with a symbol key #977

Merged
merged 3 commits into from
Jun 29, 2022

Conversation

phylor
Copy link
Contributor

@phylor phylor commented Jun 16, 2022

Description

Webhooks fail when there is a header with a symbol as a key:

ShopifyAPITest::Webhooks::WebhookRequestTest#test_with_symbol_headers:
NoMethodError: undefined method `sub' for :clearance:Symbol
Did you mean?  stub

This happens e.g. when using Clearance, which adds :clearance as a header, containing the Clearance session. This means that webhooks will always fail as soon as you include Clearance into the project. Its middleware always adds that header, even if there is no authenticated request.

How has this been tested?

Test case within this PR.

Checklist:

  • My commit message follow the pattern described in here
  • I have performed a self-review of my own code.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have updated the project documentation.
  • I have added a changelog line.

@phylor phylor requested a review from a team as a code owner June 16, 2022 13:26
Copy link
Contributor

@paulomarg paulomarg left a comment

Choose a reason for hiding this comment

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

Looks good, thank you for adding a test as well!

Seems we got a couple of rubocop failures, could you please address those so we can merge?

@phylor phylor force-pushed the feature/support-symbol-headers branch from 2032792 to a4e8dbc Compare June 21, 2022 14:00
@phylor
Copy link
Contributor Author

phylor commented Jun 21, 2022

Seems we got a couple of rubocop failures, could you please address those so we can merge?

@paulomarg Fixed!

@paulomarg
Copy link
Contributor

Sorry for the delay in getting back to you @phylor, but it seems like we still have one failure when running the sorbet checks (bundle exec srb tc). Can you make sure that's passing so we can merge the PR? Thank you!

phylor added 3 commits June 28, 2022 21:40
E.g. the clearance session is called `:clearance` and it then fails with the error:

```
ShopifyAPITest::Webhooks::WebhookRequestTest#test_with_symbol_headers:
NoMethodError: undefined method `sub' for :clearance:Symbol
Did you mean?  stub
```
@phylor phylor force-pushed the feature/support-symbol-headers branch from a4e8dbc to 12abba9 Compare June 28, 2022 19:41
@phylor
Copy link
Contributor Author

phylor commented Jun 28, 2022

Sorry for the delay in getting back to you @phylor, but it seems like we still have one failure when running the sorbet checks (bundle exec srb tc). Can you make sure that's passing so we can merge the PR? Thank you!

No problem, sorry for the failures 🙂 Rebased on main and sorbet is green for me locally.

@phylor
Copy link
Contributor Author

phylor commented Jun 28, 2022

@paulomarg Sorry for the trouble. We're now getting Rubocop issues not relevant to my changes. Shall I still fix them in this PR?

@paulomarg paulomarg merged commit a176969 into Shopify:main Jun 29, 2022
@paulomarg
Copy link
Contributor

No need, we'll fix those! Thank you.

@shopify-shipit shopify-shipit bot temporarily deployed to rubygems July 4, 2022 16:27 Inactive
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.

2 participants