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 JSON string comparison. #475

Merged
merged 2 commits into from Jun 30, 2021
Merged

Conversation

ajnavarro
Copy link
Contributor

Fix when a JSON is compared with a string, and apply
unquote as MySQL does.

Fixes #474

Signed-off-by: Antonio Navarro Perez antnavper@gmail.com

Fix when a JSON is compared with a string, and apply
unquote as MySQL does.

Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
@@ -272,7 +273,15 @@ func (t stringType) Convert(v interface{}) (interface{}, error) {
return "", nil
}

return s.ToString(nil)
str, err := s.ToString(nil)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this case be handling all JSONValues, not just JSONDocuments?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just leave as it was, but makes sense. If we use JSONValue then we need to remove the following piece of code:

if s.Val == nil {
	return "", nil
}

tests still pass without it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't handle arbitrary JSONValues somehow in this function then JSONValues that aren't JSONDocuments won't be supported, right? Are users supposed to be able to use their own implementations of JSONValue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed! Also, it fixes another bug related to JSON null values. They were translated to "" instead of "null"

It also uncovered other bug related with null JSONValues. They were
translated to "" instead of "null".

Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution, Antonio!

@zachmu zachmu merged commit 967d603 into dolthub:master Jun 30, 2021
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.

JSON string comparison seems broken (change in behavior from v0.6.0 and v0.10.0)
3 participants