Skip to content
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.

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

Support JSON in codegen #388

Merged
merged 2 commits into from
Nov 1, 2022
Merged

Support JSON in codegen #388

merged 2 commits into from
Nov 1, 2022

Conversation

fantix
Copy link
Member

@fantix fantix commented Oct 31, 2022

  • Support handling JSON in query arguments and results
  • Support JSON in codegen (allows both modes with or without JSON handling)

Requires MagicStack/py-pgproto#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,
    )

@fantix fantix requested a review from 1st1 October 31, 2022 14:52
@fantix fantix mentioned this pull request Oct 31, 2022
@1st1
Copy link
Member

1st1 commented Oct 31, 2022

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

tailhook commented Nov 1, 2022

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 1st1 merged commit c6f5919 into master Nov 1, 2022
@1st1 1st1 deleted the codegen-json branch November 1, 2022 17:58
@1st1 1st1 restored the codegen-json branch November 1, 2022 18:01
@1st1
Copy link
Member

1st1 commented Nov 1, 2022

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

@1st1 1st1 mentioned this pull request Nov 1, 2022
@fantix fantix deleted the codegen-json branch August 17, 2023 21:48
@fantix fantix restored the codegen-json branch August 17, 2023 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants