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

Implement column defaults for INSERT/UPDATE #206

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

GarbageHamburger
Copy link

@GarbageHamburger GarbageHamburger commented May 4, 2020

This PR implements usage of column defaults from SQLAlchemy. In regular SQLAlchemy the default column values are fetched using ExecutionContext._exec_default. However, we don't use the ExecutionContext here, so a function is implemented to take its place for column defaults.

Fixes #72

Co-authored-by: Rafał Pitoń <[email protected]>
@GarbageHamburger
Copy link
Author

Any updates on this?

@tomchristie
Copy link
Member

It's difficult because I'm not really sure if we want this functionality or not.

@andre-dasilva
Copy link

@tomchristie I am just curious. Why would you not want this functionality? I am currently writing a application with fastapi and encode/databases and i want the timestamp to be created on insert. currently i don't have a way of doing this.

@camgunz
Copy link

camgunz commented May 30, 2020

It also seems necessary for UUID primary keys, which are pretty common these days.

@vmarkovtsev
Copy link
Contributor

vmarkovtsev commented Jun 2, 2020

The column defaults are higher level sqla API ("Query") than the API used in databases ("Core"). Emulating them properly is quite hard. For example, the functions can access the context in several ways, and filling it is twisted.

The problem can be solved outside of databases package by adding a new method to your model base class to explicitly create defaults.

@GarbageHamburger
Copy link
Author

@vmarkovtsev SQLAlchemy core does have defaults, see the default parameter for Column.__init__. I agree that correctly emulating defaults is hard due to the types of things a column default can be (it can be a ClauseElement!), but that's no reason for it to not be included.

@ricosuave73
Copy link

+1 for this. I followed the FastAPI tutorial and tried to implement a UUID for sqlite. I couldn't figure out why it didn't work until I specifically checked this repo for issues around the defaults.
And defaults are indeed part of sqlalchemy core.

@elpapi42
Copy link

This is a must, thanks

@elpapi42
Copy link

Approve this pls

Copy link
Contributor

@vmarkovtsev vmarkovtsev left a comment

Choose a reason for hiding this comment

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

Sorry fellows, I wish I approved this, but this looks too hacky right now to be included as a general solution.

Copy link
Contributor

@vmarkovtsev vmarkovtsev left a comment

Choose a reason for hiding this comment

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

These are my suggestions 👍

Comment on lines +299 to +307

# 2 paths where we apply column defaults:
# - values are supplied (the object must be a ValuesBase)
# - values is None but the object is a ValuesBase
if values is not None and not isinstance(query, ValuesBase):
raise TypeError("values supplied but query doesn't support .values()")

if values is not None or isinstance(query, ValuesBase):
values = Connection._apply_column_defaults(query, values)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# 2 paths where we apply column defaults:
# - values are supplied (the object must be a ValuesBase)
# - values is None but the object is a ValuesBase
if values is not None and not isinstance(query, ValuesBase):
raise TypeError("values supplied but query doesn't support .values()")
if values is not None or isinstance(query, ValuesBase):
values = Connection._apply_column_defaults(query, values)
elif values:
assert isinstance(query, ValuesBase)
values = self._apply_column_defaults(query, values)

Comment on lines +325 to +327
if default.is_sequence: # pragma: no cover
# TODO: support sequences
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

assert not default.is_sequence, "sequences are not supported, PRs welcome"

values = values or {}

for column in query.table.c:
if column.name in values:
Copy link
Contributor

Choose a reason for hiding this comment

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

values can have columns as keys, in such cases this will be always false

continue
elif default.is_callable:
value = default.arg(FakeExecutionContext())
elif default.is_clause_element: # pragma: no cover
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
elif default.is_clause_element: # pragma: no cover
assert not default.is_clause_element, "clause defaults are not supported, PRs welcome"

Copy link
Contributor

Choose a reason for hiding this comment

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

  • can you please group assertions together

continue
else:
value = default.arg

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

blows up loudly and clearly.
"""

def __getattr__(self, _: str) -> typing.NoReturn: # pragma: no cover
Copy link
Contributor

Choose a reason for hiding this comment

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

We should cover this by another test which tests raising NotImplementedError

@@ -489,3 +531,20 @@ def __repr__(self) -> str:

def __eq__(self, other: typing.Any) -> bool:
return str(self) == str(other)


class FakeExecutionContext:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
class FakeExecutionContext:
class DummyExecutionContext:

(it's not completely fake, after all)

"""

def __getattr__(self, _: str) -> typing.NoReturn: # pragma: no cover
raise NotImplementedError(
Copy link
Contributor

Choose a reason for hiding this comment

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

btw: my own custom implementation of this (yep, I have a similar hack in my prod), I pass the current values to this context so that it becomes much more useful.

@etscript
Copy link

Any updates on this?

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.

Support for "default" parameter in sqlalchemy.Column
10 participants