Revert "Revert "8903 layer table references in analyses""

Makefile

@@ -247,6 +247,7 @@ WORKING_SPECS_9 = \
 SPEC_HELPER_MIN_SPECS = \
 	spec/models/carto/analysis_spec.rb \
 	spec/models/carto/analysis_node_spec.rb \
+	spec/models/carto/layer_spec.rb \
 	spec/models/carto/mobile_app_presenter_spec.rb \
 	spec/models/carto/overlay_spec.rb \
 	spec/models/carto/user_migration_spec.rb \

app/models/carto/analysis.rb

@@ -13,6 +13,7 @@ class Carto::Analysis < ActiveRecord::Base
   belongs_to :visualization, class_name: Carto::Visualization
   belongs_to :user, class_name: Carto::User
 
+  after_save :update_map_dataset_dependencies
   after_save :notify_map_change
   after_destroy :notify_map_change
 
@@ -62,6 +63,10 @@ class Carto::Analysis < ActiveRecord::Base
     valid
   end
 
+  def update_map_dataset_dependencies
+    map.update_dataset_dependencies
+  end
+
   def notify_map_change
     map.notify_map_change if map
   end

app/models/carto/analysis_node.rb

@@ -7,6 +7,11 @@ class Carto::AnalysisNode
 
   attr_reader :definition
 
+  def self.find_by_natural_id(visualization_id, natural_id)
+    analyses = Carto::Analysis.where(visualization_id: visualization_id).all
+    analyses.lazy.map { |analysis| analysis.analysis_node.find_by_id(natural_id) }.find(&:present?)
+  end
+
   def id
     definition[:id]
   end
@@ -45,6 +50,11 @@ class Carto::AnalysisNode
     source? && options && options[:table_name] == table_name
   end
 
+  def source_descendants
+    return [self] if source?
+    children.map(&:source_descendants).flatten
+  end
+
   private
 
   MANDATORY_KEYS_FOR_ANALYSIS_NODE = [:id, :type, :params, :options].freeze

app/models/carto/layer.rb

@@ -2,7 +2,53 @@ require 'active_record'
 require_relative './carto_json_serializer'
 
 module Carto
+  module LayerTableDependencies
+    def affected_tables
+      return [] unless maps.first.present? && options.present?
+      node_id = options.symbolize_keys[:source]
+      if node_id.present?
+        visualization_id = map.visualization.id
+        node = AnalysisNode.find_by_natural_id(visualization_id, node_id)
+        return [] unless node
+        dependencies = node.source_descendants.map do |source_node|
+          tables_by_query = tables_from_query(source_node.params[:query])
+          table_name = source_node.options[:table_name]
+          tables_by_name = table_name ? tables_from_names([table_name], user) : []
+
+          tables_by_query + tables_by_name
+        end
+        dependencies.flatten.compact.uniq
+      else
+        (tables_from_query_option + tables_from_table_name_option).compact.uniq
+      end
+    end
+
+    def tables_from_query_option
+      tables_from_query(query)
+    end
+
+    def tables_from_query(query)
+      tables_from_names(affected_table_names(query), user)
+    rescue => e
+      # INFO: this covers changes that CartoDB can't track.
+      # For example, if layer SQL contains wrong SQL (uses a table that doesn't exist, or uses an invalid operator).
+      CartoDB::Logger.debug(message: 'Could not retrieve tables from query', exception: e, user: user, layer: self)
+      []
+    end
+
+    def tables_from_table_name_option
+      return [] if options.empty?
+      sym_options = options.symbolize_keys
+      user_name = sym_options[:user_name]
+      table_name = sym_options[:table_name]
+      schema_prefix = user_name.present? && table_name.present? && !table_name.include?('.') ? %{"#{user_name}".} : ''
+      tables_from_names(["#{schema_prefix}#{table_name}"], user)
+    end
+  end
+
   class Layer < ActiveRecord::Base
+    include LayerTableDependencies
+
     serialize :options, CartoJsonSerializer
     serialize :infowindow, CartoJsonSerializer
     serialize :tooltip, CartoJsonSerializer
@@ -28,10 +74,6 @@ module Carto
       'table/views/infowindow_header_with_image' =>   'infowindow_header_with_image'
     }.freeze
 
-    def affected_tables
-      (tables_from_query_option + tables_from_table_name_option).compact.uniq
-    end
-
     def affected_tables_readable_by(user)
       affected_tables.select { |ut| ut.readable_by?(user) }
     end
@@ -163,34 +205,25 @@ module Carto
       end
     end
 
+    def register_table_dependencies
+      self.user_tables = affected_tables if data_layer?
+    end
+
     private
 
-    def tables_from_query_option
-      ::Table.get_all_user_tables_by_names(affected_table_names, user)
-    rescue => exception
-      # INFO: this covers changes that CartoDB can't track. For example, if layer SQL contains wrong SQL (uses a table that doesn't exist, or uses an invalid operator).
-      CartoDB.notify_debug('Could not retrieve tables from query', { user: user, layer: self })
-      []
+    def tables_from_names(table_names, user)
+      ::Table.get_all_user_tables_by_names(table_names, user)
     end
 
-    def affected_table_names
+    def affected_table_names(query)
       return [] unless query.present?
 
       # TODO: This is the same that CartoDB::SqlParser().affected_tables does. Maybe remove that class?
       query_tables = user.in_database.execute("SELECT CDB_QueryTables(#{user.in_database.quote(query)})").first
-      query_tables['cdb_querytables'].split(',').map do |table_name|
+      query_tables['cdb_querytables'].split(',').map { |table_name|
         t = table_name.gsub!(/[\{\}]/, '')
         (t.blank? ? nil : t)
-      end.compact.uniq
-    end
-
-    def tables_from_table_name_option
-      return[] if options.empty?
-      sym_options = options.symbolize_keys
-      user_name = sym_options[:user_name]
-      table_name = sym_options[:table_name]
-      schema_prefix = user_name.present? && table_name.present? && !table_name.include?('.') ? %{"#{user_name}".} : ''
-      ::Table.get_all_user_tables_by_names(["#{schema_prefix}#{table_name}"], user)
+      }.compact.uniq
     end
 
     def query

app/models/carto/map.rb

@@ -119,6 +119,10 @@ class Carto::Map < ActiveRecord::Base
     visualizations.first
   end
 
+  def update_dataset_dependencies
+    data_layers.each(&:register_table_dependencies)
+  end
+
   private
 
   def get_the_last_time_tiles_have_changed_to_render_it_in_vizjsons

app/models/layer.rb

@@ -3,9 +3,12 @@
 require_relative 'layer/presenter'
 require_relative 'table/user_table'
 require_relative '../../lib/cartodb/stats/editor_apis'
+require_relative 'carto/layer'
 
 
 class Layer < Sequel::Model
+  include Carto::LayerTableDependencies
+
   plugin :serialization, :json, :options, :infowindow, :tooltip
 
   ALLOWED_KINDS = %W{ carto tiled background gmapsbase torque wms }
@@ -88,12 +91,6 @@ class Layer < Sequel::Model
     super
   end
 
-  # Returns an array of tables used on the layer
-  def affected_tables
-    return [] unless maps.first.present? && options.present?
-    (tables_from_query_option + tables_from_table_name_option).compact.uniq
-  end
-
   def key
     "rails:layer_styles:#{self.id}"
   end
@@ -206,22 +203,11 @@ class Layer < Sequel::Model
     affected_tables.map { |table| add_user_table(table) }
   end
 
-  def tables_from_query_option
-    return [] unless query.present?
-    ::Table.get_all_by_names(affected_table_names, user)
-  rescue => exception
-    []
-  end
-
-  def tables_from_table_name_option
-    sym_options = options.symbolize_keys
-    user_name = sym_options[:user_name]
-    table_name = sym_options[:table_name]
-    schema_prefix = user_name.present? && table_name.present? && !table_name.include?('.') ? %{"#{user_name}".} : ''
-    ::Table.get_all_by_names(["#{schema_prefix}#{table_name}"], user)
+  def tables_from_names(table_names, user)
+    ::Table.get_all_by_names(table_names, user)
   end
 
-  def affected_table_names
+  def affected_table_names(query)
     CartoDB::SqlParser.new(query, connection: user.in_database).affected_tables
   end
 

app/models/map.rb

@@ -158,6 +158,10 @@ class Map < Sequel::Model
     @visualizations_collection ||= CartoDB::Visualization::Collection.new.fetch(map_id: [id]).to_a
   end
 
+  def visualization
+    visualizations.first
+  end
+
   def process_privacy_in(layer)
     return self unless layer.uses_private_tables?
 

app/services/carto/visualizations_export_persistence_service.rb

@@ -55,14 +55,14 @@ module Carto
           end
           layer.save if changed
         end
+
+        map.data_layers.each(&:register_table_dependencies)
       end
 
       # Propagate changes (named maps, default permissions and so on)
       visualization_member = CartoDB::Visualization::Member.new(id: visualization.id).fetch
       visualization_member.store
 
-      visualization_member.map.layers.map(&:register_table_dependencies)
-
       visualization
     end
 

spec/factories/analyses.rb

@@ -47,4 +47,24 @@ FactoryGirl.define do
       }
     end
   end
+
+  factory :analysis_point_in_polygon, class: Carto::Analysis do
+    ignore do
+      source_table 'subway_stops'
+      source_query nil
+      target_table 'districts'
+      target_query nil
+    end
+
+    analysis_definition do
+      {
+        id: unique_string,
+        type: "intersection",
+        params: {
+          source: AnalysisFactoryHelper.source_analysis_for_table(source_table, source_query),
+          target: AnalysisFactoryHelper.source_analysis_for_table(target_table, target_query)
+        }
+      }
+    end
+  end
 end

spec/models/carto/analysis_node_spec.rb

@@ -78,4 +78,12 @@ describe Carto::AnalysisNode do
       @node.find_by_id('404').should be_nil
     end
   end
+
+  describe '#source_descendants' do
+    it 'find all descendant of a node of type source' do
+      sources = @node.source_descendants
+      sources.count.should eq 2
+      sources.all?(&:source?).should be_true
+    end
+  end
 end

spec/models/carto/analysis_spec.rb

@@ -123,6 +123,8 @@ describe Carto::Analysis do
     it 'triggers notify_map_change on related map(s)' do
       map = mock
       map.stubs(:id).returns(@map.id)
+      map.stubs(:data_layers).returns([])
+      map.expects(:update_dataset_dependencies).once
       map.expects(:notify_map_change).once
       Map.stubs(:where).with(id: map.id).returns([map])
       @analysis.stubs(:map).returns(map)

spec/models/carto/layer_spec.rb

@@ -0,0 +1,80 @@
+# encoding: utf-8
+
+require 'spec_helper_min'
+
+describe Carto::Layer do
+  include Carto::Factories::Visualizations
+
+  describe '#affected_tables' do
+    before(:all) do
+      bypass_named_maps
+      @user = FactoryGirl.create(:carto_user)
+      @map, @table1, @table_visualization, @visualization = create_full_visualization(@user)
+      @table2 = FactoryGirl.create(:carto_user_table, user_id: @user.id, map_id: @map.id)
+      @analysis = FactoryGirl.create(:analysis_point_in_polygon,
+                                     user: @user, visualization: @visualization,
+                                     source_table: @table1.name, target_table: @table2.name)
+      @layer = @map.data_layers.first
+    end
+
+    after(:all) do
+      @analysis.destroy
+      destroy_full_visualization(@map, @table1, @table_visualization, @visualization)
+      @user.destroy
+    end
+
+    let(:source_analysis_id) { @analysis.analysis_node.children[0].id }
+    let(:target_analysis_id) { @analysis.analysis_node.children[1].id }
+
+    it 'returns all tables from the root analysis' do
+      @layer.stubs(:options).returns(source: @analysis.natural_id)
+      @layer.stubs(:affected_table_names).returns([]).twice
+      affected = @layer.affected_tables
+      affected.count.should eq 2
+      affected.should include @table1
+      affected.should include @table2
+    end
+
+    it 'returns only tables from the sub-analysis' do
+      @layer.stubs(:options).returns(source: source_analysis_id)
+      @layer.stubs(:affected_table_names).returns([]).once
+      affected = @layer.affected_tables
+      affected.count.should eq 1
+      affected.should include @table1
+
+      @layer.stubs(:options).returns(source: target_analysis_id)
+      @layer.stubs(:affected_table_names).returns([]).once
+      affected = @layer.affected_tables
+      affected.count.should eq 1
+      affected.should include @table2
+    end
+
+    it 'ignores query/table_name if source is specified' do
+      @layer.stubs(:options).returns(source: 'wadus', table_name: @table1.name)
+      affected = @layer.affected_tables
+      affected.should be_empty
+    end
+
+    it 'fallbacks to query/table_name if source is not specified' do
+      @layer.stubs(:options).returns(table_name: @table1.name)
+      affected = @layer.affected_tables
+      affected.should eq [@table1]
+
+      query = "SELECT * FROM #{@table2.name}"
+      @layer.stubs(:options).returns(query: query)
+      @layer.stubs(:affected_table_names).with(query).returns([@table2.name]).once
+      affected = @layer.affected_tables
+      affected.should eq [@table2]
+    end
+
+    it 'returns values from both query and table_name' do
+      query = "SELECT * FROM #{@table2.name}"
+      @layer.stubs(:options).returns(table_name: @table1.name, query: query)
+      @layer.stubs(:affected_table_names).with(query).returns([@table2.name]).once
+      affected = @layer.affected_tables
+      affected.count.should eq 2
+      affected.should include @table1
+      affected.should include @table2
+    end
+  end
+end

spec/requests/carto/api/widgets_controller_spec.rb

@@ -8,7 +8,7 @@ include Carto::UUIDHelper
 
 shared_context 'layer hierarchy' do
   before(:each) do
-    @map = FactoryGirl.create(:carto_map_with_layers)
+    @map = FactoryGirl.create(:carto_map_with_layers, user_id: @user1.id)
     @layer = @map.layers.first
     @widget = FactoryGirl.create(:widget, layer: @layer)
     @visualization = FactoryGirl.create(:carto_visualization, map: @map, privacy: Carto::Visualization::PRIVACY_PRIVATE, user_id: @user1.id)
@@ -72,7 +72,7 @@ describe Carto::Api::WidgetsController do
   include_context 'layer hierarchy'
 
   before(:each) do
-    @public_map = FactoryGirl.create(:carto_map_with_layers)
+    @public_map = FactoryGirl.create(:carto_map_with_layers, user_id: @user1.id)
     @public_layer = @public_map.layers.first
     @public_widget = FactoryGirl.create(:widget, layer: @public_layer)
 
@@ -190,7 +190,7 @@ describe Carto::Api::WidgetsController do
     end
 
     it 'returns 422 if layer id do not match map' do
-      other_map = FactoryGirl.create(:carto_map_with_layers)
+      other_map = FactoryGirl.create(:carto_map_with_layers, user_id: @user1.id)
       other_layer = other_map.data_layers.first
       other_layer.should_not be_nil