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

X-Profile-Data value is a string #40

Merged
merged 1 commit into from
Dec 14, 2021
Merged

Conversation

pedropb
Copy link
Contributor

@pedropb pedropb commented Dec 14, 2021

Some middlewares expect all of the response headers values to be String but X-Profile-Data is currently emitting a Pathname. This commit change it so that it is explicitly converted to a String, before sending the value downstream.

I ran into an issue specifically with thin where it was parsing the response headers and raised the following error:

Unexpected error while processing request: undefined method `each' for #<Pathname:0x000000010a196080>

Is there a reason to use Pathname over String?

Some middlewares expect all of the response headers to be String but
X-Profile-Data is currently a Pathname. This commit change it so that
it's explicitly converted to a String, before sending the value
downstream.
Copy link
Member

@gmcgibbon gmcgibbon left a comment

Choose a reason for hiding this comment

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

cc @Shopify/sysperf

@@ -44,6 +44,7 @@ class UploadActionTest < AppProfiler::TestCase

assert_predicate(@response[1][AppProfiler.profile_header], :present?)
assert_predicate(@response[1][AppProfiler.profile_data_header], :present?)
assert_equal(@response[1][AppProfiler.profile_data_header].class, String)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assert_equal(@response[1][AppProfiler.profile_data_header].class, String)
assert_kind_of(String, @response[1][AppProfiler.profile_data_header])

@pedropb pedropb merged commit 1e0a81d into main Dec 14, 2021
@pedropb pedropb deleted the x-profile-data-is-a-string branch December 14, 2021 21:30
@pedropb
Copy link
Contributor Author

pedropb commented Dec 15, 2021

FWIW the Pathname inside the response header was also breaking unicorn

ERROR -- : app error: undefined method `=~' for #<Pathname:0x0000041a55ab6fd0> (NoMethodError)

@shopify-shipit shopify-shipit bot temporarily deployed to production March 2, 2022 15:30 Inactive
This pull request was closed.
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.

3 participants