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

Add database_exists? method to connection adapters #36471

Merged
merged 2 commits into from Jul 8, 2019

Conversation

Projects
None yet
6 participants
@itsWill
Copy link
Contributor

commented Jun 12, 2019

Summary

Extracted from: #36025

When SQLite connects it will silently create a database if the database does not
exist. This behaviour causes different issues because of inconsistent behaviour
between adapters: #36383 , #32914. This commit adds a database_exists? method
as a way to check the database without creating it. This is a stepping stone to
fully resolving the above issues.

cc: @guilleiguaran @eileencodes

Add database_exists? method to connection adapters
When SQLite connects it will silently create a database if the database does not
exist. This behaviour causes different issues because of inconsistent behaviour
between adapters: #36383, #32914. This commit adds a `database_exists?` method
as a way to check the database without creating it. This is a stepping stone to
fully resolving the above issues.

@itsWill itsWill force-pushed the itsWill:add_database_exist_method branch from 9abf533 to fe30211 Jun 17, 2019

@guilleiguaran guilleiguaran requested a review from eileencodes Jun 17, 2019

@guilleiguaran

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

This looks good for me!!

I'm going to wait for the review of @eileencodes for confirmation before of merging

@@ -264,6 +264,11 @@ def adapter_name
self.class::ADAPTER_NAME
end

# Does the database for this adapter exist?
def self.database_exists?(config)

This comment has been minimized.

Copy link
@kamipo

kamipo Jun 17, 2019

Member

Do we want to expose this method in the api doc as public API?

This comment has been minimized.

Copy link
@itsWill

itsWill Jun 17, 2019

Author Contributor

I thought this could be quite useful outside of the database tasks so I opted to keep it public, but I don't feel strongly about this I'm happy to make :no doc: it.

@morgoth

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

Is there anything, except merge conflict, preventing to get it in?

@robertomiranda

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

I would like to base my fix on this PR for close #36383, @guilleiguaran @eileencodes what do you think? the same wondering as @morgoth is the anything preventing to merge this PR?

@guilleiguaran guilleiguaran merged commit bc89116 into rails:master Jul 8, 2019

2 checks passed

buildkite/rails Build #62070 passed (10 minutes, 2 seconds)
Details
codeclimate All good!
Details
@n-rodriguez

This comment has been minimized.

Copy link

commented Jul 16, 2019

I think it might be related : #32994.
Hope it will fix it :)
Thank you 👍

@itsWill

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2019

This unfortunately won't fix it, the PG::BadConnection error will still be raised if the user initiating the connection doesn't have the permissions, since that permission error will surface before the no database error. If the user has the proper permission to access a database, tries to access it, and the database does not exist then we would get the ActiveRecord::NoDatabaseError and this method would work.

The main benefit of this method is with regards to working with sqlite3, which theres more info in the commit and PR description.

koic added a commit to koic/oracle-enhanced that referenced this pull request Jul 24, 2019

Add database_exists? method to connection adapters
### Summary

Follow up rails/rails#36471.

Oracle enhanced adapter has no implementation because
Oracle Database cannot detect `NoDatabaseError`.

Please refer to the following discussion for details.
rsim#1900

### Other Information

The following is an implementation example I gave up.

```diff
diff --git
a/lib/active_record/connection_adapters/oracle_enhanced_adapter.rb
b/lib/active_record/connection_adapters/oracle_enhanced_adapter.rb
index af99718..77a5b90 100644
--- a/lib/active_record/connection_adapters/oracle_enhanced_adapter.rb
+++ b/lib/active_record/connection_adapters/oracle_enhanced_adapter.rb
@@ -249,6 +249,12 @@ module ActiveRecord
         ADAPTER_NAME
       end

+      def self.database_exists?(config)
+        !!ActiveRecord::Base.oracle_enhanced_connection(config)
+      rescue ActiveRecord::NoDatabaseError
+        false
+      end
+
```

This is an implementation example when `NoDatabaseError` can be
supported in the future.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.