amaslyaev / noorm

NoORM (Not only ORM) - Python library that makes your database operations convenient and natural
MIT License
16 stars 0 forks source link

Discussion of using type hints for return data #6

Closed LecronRu closed 3 weeks ago

LecronRu commented 1 month ago

Фактически, всем декораторам соответствует определённый тип, который мы указываем в аннотации функции. sql_fetch_all -> list[classname] sql_fetch_scalars -> list[datatype] sql_one_or_none -> Optional[classname] sql_scalar_or_none -> Optional[datatype] sql_execute -> None sql_iterate -> Iterator[classname] sql_iterate_scalars -> Iterator[datatype]

Где datatype простые типы str, int, float и прочие.

С учетом тенденции использования аннотаций типов и популяризации данного подхода, разумно брать информацию из них. Тогда пачку декораторов можно будет свести к одному @nm.sql

@nm.sql(statement)
def get_all_users() -> list[DbUser]:
    pass  # no parameters, so just "pass"

@nm.sql(statement)
def get_user_by_id(id_: int) -> DbUser | None:
    return nm.params(id=id_)

Очень наглядно.

Так как это тоже требует потери обратной совместимости, можно совместить с возможностью указания соединения по умолчанию опциональным параметром. @nm.sql(statement, connect=...)

amaslyaev commented 1 month ago

Теоретически такое можно сделать, но очень не хочется, потому что будет вносить дополнительный когнитивный диссонанс.

Допустим, где-то в коде мы видим такую функцию:

@some_decorator
def my_func() -> list[int]:
    return None

Не знаю как у кого, но у меня сразу в мозгу загорается красная лампочка "Ошибка!". Функция пообещала вернуть список интеджеров, а возвращает None. Мне нужно поднять глаза на строчку выше, увидеть декоратор, вспомнить, что этот декоратор меняет сигнатуру, и поэтому list[int] это не враньё, а очень важная инструкция для @some_decorator. Пока я на это всё отвлекался, уже забыл, зачем забрёл в этот кусок кода, потерял мысль, выпал из потока.

Возможно, есть красивый способ уговорить mypy не ныть типа так:

error: Incompatible return value type (got "None", expected "List[int]")  [return-value]

но когнитивный диссонанс никуда не денется.

Кроме того, если мне память не изменяет, сам Гвидо рекомендовал по возможности воздерживаться от использования аннотаций типов в рантайм-логике. Даже была история, как в какой-то из альфа-версий их совсем убрали из рантайма, но из-за этого совсем сломался pydantic и был крупный скандал.

Так что идея интересная, но давайте обойдёмся без этой фичи.

LecronRu commented 1 month ago

Когда есть привычка работать с аннотациями, код с прописанными

@some_decorator
def my_func() -> list[int]:
    pass  # он же, неявный return None

так или иначе вызвает когнитивный диссонанс. Независимо, берем ли мы данные из типа в рантайме или это просто аннотация. Более того, нужно не просто вспомнить что декоратор меняет тип, а как меняет тип конкретный декоратор из 7 доступных. Я уже столкнулся с этой проблемой и чтобы избежать такого конфуза, прописываю return nm.params() без параметров. Мозг видит черный ящик и не стопорится на деталях. Так что это ничуть не проблема.

Но раз не считаете нужным, не стану настивать.

LecronRu commented 1 month ago

Еще вариант. Мы уже указываем тип в декораторе. Декоратор может определить комплексный ли это тип (classname) или скалярный (datatype). Осталось только указать "обертку" — list, Optional или Iterator. То есть количество декораторов можно сократить с 7 до 4 без вреда для функциональности и с пользой для читабельности. sql_list -> list[classname | datatype] sql_optional -> Optional[classname | datatype] sql_iterate -> Iterator[classname | datatype] sql_execute -> None Названия декротаоров изменил для улучшения понятности идеи и возможно устранения проблем обратной совместимости.

amaslyaev commented 1 month ago

Тут такое дело. В Питоне всё есть объект, и грань между "простыми" и "комплексными" типами данных весьма тонкая. Date, например, можно инициализировать как из ISO-строки (такая удобняшка сделана в noorm-адаптерах для SQLite), так и ничто не мешает получать дату из колонок year, month и day. Какой-нибудь самодельный class – как понять, это скаляр или комплексный тип? Если скаляр, нам нужно будет при инициализации скормить в него значение из первой колонки, а если не скаляр, то все колоники результата как именованные параметры.

Мне не нравится пользоваться инструментами, которые пытаются мне угодить, но в результате отодвигают меня от принятия решения, как оно должно сработать и чего конкретно я хочу добиться. В "Zen of Python" написана очень хорошая мысль: "Explicit is better than implicit". 7 декораторов (в песпективе 8) вполне себе explicit. Попытка сократить их количество за счёт мудрого, сложного, но иногда преподносящего неприятные сюрпризы механизма распознавания скалярности – это будет тот самый implicit, который по-любому хуже имеющегося explicit.

LecronRu commented 1 month ago

Вы же читаете description запроса. Кто мешает по количеству возвращенных столбцов определить метод распаковки — значение из единственной первой колонки или именованные параметры? Более того, метод можно даже закешировать, присвоив переменной при первом запросе, а в дальнешем если он определен, использовать сразу без анализа. Или даже на этапе декорирования, выполнив запрос с фейковыми параметрами.

amaslyaev commented 1 month ago

Допустим, такая ситуация:

@dataclass
class MeteoData:
    time: datetime | None = None
    point_id: int | None = None
    temperature: float | None = None

@nm.sql_optional(  # Типа пусть само догадается, скаляр это или не скаляр
    MeteoData, "select avg(temperature) as temperature from meteo_log where time >= current_date"
)
def get_average_temperature_today_all_points():
    pass

У результата запроса одна колонка, но её нужно подсунуть датаклассу как именованный параметр. Всё равно получается зло.

LecronRu commented 1 month ago

Ну не прям так в лоб проверка по количеству, а с учетом типа:

maybe_scalar = (int, float, str, bool, date, datetime, Decimal)
is_scalar = len(q_res) == 1 and issubclass(res_type, maybe_scalar)
amaslyaev commented 1 month ago

Ну то есть по-любому приходим к угадыванию операции по типу результата.

LecronRu commented 1 month ago

Не угадыванию, а определению. Есть спорные моменты? Предлагайте. Обсудим возможность их устранения. У нас фактически только 3 спорных типа — date, datetime, Decimal. Которые могут принимать как 1 аргумент, так и несколько. int, float, str, bool — априори скаляр. Остальное — априори пользовательский класс.

amaslyaev commented 1 month ago

Допустим, такой пользовательский класс:

class GeoJsonFeature:
    def __init__(self, json_str: str | None = None, **kwargs) -> None:
        if json_str:
            data: dict[str, Any] = json.loads(json_str)
        else:
            data = {}

        if kwargs:
            data.update(kwargs)
        geom = data.pop("geometry", None)
        self.geometry: dict[str, Any] | None = json.loads(geom) if isinstance(geom, str) else geom
        self.properties: dict[str, Any] = data.pop("properties", None)
        if data:
            self.properties.update(data)

Можно проинициализировать строкой в формате GeoJSON Feature, а можно именованными параметрами, которые все за исключением geometry пойдут в атрибут properties. А можно подать и то и другое, и тогда за основу возьмётся json_str, и на него наложатся именованные параметры.

Три способа, как это добывать из базы. Способ 1 (скаляры):

@nm.sql_fetch_scalars(
    GeoJsonFeature, "select orig_geojson from geo_objects where parent_id = ?"
)
def get_children_orig_geo(parent_id: int):
    return nm.params(parent_id)

Способ 2 (объекты, хоть и одна колонка):

@nm.sql_fetch_all(
    GeoJsonFeature, "select centroid as geometry from geo_objects where parent_id = ?"
)
def get_children_centroids(parent_id: int):
    return nm.params(parent_id)

Способ 3 (объекты по-нормальному):

@nm.sql_fetch_all(
    GeoJsonFeature, "select id, name, orig_geojson as json_str from geo_objects where parent_id = ?"
)
def get_children(parent_id: int):
    return nm.params(parent_id)
LecronRu commented 1 month ago

Скаляр, это извлеченное единственное поле (распаковка единичного кортежа), с типом уже созданным драйвером базы данных. Ну, или в случае noorm, с адаптерами для некоторых типов полей. Остальное пользовательский объект, пусть и с одним полем. Сейчас вы вводите концепцию псевдоскаляра. Зачем?

Пример искусственный. Можно переписать первый вариант на orig_geojson as json_str, аналогично третьему. Зачем игнорировать конвенцию совпадения имен полей и возвращать через псевдоскаляр? Просто потому что прокатит? Два способа сделать одну вещь, не есть гуд.

Кстати, для этого даже не надо было такого сложного примера. Достаточно было класса с одним параметром в __init__.

amaslyaev commented 1 month ago

Юмор в том, что в большинстве случаев GeoJsonFeature это и есть скаляр. "Какая-то строка с циферками и скобочками". Чаще всего мы его из строки и инициализируем. Чтобы всунуть в геопандас и показать в юпитер-ноутбуке карту, или прям как есть отдать на фронтенд. Но в отдельных экзотических случаях бывает полезно докинуть туда немножко properties, или, что случается совсем редко, собрать эту штуку из компонент. Именно поэтому все тонкие манипуляции "для продвинутых" вынесены в невнятные kwargs.

Кстати, в GraphQL-схемах для фронтенда эти геофичи объявляются обычно именно как скалярный тип, потому что слишком сложно формализовывать все варианты того, что там может быть внутри. Да и не нужно это никому, потому что фронтендные JS-библиотеки для отрисовки карт прекрасно умеют хавать GeoJSON в сыром виде.

Кстати, для этого даже не надо было такого сложного примера. Достаточно было класса с одним параметром в __init__.

Тогда мы имели бы шанс проинтуичить, что "результат запроса с одной колонкой и конструктор объекта с одним параметром, значит можем трактовать как скаляр". Самый интересный случай ("select centroid as geometry ...") не удалось бы показать.

LecronRu commented 1 month ago

Еще раз повторю свое видение. Пользовательский класс, позволяющий инициализацию неименованным параметром, не становится автоматически скаляром. Его конечно можно так инициализоварать, но ничто не мешает передать именованый

@nm.sql_fetch_all(
    GeoJsonFeature, "select orig_geojson as json_str from geo_objects where parent_id = ?"
)

Автоопределение скаляра, не ограничивает пользователя библиотеки в решении задач. Максимум, в способе решения. Причем оставшийся способ не сложнее. Всего лишь добавить псевдоним поля в запрос. Проблема? При этом из-за сокращения вдвое количества используемых декораторов упростится как использование в целом, так и предположу, код самой библиотеки.

Не хочу вырождения дискуссии в неконструктивную. Нет, значит нет. Вы автор, вы так видете. Не имею права настаивать.

amaslyaev commented 1 month ago

В принципе, 7 сущностей это не так уж и много. Как раз тот самый "кошелёк Мюллера", количество сущностей, которые мозг может мыслить одновременно. По опыту использования библиотечки (оно уже в Проде, в разных компонентах, и уже довольно давно), отдельные декораторы это удобно, практично и никакого дискомфорта не вызывает.

Но, конечно, это всегда полезно подумать над альтернативными решениями, поиграть в "что если...". Вспомнить, как и почему были приняты те решения, которые были приняты. И за это вам спасибо.

LecronRu commented 1 month ago

Возращаясь к исходной теме... Думал разбор аннотаций дело сложное. Для чекеров, может быть. Но для обсуждаемой задачи оказалось простейшим.

def get_returning(func):
    result = func.__annotations__['return']
    container = None
    if hasattr(result, '__args__'): # расширенный тип
        args = result.__args__
        if type(None) in args:  # optional
            mode = 'fetchone'
            res_type = next(_ for _ in args if _ != type(None))
        else: # container
            mode = 'fetchall'
            container = result.__origin__
            res_type = args[0]
    else: # простой тип
        mode = 'fetchone'
        res_type = result
    return (mode, container, res_type)

Что для проверенных мной аннотаций выдало:

#list[str] ('fetchall', <class 'list'>, <class 'str'>)
#typing.List[str] ('fetchall', <class 'list'>, <class 'str'>)
#typing.Union[str, None] ('fetchone', None, <class 'str'>)
#typing.Optional[str] ('fetchone', None, <class 'str'>)
#None | str ('fetchone', None, <class 'str'>)
#typing.Iterator[str] ('fetchall', <class 'collections.abc.Iterator'>, <class 'str'>)
#str ('fetchone', None, <class 'str'>)
#list[Foo] ('fetchall', <class 'list'>, <class '__main__.Foo'>)
#None ('fetchone', None, None)

Несомненно, функция окажется сложнее. Не проверял невозможные для возврата аннотации. Но не думаю, что намного сложнее. А вместе с предложеным способом определения скаляров, даст полное автоопределение параметров запроса данных. Достаточно указать @sql(statement). Ну лепота же?!

LecronRu commented 1 month ago

Сделал себе отдельный модуль. Цель — простое должно быть просто. Код модуля чуть больше 100 строк. Можете глянуть, вдруг еще какие идеи придут.

Нет и не будет поддержки статистики, других баз данных, алхимии, асинхронщины. Тип запроса определяется автоматически, по типу результата. Заодно, в большинстве случаев, автоматически определяется маппинг параметров, исходя из параметров функции, но можно и вручную. Правда пока нет адаптеров-полезняшек, ну да это дело наживное.

sqliteEx.zip

amaslyaev commented 1 month ago

Посмотрел. Выглядит симпатичненько. Тоже немножко поэкспериментировал. Помимо всяких краевых случаев, с которыми может быть затык, самая бесячая штука это mypy, ноющий, что типа пообещали вернуть из функции list, а не возвращаем ничего. Лечится распихиванием уродливых # type: ignore, но радости от этого мало. У нас, например, все критически важные куски кодовой базы сидят под mypy, и если там error, оно в облако не задеплоится, даже в staging environment.

Ну и, конечно, поддержка других СУБД, где list[int] и dict[str, Any] вполне бывают сидящими в колонке скалярами (в Постгресе соответственно типы ARRAY и JSONB), добавляет прикольчиков.

LecronRu commented 1 month ago

Спасибо за ответ. Только сейчас понял, что вам такое действительно противопоказано. Слишком широкий охват. И тоже только сейчас придумал странный способ модификации под более специфичную задачу — simple sqlite.

Конфигуратор! Извлечением данных будет по прежнему заниматься ваша библиотека, а декоратор query, будет анализировать параметры, выбирать нужный noorm декоратор, подставлять в него тип, оборачивать результат для кмк более удобной работы с default_db и где-то в процессе, генерировать nm.params. Так как все это выполняется однократно, накладных расходов не будет.

Такая вот матрешка. Не знаю что получится в итоге и потяну ли, но если получится, будет даже лучше ранее предложенного. Вплоть до включения непосредственно в ваш проект, частично или полностью. Интересно?

пообещали вернуть из функции list, а не возвращаем ничего. Лечится распихиванием уродливых # type: ignore, но радости от этого мало.

Ну знаете ли... распихивать ignore уродливо, а распихивать nm.params там, где соблюдается конвенция именований, а это большинство случаев, будто бы не бойлерплейт, а красота. Причем даже у тех, кто mypy не пользуется. Впрочем, есть еще вариант костыля. В декораторе модифицировать тип с list[T] на Optional[list[T]]. Для *_or_none модификация естественно не нужна.

PS. Кстати... не хочу создавать новую "проблему", но хочу попросить анализировать не только типы полей датакласса, но и обычного класса, и наверное typing.NamedTuple. Для применения удобняшек с data/decimal. Пока еще не сталкивался с реальной необходимостью, но раз техническая возможность есть...

amaslyaev commented 1 month ago

Кстати, нашёл у вас весьма рисковый кусок:

    @run_once
    def is_unnamed_placeholder(self, connect):
        try:
            connect.execute(self.sql, {'': ''})  # <<<<<< ЗДЕСЬ
        except sqlite3.ProgrammingError as e:
            # Binding 1 has no name, but you supplied a dictionary (which has only names).
            return e.args[0].find('has no name') >= 0

Допустим, такая функция, которая дропает некую временную таблицу:

@query("drop table my_tmp_tab", default_connect=db)
def drop_tmp_tab() -> None:
    pass

Когда мы её вызываем первый раз, она сначала дропнет my_tmp_tab в функции is_unnamed_placeholder, а потом попытается выполнить запрос ещё раз и отвалится с ошибкой потому что такой таблицы уже нет.

Завёрнутое в @query создание таблицы my_tmp_tab тоже, кстати, не будет работать.

Как по мне, истолнение чего-то в БД чисто чтобы попробовать, выглядит капец страшно.

LecronRu commented 1 month ago

Спасибо, что заметили. Когда писал, была мысль про вложенные транзакции (savepoint/rollback to), но что-то не срослось. А чтобы вообще лишний раз не дергать базу, можно предварительно проверить `if not self.args: return False'.

>>> db = sqlite3.connect(':memory:')
>>> db.execute('SAVEPOINT a;', {})
>>> db.execute('CREATE TABLE y (x,z)', {})
>>> db.execute('ROLLBACK TO a', {})
>>> db.execute('CREATE TABLE y (x,z)', {})
<sqlite3.Cursor object at 0x00000211DEFE5DC0>

Без транзакции, получил бы sqlite3.OperationalError: table y already exists

amaslyaev commented 1 month ago

И ещё момент. По опыту использования этой беды могу сказать, что функции это не всегда простые прямолинейные штуки в стиле "принять параметры и запихнуть их в nm.params". То есть в большинстве случаев именно так, но есть и функции, в которых нафигачено немножко кода. Классика жанра – всякие апдейтеры, которые обычно сделаны как-то так:

UPDATABLE_USERS_FIELDS = set(("username", "firstname", "lastname", "password_hash"))

@nm.sql_execute
def upd_user(user_id: int, **kwargs):
    if not kwargs:
        raise nm.CancelExecException
    if (wrong := set(kwargs.keys()) - UPDATABLE_USERS_FIELDS):
        raise RuntimeError("Cannot update user fields: " + ", ".join(wrong))
    set_stmts = ", ".join(f"{k} = :{k}" for k in kwargs.keys())
    upd_stmt = f"update users set {set_stmts} where id = :user_id"
    return nm.query_and_params(upd_stmt, user_id=user_id, **kwargs)

Это лучше, чем писать тонну функций-апдейтеров отдельно на каждое поле и на каждое возможное их сочетание.

Ещё бывает полезно делать всякую гибкую фильтрацию. То есть в зависимости от входных параметров дописывать условия во WHERE.

LecronRu commented 1 month ago

Знаю. Понимаю. Более того, хотел вас спросить как делать не только пред-, но постобработку полученных данных. Но не стал, а просто создаю еще одну функцию.

Однако наличие сложных примеров, ничего не меняет. Желание: простое просто. Что требует интерфейса, при котором пользователь "может", а не "должен". Как пример, уже приводил FastAPI. Тип может быть указан в декораторе параметром response_model и он становится главным. Ни у кого нет проблем с неоднозначностью.

В применении к noorm:

  1. Не указан тип результата, выводим из типа функции.
  2. Не указан маппинг (скрытый return None), берем из параметров.
  3. Не указан декоратор определяющий контейнер и скалярность (@query), определяем из количества полей и типа возврата.
amaslyaev commented 1 month ago

просто создаю еще одну функцию

Я тоже :smile_cat:

Что касается остального, то да, есть над чем подумать. Но очень сильно не сейчас.

LecronRu commented 1 month ago

Больше всего напрягало создание ленивой обработки, в ожиданнии получения реального connect. Приходилось хранить много состояния. И согласен, в базу так лазить не совсем красиво. Подтянул 1 зависимость — парсер sql. Выбрал sqlglot. На основе rust-токенайзера, очень быстрый. Впрочем, если время парсинга начнет напрягать, несложно сделать персистент-кеш, с хранением результата парсинга (список полей и параметров) между сессиями.

Полученный mvp сразу оказался предельно прост и понятен. Преобразовал прошлую версию в нынешнюю буквально за час. Осталось сделать параметры по умолчанию для других баз, о чем говорил раньше (пользователь может конкретизировать, а не должен). Но для sqlite всё устраивает. Максимум повозится с типами ParamSpec, для подсказок в IDE.

decor.py.zip

amaslyaev commented 3 weeks ago

Лучшее – враг хорошего. Иногда злейший. Об этом в нашей айтишечке слишком часто забывают, от души навешивая фичи, одна вкуснее другой, не замечая момент, когда следовало остановиться.

Очень извиняюсь, но эту прекрасную фичу в библиотеку пускать не буду. Ни в каком виде. Пожалуйста не обижайтесь. Чуйка подсказывает, что не надо этого делать. Не просто подсказывает, а мигает всеми красными лампочками.

Я закрываю эту дискуссию, ОК?

P.S. Баг с sqlite3.PARSE_DECLTYPES я, кстати, пофиксил. Спасибо, что нашли.

LecronRu commented 3 weeks ago

В принципе, ранее, я уже согласился, что для вашей библиотеки, такое противопоказано. Однако, в процессе внедрения, постоянно возникал когнитивный диссонанс позиционирования. Для больших проектов, таки ORM препочтительней. Сложно, но мощно. Но sqlite, обычно используется как внутреннее хранилище приложений. Нередко многофайловое. С сравнительно ограниченным списком запросов, к каждой базе. И здесь, многословность использования, вытекающая из широких возможностей вашей библиотеки, оказалась также избыточна. Не настолько как ORM, но где-то посередине между ним и ожидаемой простотой.

Нужен простой ограниченный инструмент. Свел возврат всего к двум вариантам — Iterator и Optional. Адаптеры оставил на штатные возможности драйвера. Конфигугирование извлекается из аннотаций и запроса. На случай конфликта, scalar можно перекрыть принудительно. Когда в клиентском коде обернул передачу default_db в partial, осталось только указать запрос.

Внедрил во все проекты, где раньше использовалась noorm (благодаря похожести интерфейса, заняло не больше часа) — ни одного конфликта. Единственно, пока не занимался передачей списка (два запроса).

Резюме: тема отыграла свою роль, можно закрывать. noorm отличный инструмент, но для некторых задач просто неподходящий. Спасибо за идеи!