edgedb / edgedb-python

The official Python client library for EdgeDB
https://edgedb.com
Apache License 2.0
369 stars 45 forks source link

Support JSON in codegen #388

Closed fantix closed 2 years ago

fantix commented 2 years ago

Requires https://github.com/MagicStack/py-pgproto/pull/18

Sample code to enable JSON handling:

>>> import edgedb
>>> c = edgedb.create_client()
>>> c.query("select to_json('[1,2,3]')")
['[1, 2, 3]']
>>> ctx = edgedb.get_default_codec_context(handle_json=True)
>>> c.with_codec_context(ctx).query("select to_json('[1,2,3]')")
[[1, 2, 3]]

Sample generated code with JSON handling turned ON:

# edgedb-py --handle-json --target blocking
def test_json(
    client: edgedb.Client,
    arg0: typing.Any,
) -> typing.Any:
    client = client.with_codec_context(
        edgedb.get_default_codec_context(handle_json=True),
        only_replace_default=True,
    )
    return client.query_single(
        """\
        select <json>$0;\
        """,
        arg0,
    )

Sample generated code with JSON handling turned OFF (default behavior):

# edgedb-py --target blocking
def test_json(
    client: edgedb.Client,
    arg0: str,
) -> str:
    return client.query_single(
        """\
        select <json>$0;\
        """,
        arg0,
    )
1st1 commented 2 years ago

I don't like the codec context object -- do we actually need it (or to expose it)? Why can't we copy the config API here, something like

client=client.with_client_options(
    auto_unpack_json=True
)

IOW not even tie this to codecs, which we should allow overriding in some remote future. Thoughts?

cc @elprans @tailhook

tailhook commented 2 years ago

A couple of thoughts:

  1. I assume that what @fantix meant by "codec context" is (eventually) reviving set_type_codec API, where you can change the decoded type of any EdgeDB type.
  2. This basically means that codegen should always set their "codec context" because otherwise, types will be wrong, i.e. always cli.with_codec_context(edgedb.codecs.DEFAULT).query(...)
  3. So codegen could have --codecs edgedb.codecs.DEFAULT_WITH_JSON_UNPACK param and --unpack-json as a shortcut.
  4. This also opens a question of how to properly find out static types for user codecs, but should be doable.

To state it again: changing connection params must not influence types for generated code. I.e these must be equivalent:

from generated import test_json
await test_json(cli)
await test_json(cli.with_codec_context(whatever_codec_i_like))

This also means that only_replace_default has not much sense.


Something like this, could be a shortcut to changing codecs:

client = client.with_client_options(
    auto_unpack_json=True
)

But the problem is that we also need resetting those options. And we have a confusing dichotomy where with_config is incremental but with_transaction_options is not.


All in all, it's more complex that we would like it to be. If we need set_type_codec API, it's probably better to go with with_codec_context (or similar).

If we don't need set_type_codec API it's probably better to always unpack JSONs and release 2.0. (and advise <str><json>x for anyone who wants a string).

1st1 commented 2 years ago

@fantix Sigh, I did not intend to merge this PR and I've already reverted the merge. I'll open a new issue.