Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
mhalle opened this issue Sep 30, 2022 · 3 comments
Closed

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

mhalle opened this issue Sep 30, 2022 · 3 comments
Labels
enhancement New feature or request python-library

Comments

@mhalle
Copy link

mhalle commented Sep 30, 2022 •
edited
Loading

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 :)
@mhalle mhalle changed the title convert of JSON fields requires an explicit conversion back to string convert of JSON columns requires an explicit conversion back to string Sep 30, 2022
@mhalle mhalle changed the title convert of JSON columns requires an explicit conversion back to string convert of JSON columns requires explicit conversion back to string Sep 30, 2022
@simonw
Copy link
Owner

simonw commented Oct 25, 2022

This makes sense to me. There are other places in the codebase where JSON is automatically stringified:

sqlite-utils/sqlite_utils/db.py

Lines 2759 to 2766 in c7e4308

value = jsonify_if_needed(
record.get(
key,
None
if key != hash_id
else hash_record(record, hash_id_columns),
)
)

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 simonw added enhancement New feature or request python-library labels Oct 25, 2022
@simonw
Copy link
Owner

simonw commented Oct 25, 2022

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
Copy link
Owner

simonw commented Oct 25, 2022

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

@simonw simonw closed this as completed in defa297 Oct 25, 2022
@simonw simonw changed the title convert of JSON columns requires explicit conversion back to string Support JSON values returned from .convert() functions Oct 25, 2022
@simonw simonw mentioned this issue Oct 25, 2022
Closed
simonw added a commit that referenced this issue Oct 25, 2022
@simonw
Release 3.30
0d45ee1 
Refs #480, #483, #485, #495, #500, #502, #504
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request python-library
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@simonw @mhalle