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

Nested jsonb in insert mutation #206

Open
ryandvmartin opened this issue Jun 20, 2022 · 6 comments
Open

Nested jsonb in insert mutation #206

ryandvmartin opened this issue Jun 20, 2022 · 6 comments

Comments

@ryandvmartin
Copy link

ryandvmartin commented Jun 20, 2022

Hi. Having a specific issue inserting data where one of the fields is jsonb (or json). I keep getting the error:

{'errors': [{'extensions': {'path': '$.query', 'code': 'validation-failed'}, 'message': 'not a valid graphql query'}]}

where the generated sgqlc mutation looks like:

op = Operation(mutation_root)
op.insert_task_one(
    object=dict(
        created_by=10, 
        project_id=1, 
        method_kwargs={"some": "data"}, 
    )
).id()
op
>> 
mutation {
  insert_task_one(object: {created_by: 10, method_kwargs: {"some": "data"}, project_id: 1}) {
    id
  }
}

At first glance the error seems to come from the auto-generated jsonb from the schema-derived sgqlc types.

class jsonb(sgqlc.types.Scalar):
    __schema__ = schema

Building the mutation, when the jsonb field is encountered, it is dumped with json.dumps, which , of course, retains quoted dictionary keys as above. When I copy the failing query into iQL and fix the json keys to omit quotes the mutation works, e.g., changing to this is working:

mutation {
  insert_task_one(object: {created_by: 10, method_kwargs: {some: "data"}, project_id: 1}) {
    id
  }
}

This could be implemented by adding the following lines to the end of types.Scalar.__to_graphql_input__() - no idea how robust/performant this would be..

        if isinstance(value, dict):
            import re
            out = re.sub(r'"(.*?)"(?=:)', r'\1', out)
        return out

Dropping to the variable paradigm also works, though there is another quirk there on the renaming of the variable name in the mutation - if I wanted to use my schema's naming method_kwargs is automatically changed to methodKwargs in sqglc's variable setup, but the same processing is not applied to the dictionary given to the endpoint.

I guess the question is should I expect the direct usage of jsonb in my insert_task_one mutation to work? Could it work? Would this be reasonable to add?

Otherwise, thanks for the terrific project! Much appreciated

@ryandvmartin
Copy link
Author

I ended up writing a post-processing step to the generated code, where my jsonb type has the added classmethod below:

class jsonb(sgqlc.types.Scalar):
    __schema__ = amp_schema

    @classmethod
    def __to_graphql_input__(cls, value, *args, **kwargs):
        out = super().__to_graphql_input__(value, *args, **kwargs)
        if isinstance(value, dict):
            import re

            out = re.sub('(?<!: )"(\S*?)"', "\\1", out)
        return out

The previous regex had some issues removing quotes but this one seems to do the trick!

@barbieri
Copy link
Member

to fix it properly I'll need to investigate if there is something better to do.

Is method_kwargs the jsonb field? and is the server expecting it as an object rather than a string? Usually when I see people sending JSON over GraphQL, the JSON itself is encoded as a string, that is, your example would be:

  insert_task_one(object: {created_by: 10, method_kwargs: "{\"some\": \"data\"}", project_id: 1}) {

Which is what I'd expect "out of the box", seems we're missing the extra quotes/escaping in case it's a JSON array/object, but it is correct for strings/integer/float/booleans/null. Then, likely this should be fixed.

Of course you can/should always customize your serialization as you did with __to_graphql_input(), the only think I don't like is the regexp per se, to me it looks a possible source of problems. Maybe use https://github.com/graphql-python/graphql-core to print directly to the DSL?

@ryandvmartin
Copy link
Author

ryandvmartin commented Jun 22, 2022

Is method_kwargs the jsonb field?

Yes. Previously we were using the $variable placeholders in our query strings and sending all data/variables through the json of the request. In that context, the server recreates the object from the python side json.dumps without any troubles.

I tried manually changing the query to include the extra quotes and the only thing that actually passes is to write the query like it appears in the GQL dsl, with unquoted keys. There are definitely edgecases this cannot support... I think the only safe thing is to use Variable on json/b typed fields, and send the data through on the request.

@barbieri
Copy link
Member

@ryandvmartin yes, actually that's the recommended way. While you can manually write different operations with hard coded values in it, the recommendation is to use the same operation with variables. Some servers will cache based on the document hash, so when you send it again, it will get from the cache without the need to re-parse/validate the AST, saving tons of work. It's also the foundation to get Automatic Persisted Queries (which we don't support out of the box, but could be implemented manually by users).

Could you elaborate/paste an example of what you mean with:

I tried manually changing the query to include the extra quotes and the only thing that actually passes is to write the query like it appears in the GQL dsl, with unquoted keys.

Usually scalars are encode as GraphQL basic data types, these are Int, Float, Boolean or more commonly String. Then server-side you declare your scalar's parse to execute json.loads() and serialize to json.dumps(). Locally, client-side, you do the same, similar to what we do for datetime at https://github.com/profusion/sgqlc/blob/master/sgqlc/types/datetime.py#L221-L304

@palmoreck
Copy link

Hi @ryandvmartin thanks for creating this issue

Would it be possible that you share the way you did it using $variable placeholders option? My team and I are struggling dealing with this exactly issue 🙈

Cheers

@palmoreck
Copy link

BTW thanks for your solution using re works like a charm :) @ryandvmartin

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

No branches or pull requests

3 participants