Rely on search_path (instead of "public") when deciding to fully-qualify view name#401
Rely on search_path (instead of "public") when deciding to fully-qualify view name#401smudge wants to merge 2 commits into
Conversation
|
|
||
| def to_scenic_view(result) | ||
| namespace, viewname = result.values_at "namespace", "viewname" | ||
| namespace, viewname, current_schema = result.values_at "namespace", "viewname", "current_schema" |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [106/80]
| view_definition = "SELECT 'needle'::text AS haystack" | ||
| Search.connection.execute "CREATE SCHEMA scenic; SET search_path TO scenic, public" | ||
| Search.connection.create_view :"scenic.searches", sql_definition: view_definition | ||
| Search.connection.create_view :"public.searches", sql_definition: view_definition |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [89/80]
| it "dumps a create_view including namespace for a view in the database" do | ||
| view_definition = "SELECT 'needle'::text AS haystack" | ||
| Search.connection.execute "CREATE SCHEMA scenic; SET search_path TO scenic, public" | ||
| Search.connection.create_view :"scenic.searches", sql_definition: view_definition |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [89/80]
| context 'when "public" is not the primary search path' do | ||
| it "dumps a create_view including namespace for a view in the database" do | ||
| view_definition = "SELECT 'needle'::text AS haystack" | ||
| Search.connection.execute "CREATE SCHEMA scenic; SET search_path TO scenic, public" |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [91/80]
| Search.connection.execute "CREATE SCHEMA scenic; SET search_path TO scenic, public" | ||
| Search.connection.execute "CREATE SCHEMA scenic; SET search_path TO public, scenic" | ||
| Search.connection.create_view :"scenic.searches", sql_definition: view_definition | ||
| Search.connection.create_view :"public.searches", sql_definition: view_definition |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [87/80]
| it "dumps a create_view including namespace for a view in the database" do | ||
| view_definition = "SELECT 'needle'::text AS haystack" | ||
| Search.connection.execute "CREATE SCHEMA scenic; SET search_path TO scenic, public" | ||
| Search.connection.execute "CREATE SCHEMA scenic; SET search_path TO public, scenic" |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [89/80]
|
Hmm, it seems the existing code was already in violation of the line length -- I assume hound only runs on the changed lines? 🤔 |
|
Hmm I’m inclined to like this approach but I’ll want to take a closer look. Thank you!On Jan 17, 2024, at 10:53 AM, Nathan Griffith ***@***.***> wrote:
Hmm, it seems the existing code was already in violation of the line length -- I assume hound only runs on the changed lines? 🤔
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>
|
bb11555 to
6f15271
Compare
|
(rebased due to file conflicts) |
|
I'd be interesting in hearing what you reason about this @calebhearth - as @smudge has laid forth a similar proposal in fx (teoljungberg/fx#140). I'd like if Scenic and fx handled this the same way. |
|
My thought right now is that adding specific handling of schema to scenic was a mistake and that we should remove it. It has led to a number of issues raised by users. I think it makes sense to behave exactly as rails behaves with tables. If you want to store views in a different schema then you need a different search path. I think #402 would make that something that could be done rather trivially and in line with Rails conventions. |
|
I'd like to throw my vote in to the default to what rails does camp. We use a different schema other than public in production (legacy Apartment reasons). Ultimately it's not a huge issue because we just set the same schema in dev and it just works. However I was going to start working on a local workflow where I used schemas a little differently on dev, but am stuck using the same schema as prod in our search_path. |
This should resolve the discrepancies in #327 and #325, and is in response to a behavior introduced in #152.
The heart of the issue is in how "public" was hardcoded in #152, because it assumes that "public" will be at the front of the search path (and is therefore the reason a view name should or should not be fully-qualified in schema.rb). But "public" being first in the search path is just a default configuration1 that can be overridden in a few different ways (e.g.
schema_search_pathindatabase.yml, or by setting it at the user/connection level, which some PAAS providers do).In reality, PostgreSQL will create unqualified tables/views in
current_schema(), and so we can rely on that to determine whether a view was created in the observer's default schema. (This is how Rails handles enum names when returning a list of all defined enums, a lookup case which otherwise appears very similar to what thescenicgem does for views.2)This should resolve issues where laptop 1 and laptop 2 have different default schemas (or where different environments/deployments use different default schema names) but must share a
schema.rbfile; as long as the view is locally created in the (local's) default schema, the view will go unnamespaced in the generated schema file, but for anyone who writes a migration that explicitly creates a view in a non-default schema, that explicit schema name will survive the round-trip, as desired. Win-win 👍 .Footnotes
Technically "$user" (which resolves to the current PG user's name) is first by default, but that schema also doesn't exist by default, so
current_schema()ends up being "public" by default. ↩This was introduced in Support schemas in Postgresql
enum_typesrails/rails#45740 ↩