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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add support for json data type #593

Merged
merged 9 commits into from Oct 4, 2021
Merged

fix: add support for json data type #593

merged 9 commits into from Oct 4, 2021

Conversation

vi3k6i5
Copy link
Contributor

@vi3k6i5 vi3k6i5 commented Sep 27, 2021

Add support for json datatype, but adding mapping from dictionary to json for db insert.

Fixes #592 馃

@vi3k6i5 vi3k6i5 requested a review from larkee Sep 27, 2021
@vi3k6i5 vi3k6i5 requested a review from as a code owner Sep 27, 2021
@product-auto-label product-auto-label bot added the api: spanner label Sep 27, 2021
@google-cla google-cla bot added the cla: yes label Sep 27, 2021
@vi3k6i5 vi3k6i5 requested a review from asthamohta Sep 27, 2021
Copy link
Contributor

@asthamohta asthamohta left a comment

LGTM

Copy link
Contributor

@larkee larkee left a comment

Could you link to evidence that DB-API expects JSON to be passed in as a dict?

This code LGTM but I'm cautious about using dict unless there is precedent

@vi3k6i5
Copy link
Contributor Author

@vi3k6i5 vi3k6i5 commented Sep 28, 2021

Could you link to evidence that DB-API expects JSON to be passed in as a dict?

This code LGTM but I'm cautious about using dict unless there is precedent

If we pass a string from django, then type casting will be to "STRING", which causes this error. DB-api needs to know that json has to be passed, but that conversion happens based on instance type.

except InvalidArgument as e:
>           raise ProgrammingError(e.details if hasattr(e, "details") else e)
E           django.db.utils.ProgrammingError: 400 Value of type STRING cannot be assigned to value, which has type JSON [at 1:33]\nUPDATE tests_detail SET value = @a0 WHERE tests_detail.id = @a1\n

@larkee
Copy link
Contributor

@larkee larkee commented Sep 28, 2021

Could you link to evidence that DB-API expects JSON to be passed in as a dict?
This code LGTM but I'm cautious about using dict unless there is precedent

If we pass a string from django, then type casting will be to "STRING", which causes this error. DB-api needs to know that json has to be passed, but that conversion happens based on instance type.

Hmm.. I see your point. Perhaps we should follow the route of pyscopg and add a custom type class Json. The benefit of doing so is we could allow JSON values to be specified as either an object or a string:

# Use object for JSON.
cur.execute(
        sql="INSERT INTO JsonDetails (DataId, Details) VALUES (%s, %s)",
        args=(123, Json({"name": "Jakob", "age": "26"})),
    )

# Use string for JSON.
cur.execute(
        sql="INSERT INTO JsonDetails (DataId, Details) VALUES (%s, %s)",
        args=(123, Json('{"age":"26","name":"Jakob"}')),
    )

WDYT?

@vi3k6i5
Copy link
Contributor Author

@vi3k6i5 vi3k6i5 commented Sep 28, 2021

Could you link to evidence that DB-API expects JSON to be passed in as a dict?
This code LGTM but I'm cautious about using dict unless there is precedent

If we pass a string from django, then type casting will be to "STRING", which causes this error. DB-api needs to know that json has to be passed, but that conversion happens based on instance type.

Hmm.. I see your point. Perhaps we should follow the route of pyscopg and add a custom type class Json. The benefit of doing so is we could allow JSON values to be specified as either an object or a string:

# Use object for JSON.
cur.execute(
        sql="INSERT INTO JsonDetails (DataId, Details) VALUES (%s, %s)",
        args=(123, Json({"name": "Jakob", "age": "26"})),
    )

# Use string for JSON.
cur.execute(
        sql="INSERT INTO JsonDetails (DataId, Details) VALUES (%s, %s)",
        args=(123, Json('{"age":"26","name":"Jakob"}')),
    )

WDYT?

This makes sense, but then users would be tied to using our data type in their code.

The mapping would still have to be added
json_type: spanner.param_types.JSON,

and the conversion from json to json_dump

    if isinstance(value, json_type):
        return Value(
            string_value=json.dumps(value, sort_keys=True, separators=(",", ":"),)
        )

@vi3k6i5 vi3k6i5 requested a review from larkee Sep 28, 2021
@IlyaFaer
Copy link
Member

@IlyaFaer IlyaFaer commented Sep 28, 2021

Apologies for bursting in without an invitation, but it should be mentioned we support such a params style:

(
"INSERT INTO t (f1, f2, f2) VALUES (@a0, @a1, @a2)",
{"a0": "app", "a1": "name", "a2": "applied"},
),

Also there is a request to support pyformat formatting style, which'll also use dict:
https://www.python.org/dev/peps/pep-0249/#paramstyle

Considering it, using dict as a JSON data type can be little bit confusing. I second @larkee proposal to add a separate JSON data type class.

@vi3k6i5
Copy link
Contributor Author

@vi3k6i5 vi3k6i5 commented Sep 28, 2021

Apologies for bursting in without an invitation, but it should be mentioned we support such a params style:

(
"INSERT INTO t (f1, f2, f2) VALUES (@a0, @a1, @a2)",
{"a0": "app", "a1": "name", "a2": "applied"},
),

Also there is a request to support pyformat formatting style, which'll also use dict:
https://www.python.org/dev/peps/pep-0249/#paramstyle

Considering it, using dict as a JSON data type can be little bit confusing. I second @larkee proposal to add a separate JSON data type class.

Makes sense, will add a separate JSON type for the same.

Update: I don't think the same rules apply because pyformat will apply to the whole param passed, where as the dict to json conversion only applies to individual parameters. Still going ahead making the change as suggested.

@vi3k6i5 vi3k6i5 requested a review from IlyaFaer Sep 28, 2021
@vi3k6i5
Copy link
Contributor Author

@vi3k6i5 vi3k6i5 commented Sep 28, 2021

@larkee @IlyaFaer Not able to find the right place to define the json type, any suggestions ?

@IlyaFaer
Copy link
Member

@IlyaFaer IlyaFaer commented Sep 29, 2021

I guess it should live somewhere in https://github.com/googleapis/python-spanner/tree/main/google/cloud/spanner_v1/types, as a separate file (because in other files there are protos).

Also there is a types.py file for DB API, but writing a class into this file looks like it's going to be used only by the DB API.

@vi3k6i5
Copy link
Contributor Author

@vi3k6i5 vi3k6i5 commented Sep 29, 2021

@IlyaFaer : Thank you for the suggestion, added a new object called JsonObject and moved it to types.

google/cloud/spanner_v1/types/data_types.py Outdated Show resolved Hide resolved
tests/system/test_dbapi.py Outdated Show resolved Hide resolved
@vi3k6i5 vi3k6i5 requested a review from IlyaFaer Sep 30, 2021
@larkee
Copy link
Contributor

@larkee larkee commented Oct 1, 2021

I guess it should live somewhere in https://github.com/googleapis/python-spanner/tree/main/google/cloud/spanner_v1/types, as a separate file (because in other files there are protos).

Ahh, unfortunately, that is a generated directory which means we shouldn't put handwritten code there as it will be overwritten/removed whenever the code is regenerated.

I think the best option is to move data_types.py to google/cloud/spanner_v1 and add JsonObject to the google/cloud/spanner_v1/__init__ file so it can be easily imported e.g.

from google.cloud.spanner_v1 import JsonObject

@vi3k6i5
Copy link
Contributor Author

@vi3k6i5 vi3k6i5 commented Oct 1, 2021

@larkee : Done, Thanks.

larkee
larkee approved these changes Oct 1, 2021
@vi3k6i5 vi3k6i5 merged commit bc5ddc3 into googleapis:main Oct 4, 2021
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants