From d5be0b5011f06f224595b9bf9caf5ee0a0f17257 Mon Sep 17 00:00:00 2001 From: syasonik <syasonik@gitlab.com> Date: Mon, 2 Mar 2020 17:27:29 -0500 Subject: [PATCH] Make dashboard embedded check more explicit When a metrics dashboard is requested, the key :embedded in the query params is used to determine whether a request is for an embed or a full dashboard. Instead of relying on just the presence of the param, this swaps the check over to whether the param's value represents true/false. --- app/services/metrics/dashboard/base_embed_service.rb | 4 ++++ .../metrics/dashboard/custom_metric_embed_service.rb | 2 +- app/services/metrics/dashboard/default_embed_service.rb | 2 +- app/services/metrics/dashboard/dynamic_embed_service.rb | 2 +- .../metrics/dashboard/grafana_metric_embed_service.rb | 4 ++-- .../metrics/dashboard/gitlab_alert_embed_service.rb | 2 +- .../metrics/dashboard/gitlab_alert_embed_service_spec.rb | 8 +++++++- .../metrics/dashboard/custom_metric_embed_service_spec.rb | 8 +++++++- .../metrics/dashboard/default_embed_service_spec.rb | 2 +- .../metrics/dashboard/dynamic_embed_service_spec.rb | 8 +++++++- .../dashboard/grafana_metric_embed_service_spec.rb | 8 +++++++- 11 files changed, 39 insertions(+), 11 deletions(-) diff --git a/app/services/metrics/dashboard/base_embed_service.rb b/app/services/metrics/dashboard/base_embed_service.rb index 8aef9873ac1..4c7fa454460 100644 --- a/app/services/metrics/dashboard/base_embed_service.rb +++ b/app/services/metrics/dashboard/base_embed_service.rb @@ -5,6 +5,10 @@ module Metrics module Dashboard class BaseEmbedService < ::Metrics::Dashboard::BaseService + def self.embedded?(embed_param) + ActiveModel::Type::Boolean.new.cast(embed_param) + end + def cache_key "dynamic_metrics_dashboard_#{identifiers}" end diff --git a/app/services/metrics/dashboard/custom_metric_embed_service.rb b/app/services/metrics/dashboard/custom_metric_embed_service.rb index 9e616f4e379..456074ae6ad 100644 --- a/app/services/metrics/dashboard/custom_metric_embed_service.rb +++ b/app/services/metrics/dashboard/custom_metric_embed_service.rb @@ -18,7 +18,7 @@ module Metrics # custom metrics from the DB. def valid_params?(params) [ - params[:embedded], + embedded?(params[:embedded]), valid_dashboard?(params[:dashboard_path]), valid_group_title?(params[:group]), params[:title].present?, diff --git a/app/services/metrics/dashboard/default_embed_service.rb b/app/services/metrics/dashboard/default_embed_service.rb index 39f7c3943dd..30a8150d6be 100644 --- a/app/services/metrics/dashboard/default_embed_service.rb +++ b/app/services/metrics/dashboard/default_embed_service.rb @@ -22,7 +22,7 @@ module Metrics class << self def valid_params?(params) - params[:embedded].present? + embedded?(params[:embedded]) end end diff --git a/app/services/metrics/dashboard/dynamic_embed_service.rb b/app/services/metrics/dashboard/dynamic_embed_service.rb index db5b7c9e32a..ff540c30579 100644 --- a/app/services/metrics/dashboard/dynamic_embed_service.rb +++ b/app/services/metrics/dashboard/dynamic_embed_service.rb @@ -22,7 +22,7 @@ module Metrics # for additional info on defining custom dashboards. def valid_params?(params) [ - params[:embedded], + embedded?(params[:embedded]), params[:group].present?, params[:title].present?, params[:y_label] diff --git a/app/services/metrics/dashboard/grafana_metric_embed_service.rb b/app/services/metrics/dashboard/grafana_metric_embed_service.rb index 44b58ad9729..3ad3a2c609e 100644 --- a/app/services/metrics/dashboard/grafana_metric_embed_service.rb +++ b/app/services/metrics/dashboard/grafana_metric_embed_service.rb @@ -6,7 +6,7 @@ # Use Gitlab::Metrics::Dashboard::Finder to retrive dashboards. module Metrics module Dashboard - class GrafanaMetricEmbedService < ::Metrics::Dashboard::BaseService + class GrafanaMetricEmbedService < ::Metrics::Dashboard::BaseEmbedService include ReactiveCaching SEQUENCE = [ @@ -24,7 +24,7 @@ module Metrics # to uniquely identify a grafana dashboard. def valid_params?(params) [ - params[:embedded], + embedded?(params[:embedded]), params[:grafana_url] ].all? end diff --git a/ee/app/services/metrics/dashboard/gitlab_alert_embed_service.rb b/ee/app/services/metrics/dashboard/gitlab_alert_embed_service.rb index 97391a2817c..5515b84f112 100644 --- a/ee/app/services/metrics/dashboard/gitlab_alert_embed_service.rb +++ b/ee/app/services/metrics/dashboard/gitlab_alert_embed_service.rb @@ -18,7 +18,7 @@ module Metrics # custom metrics from the DB. def valid_params?(params) [ - params[:embedded], + embedded?(params[:embedded]), params[:prometheus_alert_id].is_a?(Integer) ].all? end diff --git a/ee/spec/services/metrics/dashboard/gitlab_alert_embed_service_spec.rb b/ee/spec/services/metrics/dashboard/gitlab_alert_embed_service_spec.rb index 37f89b1c419..349e4ea693b 100644 --- a/ee/spec/services/metrics/dashboard/gitlab_alert_embed_service_spec.rb +++ b/ee/spec/services/metrics/dashboard/gitlab_alert_embed_service_spec.rb @@ -29,12 +29,18 @@ describe Metrics::Dashboard::GitlabAlertEmbedService do it { is_expected.to be_truthy } - context 'not embedded' do + context 'missing embedded' do let(:params) { valid_params.except(:embedded) } it { is_expected.to be_falsey } end + context 'not embedded' do + let(:params) { valid_params.merge(embedded: 'false') } + + it { is_expected.to be_falsey } + end + context 'missing alert id' do let(:params) { valid_params.except(:prometheus_alert_id) } diff --git a/spec/services/metrics/dashboard/custom_metric_embed_service_spec.rb b/spec/services/metrics/dashboard/custom_metric_embed_service_spec.rb index 744693dad15..2f03d18cd1f 100644 --- a/spec/services/metrics/dashboard/custom_metric_embed_service_spec.rb +++ b/spec/services/metrics/dashboard/custom_metric_embed_service_spec.rb @@ -35,12 +35,18 @@ describe Metrics::Dashboard::CustomMetricEmbedService do it { is_expected.to be_truthy } - context 'not embedded' do + context 'missing embedded' do let(:params) { valid_params.except(:embedded) } it { is_expected.to be_falsey } end + context 'not embedded' do + let(:params) { valid_params.merge(embedded: 'false') } + + it { is_expected.to be_falsey } + end + context 'non-system dashboard' do let(:dashboard_path) { '.gitlab/dashboards/test.yml' } diff --git a/spec/services/metrics/dashboard/default_embed_service_spec.rb b/spec/services/metrics/dashboard/default_embed_service_spec.rb index 1b88276368c..8e32316433d 100644 --- a/spec/services/metrics/dashboard/default_embed_service_spec.rb +++ b/spec/services/metrics/dashboard/default_embed_service_spec.rb @@ -27,7 +27,7 @@ describe Metrics::Dashboard::DefaultEmbedService, :use_clean_rails_memory_store_ end context 'not embedded' do - let(:params) { { embedded: false } } + let(:params) { { embedded: 'false' } } it { is_expected.to be_falsey } end diff --git a/spec/services/metrics/dashboard/dynamic_embed_service_spec.rb b/spec/services/metrics/dashboard/dynamic_embed_service_spec.rb index c1ce9818f21..ee75284b4ce 100644 --- a/spec/services/metrics/dashboard/dynamic_embed_service_spec.rb +++ b/spec/services/metrics/dashboard/dynamic_embed_service_spec.rb @@ -35,12 +35,18 @@ describe Metrics::Dashboard::DynamicEmbedService, :use_clean_rails_memory_store_ it { is_expected.to be_truthy } - context 'not embedded' do + context 'missing embedded' do let(:params) { valid_params.except(:embedded) } it { is_expected.to be_falsey } end + context 'not embedded' do + let(:params) { valid_params.merge(embedded: 'false') } + + it { is_expected.to be_falsey } + end + context 'undefined dashboard' do let(:params) { valid_params.except(:dashboard_path) } diff --git a/spec/services/metrics/dashboard/grafana_metric_embed_service_spec.rb b/spec/services/metrics/dashboard/grafana_metric_embed_service_spec.rb index a772b911d8a..034d6aba5d6 100644 --- a/spec/services/metrics/dashboard/grafana_metric_embed_service_spec.rb +++ b/spec/services/metrics/dashboard/grafana_metric_embed_service_spec.rb @@ -28,12 +28,18 @@ describe Metrics::Dashboard::GrafanaMetricEmbedService do it { is_expected.to be_truthy } - context 'not embedded' do + context 'missing embedded' do let(:params) { valid_params.except(:embedded) } it { is_expected.to be_falsey } end + context 'not embedded' do + let(:params) { valid_params.merge(embedded: 'false') } + + it { is_expected.to be_falsey } + end + context 'undefined grafana_url' do let(:params) { valid_params.except(:grafana_url) } -- 2.30.9