simonw / sqlite-utils

Python CLI utility and library for manipulating SQLite databases
https://sqlite-utils.datasette.io
Apache License 2.0
1.63k stars 111 forks source link

Support JSON values returned from .convert() functions #495

Closed mhalle closed 1 year ago

mhalle commented 1 year ago

When using the convert function on a JSON column, the result of the conversion function must be a string. If the return value is either a dict (object) or a list (array), the convert call will error out with an unhelpful user defined function exception.

It makes sense that since the original column value was a string and required conversion to data structures, the result should be converted back into a JSON string as well. However, other functions auto-convert to JSON string representation, so the fact that convert doesn't could be surprising.

At least the documentation should note this requirement, because the sqlite error messages won't readily reveal the issue.

Jf only sqlite's JSON column type meant something :)

simonw commented 1 year ago

This makes sense to me. There are other places in the codebase where JSON is automatically stringified: https://github.com/simonw/sqlite-utils/blob/c7e4308e6f49d929704163531632e558f9646e4a/sqlite_utils/db.py#L2759-L2766

I don't see why the return value from a convert function shouldn't do the same thing.

Since this will result in previous errors working, I don't think it warrants a major version bump either.

simonw commented 1 year ago

There is a case where the function can return a dictionary at the moment: multi=True

table.convert(
    "title", lambda v: {"upper": v.upper(), "lower": v.lower()}, multi=True
)

But I think this change is still compatible with that. if you don't use multi=True then the return value will be stringified. If you DO use multi=True then something like this could work:

table.convert(
    "title", lambda v: {"upper": {"str": v.upper()}, "lower": {"str": v.lower()}}, multi=True
)

This would result in a upper and lower column, each containing the JSON string {"str": "UPPERCASE"}.

simonw commented 1 year ago

I've decided not to explicitly document this, since it's consistent with how other parts of the library work already.