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

fix: if JsonObject serialized to None then return null_value instead of string_value #771

Merged
merged 8 commits into from
Aug 25, 2022

Conversation

o-aleks
Copy link
Contributor

@o-aleks o-aleks commented Jul 15, 2022

No description provided.

@o-aleks o-aleks requested review from a team as code owners July 15, 2022 14:58
@google-cla
Copy link

google-cla bot commented Jul 15, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@product-auto-label product-auto-label bot added size: xs Pull request size is extra small. api: spanner Issues related to the googleapis/python-spanner API. labels Jul 15, 2022
@asthamohta asthamohta added the kokoro:run Add this label to force Kokoro to re-run the tests. label Jul 19, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Jul 19, 2022
@parthea parthea added kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Aug 12, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Aug 12, 2022
@yoshi-kokoro yoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Aug 12, 2022
@asthamohta
Copy link
Contributor

hi @o-aleks, what is this PR for?

@o-aleks
Copy link
Contributor Author

o-aleks commented Aug 22, 2022

Hi @asthamohta,

There is a bug in _make_value_pb function.
If the value is an instance of JsonObject and it is None after serializing, then it should be translated to NULL_VALUE, but in the original code, it is a string.

My fix is:

def _make_value_pb(value):
    ...
    if isinstance(value, JsonObject):
        value = value.serialize()
        if value is None:
            return Value(null_value="NULL_VALUE")
        else:
            return Value(string_value=value)

Could you please check it and approve the PR?
Thank you.

@asthamohta asthamohta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 24, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 24, 2022
@IlyaFaer
Copy link
Contributor

Should be okay. There is an if statement upper in the method, which does the same for all the data types, but JsonObject is more complicated than other types, so it passes the condition (and we can't change this behavior for the class, because there is no magic method for is None statements).

A test should be added though. @o-aleks, will you find time to add a test? Or I can do it in a separate issue.

@o-aleks
Copy link
Contributor Author

o-aleks commented Aug 24, 2022

A test should be added though. @o-aleks, will you find time to add a test? Or I can do it in a separate issue.
Oh, yes, of course, I'll add the test.

@product-auto-label product-auto-label bot added size: s Pull request size is small. and removed size: xs Pull request size is extra small. labels Aug 24, 2022
@asthamohta asthamohta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 25, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 25, 2022
@IlyaFaer IlyaFaer merged commit 82170b5 into googleapis:main Aug 25, 2022
@IlyaFaer
Copy link
Contributor

Thanks, @o-aleks, looks good, and tests are passing fine. I'm merging it.

gcf-merge-on-green bot pushed a commit that referenced this pull request Aug 30, 2022
🤖 I have created a release *beep* *boop*
---


## [3.20.0](v3.19.0...v3.20.0) (2022-08-30)


### Features

* Adds TypeAnnotationCode PG_JSONB ([#792](#792)) ([6a661d4](6a661d4))


### Bug Fixes

* if JsonObject serialized to None then return `null_value` instead of `string_value` ([#771](#771)) ([82170b5](82170b5))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/python-spanner API. size: s Pull request size is small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants