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 NoDatabaseError when db does not exist #1900

Closed
wants to merge 1 commit into from

Conversation

koic
Copy link
Collaborator

@koic koic commented Jul 9, 2019

Follow up rails/rails#13469 and rails/rails#13427.

This PR raises NoDatabaseError when db does not exist.

OCIError was raised when database does not exist.
This change uses NoDatabaseError as other core adapters.

Follow up rails/rails#13469 and
rails/rails#13427.

This PR raises `NoDatabaseError` when db does not exist.

`OCIError` was raised when database does not exist.
This change uses `NoDatabaseError` as other core adapters.
@yahonda
Copy link
Collaborator

yahonda commented Jul 9, 2019

Thanks for opening a pull request.

From my point of view ORA-12154 does not always mean database does not exist.
It just means TNS alias cannot be resolved, then it could reproduce database just exists but not started up and something like that.

https://docs.oracle.com/en/database/oracle/oracle-database/19/errmg/ORA-12150.html#GUID-FF3C136B-78D5-4573-BD45-7D9CAC2A1C0F

ORA-12154: TNS:could not resolve the connect identifier specified
Cause: A connection to a database or other service was requested using a connect identifier, and the connect identifier specified could not be resolved into a connect descriptor using one of the naming methods configured. For example, if the type of connect identifier used was a net service name then the net service name could not be found in a naming method repository, or the repository could not be located or reached.

@@ -73,6 +73,12 @@ def oracle_enhanced_connection(config) #:nodoc:
ConnectionAdapters::OracleEnhancedAdapter.new(
ConnectionAdapters::OracleEnhanced::Connection.create(config), logger, config)
end
rescue OCIError => error
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops. It should also support JDBC.

expected ActiveRecord::NoDatabaseError, got #<NameError: uninitialized constant ActiveRecord::ConnectionHandling::OCIError> with backtrace:

https://travis-ci.org/rsim/oracle-enhanced/jobs/556237777#L651

@yahonda
Copy link
Collaborator

yahonda commented Jul 9, 2019

Let me have some more look at which error is appropriate to represent the database does not exist.

@yahonda yahonda self-requested a review July 10, 2019 00:40
@yahonda
Copy link
Collaborator

yahonda commented Jul 14, 2019

Summary:

From my point of view there is no good Oracle error message which represents NoDatabaseError exception. ORA-01017 is a candidate one but it could make users misunderstanding the cause of this NoDatabaseError exception, then I prefer not to implements exception yet.

Backgrounds:

As far as I understand database_exists? method by rails/rails#36471 is necessary to see if users need to execute rails db:create or not.

Unlike other mysql2 and postgresql adapters, Oracle enhanced adapter db:create task creates a database user (someone call it a database schema), not "Oracle database" since the definition of database are different between Oracle and others.

def create
system_password = ENV.fetch("ORACLE_SYSTEM_PASSWORD") {
print "Please provide the SYSTEM password for your Oracle installation (set ORACLE_SYSTEM_PASSWORD to avoid this prompt)\n>"
$stdin.gets.strip
}
establish_connection(@config.merge("username" => "SYSTEM", "password" => system_password))
begin
connection.execute "CREATE USER #{@config['username']} IDENTIFIED BY #{@config['password']}"
rescue => e
if /ORA-01920/.match?(e.message) # user name conflicts with another user or role name
connection.execute "ALTER USER #{@config['username']} IDENTIFIED BY #{@config['password']}"
else
raise e
end
end

Then database_exists? needs to return if the database user(schema) exists or not, then I think one of the candidates error message should be: ORA-01017: invalid username/password; logon denied.

Unfortunately this error message could be reported:

  • When the database user does not exist
  • When the database user exists but the password is incorrect

This error message can be used to raise NoDatabaseError for the first case only, for the second one the database (Here I mean database schema in Oracle) exists but the password is incorrect. By handling all of ORA-01017 Oracle error message as NoDatabaseError exception, which cause users misunderstanding the cause of this NoDatabaseError like setting incorrect password in database.yml .

koic added a commit to koic/oracle-enhanced that referenced this pull request Jul 24, 2019
### 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.
@koic
Copy link
Collaborator Author

koic commented Jul 24, 2019

Thank you for your investigation. I opened this PR in preparation for PR #1906. However, I will close this PR because Oracle database cannot detect NoDatabaseError. Thank you again.

@koic koic closed this Jul 24, 2019
@koic koic deleted the no_database_error branch July 24, 2019 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants