Skip to content

Validate the backend in params #141

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.

Sign up for GitHub

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

Open
sam-kayg wants to merge 1 commit into
base: main
Choose a base branch
from
+17 −3
Open

Validate the backend in params #141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions lib/app_profiler/request_parameters.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,13 @@ def backend
end

def valid?
if mode.blank?
return false if mode.blank?

unless valid_backend?
AppProfiler.logger.info("[AppProfiler] unsupported backend='#{backend}'")
return false
end

return false if backend != AppProfiler::Backend::StackprofBackend.name && !AppProfiler.vernier_supported?

if AppProfiler.vernier_supported? && backend == AppProfiler::Backend::VernierBackend.name &&
!AppProfiler::Backend::VernierBackend::AVAILABLE_MODES.include?(mode.to_sym)
AppProfiler.logger.info("[AppProfiler] unsupported profiling mode=#{mode} for backend #{backend}")
Expand All @@ -50,6 +51,12 @@ def valid?
true
end

def valid_backend?
return true if AppProfiler::Backend::StackprofBackend.name == backend
Copy link
Contributor

@bmansoob bmansoob Oct 18, 2024

Choose a reason for hiding this comment

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

I know it achieves the same outcome and it is probably a matter of personal preference - but I would rather get rid of AppProfiler::Backend::StackprofBackend.name == backend and have something which checks if backend is one of the two. We can define a constant AVAILABLE_BACKENDS


AppProfiler.vernier_supported? && AppProfiler::Backend::VernierBackend.name == backend
end

def to_h
{
mode: mode.to_sym,
Expand Down
7 changes: 7 additions & 0 deletions test/app_profiler/request_parameters_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,13 @@ class RequestParametersTest < TestCase
end
end

test "#valid? returns false when backend is not supported" do
AppProfiler.logger.expects(:info).with { |value| value =~ /unsupported backend='not-a-real-backend'/ }
Copy link
Contributor

@bmansoob bmansoob Oct 18, 2024

Choose a reason for hiding this comment

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

Maybe we do not need check the logs and valid? is sufficient.

params = request_params(headers: { AppProfiler.request_profile_header => "mode=cpu;backend=not-a-real-backend" })

assert_not_predicate(params, :valid?)
end

test "#context is AppProfiler.context by default" do
with_context("test-context") do
AppProfiler.logger.expects(:info).never
Expand Down