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 key collision with Thread[:current_request] #20973

Merged
merged 1 commit into from
Jan 21, 2021

Conversation

Fryguy
Copy link
Member

@Fryguy Fryguy commented Jan 20, 2021

Now that we've moved to manageiq-loggers, the ContainerLogger has code
that expects that if Thread[:current_request] is set, it will be a
request object that responds to :request_id. The
RequestStartedOnMiddleware, however, happens to use the same key and
stores only the request path, leading to undefined method request_id for "/path/to/wherever"

Since this particular key's usage is isolated to the middleware, we can
name it whatever we want, so this commit chooses a name that is less
likely to collide.

Now that we've moved to manageiq-loggers, the ContainerLogger has code
that expects that if Thread[:current_request] is set, it will be a
request object that responds to :request_id.  The
RequestStartedOnMiddleware, however, happens to use the same key and
stores only the request path, leading to `undefined method request_id
for "/path/to/wherever"`

Since this particular key's usage is isolated to the middleware, we can
name it whatever we want, so this commit chooses a name that is less
likely to collide.
@Fryguy
Copy link
Member Author

Fryguy commented Jan 20, 2021

cc @abellotti

Copy link
Member

@NickLaMuro NickLaMuro left a comment

Choose a reason for hiding this comment

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

Just an idea to make it a bit more unique.

(other than that, though, I think this change makes sense, to 👍 from me)

lib/request_started_on_middleware.rb Show resolved Hide resolved
@miq-bot
Copy link
Member

miq-bot commented Jan 20, 2021

Checked commit Fryguy@0109f04 with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. 👍

@jrafanie jrafanie merged commit c52be90 into ManageIQ:master Jan 21, 2021
@Fryguy Fryguy deleted the fix_current_request_bug branch January 21, 2021 14:26
@jrafanie jrafanie added the core label Jan 21, 2021
@jrafanie
Copy link
Member

@Fryguy should this go back to kasparov?

simaishi pushed a commit that referenced this pull request Jan 29, 2021
Fix key collision with Thread[:current_request]

(cherry picked from commit c52be90)
@simaishi
Copy link
Contributor

Kasparov backport details:

$ git log -1
commit cd7f4028fc66146c0cec34cecee97dbc89a599fe
Author: Joe Rafaniello <jrafanie@users.noreply.github.com>
Date:   Thu Jan 21 09:25:21 2021 -0500

    Merge pull request #20973 from Fryguy/fix_current_request_bug

    Fix key collision with Thread[:current_request]

    (cherry picked from commit c52be90a6808465b28952eca06121029bd9fa3ec)

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

Successfully merging this pull request may close these issues.

6 participants