Merge pull request #27 from rdytech/NEP-18011-adjust-duplicate-dashboard-filter-validations

NEP-18011-adjust-duplicate-dashboard-filter-validations

Author: jbat

lib/superset/dashboard/datasets/list.rb

@@ -1,5 +1,7 @@
 # WARNING: Does not take into account datasets with queries that have embedded schema references.
-# ie " select * from schema1.table join schema2.table" for a dataset query will not only return the config schema setting not the sql schema reference.
+# ie " select * from schema1.table join schema2.table" for a dataset query will ONLY return the datasets config schema setting not the sql query schema references which refers to 2 distinct schemas.
+#
+# WARNING: Does not return Filter Datasets for the dashboard
 
 module Superset
   module Dashboard

lib/superset/services/duplicate_dashboard.rb

@@ -1,6 +1,8 @@
-# Assumptions
+# Data Sovereignty validations are enforced, ie confirming
+# - all charts datasets on the source dashboard are pointing to the same database schema
+# - all filter datasets on the source dashboard are pointing to the same database schema as the charts
+
 # - The source dashboard is in the same Superset instance as the target database and target schema
-# - All charts datasets on the source dashboard are pointing to the same database schema
 
 
 module Superset
@@ -195,7 +197,7 @@ def validate_params
         # schema validations
         raise ValidationError, "Schema #{target_schema} does not exist in target database: #{target_database_id}" unless target_database_available_schemas.include?(target_schema)
         raise ValidationError, "The source dashboard datasets are required to point to one schema only. Actual schema list is #{source_dashboard_schemas.join(',')}" if source_dashboard_has_more_than_one_schema?
-        raise ValidationError, "One or more source dashboard filters point to a different dataset than the dashboard charts. Identified Unpermittied Filter Dataset Ids are #{unpermitted_filter_dataset_ids.to_s}" if unpermitted_filter_dataset_ids.any?
+        raise ValidationError, "One or more source dashboard filters point to a different schema than the dashboard charts. Identified Unpermittied Filter Dataset Ids are #{unpermitted_filter_dataset_ids.to_s}" if unpermitted_filter_dataset_ids.any?
 
         # new dataset validations
         raise ValidationError, "DATASET NAME CONFLICT: The Target Schema #{target_schema} already has existing datasets named: #{target_schema_matching_dataset_names.join(',')}" unless target_schema_matching_dataset_names.empty?
@@ -225,6 +227,7 @@ def source_dashboard_has_more_than_one_schema?
         source_dashboard_schemas.count > 1
       end
 
+      # Data Sovereignty rules expect only 1 value here, and raise a validation error if there is > 1
       def source_dashboard_schemas
         source_dashboard_datasets.map { |dataset| dataset[:schema] }.uniq
       end
@@ -259,10 +262,20 @@ def source_dashboard_filter_dataset_ids
         filters_configuration.map { |c| c['targets'] }.flatten.compact.map { |c| c['datasetId'] }.flatten.compact
       end
 
-      # identify any filters dataset that is not part of the dashboard charts datasets
-      # unpermitted meaning we only allow filters to be sourced from the dashboard datasets
+      # Primary Assumption is that all charts datasets on the source dashboard are pointing to the same database schema
+      # An unpermitted filter will have a dataset that is pulling data from a datasource that is
+      # different to the dashboard charts database schema
       def unpermitted_filter_dataset_ids
-        source_dashboard_filter_dataset_ids - source_dashboard_dataset_ids
+        @unpermitted_filter_dataset_ids ||= begin
+          filter_datasets_not_used_in_charts = source_dashboard_filter_dataset_ids - source_dashboard_dataset_ids
+
+          # retrieve any filter_datasets_not_used_in_charts that do not match the source_dashboard_schema
+          filter_datasets_not_used_in_charts.map do |filter_dataset|
+            filter_dataset_schema = Superset::Dataset::Get.new(filter_dataset).schema
+            # return any filter datasets not used in charts that are from a different schema
+            {  filter_dataset_id: filter_dataset, filter_schema: filter_dataset_schema  } if [filter_dataset_schema] != source_dashboard_schemas
+          end.compact
+        end
       end
 
       def logger

spec/superset/services/duplicate_dashboard_spec.rb

@@ -48,7 +48,7 @@
   let(:existing_target_datasets_list) {[]}
 
   # initial json metadata settings will be copied to the new dashboard and requires updating
-  let(:json_metadata_initial_settings) do 
+  let(:json_metadata_initial_settings) do
     {
       "chart_configuration"=>
         { "#{source_chart_1}"=>{"id"=>source_chart_1, "crossFilters"=>{"scope"=>"global", "chartsInScope"=>[source_chart_2]}},
@@ -70,7 +70,7 @@
   end
 
   # expected json metadata settings after the new dashboard is created and json is updated
-  let(:json_metadata_updated_settings) do 
+  let(:json_metadata_updated_settings) do
     {
       "chart_configuration"=>
         { "#{new_chart_1}"=>{"id"=>new_chart_1, "crossFilters"=>{"scope"=>"global", "chartsInScope"=>[new_chart_2]}},
@@ -141,15 +141,15 @@
           context 'with stardard json metadata ids' do
             specify do
               expect(subject.new_dashboard_json_metadata_configuration).to eq(json_metadata_initial_settings)
-              subject.perform 
+              subject.perform
               expect(subject.new_dashboard_json_metadata_configuration).to eq(json_metadata_updated_settings)
             end
           end
 
           context 'with non stardard json metadata ids to confirm gsub' do
             let(:source_chart_1) { 11 }
             let(:source_chart_2) { 1111 }
-            let(:json_metadata_initial_settings) do 
+            let(:json_metadata_initial_settings) do
               {
                 "chart_configuration"=>
                   { "#{source_chart_1}"=>{"id"=>source_chart_1, "crossFilters"=>{"scope"=>"global", "chartsInScope"=>[source_chart_2]}} ,
@@ -161,7 +161,7 @@
 
             let(:new_chart_1) { 222 }
             let(:new_chart_2) { 22222 }
-            let!(:json_metadata_updated_settings) do 
+            let!(:json_metadata_updated_settings) do
               {
                 "chart_configuration"=>
                   { "#{new_chart_1}"=>{"id"=>new_chart_1, "crossFilters"=>{"scope"=>"global", "chartsInScope"=>[new_chart_2]}} ,
@@ -173,12 +173,11 @@
 
             specify do
               expect(subject.new_dashboard_json_metadata_configuration).to eq(json_metadata_initial_settings)
-              subject.perform 
+              subject.perform
               expect(subject.new_dashboard_json_metadata_configuration).to eq(json_metadata_updated_settings)
             end
           end
         end
-        
       end
 
       context 'and embedded domains' do
@@ -289,12 +288,41 @@
         end
       end
 
-      context 'filters set is outside source schema' do
+      context 'one filters dataset schema not matching the source dashboards schema' do
         let(:target_schema) { 'schema_two' }
-        let(:source_dashboard_filter_dataset_ids) { [101, 202] }
+        let(:invalid_filter_dataset_id) { 404 }
+        let(:invalid_filter_dataset_schema) { 'schema_four' }
+        let(:source_dashboard_filter_dataset_ids) { [101, invalid_filter_dataset_id] }
 
         specify do
-          expect { subject.perform }.to raise_error(Superset::Request::ValidationError, "One or more source dashboard filters point to a different dataset than the dashboard charts. Identified Unpermittied Filter Dataset Ids are [202]")
+          # additional filter checks for validations on unpermitted_filter_dataset_ids
+          expect(Superset::Dataset::Get).to receive(:new).with(invalid_filter_dataset_id).and_return(double(schema: invalid_filter_dataset_schema))
+
+          expect { subject.perform }.to raise_error(Superset::Request::ValidationError,
+            "One or more source dashboard filters point to a different schema than the dashboard charts. " \
+            "Identified Unpermittied Filter Dataset Ids are " \
+            "[{:filter_dataset_id=>#{invalid_filter_dataset_id}, :filter_schema=>\"#{invalid_filter_dataset_schema}\"}]")
+        end
+      end
+
+      context 'more than one filters dataset schema not matching the source dashboards schema' do
+        let(:target_schema) { 'schema_two' }
+        let(:invalid_filter_dataset_id) { 404 }
+        let(:invalid_filter_dataset_schema) { 'schema_four' }
+        let(:invalid_filter_dataset_id_2) { 505 }
+        let(:invalid_filter_dataset_schema_2) { 'schema_five' }
+        let(:source_dashboard_filter_dataset_ids) { [101, invalid_filter_dataset_id, invalid_filter_dataset_id_2] }
+
+        specify do
+          # additional filter checks for validations on unpermitted_filter_dataset_ids
+          expect(Superset::Dataset::Get).to receive(:new).with(invalid_filter_dataset_id).and_return(double(schema: invalid_filter_dataset_schema))
+          expect(Superset::Dataset::Get).to receive(:new).with(invalid_filter_dataset_id_2).and_return(double(schema: invalid_filter_dataset_schema_2))
+
+          expect { subject.perform }.to raise_error(Superset::Request::ValidationError,
+            "One or more source dashboard filters point to a different schema than the dashboard charts. " \
+            "Identified Unpermittied Filter Dataset Ids are " \
+            "[{:filter_dataset_id=>#{invalid_filter_dataset_id}, :filter_schema=>\"#{invalid_filter_dataset_schema}\"}, " \
+            "{:filter_dataset_id=>#{invalid_filter_dataset_id_2}, :filter_schema=>\"#{invalid_filter_dataset_schema_2}\"}]")
         end
       end
 
@@ -311,7 +339,7 @@
       context 'target schema already has matching dataset names' do
         let(:existing_target_datasets_list) {[ 'Dataset 1 (COPY)', 'Dataset 2 (COPY)' ]}
         specify do
-          expect { subject.perform }.to raise_error(Superset::Request::ValidationError, 
+          expect { subject.perform }.to raise_error(Superset::Request::ValidationError,
             "DATASET NAME CONFLICT: The Target Schema schema_two already has existing datasets named: Dataset 1 (COPY),Dataset 2 (COPY)")
         end
       end