-
Notifications
You must be signed in to change notification settings - Fork 154
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
Fix sequence name #187
Fix sequence name #187
Changes from 9 commits
500fe0a
94a20d9
e698003
ad4abdd
f0f86d9
8d17dc2
4ba539d
71d3ce0
896e34c
98b52c6
6c7fec2
472027c
a826d25
8d9bb82
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
# frozen_string_literal: true | ||
|
||
# rubocop:disable Style/ClassAndModuleChildren | ||
|
||
# NOTE: This patch is meant to remove any schema_prefix appart from the ones for | ||
# excluded models. The schema_prefix would be resolved by apartment's setting | ||
# of search path | ||
module Apartment::PostgreSqlAdapterPatch | ||
def default_sequence_name(table, _column) | ||
res = super | ||
schema_prefix = "#{Apartment::Tenant.current}." | ||
|
||
if res&.starts_with?(schema_prefix) | ||
if Apartment.excluded_models.none? { |m| m.constantize.table_name == table } | ||
rpbaltazar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
res.delete_prefix!(schema_prefix) | ||
else | ||
res.sub!(schema_prefix, "#{Apartment::Tenant.default_tenant}.") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This causes extra work when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On second thought, this is not strictly correct, because it should happen even if the sequence name is prefixed with some other scheam. No idea if that can happen though 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought of that when i was writing this and then forgot to address it. tks There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree w your second concern. addressed it as well |
||
end | ||
end | ||
res | ||
end | ||
end | ||
|
||
require 'active_record/connection_adapters/postgresql_adapter' | ||
|
||
# NOTE: inject this into postgresql adapters | ||
class ActiveRecord::ConnectionAdapters::PostgreSQLAdapter | ||
include Apartment::PostgreSqlAdapterPatch | ||
end | ||
# rubocop:enable Style/ClassAndModuleChildren |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would force the loading of the pg adapter. Are other dbs no longer supported? This was my comment on #143 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I have not looked at how can we fix this and test it on a non postgresql db. I currently run only postgres so it's also hard for me to test these details on all dbs. I've been tempted on starting refactoring the gem to have one gem per adapter and a core, but that's discussion for another day.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps we can go the "easy" route:
Perhaps in an initializer so that this happens after all gems are loaded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think i have a clearer working solution. We only require the adapter once we eval the config. I'm requiring this patch after requiring the postgres adapter