Re: [Sqlalchemy-tickets] [sqlalchemy] #2910: simplify primary key reflection and support empty PKConstraint + arguments that gathers PK cols

[sqlalchemy <mi...@zz...>]

#2910: simplify primary key reflection and support empty PKConstraint + arguments
that gathers PK cols
------------------------------+----------------------------------
      Reporter:  zzzeek       |      Owner:  zzzeek
          Type:  enhancement  |     Status:  new
      Priority:  high         |  Milestone:  0.9.xx
     Component:  sql          |   Severity:  minor - half an hour
    Resolution:               |   Keywords:
Progress State:  needs tests  |
------------------------------+----------------------------------
Description changed by zzzeek:

Old description:

> {{{
> #!diff
> diff --git a/lib/sqlalchemy/engine/reflection.py
> b/lib/sqlalchemy/engine/reflection.py
> index badec84..5e06ad4 100644
> --- a/lib/sqlalchemy/engine/reflection.py
> +++ b/lib/sqlalchemy/engine/reflection.py
> @@ -461,6 +461,9 @@ class Inspector(object):
>          found_table = False
>          cols_by_orig_name = {}
>
> +        existing_pk_cols = list(table.primary_key.columns)
> +        table.primary_key.columns.clear()
> +
>          for col_d in self.get_columns(table_name, schema, **tblkw):
>              found_table = True
>              orig_name = col_d['name']
> @@ -518,17 +521,17 @@ class Inspector(object):
>                  for pk in pk_cons['constrained_columns']
>                  if pk in cols_by_orig_name and pk not in exclude_columns
>              ]
> -            pk_cols += [
> -                pk
> -                for pk in table.primary_key
> -                if pk.key in exclude_columns
> -            ]
> -            primary_key_constraint = sa_schema.PrimaryKeyConstraint(
> -                name=pk_cons.get('name'),
> -                *pk_cols
> -            )
> +            # update pk constraint name
> +            table.primary_key.name = pk_cons.get('name')
> +
> +            # set the primary key flag on new columns
> +            for col in pk_cols:
> +                col.primary_key = True
> +
> +            # fire a new event; this will add all existing
> +            # primary key columns
> +            table.primary_key._set_parent_with_dispatch(table)
>
> -            table.append_constraint(primary_key_constraint)
>
>          # Foreign keys
>          fkeys = self.get_foreign_keys(table_name, schema, **tblkw)
> diff --git a/lib/sqlalchemy/sql/schema.py b/lib/sqlalchemy/sql/schema.py
> index 6ee9287..5b50ea2 100644
> --- a/lib/sqlalchemy/sql/schema.py
> +++ b/lib/sqlalchemy/sql/schema.py
> @@ -434,11 +434,6 @@ class Table(SchemaItem, TableClause):
>
>      def _autoload(self, metadata, autoload_with, include_columns,
>                    exclude_columns=()):
> -        if self.primary_key.columns:
> -            PrimaryKeyConstraint(*[
> -                c for c in self.primary_key.columns
> -                if c.key in exclude_columns
> -            ])._set_parent_with_dispatch(self)
>
>          if autoload_with:
>              autoload_with.run_callable(
> @@ -2528,10 +2523,12 @@ class
> PrimaryKeyConstraint(ColumnCollectionConstraint):
>      def _set_parent(self, table):
>          super(PrimaryKeyConstraint, self)._set_parent(table)
>
> -        if table.primary_key in table.constraints:
> -            table.constraints.remove(table.primary_key)
> -        table.primary_key = self
> -        table.constraints.add(self)
> +        if table.primary_key is not self:
> +            table.constraints.discard(table.primary_key)
> +            table.primary_key = self
> +            table.constraints.add(self)
> +
> +        self.columns.extend(c for c in table.c if c.primary_key)
>
>          for c in self.columns:
>              c.primary_key = True
> }}}
>
> test such as:
>
> {{{
> from sqlalchemy import *
>
> t = Table('t', MetaData(), Column('x', Integer, primary_key=True),
>                 PrimaryKeyConstraint(mssql_clustered=True)
>             )
> print(t.primary_key)
> print(t.primary_key.kwargs)
> }}}

New description:

 {{{
 #!diff
 diff --git a/lib/sqlalchemy/engine/reflection.py
 b/lib/sqlalchemy/engine/reflection.py
 index badec84..5e06ad4 100644
 --- a/lib/sqlalchemy/engine/reflection.py
 +++ b/lib/sqlalchemy/engine/reflection.py
 @@ -461,6 +461,9 @@ class Inspector(object):
          found_table = False
          cols_by_orig_name = {}

 +        existing_pk_cols = list(table.primary_key.columns)
 +        table.primary_key.columns.clear()
 +
          for col_d in self.get_columns(table_name, schema, **tblkw):
              found_table = True
              orig_name = col_d['name']
 @@ -518,17 +521,17 @@ class Inspector(object):
                  for pk in pk_cons['constrained_columns']
                  if pk in cols_by_orig_name and pk not in exclude_columns
              ]
 -            pk_cols += [
 -                pk
 -                for pk in table.primary_key
 -                if pk.key in exclude_columns
 -            ]
 -            primary_key_constraint = sa_schema.PrimaryKeyConstraint(
 -                name=pk_cons.get('name'),
 -                *pk_cols
 -            )
 +            # update pk constraint name
 +            table.primary_key.name = pk_cons.get('name')
 +
 +            # set the primary key flag on new columns
 +            for col in pk_cols:
 +                col.primary_key = True
 +
 +            # fire a new event; this will add all existing
 +            # primary key columns
 +            table.primary_key._set_parent_with_dispatch(table)

 -            table.append_constraint(primary_key_constraint)

          # Foreign keys
          fkeys = self.get_foreign_keys(table_name, schema, **tblkw)
 diff --git a/lib/sqlalchemy/sql/schema.py b/lib/sqlalchemy/sql/schema.py
 index 6ee9287..87f7d79 100644
 --- a/lib/sqlalchemy/sql/schema.py
 +++ b/lib/sqlalchemy/sql/schema.py
 @@ -434,11 +434,6 @@ class Table(SchemaItem, TableClause):

      def _autoload(self, metadata, autoload_with, include_columns,
                    exclude_columns=()):
 -        if self.primary_key.columns:
 -            PrimaryKeyConstraint(*[
 -                c for c in self.primary_key.columns
 -                if c.key in exclude_columns
 -            ])._set_parent_with_dispatch(self)

          if autoload_with:
              autoload_with.run_callable(
 @@ -2528,10 +2523,18 @@ class
 PrimaryKeyConstraint(ColumnCollectionConstraint):
      def _set_parent(self, table):
          super(PrimaryKeyConstraint, self)._set_parent(table)

 -        if table.primary_key in table.constraints:
 -            table.constraints.remove(table.primary_key)
 -        table.primary_key = self
 -        table.constraints.add(self)
 +        if table.primary_key is not self:
 +            table.constraints.discard(table.primary_key)
 +            table.primary_key = self
 +            table.constraints.add(self)
 +
 +        table_pks = set(c for c in table.c if c.primary_key)
 +        conflicts = set(c for c in self.columns if not c.primary_key)
 +        if table_pks and conflicts:
 +            raise exc.InvalidRequestError(
 +                    "table already specifies %d columns as primary key" %
 +                    len(table_pks))
 +        self.columns.extend(table_pks)

          for c in self.columns:
              c.primary_key = True
           c.primary_key = True
 }}}

 test such as:

 {{{
 from sqlalchemy import *

 t = Table('t', MetaData(), Column('x', Integer, primary_key=True),
                 PrimaryKeyConstraint(mssql_clustered=True)
             )
 print(t.primary_key)
 print(t.primary_key.kwargs)
 }}}

 also this should raise an error:

 {{{
 from sqlalchemy import *

 t = Table('t', MetaData(), Column('x', Integer, primary_key=True),
             Column('q', Integer),
                 PrimaryKeyConstraint('q', mssql_clustered=True)
             )
 print(t.primary_key)
 print(t.primary_key.kwargs)
 }}}

--

--
Ticket URL: <http://www.sqlalchemy.org/trac/ticket/2910#comment:1>
sqlalchemy <http://www.sqlalchemy.org/>
The Database Toolkit for Python