Skip to content

fix: return null values properly in proxy mode #721

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

Merged
merged 3 commits into from
Aug 23, 2022

Conversation

btoonk
Copy link
Contributor

@btoonk btoonk commented Jul 22, 2022

When using go-mysql in proxy mode, the null values are not handled correctly.
This PR is an attempt to solve this.

The same logic is already in use in the BuildSimpleTextResultset function.

@skoef
Copy link
Contributor

skoef commented Jul 22, 2022

Thanks for your contribution! Could you extend any existing test case to prove your fix solves the issue?

@lance6716
Copy link
Collaborator

Hi @btoonk , I found a bit different place to fix this issue.

LengthEncodedInt and PutLengthEncodedInt in util.go seems are inverse functions of each other, can you check if we add the n=0 case in LengthEncodedInt the issue will also be fixed?

If it's OK, can you add an unit test to verify that the two functions can handle the output of each other and return the original input?

@btoonk
Copy link
Contributor Author

btoonk commented Aug 17, 2022

@skoef I added a test that proves this fix solved the issue.

@btoonk
Copy link
Contributor Author

btoonk commented Aug 17, 2022

@lance6716 I don't think we can do it in PutLengthEncodedInt (or in PutLengthEncodedString), I think not all data that is nil (or n=0) is equal to NULL (0xfb), I think that is only the case for row values. That is why we can do it in the writeFieldValues function.

@lance6716
Copy link
Collaborator

lance6716 commented Aug 17, 2022

Hi, I'll try to fix the CI problem and then come back to your PR 😄

@lance6716 lance6716 merged commit 8649474 into go-mysql-org:master Aug 23, 2022
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