pydantic error none.not_allowed on select().output(nested=True)

[metakot]

Hello! I've using piccolo with fastapi and pydantic. I need to select() db records as dicts and throw them to pydantic, but it fails on ForeignKey because piccolo returns nested dicts with nested=True param as dicts with filled keys and None values

{'id': 1, 'one': {'desc': None, 'id': None, 'name': None, 'number': None}}

instead of just

{'id': 1, 'one': None}

import asyncio
from typing import Optional

import piccolo.columns as cl
from piccolo.engine.sqlite import SQLiteEngine
from piccolo.table import Table
from pydantic import BaseModel


DB = SQLiteEngine()


class TableOne(Table, db=DB):
    name = cl.Varchar()
    desc = cl.Varchar()
    number = cl.Integer()


class TableTwo(Table, db=DB):
    one = cl.ForeignKey(TableOne)


class TableOneModel(BaseModel):
    id: int
    name: str
    desc: str
    number: int


class TableTwoModel(BaseModel):
    id: int
    one: Optional[TableOneModel] = None


async def main():
    await TableOne.alter().drop_table(if_exists=True)
    await TableOne.create_table()
    await TableTwo.alter().drop_table(if_exists=True)
    await TableTwo.create_table()

    await TableTwo.objects().create()

    result = await TableTwo.select(
        TableTwo.all_columns(), TableTwo.one.all_columns()
    ).output(nested=True).first()
    print(result)
    # fails here
    TableTwoModel(**result)


if __name__ == '__main__':
    asyncio.run(main())

[dantownsend]

Ah, that seems like a bug. Thanks for reporting it. I'll take a look.

[sinisaos]

@dantownsend It seems that if we change this https://github.com/piccolo-orm/piccolo/blob/14012e59759c849ad732b4c4e4fb40636dc88aa6/piccolo/utils/dictionary.py#L30 to

if len(path) == 1 or value is None:

everything works. Unittest also pass and we just need to add a test for None values.

If we change code from example issue to this

await TableTwo.objects().create()
await TableTwo.objects().create()
table_3 = await TableOne.objects().create(name="a", desc="b", number=1)
await TableTwo.objects().create(one=table_3)

result = await TableTwo.select(
    TableTwo.all_columns(), TableTwo.one.all_columns()
).output(nested=True)
print(result)

result is [{'id': 1, 'one': None}, {'id': 2, 'one': None}, {'id': 3, 'one': {'desc': 'b', 'id': 1, 'name': 'a', 'number': 1}}] which is correct.

[dantownsend]

@sinisaos Yeah, that might work. We need to write some tests to make sure it works even if the data structure is super nested.

For example, in the playground:

data = await Concert.select(
    Concert.all_columns(),
    Concert.band_1.all_columns(),
    Concert.band_1.manager.all_columns(),
)

[sinisaos]

data = await Concert.select(
    Concert.all_columns(),
    Concert.band_1.all_columns(),
    Concert.band_1.manager.all_columns(), <-- this work if values are None but not work if not None
)

Unfortunately, the last line only works if the value is None (without ValidationError). It's strange, but without and with the latest changes deep nested doesn't work well even though it seems to work well before. Maybe I'm doing something wrong and it would be good for you to check it out as well.

The playground result from the query above is:

{
    "band_1": {
        "id": 1,
        "manager": 1,
        "name": "Pythonistas",
        "popularity": 1000,
    },
    "band_2": {"id": 2, "manager": 2, "name": "Rustaceans", "popularity": 500}, <-- this is problem
    "duration": datetime.timedelta(seconds=7200),
    "id": 1,
    "starts": datetime.datetime(2022, 12, 23, 19, 33, 4, 55214),
    "venue": 1,
}

but i think it should be and then we wouldn't have a ValidationError

{
    "band_1": {
        "id": 1,
        "manager": 1,
        "name": "Pythonistas",
        "popularity": 1000,
    },
    "band_2": {
        "id": 2,
        "manager": {"id": 2, "name": "Graydon"},
        "name": "Rustaceans",
        "popularity": 500,
    },
    "duration": datetime.timedelta(seconds=7200),
    "id": 1,
    "starts": datetime.datetime(2022, 12, 23, 19, 33, 4, 55214),
    "venue": 1,
}

Sorry if I'm wrong

[sinisaos]

@dantownsend I can confirm that if we add

if len(path) == 1 or value is None:

everything works and that is the solution to this issue. But there is a bug with deeply nested all_columns() which leads to a Pydantic ValidationError. If we use query like this (without all_columns())

await Concert.select(
    Concert.all_columns(),
    Concert.band_1.id,
    Concert.band_1.name,
    Concert.band_1.manager.id,
    Concert.band_1.manager.name,
    Concert.band_2.id,
    Concert.band_2.name,
    Concert.band_2.manager.id,
    Concert.band_2.manager.name,
)
.first()
.output(nested=True)

result is

{
    "band_1": {
        "id": 1,
        "manager": {"id": 1, "name": "manager"},
        "name": "band",
    },
    "band_2": {
        "id": 2,
        "manager": {"id": 2, "name": "manager2"},
        "name": "band2",
    },
    "id": 1,
}

which is correct and there is no ValidationError.

But if we use query like this:

await Concert.select(
    Concert.all_columns(),
    Concert.band_1.all_columns(),
    Concert.band_2.manager.all_columns(),
)
.first()
.output(nested=True)

result is

{
    "band_1": {"id": 1, "manager": 1, "name": "band"},
    "band_2": {"manager": {"id": 2, "name": "manager2"}},
    "id": 1,
}

which is wrong and raise ValidationError like this

pydantic.error_wrappers.ValidationError: 3 validation errors for ConcertModel
band_1 -> manager
  value is not a valid dict (type=type_error.dict)
band_2 -> id
  field required (type=value_error.missing)
band_2 -> name
  field required (type=value_error.missing)

[metakot]

Also, if we select records with TableTwo.objects(TableTwo.one).first() it populates the "one" field with TableOne instance filled with None values.

[sinisaos]

@metakot Changing this line https://github.com/piccolo-orm/piccolo/blob/14012e59759c849ad732b4c4e4fb40636dc88aa6/piccolo/utils/dictionary.py#L30 to

if len(path) == 1 or value is None:

should also fix that.

[metakot]

Any news on this?

[dantownsend]

We need to tread carefully with this one - I haven't merged in a fix yet, because need to be sure there are no unintended side effects. We need to write a bunch of tests.

[metakot]

Recently stumbled upon this again. Such a pain :)

[dantownsend]

Yeah, I did come across this the other day in a project. I was able to work around it, but not ideal.

[metakot]

So I made a patch for myself.

Features: doesn't create a list; doesn't sort a list (seems to be unnecessary anyway); doesn't rewrite correct values in output; leaves original input dictionary intact; drops null foreignkey values (the point of a patch in the first place).

I have tested it a bit, seems to be working fine. @dantownsend Can you take a look? :)

from __future__ import annotations

import typing as t


def make_nested(dictionary: t.Dict[str, t.Any]) -> t.Dict[str, t.Any]:
    output: t.Dict[str, t.Any] = {}
    for key, value in dictionary.items():
        path = key.split('.')
        if len(path) > 1:
            root = output
            for elem in path[:-1]:
                root2 = root.get(elem, ...)
                if root2 is None:
                    break
                elif not isinstance(root2, dict):
                    root[elem] = root2 = {}
                root = root2
            else:
                root[path[-1]] = value
        else:
            output.setdefault(key, value)
    return output