Skip to content
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

Use view without namespace when dumping schema. #327

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rapito
Copy link
Contributor

@rapito rapito commented Apr 8, 2021

Why?

Without this, default behavior of rake db:schema:load is broken because ActiveRecord config option schema_search_path for Postgres adapters is automatically used to identify where is the view being loaded to; By explicitly adding the namespace/search_path in the schema.rb it's wrong as you can run rake db:migrate from multiple computers and configurations which can result in namespace_from_pc_1.view_name and namespace_from_pc_2.view_name on different commits.

Scenarios with issues

  • Have a database.yml using an environment variable such as schema_search_path: <%= ENV['PG_DB_SCHEMA'] %>
  • Have a scenic view called my_view
  • Have person 1 from your team export PG_DB_SCHEMA=my_schema_1
  • Have person 2 from your team export PG_DB_SCHEMA=my_schema_2
  • Have person 1 execute rake db:migrate which generates "my_schema_1.my_view" on schema.rb
  • Have person 2 execute rake db:migrate which generates "my_schema_2.my_view" on schema.rb
  • Have CI runner with env variable PG_DB_SCHEMA=my_ci_schema_3 run rake db:schema:load, it fails.

Changes

  • Added a new private method unscoped_view to make sure to not include the namespace when dumping the schema name
  • Updated tests to make sure the namespace is excluded rather than included.

Extra

An additional justification is that schema.rb does not include namespaces ever, and the default Rails/Activerecord behavior should be conserved.

Fixes: #325

Without this, default behavior of `rake db:schema:load` is broken becuase Activerecord config option `schema search path` for Postgres adapters is automatically used to identify where is the schema being loaded to; By explicitly adding the namespace/search_path in the schema.rb it's wrong as you can run rake db:migrate from multiple computers and configurations which result in namespace_from_pc_1.view_name and namespace_from_pc_2.view_name on different commits.

+ Added a new private method unscoped_view to make sure to not include the namespace when dumping the schema name
+ Updated tests to make sure the namespace is excluded rather than included.
@derekprior
Copy link
Contributor

@rapito This is in direct conflict with #152, which introduced this behavior. Honestly, I have zero intention of ever using Scenic with multiple namespaces so I'm not sure what the correct solution is here. Can you convince me and/or @calebhearth which decision is correct?

I see your point that we can allow the schema search path to determine this for us. However, what about the case where your app actually uses multiple schemas? If we rely on schema search path alone, wouldn't all views be created in whatever the first listed schema is when in reality we might want some in the first listed schema and some others in the second?

How does Rails itself handle this with tables?

cc @Willianvdv

@rapito
Copy link
Contributor Author

rapito commented Dec 15, 2021

To that end I would suggest making the scenic migration accept optional parameters to include the schema search path.

And to still allow specifying schemas, the method should not include the schema on the scenic view name but as an argument of the method.
I.e:

create_view "tablename", schema: "schema_name"

The gem would translate that to "schema_name.tablename" when executing it.

Furthermore, the schema parameter should only be set if the gem config option include_search_path_on_schema is turned on.

Imo, Ideally the primary feature that the gem should support is creating the scenic views. Everything else should be secondary such as supporting multiple schemas, and if they break default behaviors or patterns then they should be optional.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Schema search path should not be included in schema.rb when a migration is executed.
3 participants