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

Create database via UI when ActiveRecord::NoDatabaseError #39723

Merged
merged 2 commits into from
Aug 6, 2021

Conversation

hahmed
Copy link
Contributor

@hahmed hahmed commented Jun 25, 2020

Summary

Create the database via the UI when database has not been created in development mode.

Related: https://discuss.rubyonrails.org/t/pending-migration-error-usability-concerns/75330/3

I have split out the errors for connection and no database errors and added the button to create the database.

Screenshot 2020-07-17 at 21 09 08

Screenshot 2020-07-17 at 21 09 59

@rails-bot rails-bot bot added the actionpack label Jun 25, 2020
@jonathanhefner
Copy link
Member

This looks like a job for ActionableError! 🦸 (See PendingMigrationError for an example of usage.)

@hahmed
Copy link
Contributor Author

hahmed commented Jun 26, 2020

I have uploaded screenshots for the 2 interfaces.

Would love some feedback on how I can make the pages friendly and welcoming?

As this style of "error pages" are different than the existing ones, happy to move stuff like the logo into a partial once we have an agreed upon layout.

@hahmed hahmed marked this pull request as ready for review June 26, 2020 14:09
@jonathanhefner
Copy link
Member

This is only my opinion, but I'm not sure I see the benefit of dedicated error pages. I feel like a combination of the following would accomplish our goals:

  • automatically handling errors, e.g. automatically running migrations in dev / test
  • using ActionableError
  • improving error messages on the exceptions themselves
  • improving the friendliness of the shared error template, if necessary

(I think it would be good to break these into separate PRs, too.)

@hahmed
Copy link
Contributor Author

hahmed commented Jun 29, 2020

Agreed 👍

I'm going to update this PR and description shortly, to only include the action for creating the database via the interface, similar to migrations.

@hahmed hahmed changed the title Show friendly database error when db does not exist Create database via UI when ActiveRecord::NoDatabaseError Jun 29, 2020
@hahmed hahmed force-pushed the db/friendly-error-when-no-db branch 2 times, most recently from dde10f0 to 600b948 Compare June 29, 2020 21:50
@hahmed hahmed closed this Jul 9, 2020
@hahmed hahmed force-pushed the db/friendly-error-when-no-db branch from 600b948 to c9abc68 Compare July 9, 2020 15:55
@hahmed hahmed reopened this Jul 9, 2020
Copy link
Member

@rafaelfranca rafaelfranca left a comment

Choose a reason for hiding this comment

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

I don't think we can tell people to run this command every time this specific exception happens.

The database may not exist because they changed by mistake the config/database.yml to point to a different database. So to resolve the issue they should not be running bin/rails db:create instead they should be fixing the database name.

I also not sure if this error would not also happen if the username or the password is wrong, or the database host is also wrong. If the same exception happens for those cases, we have more reason to not add this button by default, neither show an error message telling people to create the db since that would not fix the exception, it would probably just give another error when trying to create the database with the wrong credentials.

I feel we need to understand better the cases where this error happen.

@hahmed
Copy link
Contributor Author

hahmed commented Jul 15, 2020

I had a look into this and agree with you, I'm going to update this PR to include a detailed error message instead.

The database may not exist because they changed by mistake the config/database.yml to point to a different database.

Perhaps a more detailed error like this would be user friendly?

We could not find your database your_app_database_name_here, which can be found in the database configuration file config/database.yml.

What you could do to resolve this issue:

- Has your database name inadvertently been changed? If so, your config file would need to be the name of your database.
- Perhaps you have never created a database for this app, or deleted it? If so you could run `bin/rails db:create` to create your database.

EDIT:

I dug into this a little more and checked out what happens when the username/password/hostname is wrong re:

I also not sure if this error would not also happen if the username or the password is wrong, or the database host is also wrong.

When the username is wrong, the error message we get back is:

# pg error
PG::ConnectionBad (#<PG::ConnectionBad: FATAL:  role "randomuserrole" does not exist>)
# mysql error
Mysql2::Error::ConnectionError

When the hostname is wrong, we get the same exception as above:

# pg error
#<PG::ConnectionBad: could not connect to server: Operation timed out
Is the server running on host "notlocalhost" (92.242.132.24) and accepting
TCP/IP connections on port 5432?
# mysql error
Mysql2::Error::ConnectionError (Can't connect to MySQL server on 'notlocalhost' (60))

For sqlite3 when the username/password or hostname was random, there was no errors thrown.

Based on the errors that are returned, I feel like we could add a better error message and show the button to create the database, what do you think?

@hahmed hahmed force-pushed the db/friendly-error-when-no-db branch from a95daf1 to f4ee222 Compare July 16, 2020 21:39
@@ -8,7 +8,10 @@

module ActiveRecord
module ConnectionHandling # :nodoc:
ER_BAD_DB_ERROR = 1049
ER_BAD_DB_ERROR = 1049
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hahmed hahmed force-pushed the db/friendly-error-when-no-db branch from f4ee222 to c5bb74e Compare July 17, 2020 20:11
@hahmed hahmed requested a review from rafaelfranca August 3, 2020 09:54
@hahmed
Copy link
Contributor Author

hahmed commented Sep 22, 2020

@rafaelfranca wonder if you have any more feedback on this, I ended up looking at your suggestion about better understanding the error message and I have posted the results in my previous comment -- #39723 (comment).

With mysql we get back a number of different error messages, which I have highlighted here https://github.com/rails/rails/pull/39723/files#r456110152 and only show the button to create the database via the UI when we get a bad connection error.

Similarly to postgresql we only show the create database button in the UI when the database name exists in the error message from pg here https://github.com/rails/rails/pull/39723/files#diff-cc31fc1dd1e78db64638200f710b2f59R40

@rafaelfranca
Copy link
Member

Let's move this forward. Can you rebase?

@hahmed hahmed force-pushed the db/friendly-error-when-no-db branch 3 times, most recently from 8813595 to 4371894 Compare September 24, 2020 21:22
@hahmed
Copy link
Contributor Author

hahmed commented Sep 24, 2020

I rebased and found an error on CI (below), which seems to be related to the stuff I done. Will have a look...

Failure:
ActiveRecord::ConnectionAdapters::PostgreSQLAdapterTest#test_reconnection_error [/Users/haroon/projects/rails/activerecord/test/cases/adapters/postgresql/postgresql_adapter_test.rb:56]:
ActiveRecord::ConnectionNotEstablished expected but nothing was raised.


rails test Users/haroon/projects/rails/activerecord/test/cases/adapters/postgresql/postgresql_adapter_test.rb:25

@hahmed hahmed force-pushed the db/friendly-error-when-no-db branch 3 times, most recently from f6feb9a to c16e94c Compare October 1, 2020 20:33
@hahmed
Copy link
Contributor Author

hahmed commented Oct 21, 2020

@rafaelfranca I ended up reverting the changes for the formatted error message, as it broke a few other tests. I think I will add an attribute onto the exception's to opt-into the simple_formatted display, that way other tests are not impacted. Will follow that up in another PR if that's ok?

Let me know if there is there anything else I need to do on this PR.

Base automatically changed from master to main January 14, 2021 17:01
@hahmed
Copy link
Contributor Author

hahmed commented Feb 8, 2021

@rafaelfranca is there anything else I can do on this PR to push it forward? 😀

I split out the changes for the formatting which got merged, this PR now only deals with creating the database.

@rails-bot
Copy link

rails-bot bot commented May 9, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label May 9, 2021
@rails-bot rails-bot bot closed this May 16, 2021
@zzak
Copy link
Member

zzak commented May 16, 2021

@hahmed This change seems like we were close to merging it before, so going to re-open and give it some thought.

@zzak zzak reopened this May 16, 2021
@rails-bot rails-bot bot removed the stale label May 16, 2021
@zzak
Copy link
Member

zzak commented May 16, 2021

@hahmed Is the button still there to run the rake task for you? If not can you update the screenshots (after updating the knits I suggested)? 🙇

hahmed and others added 2 commits May 17, 2021 19:58
mysql. Allow database to be create via a button on the UI for some
connection issues.
@hahmed hahmed force-pushed the db/friendly-error-when-no-db branch from 3eadb6a to 1b8ceee Compare May 17, 2021 18:58
@hahmed
Copy link
Contributor Author

hahmed commented May 17, 2021

Thanks for reviewing @zzak ♥️

Updated screenshot has the "Create database" button:

Screenshot 2021-05-17 at 20 21 13

@zzak
Copy link
Member

zzak commented May 17, 2021

@hahmed Thanks! I will check this out and give it a closer look soon 🙏

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.

5 participants