Refactor: Change the DeclarativeBase sqlalchemy style#748
Refactor: Change the DeclarativeBase sqlalchemy style#748HeloiseJoffe wants to merge 5 commits intoDIRACGrid:mainfrom
Conversation
|
All the steps under https://docs.sqlalchemy.org/en/20/changelog/whatsnew_20.html#migrating-an-existing-mapping should be followed before closing #612 substituting |
ryuwd
left a comment
ryuwd
left a comment
There was a problem hiding this comment.
Thanks for working on this! Next steps:
Columntomapped_column(see diff comment example injob_logging/schema.py)- Add
Mapped[...]annotations for all schema files - Remove
cast(__table__, Table)everywhere
HeloiseJoffe
force-pushed
the
refactor/sqlalchemy-DeclarativeBase
branch
from
7c3dfb0 to
c258093
Compare
ryuwd
left a comment
ryuwd
left a comment
There was a problem hiding this comment.
This is looking great! I only have small questions remaining
| device_code: Mapped[str128] = mapped_column( | ||
| "DeviceCode", unique=True | ||
| ) # Should be a hash | ||
| id_token: Mapped[Optional[dict[str, Any]]] = mapped_column("IDToken") |
There was a problem hiding this comment.
Is it a good idea to have dict[str, Any] as opposed to simply JSON?
There was a problem hiding this comment.
Mapped[] only accepts Python types and not SQLAlchemy types as JSON. So I use the Python type dict[str, Any] and map it to the SQL JSON type with type_annotation_map.
|
|
||
|
|
||
| def enum_column(name, enum_type, **kwargs): | ||
| return mapped_column(name, Enum(enum_type, native_enum=False, length=16), **kwargs) |
There was a problem hiding this comment.
Since name and enum_type change with each call, there is nothing consistent to bind with partial. I think it would just add an extra layer.
|
|
||
| Column: partial[RawColumn] = partial(RawColumn, nullable=False) | ||
| NullColumn: partial[RawColumn] = partial(RawColumn, nullable=True) | ||
| DateNowColumn = partial(Column, type_=DateTime(timezone=True), server_default=utcnow()) |
There was a problem hiding this comment.
I kept them for compatibility reasons, in case other extensions or modules use them. Once it's confirmed that no other code uses them, they can be removed.
HeloiseJoffe
force-pushed
the
refactor/sqlalchemy-DeclarativeBase
branch
from
60790a8 to
87d433c
Compare
HeloiseJoffe
force-pushed
the
refactor/sqlalchemy-DeclarativeBase
branch
from
87d433c to
09ba1cc
Compare
2 participants
closed: #612
Changes:
Questions:
There is other new styles in SQLAlchemy, for example the declarative use of Column is replaced by mapped_column.
Should this be applied as well?
In the Bookkeeping schema, this has been done, but the other changes haven’t been applied. Is there a reason for that?