- FOR UPDATE is emitted in the syntactically correct position

when limit/offset is used, i.e. the ROWNUM subquery.
However, Oracle can't really handle FOR UPDATE with ORDER BY
or with subqueries, so its still not very usable, but at
least SQLA gets the SQL past the Oracle parser.
[ticket:1815]

3 files changed, 77 insertions, 11 deletions

diff --git a/CHANGES b/CHANGES
index 997e046c1..2c9aef218 100644
--- a/CHANGES
+++ b/CHANGES
@@ -85,6 +85,13 @@ CHANGES
     in connect strings here since we don't know what encoding
     we could use.  [ticket:1670]
 
+  - FOR UPDATE is emitted in the syntactically correct position
+    when limit/offset is used, i.e. the ROWNUM subquery.
+    However, Oracle can't really handle FOR UPDATE with ORDER BY
+    or with subqueries, so its still not very usable, but at
+    least SQLA gets the SQL past the Oracle parser.
+    [ticket:1815]
+    
 - firebird
   - Added a label to the query used within has_table() and 
     has_sequence() to work with older versions of Firebird
diff --git a/lib/sqlalchemy/dialects/oracle/base.py b/lib/sqlalchemy/dialects/oracle/base.py
index 6c8055138..cd232fa00 100644
--- a/lib/sqlalchemy/dialects/oracle/base.py
+++ b/lib/sqlalchemy/dialects/oracle/base.py
@@ -479,7 +479,7 @@ class OracleCompiler(compiler.SQLCompiler):
 
                 limitselect._oracle_visit = True
                 limitselect._is_wrapper = True
-
+                
                 # If needed, add the limiting clause
                 if select._limit is not None:
                     max_row = select._limit
@@ -490,22 +490,24 @@ class OracleCompiler(compiler.SQLCompiler):
 
                 # If needed, add the ora_rn, and wrap again with offset.
                 if select._offset is None:
+                    limitselect.for_update = select.for_update
                     select = limitselect
                 else:
-                     limitselect = limitselect.column(
+                    limitselect = limitselect.column(
                              sql.literal_column("ROWNUM").label("ora_rn"))
-                     limitselect._oracle_visit = True
-                     limitselect._is_wrapper = True
+                    limitselect._oracle_visit = True
+                    limitselect._is_wrapper = True
 
-                     offsetselect = sql.select(
+                    offsetselect = sql.select(
                              [c for c in limitselect.c if c.key!='ora_rn'])
-                     offsetselect._oracle_visit = True
-                     offsetselect._is_wrapper = True
+                    offsetselect._oracle_visit = True
+                    offsetselect._is_wrapper = True
 
-                     offsetselect.append_whereclause(
+                    offsetselect.append_whereclause(
                              sql.literal_column("ora_rn")>select._offset)
 
-                     select = offsetselect
+                    offsetselect.for_update = select.for_update
+                    select = offsetselect
 
         kwargs['iswrapper'] = getattr(select, '_is_wrapper', False)
         return compiler.SQLCompiler.visit_select(self, select, **kwargs)
@@ -514,7 +516,9 @@ class OracleCompiler(compiler.SQLCompiler):
         return ""
 
     def for_update_clause(self, select):
-        if select.for_update == "nowait":
+        if self.is_subquery():
+            return ""
+        elif select.for_update == "nowait":
             return " FOR UPDATE NOWAIT"
         else:
             return super(OracleCompiler, self).for_update_clause(select)
diff --git a/test/dialect/test_oracle.py b/test/dialect/test_oracle.py
index b9fb9a133..9508b38bf 100644
--- a/test/dialect/test_oracle.py
+++ b/test/dialect/test_oracle.py
@@ -5,7 +5,7 @@ from sqlalchemy import *
 from sqlalchemy import types as sqltypes, exc
 from sqlalchemy.sql import table, column
 from sqlalchemy.test import *
-from sqlalchemy.test.testing import eq_, assert_raises
+from sqlalchemy.test.testing import eq_, assert_raises, assert_raises_message
 from sqlalchemy.test.engines import testing_engine
 from sqlalchemy.dialects.oracle import cx_oracle, base as oracle
 from sqlalchemy.engine import default
@@ -113,6 +113,25 @@ class CompileTest(TestBase, AssertsCompiledSQL):
         self.assert_compile(s, "SELECT col1, col2 FROM (SELECT col1, col2, ROWNUM "
             "AS ora_rn FROM (SELECT sometable.col1 AS col1, sometable.col2 AS col2 FROM sometable "
             "ORDER BY sometable.col2) WHERE ROWNUM <= :ROWNUM_1) WHERE ora_rn > :ora_rn_1")
+
+        s = select([t], for_update=True).limit(10).order_by(t.c.col2)
+        self.assert_compile(
+            s,
+            "SELECT col1, col2 FROM (SELECT sometable.col1 "
+            "AS col1, sometable.col2 AS col2 FROM sometable "
+            "ORDER BY sometable.col2) WHERE ROWNUM <= :ROWNUM_1 FOR UPDATE"
+        )
+        
+        s = select([t], for_update=True).limit(10).offset(20).order_by(t.c.col2)
+        self.assert_compile(
+            s,
+            "SELECT col1, col2 FROM (SELECT col1, col2, ROWNUM "
+            "AS ora_rn FROM (SELECT sometable.col1 AS col1, "
+            "sometable.col2 AS col2 FROM sometable ORDER BY "
+            "sometable.col2) WHERE ROWNUM <= :ROWNUM_1) WHERE "
+            "ora_rn > :ora_rn_1 FOR UPDATE"
+        )
+        
     
     def test_long_labels(self):
         dialect = default.DefaultDialect()
@@ -924,9 +943,45 @@ class SequenceTest(TestBase, AssertsCompiledSQL):
 
 class ExecuteTest(TestBase):
     __only_on__ = 'oracle'
+    
+    
     def test_basic(self):
         eq_(
             testing.db.execute("/*+ this is a comment */ SELECT 1 FROM DUAL").fetchall(),
             [(1,)]
         )
 
+    @testing.provide_metadata
+    def test_limit_offset_for_update(self):
+        # oracle can't actually do the ROWNUM thing with FOR UPDATE
+        # very well.
+        
+        t = Table('t1', metadata, Column('id', Integer, primary_key=True),
+            Column('data', Integer)
+        )
+        metadata.create_all()
+        
+        t.insert().execute(
+            {'id':1, 'data':1},
+            {'id':2, 'data':7},
+            {'id':3, 'data':12},
+            {'id':4, 'data':15},
+            {'id':5, 'data':32},
+        )
+        
+        # here, we can't use ORDER BY.
+        eq_(
+            t.select(for_update=True).limit(2).execute().fetchall(),
+            [(1, 1),
+             (2, 7)]
+        )
+
+        # here, its impossible.  But we'd prefer it to raise ORA-02014
+        # instead of issuing a syntax error.
+        assert_raises_message(
+            exc.DatabaseError,
+            "ORA-02014",
+            t.select(for_update=True).limit(2).offset(3).execute
+        )
+        
+