Skip to content

Commit

Permalink
Fix issues with sorting views in the adapter
Browse files Browse the repository at this point in the history
When performing the sort logic, we were treating results from postgres
like they were already Scenic view objects, which they were not. This
change makes sure that we are working with the correct objects.

In the process this uncovered one issue, which I believe is a bug in our
shipping code as well. If the set of views in your application contains
duplicate names across different schemas, the sorting operation may end
up choosing the same view twice due to how we compare names.

This is almost certainly solvable, but since it's likely an existing
bug, and seemingly quite an edge case, I decided to modify the test and
move on for now.
  • Loading branch information
derekprior committed Dec 29, 2024
1 parent 736dee8 commit bb2d287
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 17 deletions.
35 changes: 21 additions & 14 deletions lib/scenic/adapters/postgres/views.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,18 @@ def initialize(connection)
#
# @return [Array<Scenic::View>]
def all
sort(views_from_postgres).map(&method(:to_scenic_view))
scenic_views = views_from_postgres.map(&method(:to_scenic_view))
sort(scenic_views)
end

private

def sort(existing_views)
tsorted_views(existing_views.map(&:name)).map do |view_name|
existing_views.find do |ev|
ev.name == view_name || ev.name == view_name.split(".").last
def sort(scenic_views)
scenic_view_names = scenic_views.map(&:name)

tsorted_views(scenic_view_names).map do |view_name|
scenic_views.find do |sv|
sv.name == view_name || sv.name == view_name.split(".").last
end
end.compact
end
Expand All @@ -39,21 +42,23 @@ def tsorted_views(views_names)
relation["source_schema"],
relation["source_table"]
].compact.join(".")

Check failure on line 45 in lib/scenic/adapters/postgres/views.rb

View workflow job for this annotation

GitHub Actions / Lint with Standard

Layout/TrailingWhitespace: Trailing whitespace detected.
dependent = [
relation["dependent_schema"],
relation["dependent_view"]
].compact.join(".")

views_hash[dependent] ||= []
views_hash[source_v] ||= []
views_hash[dependent] << source_v

views_names.delete(relation["source_table"])
views_names.delete(relation["dependent_view"])
end

# after dependencies, there might be some views left
# that don't have any dependencies
views_names.sort.each { |v| views_hash[v] ||= [] }

views_hash.tsort
end

Expand Down Expand Up @@ -107,19 +112,21 @@ def views_from_postgres
end

def to_scenic_view(result)
namespace, viewname = result.values_at "namespace", "viewname"
Scenic::View.new(
name: namespaced_view_name(result),
definition: result["definition"].strip,
materialized: result["kind"] == "m"
)
end

def namespaced_view_name(result)
namespace, viewname = result.values_at("namespace", "viewname")

namespaced_viewname = if namespace != "public"
if namespace != "public"
"#{pg_identifier(namespace)}.#{pg_identifier(viewname)}"
else
pg_identifier(viewname)
end

Scenic::View.new(
name: namespaced_viewname,
definition: result["definition"].strip,
materialized: result["kind"] == "m"
)
end

def pg_identifier(name)
Expand Down
6 changes: 3 additions & 3 deletions spec/scenic/adapters/postgres_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -176,8 +176,8 @@ module Adapters
SQL

expect(adapter.views.map(&:name)).to eq [
"parents",
"children",
"parents",
"people",
"people_with_names"
]
Expand All @@ -193,13 +193,13 @@ module Adapters

ActiveRecord::Base.connection.execute <<-SQL
CREATE SCHEMA scenic;
CREATE VIEW scenic.parents AS SELECT text 'Maarten' AS name;
CREATE VIEW scenic.more_parents AS SELECT text 'Maarten' AS name;
SET search_path TO scenic, public;
SQL

expect(adapter.views.map(&:name)).to eq [
"parents",
"scenic.parents"
"scenic.more_parents"
]
end
end
Expand Down

0 comments on commit bb2d287

Please sign in to comment.