make service_name a URL option

also update docstring and add test.

2 files changed, 39 insertions, 8 deletions

diff --git a/lib/sqlalchemy/dialects/oracle/cx_oracle.py b/lib/sqlalchemy/dialects/oracle/cx_oracle.py
index 8cd4a228f..4cd6fe639 100644
--- a/lib/sqlalchemy/dialects/oracle/cx_oracle.py
+++ b/lib/sqlalchemy/dialects/oracle/cx_oracle.py
@@ -61,6 +61,12 @@ on the URL, or as keyword arguments to :func:`.create_engine()` are:
   Defaults to ``True``.  Note that this is the opposite default of the
   cx_Oracle DBAPI itself.
 
+* ``service_name`` - An option to use connection string (DSN) with
+  ``SERVICE_NAME`` instead of ``SID``. It can't be passed when a ``database``
+  part is given.
+  E.g. ``oracle+cx_oracle://scott:tiger@host:1521/?service_name=hr``
+  is a valid url. This value is only available as a URL query string argument.
+
 .. _cx_oracle_unicode:
 
 Unicode
@@ -857,12 +863,14 @@ class OracleDialect_cx_oracle(OracleDialect):
     def create_connect_args(self, url):
         dialect_opts = dict(url.query)
         for opt in ('use_ansi', 'auto_setinputsizes', 'auto_convert_lobs',
-                    'threaded', 'allow_twophase', 'use_service_name'):
+                    'threaded', 'allow_twophase'):
             if opt in dialect_opts:
                 util.coerce_kw_type(dialect_opts, opt, bool)
                 setattr(self, opt, dialect_opts[opt])
 
-        if url.database:
+        database = url.database
+        service_name = dialect_opts.get('service_name', None)
+        if database or service_name:
             # if we have a database, then we have a remote host
             port = url.port
             if port:
@@ -870,13 +878,16 @@ class OracleDialect_cx_oracle(OracleDialect):
             else:
                 port = 1521
 
-            makedsn_args = (url.database,)
-            makedsn_kwargs = {}
-            if hasattr(self, 'use_service_name') and self.use_service_name is True:
-                makedsn_args = ()
-                makedsn_kwargs = {'service_name': url.database}
+            if database and service_name:
+                raise exc.InvalidRequestError(
+                    '"service_name" option shouldn\'t '
+                    'be used with a "database" part of the url')
+            if database:
+                makedsn_kwargs = {'sid': database}
+            if service_name:
+                makedsn_kwargs = {'service_name': service_name}
 
-            dsn = self.dbapi.makedsn(url.host, port, *makedsn_args, **makedsn_kwargs)
+            dsn = self.dbapi.makedsn(url.host, port, **makedsn_kwargs)
         else:
             # we have a local tnsname
             dsn = url.host
diff --git a/test/dialect/test_oracle.py b/test/dialect/test_oracle.py
index a771c5d80..72decbdf3 100644
--- a/test/dialect/test_oracle.py
+++ b/test/dialect/test_oracle.py
@@ -1938,3 +1938,23 @@ class DBLinkReflectionTest(fixtures.TestBase):
                 autoload_with=testing.db, oracle_resolve_synonyms=True)
         eq_(list(t.c.keys()), ['id', 'data'])
         eq_(list(t.primary_key), [t.c.id])
+
+
+class ServiceNameTest(fixtures.TestBase):
+    __only_on__ = 'oracle+cx_oracle'
+
+    def test_cx_oracle_service_name(self):
+        url_string = 'oracle+cx_oracle://scott:tiger@host/?service_name=hr'
+        eng = create_engine(url_string, _initialize=False)
+        cargs, cparams = eng.dialect.create_connect_args(eng.url)
+
+        assert 'SERVICE_NAME=hr' in cparams['dsn']
+        assert 'SID=hr' not in cparams['dsn']
+
+    def test_cx_oracle_service_name_bad(self):
+        url_string = 'oracle+cx_oracle://scott:tiger@host/hr1?service_name=hr2'
+        assert_raises(
+            exc.InvalidRequestError,
+            create_engine, url_string,
+            _initialize=False
+        )