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

server_default and Interval type #212

Closed
sqlalchemy-bot opened this issue Jun 22, 2014 · 9 comments
Closed

server_default and Interval type #212

sqlalchemy-bot opened this issue Jun 22, 2014 · 9 comments

Comments

@sqlalchemy-bot
Copy link

@sqlalchemy-bot sqlalchemy-bot commented Jun 22, 2014

Migrated issue, originally created by Scott Milliken (@smilliken)

Specifying a server_default for an Interval type and running autogenerate works fine the first time around:

interval = Column(Interval, server_default='14 days')

But on the second time around an exception is thrown:

  File "eggs/SQLAlchemy-0.9.4-py2.7-linux-x86_64.egg/sqlalchemy/engine/default.py", line 435, in do_execute
    cursor.execute(statement, parameters)
sqlalchemy.exc.ProgrammingError: (ProgrammingError) operator does not exist: interval = integer
LINE 1: SELECT '14 days'::interval = 14 days
                                   ^
HINT:  No operator matches the given name and argument type(s). You might need to add explicit type casts.
 "SELECT '14 days'::interval = 14 days" {}

I'm unable to figure out a work-around. Here's a few things I've tried:

interval = Column(Interval, server_default=text("'14 days'"))


 File "eggs/SQLAlchemy-0.9.4-py2.7-linux-x86_64.egg/sqlalchemy/engine/default.py", line 435, in do_execute
    cursor.execute(statement, parameters)
sqlalchemy.exc.ProgrammingError: (ProgrammingError) operator does not exist: interval = integer
LINE 1: SELECT '14 days'::interval = 14 days
                                   ^
HINT:  No operator matches the given name and argument type(s). You might need to add explicit type casts.



interval = Column(Interval, server_default=text("'14 days'::interval"))


  File "eggs/SQLAlchemy-0.9.4-py2.7-linux-x86_64.egg/sqlalchemy/engine/default.py", line 435, in do_execute
    cursor.execute(statement, parameters)
sqlalchemy.exc.ProgrammingError: (ProgrammingError) column "14 days'::interval" does not exist
LINE 1: SELECT '14 days'::interval = "14 days'::interval"
                                     ^
 'SELECT \'14 days\'::interval = "14 days\'::interval"' {}



interval = Column(Interval, server_default=text("('14 days')::interval"))



  File "eggs/SQLAlchemy-0.9.4-py2.7-linux-x86_64.egg/sqlalchemy/engine/default.py", line 435, in do_execute
    cursor.execute(statement, parameters)
sqlalchemy.exc.ProgrammingError: (ProgrammingError) column "('14 days')::interval" does not exist
LINE 1: SELECT '14 days'::interval = "('14 days')::interval"
                                     ^
 'SELECT \'14 days\'::interval = "(\'14 days\')::interval"' {}


interval = Column(Interval, server_default=text("('14 days')::\"interval\""))


  File "eggs/SQLAlchemy-0.9.4-py2.7-linux-x86_64.egg/sqlalchemy/engine/default.py", line 435, in do_execute
    cursor.execute(statement, parameters)
sqlalchemy.exc.ProgrammingError: (ProgrammingError) syntax error at or near "\"
LINE 1: SELECT '14 days'::interval = (\'14 days\')::"interval"
                                      ^
 'SELECT \'14 days\'::interval = (\\\'14 days\\\')::"interval"' {}
@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Jun 22, 2014

Michael Bayer (@zzzeek) wrote:

there's a bit of logic that is removing quotes from non-string types in the postgresql ddl module, and unfortunately when I comment it out, no tests fail, which means I have to figure out why this logic is there in the first place. the patch below includes a test for your case, please confirm that the fix in postgresql.py solves your issue, thanks.

diff --git a/alembic/ddl/postgresql.py b/alembic/ddl/postgresql.py
index 5ca0d1f..ade8edc 100644
--- a/alembic/ddl/postgresql.py
+++ b/alembic/ddl/postgresql.py
@@ -24,8 +24,8 @@ class PostgresqlImpl(DefaultImpl):
         if None in (conn_col_default, rendered_metadata_default):
             return conn_col_default != rendered_metadata_default
 
-        if metadata_column.type._type_affinity is not sqltypes.String:
-            rendered_metadata_default = re.sub(r"^'|'$", "", rendered_metadata_default)
+        #if metadata_column.type._type_affinity is not sqltypes.String:
+        #    rendered_metadata_default = re.sub(r"^'|'$", "", rendered_metadata_default)
 
         return not self.connection.scalar(
             "SELECT %s = %s" % (
diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py
index bd1c0b4..46509a3 100644
--- a/tests/test_postgresql.py
+++ b/tests/test_postgresql.py
@@ -1,6 +1,7 @@
 from unittest import TestCase
 
-from sqlalchemy import DateTime, MetaData, Table, Column, text, Integer, String
+from sqlalchemy import DateTime, MetaData, Table, Column, text, Integer, \
+        String, Interval
 from sqlalchemy.engine.reflection import Inspector
 from alembic.operations import Operations
 from sqlalchemy.sql import table, column
@@ -201,6 +202,12 @@ class PostgresqlDefaultCompareTest(TestCase):
             rendered,
             cols[0]['default'])
 
+    def test_compare_interval(self):
+        self._compare_default_roundtrip(
+            Interval,
+            "'14 days'"
+        )
+
     def test_compare_current_timestamp(self):
         self._compare_default_roundtrip(
             DateTime(),

Loading

@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Jun 22, 2014

Changes by Michael Bayer (@zzzeek):

  • removed labels: low priority

Loading

@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Jun 22, 2014

Scott Milliken (@smilliken) wrote:

Commenting those two lines does indeed work.

It did break with another column, though:

intarray = Column(postgresql.ARRAY(Integer), server_default=text("(ARRAY[]::integer[])"))

  File "eggs/SQLAlchemy-0.9.4-py2.7-linux-x86_64.egg/sqlalchemy/engine/default.py", line 435, in do_execute
    cursor.execute(statement, parameters)
sqlalchemy.exc.DataError: (DataError) array value must start with "{" or dimension information
LINE 1: SELECT '{}'::integer[] = '(ARRAY[]::integer[])'

And has similar behaviour when I wrap the interval as a TextClause:

interval = Column(Interval, server_default=text("('14 days')::interval"))

  File "eggs/SQLAlchemy-0.9.4-py2.7-linux-x86_64.egg/sqlalchemy/engine/default.py", line 435, in do_execute
    cursor.execute(statement, parameters)
sqlalchemy.exc.ProgrammingError: (ProgrammingError) column "('14 days')::interval" does not exist
LINE 1: SELECT '14 days'::interval = "('14 days')::interval"

Both of these work fine when I use a raw string with no TextClause like you showed ("'14 days'" and "'{}'"); I just mention this in case you'd like to support these as well.

In case it's helpful for anyone else, here's the workaround I'm using until this is decided:

# in model:
interval = Column(Interval, server_default=text("('14 days')::interval"))

# in alembic env.py:
def render_server_default(type_, col, autogen_context):
    if (type_ == "server_default" and col and
            isinstance(col.arg, sqlalchemy.sql.expression.TextClause)):
        return col.arg.text
    return False

context.configure(render_item=render_server_default)

Thanks for the help!

Loading

@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Jun 23, 2014

Michael Bayer (@zzzeek) wrote:

  • Some deep-in-the-weeds fixes to try to get "server default" comparison
    working better across platforms and expressions, in particular on
    the Postgresql backend, mostly dealing with quoting/not quoting of various
    expressions at the appropriate time and on a per-backend basis.
    Repaired and tested support for such defaults as Postgresql interval
    and array defaults.
    fixes #212

de24f3e

Loading

@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Jun 23, 2014

Changes by Michael Bayer (@zzzeek):

  • changed status to closed

Loading

@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Jun 23, 2014

Michael Bayer (@zzzeek) wrote:

OK. So I tried to greatly improve the testing here, adding cases for every permutation you've given me and checked in terms of what is accepted by SQLA .create_all() and similar in the first place as far as server default. I've moved around the quoting/unquoting logic and improved things on a per-backend basis. If you try this out and there are still failures, I can at least add exact tests cleanly, so please try it out and let me know, thanks.

Loading

@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Jun 25, 2014

Scott Milliken (@smilliken) wrote:

Tried out de24f3, and results look pretty good! This definitely solves our issue.

One inconvenience is that the migrations are rendering without the text() wrapper around the default expression, but from reading other related issues I understand it's a difficult problem to render these python expressions. This isn't a big deal for us to add manually.

I tried a bunch of different variations in attempts to break it, but as long as I wrapped the default expressions in text() in the migration before running them, it seemed very robust. Although minor, I did find one subtle bug in a rendered migration:

intarray = Column(postgresql.ARRAY(Integer), server_default='\'{}\'::integer[]')

# renders to: (notice the missing single quote)

op.add_column('test_table', sa.Column('intarray', postgresql.ARRAY(Integer()), server_default="{}'::integer[]", nullable=True))

But most importantly, I wasn't able to get the autogenerate script to crash anymore.

Thanks again for your help!!

Loading

@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Jun 25, 2014

Michael Bayer (@zzzeek) wrote:

One inconvenience is that the migrations are rendering without the text() wrapper around the default expression, but from reading other related issues I understand it's a difficult problem to render these python expressions.

this wasn't introduced by this fix though, correct? it's true those are hard to render.

Loading

@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Jun 30, 2014

Scott Milliken (@smilliken) wrote:

Correct, this isn't a regression.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant