Allow rake db:create in the wrapping app #87
Conversation
But also comment out the lines that require this file, as it is unclear, if the setup can actually work (in my tests it didn't fully)
| @@ -65,4 +65,8 @@ def handle_readonly_writes! | |||
| class UnsyncedProductionWrite < StandardError; end | |||
| end | |||
|
|
|||
| ActiveRecord::Base.connection.class.prepend(Stagehand::Connection::AdapterExtensions) | |||
| begin | |||
There was a problem hiding this comment.
If we found a better way to add these extensions that didn't rely on asking the connection instance what class it is, then we wouldn't need to have a connection at this point and could avoid the explosion.
There was a problem hiding this comment.
I am not sure I understand your comment. Are you saying that Stagehand should detect the reason why there is a database connection and only add its AdapterExtension in certain cases?
Is this even possible? Are there different types of ActiveRecord::Base.connections?
There was a problem hiding this comment.
Yeah, I think it does it this way because you may be connected to a different database type, e.g. the one we use is for MySQL. Don't quote me on this, but basically all we need is for these methods to be used when called for the current database adapter class. So if we don't need to actually ask the connection object for what its superclass is, maybe because we're able to figure out how its selected by Rails in the first place, then we can just prepend the methods to that class.
| @@ -51,5 +51,10 @@ def table(table_name, stream) | |||
| end | |||
| end | |||
|
|
|||
| ActiveRecord::Base.connection.class.include Stagehand::Schema::Statements | |||
| begin | |||
| ActiveRecord::Base.connection.class.include Stagehand::Schema::Statements | |||
| @@ -4,8 +4,13 @@ rescue LoadError | |||
| puts 'You must `gem install bundler` and `bundle install` to run rake tasks' | |||
| end | |||
|
|
|||
| APP_RAKEFILE = File.expand_path("../spec/internal/Rakefile", __FILE__) | |||
| load 'rails/tasks/engine.rake' | |||
| # While I am not sure, if rails' engine rake tasks are usuable with this app | |||
There was a problem hiding this comment.
The only thing we do is extend some of the Rails rake tasks, but I don't think we need to actually load the rails tasks to do that. See
stagehand/lib/tasks/stagehand_tasks.rake
Lines 36 to 38 in 88a0f9a
| @@ -7,7 +7,11 @@ module Synchronizer | |||
| BATCH_SIZE = 1000 | |||
| SESSION_BATCH_SIZE = 30 | |||
| ENTRY_SYNC_ORDER = [:delete, :update, :insert].freeze | |||
| ENTRY_SYNC_ORDER_SQL = ActiveRecord::Base.send(:sanitize_sql_for_order, ['FIELD(operation, ?), id DESC', ENTRY_SYNC_ORDER]).freeze | |||
| ENTRY_SYNC_ORDER_SQL = begin | |||
| ActiveRecord::Base.send(:sanitize_sql_for_order, ['FIELD(operation, ?), id DESC', ENTRY_SYNC_ORDER]).freeze | |||
There was a problem hiding this comment.
Here all we need to do is use Arel instead to avoid sanitizing at load time.
There was a problem hiding this comment.
What exactly are we sanitizing here? Is the whole expression not equal to:
"FIELD(operation, 'delete','update','insert'), id DESC"
? There is not dynamic input where sanitization would be necessary, or am I overlooking something?
There was a problem hiding this comment.
Btw: regarding Rails' "wrap this in Arel.sql()" deprecation warning, I've opened this PR: https://github.com/combinaut/sparkle/pull/12998
njakobsen
force-pushed
the
master
branch
4 times, most recently
from
70f45b3 to
2f5ba63
Compare
These changes enable to run
rake db:createin Sparkle - which would be most welcome on the CI, but also needed to use the parallel_tests gem.Now, I know just rescuing an error can be seen as hacky. In this case I think we only run into this error on
db:createanyway - and if we would run into the issue that the database is missing then something else would break. But I am looking forward to your opinions.