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

sa default # 49 #206

Merged
merged 4 commits into from
Nov 25, 2016
Merged

sa default # 49 #206

merged 4 commits into from
Nov 25, 2016

Conversation

vir-mir
Copy link
Member

@vir-mir vir-mir commented Nov 15, 2016

Hi!
I offer a patch for the default fields.

Do you think it makes sense for life?

@asvetlov

Issues #49

@asvetlov
Copy link
Member

Wow! Very impressive.
Let me sleep on it and make careful review.

@codecov-io
Copy link

codecov-io commented Nov 16, 2016

Current coverage is 92.26% (diff: 100%)

Merging #206 into master will increase coverage by 0.07%

@@             master       #206   diff @@
==========================================
  Files            12         12          
  Lines          1319       1331    +12   
  Methods           0          0          
  Messages          0          0          
  Branches        164        166     +2   
==========================================
+ Hits           1216       1228    +12   
  Misses           74         74          
  Partials         29         29          

Powered by Codecov. Last update d4cef36...b1ae7be

@vir-mir
Copy link
Member Author

vir-mir commented Nov 16, 2016

  • Combed code.
  • Add processing scalar, sequence and fix set value None where default not null.

@vir-mir
Copy link
Member Author

vir-mir commented Nov 17, 2016

@asvetlov any news?

@pomidoroshev
Copy link

@asvetlov Чё как?

@Eduard90
Copy link

We need default values!

def _exec_default(self, cursor, default):
if default.is_sequence:
sql = "select nextval('{}')".format(
self.dialect.identifier_preparer.format_sequence(default))
Copy link
Member

Choose a reason for hiding this comment

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

Can we avoid doing additional queries, by compiling nextval ({}) into statement like:

INSERT INTO table VALUES(nexval('table_id_seq')); 

just curious...

Copy link
Member Author

Choose a reason for hiding this comment

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

sqlalchemy makes a queries to the db.
Of course, we can make queries in advance...
such behavior we should document it explicitly

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry!
I tested.

tbl = sa.Table('sa_tbl4', meta,
               sa.Column('id', sa.Integer, nullable=False, primary_key=True),
               sa.Column('id_sequence', sa.Integer, nullable=False,
                         default=sa.Sequence('id_sequence_seq')),
               sa.Column('name', sa.String(255), nullable=False,
                         default='default test'),
               sa.Column('count', sa.Integer, default=100, nullable=None),
               sa.Column('date', sa.DateTime, default=datetime.datetime.now),
               sa.Column('count_str', sa.Integer,
                         default=sa.func.length('abcdef')),
               sa.Column('is_active', sa.Boolean, default=True))

ins = tbl.insert().values()
ins.bind = engine
print(str(ins))
INSERT INTO sa_tbl4 (id_sequence, name, count, date, count_str, is_active) VALUES (nextval('id_sequence_seq'), %(name)s, %(count)s, %(date)s, length(%(length_1)s), %(is_active)s) RETURNING sa_tbl4.id

that is, everything works correctly. I will soon do commit

Copy link
Member

@jettify jettify left a comment

Choose a reason for hiding this comment

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

@asvetlov what do you think?

@jettify
Copy link
Member

jettify commented Nov 25, 2016

@vir-mir thank you very much for this PR! Lets give @asvetlov chance to review on weekends, otherwise I will merge it on Monday.

@asvetlov
Copy link
Member

Looks good.
@jettify please merge

@jettify jettify merged commit f2123c7 into aio-libs:master Nov 25, 2016
@serg666
Copy link

serg666 commented Nov 28, 2016

Guys, what about this comment ?

@Eduard90
Copy link

I have problems with UUID (PostgreSQL) column type:

  File "/usr/lib/python3.5/uuid.py", line 137, in __init__
    hex = hex.replace('urn:', '').replace('uuid:', '')
AttributeError: 'UUID' object has no attribute 'replace'

Table:

InvoiceTable = NewTable('invoice', metadata,
                        Column('id', UUID, primary_key=True),
                        Column('affiliate_id', UUID),
                        Column('status', String, index=True, default=StatusEnum.not_approved.name),
                        Column('created_at', DateTime, default=datetime.now),
                        Column('deleted', Boolean, default=False),
                        Column('is_paid', Boolean, default=False),
                        )

Primary key (id) is UUID and "by default" it must generate new UUID on insert.
I have "monkey patch" for _execute method (class SAConnection) but it's very bad code ))

@vir-mir
Copy link
Member Author

vir-mir commented Nov 28, 2016

@Eduard90

http://docs.sqlalchemy.org/en/latest/core/custom_types.html#backend-agnostic-guid-type

class UUID(sa.types.TypeDecorator):
    impl = sa.String

    def __init__(self, length=255, *args, **kwargs):
        super().__init__(length=255, *args, **kwargs)

    def process_bind_param(self, value, dialect=None):
        if value and isinstance(value, uuid.UUID):
            return value.bytes
        elif value and not isinstance(value, uuid.UUID):
            raise ValueError('value %s is not a valid uuid.UUID' % value)
        else:
            return None

    def process_result_value(self, value, dialect=None):
        if value:
            return uuid.UUID(bytes=value)
        else:
            return None

    def is_mutable(self):
        return False

meta = sa.MetaData()
tbl = sa.Table('sa_tbl4', meta,
               sa.Column('id', UUID(), primary_key=True, default=uuid.uuid4))

...

yield from conn.execute(tbl.insert().values())
yield from conn.execute(tbl.insert().values())
yield from conn.execute(tbl.insert().values())

@jettify
Copy link
Member

jettify commented Nov 28, 2016

@vir-mir thanks! should we put this snippet to the FAQ?

@vir-mir
Copy link
Member Author

vir-mir commented Nov 28, 2016

@serg666 #56 (comment)

@vir-mir
Copy link
Member Author

vir-mir commented Nov 28, 2016

@jettify all say that it is necessary to use an approach TypeDecorator

@Eduard90
Copy link

Eduard90 commented Nov 28, 2016

@vir-mir Yes, i know about this way. But ... with this custom type Alembic generate not correct migrations.
This problem in Alembic maybe. But i want to use UUID and migrations now )))

And sqlalchemy has "builtin" type:
from sqlalchemy.dialects.postgresql import UUID
I think it's more correct than custom type. Or not?

@Eduard90
Copy link

@vir-mir How i can talk with you directly? Skype, Telegram, etc?

@vir-mir
Copy link
Member Author

vir-mir commented Nov 28, 2016

This is problem alembic

  File "../site-packages/sqlalchemy/sql/schema.py", line 79, in _init_items
    item._set_parent_with_dispatch(self)
AttributeError: 'UUID' object has no attribute '_set_parent_with_dispatch'

you can use
from sqlalchemy.dialects.postgresql import UUID

But to make it work you need to add a parameter default

from sqlalchemy.dialects.postgresql import UUID
import uuid

meta = sa.MetaData()
tbl = sa.Table('sa_tbl4', meta,
              sa.Column('id', UUID(as_uuid=True), primary_key=True, default=uuid.uuid4))

...

yield from conn.execute(tbl.insert().values())
yield from conn.execute(tbl.insert().values())
yield from conn.execute(tbl.insert().values())

My skype virmir49

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

7 participants