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

Raise helpful error when role doesn't exist #34753

Conversation

@eileencodes
Copy link
Member

@eileencodes eileencodes commented Dec 19, 2018

If you try to call connected_to with a role that doesn't have an
established connection you used to get an error that said:

>> ActiveRecord::Base.connected_to(role: :i_dont_exist) { Home.first }

ActiveRecord::ConnectionNotEstablished Exception: No connection pool
with 'primary' found.

This is confusing because the connection could be established but we
spelled the role wrong.

I've changed this to raise if the role used in connected_to doesn't
have an associated handler. Users who encounter this should either check
that the role is spelled correctly (writin -> writing), establish a
connection to that role in the model with connects_to, or use the
database keyword for the role.

I think this will provide a less confusing error message for those
starting out with multiple databases.

cc/ @tenderlove @matthewd @rafaelfranca

@eileencodes eileencodes added this to the 6.0.0 milestone Dec 19, 2018
@eileencodes eileencodes self-assigned this Dec 19, 2018
If you try to call `connected_to` with a role that doesn't have an
established connection you used to get an error that said:

```
>> ActiveRecord::Base.connected_to(role: :i_dont_exist) { Home.first }

ActiveRecord::ConnectionNotEstablished Exception: No connection pool
with 'primary' found.
```

This is confusing because the connection could be established but we
spelled the role wrong.

I've changed this to raise if the `role` used in `connected_to` doesn't
have an associated handler. Users who encounter this should either check
that the role is spelled correctly (writin -> writing), establish a
connection to that role in the model with connects_to, or use the
`database` keyword for the `role`.

I think this will provide a less confusing error message for those
starting out with multiple databases.
@eileencodes eileencodes force-pushed the eileencodes:raise-less-confusing-error-if-handler-doesnt-exist branch from 0a39676 to 22a1265 Dec 21, 2018
@eileencodes eileencodes merged commit b00e46b into rails:master Dec 21, 2018
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@eileencodes eileencodes deleted the eileencodes:raise-less-confusing-error-if-handler-doesnt-exist branch Dec 21, 2018
bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Dec 21, 2018
It should make the test more readable since `flunk` emphasizes better
than `yield` that the block shouldn't be called.

Also, it improves error message:
```
(snip)
should not call this block
(snip)
```

```
(snip)
[ArgumentError] exception expected, not Class: <LocalJumpError>
Message: <"no block given (yield)">
---Backtrace---
(snip)
```

Related to rails#34753
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

1 participant