Skip to content

Schema-dump all user-created functions, not just 'public' #140

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.

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
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
35 changes: 31 additions & 4 deletions lib/fx/adapters/postgres/functions.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,19 @@ class Functions
# dumpable into `db/schema.rb`.
FUNCTIONS_WITH_DEFINITIONS_QUERY = <<-EOS.freeze
SELECT
pp.proname AS name,
pg_get_functiondef(pp.oid) AS definition
pp.proname AS name,
pn.nspname AS schema,
pg_get_functiondef(pp.oid) AS definition,
current_schema() AS current_schema
FROM pg_proc pp
JOIN pg_namespace pn
ON pn.oid = pp.pronamespace
LEFT JOIN pg_depend pd
ON pd.objid = pp.oid AND pd.deptype = 'e'
LEFT JOIN pg_aggregate pa
ON pa.aggfnoid = pp.oid
WHERE pn.nspname = 'public' AND pd.objid IS NULL
WHERE pn.nspname NOT IN ('pg_catalog', 'information_schema')
Copy link
Author

Choose a reason for hiding this comment

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

Some strategies go as far as ignoring everything in pg_*. I'm considering making that change still.

(Unfortunately I have not found any other way to isolate user-created functions, and there are many examples that take a similar approach to what I have done here.)

Copy link

Choose a reason for hiding this comment

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

@smudge I think you can use this (I stupidly branched and created a PR before I saw this one)

AND pd.objid IS NULL
AND pa.aggfnoid IS NULL
ORDER BY pp.oid;
EOS
Expand All @@ -39,7 +42,12 @@ def initialize(connection)
#
# @return [Array<Fx::Function>]
def all
functions_from_postgres.map { |function| to_fx_function(function) }
functions_from_postgres.map do |function|
to_fx_function(
"name" => schema_aware_name(function),
"definition" => schema_aware_definition(function)
Comment on lines +47 to +48
Copy link
Author

@smudge smudge Jan 19, 2024

Choose a reason for hiding this comment

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

Thanks for the nudge @teoljungberg! I'm also a scenic user and so I dug into how that gem works, and ended up proposing this change on their end (which so far seems to be well-received, though I cannot say whether it will ultimately merge): scenic-views/scenic#401

In resolving some discrepancies within that gem, I ended up relying on the way that a core Rails contribution enabled schema-handling for enums: rails/rails#45740

I think it would make a ton of sense for both fx and scenic to behave similar to this core enum approach, which is roughly as follows:

  1. Query for all user-created structures that are accessible to the current PG user (regardless of search_path)
  2. If the structure's schema does not match current_schema(), include the fully-qualified name in schema.rb
  3. If the structure's schema does match current_schema(), do not include the schema in the generated schema.rb name.

Points 2 and 3 there are really crucial. This allows a schema.rb to be shared across contexts where the "default" search path might vary (and is entirely dependent on external/config factors -- it is not guaranteed to be public, and some PAAS providers do not even place this in the end-developer's control), while still allowing developers to explicitly specify non-default schemas and have those survive the round-trip to schema.rb.

)
end
end

private
Expand All @@ -53,6 +61,25 @@ def functions_from_postgres
def to_fx_function(result)
Fx::Function.new(result)
end

def schema_aware_name(function)
Copy link
Author

Choose a reason for hiding this comment

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

I considered moving both of these "schema-aware" mappers to Fx::Function, but it also complicates the tests for that class in ways that didn't feel necessary. (I figured I would leave it up to you @teoljungberg, as to whether that class should be a "dumb" wrapper or a "smart" translator.)

if function.fetch("schema") == function.fetch("current_schema")
function.fetch("name")
else
"#{function.fetch("schema")}.#{function.fetch("name")}"
end
end

def schema_aware_definition(function)
Copy link
Author

Choose a reason for hiding this comment

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

The reason this method is necessary is because PG itself is not schema-aware when returning the function definition, and will, for example, include the fully-qualified name (public.something) even if current_schema() matches the function's schema.

This is probably the correct behavior on PG's part, but it means that for schema.rb purposes, we should strip it. (See here for the reasoning -- this how Rails handles enum names in a way that enables a single schema.rb to be valid & shareable in many contexts that may not be configured to have the same default PG schema.)

if function.fetch("schema") == function.fetch("current_schema")
function.fetch("definition").sub(
/CREATE OR REPLACE FUNCTION #{function.fetch("schema")}\./,
"CREATE OR REPLACE FUNCTION "
)
else
function.fetch("definition")
end
end
end
end
end
Expand Down
64 changes: 42 additions & 22 deletions spec/fx/adapters/postgres/functions_spec.rb
Original file line number Diff line number Diff line change
@@ -1,33 +1,53 @@
require "spec_helper"

RSpec.describe Fx::Adapters::Postgres::Functions, :db do
def create_function_sql(name)
<<-FX.strip_heredoc
CREATE OR REPLACE FUNCTION #{name}()
RETURNS text
LANGUAGE plpgsql
AS $function$
BEGIN
RETURN 'test';
END;
$function$
FX
end

describe ".all" do
it "returns `Function` objects" do
it "returns `Function` objects in all schemas" do
connection = ActiveRecord::Base.connection
connection.execute "CREATE SCHEMA test_schema"
connection.execute create_function_sql("public.test")
connection.execute create_function_sql("test_schema.test")
functions = Fx::Adapters::Postgres::Functions.new(connection).all

expect(functions.size).to eq 2
expect(functions[0].name).to eq "test"
expect(functions[0].definition).to eq create_function_sql("test")
expect(functions[1].name).to eq "test_schema.test"
expect(functions[1].definition).to eq create_function_sql("test_schema.test")
end
end

context "when 'public' is not the default schema" do
it "returns `Function` objects with schema-aware names and definitions" do
connection = ActiveRecord::Base.connection
connection.execute <<-EOS.strip_heredoc
CREATE OR REPLACE FUNCTION test()
RETURNS text AS $$
BEGIN
RETURN 'test';
END;
$$ LANGUAGE plpgsql;
EOS
search_path_was = connection.execute("SHOW search_path")[0]["search_path"]

connection.execute "SET search_path TO test_schema"
connection.execute "CREATE SCHEMA test_schema"
connection.execute create_function_sql("public.test")
connection.execute create_function_sql("test_schema.test")
functions = Fx::Adapters::Postgres::Functions.new(connection).all

first = functions.first
expect(functions.size).to eq 1
expect(first.name).to eq "test"
expect(first.definition).to eq <<-EOS.strip_heredoc
CREATE OR REPLACE FUNCTION public.test()
RETURNS text
LANGUAGE plpgsql
AS $function$
BEGIN
RETURN 'test';
END;
$function$
EOS
expect(functions.size).to eq 2
expect(functions[0].name).to eq "public.test"
expect(functions[0].definition).to eq create_function_sql("public.test")
expect(functions[1].name).to eq "test"
expect(functions[1].definition).to eq create_function_sql("test")
ensure
connection.execute "SET search_path TO #{search_path_was}"
end
end
end