Skip to content

Conversation

@pschambacher
Copy link
Contributor

@pschambacher pschambacher commented Oct 6, 2017

/cc @bquorning @grosser @kassio @craig-day

When calling Model.connection on a sharded model when we're not on a shard returns a connection to the unsharded database. This PR fixes the issue by raising when trying to call connection without specifying a shard.

fyi @jacobat

@pschambacher pschambacher force-pushed the pschambacher/raise_when_no_shard_on_connection branch 2 times, most recently from 0ca6ae7 to 0e16bc7 Compare October 6, 2017 22:49

if ActiveRecord::VERSION::MAJOR > 3
describe "ActiveRecord::Base.connection.schema_cache.columns_hash" do
before do
Copy link
Member

Choose a reason for hiding this comment

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

Need a bit more indentation from here and down.

@pschambacher pschambacher force-pushed the pschambacher/raise_when_no_shard_on_connection branch from 0e16bc7 to 5de0c78 Compare October 6, 2017 22:57
nil
else
@shard || self.class.default_shard
@shard || self.class.default_shard || raise('You can not connect a sharded model without calling on_shard.')
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe next PR should be removing default_shard ;)

end
end

it 'explodes when a shard has been specified for sharded model' do
Copy link
Contributor

Choose a reason for hiding this comment

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

it 'works ...

@jacobat
Copy link
Contributor

jacobat commented Oct 9, 2017

How does this play with #130?

@pschambacher
Copy link
Contributor Author

@jacobat I would assume your PR would be fine. Our issue in Classic is some code paths relying on default_shard to be set, but it's actually nil. Removing default_shard would probably raise the same way it does here.

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.

7 participants