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

feat(driver): Replace value.Value with Value() return #139

Merged
merged 3 commits into from
Jul 5, 2023

Conversation

simamumu
Copy link
Contributor

resolve: #138

If you are ok with the content of #138, please use this PR.

@rahul2393
Copy link
Collaborator

@simamumu Can you please fix the failing integration tests?

@simamumu
Copy link
Contributor Author

Sorry, I missed the failing test...
I will fix the tests (but not until after 2/18 as I can't write the code now)

@simamumu
Copy link
Contributor Author

@rahul2393
The test was failing because typed nil params such as spanner.NullInt64 were converted to nil by the Value method, so the paramTypes in ExecuteSqlRequest did not match.

The following modifications were made to the code as intended by the test

  1. Do early returns for defined types such as spanner.NullInt64 and do not check/execute Value().
  2. Types that implement the Valuer interface execute Value() and replace it.
  3. If the Valuer interface is not implemented, or the return value of Value() is not the expected type, return an unsupported value error.

I have confirmed that the test passes locally.

please re-review code 🙏

@kdvy
Copy link

kdvy commented Jul 5, 2023

@simamumu - We would find this support useful as well. Would you be able to merge ?

@rahul2393 rahul2393 merged commit 6f2b96e into googleapis:main Jul 5, 2023
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.

Convert a value using the Value() method of my defined type
3 participants