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 bson.D conversion in GRPC #2083

Merged
merged 5 commits into from
May 23, 2022

Conversation

VinozzZ
Copy link
Contributor

@VinozzZ VinozzZ commented May 19, 2022

What does this change

MongoDB expects to receive bson.D for $sort and $group in a aggregation pipeline.
Prior to this PR, after the grpc server receives data from a client, it does not convert the data from []interface{} back to bson.D, which caused the query to fail.
This PR fixes this issue and adds tests to check for proper conversion.

What issue does it fix

Closes #2063

Notes for the reviewer

Put any questions or notes for the reviewer here.

Checklist

  • Did you write tests?
  • Did you write documentation?
  • Did you change porter.yaml or a storage document record? Update the corresponding schema file.
  • If this is your first pull request, please add your name to the bottom of our Contributors list. Thank you for making Porter better! 🙇‍♀️

Reviewer Checklist

  • Comment with /azp run test-porter-release if a magefile or build script was modified
  • Comment with /azp run porter-integration if it's a non-trivial PR

Copy link
Member

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

The CI build is failing but this looks like it's going in the right direction

pkg/storage/grpc_test.go Outdated Show resolved Hide resolved
pkg/storage/pluginstore/grpc.go Outdated Show resolved Hide resolved
pkg/storage/pluginstore/grpc.go Outdated Show resolved Hide resolved
@VinozzZ VinozzZ force-pushed the fix-bson-d-converter branch from 8cf0cd2 to d917d4e Compare May 19, 2022 20:12
VinozzZ added 2 commits May 19, 2022 16:37
Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
@VinozzZ VinozzZ force-pushed the fix-bson-d-converter branch 3 times, most recently from 9e97d64 to b30ddf6 Compare May 19, 2022 21:57
Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
@VinozzZ VinozzZ force-pushed the fix-bson-d-converter branch from b30ddf6 to 9d62e7c Compare May 19, 2022 21:59
@VinozzZ VinozzZ requested a review from carolynvs May 20, 2022 16:53
go.mod Outdated Show resolved Hide resolved
pkg/storage/grpc_test.go Show resolved Hide resolved
pkg/storage/grpc_test.go Show resolved Hide resolved
pkg/storage/pluginstore/grpc.go Outdated Show resolved Hide resolved
pkg/storage/pluginstore/grpc.go Outdated Show resolved Hide resolved
Comment on lines 424 to 426
if c == nil {
c = []converter{ConvertFloatToInt}
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that it's possible for c to be nil here? If you call the function without passing any optional arguments the value is an empty array, not nil.

I noticed that in AsMap you always use the ConvertFloatToInt converter, even when they specify the optional custom converter parameter. Since you are passing c through to AsMap, I believe that this results in ConvertFloatToInt being used twice in AsMap when c is nil (which is never as I said above).

I'm pretty sure that you can delete these lines here and it will work exactly the same.

Suggested change
if c == nil {
c = []converter{ConvertFloatToInt}
}

VinozzZ added 2 commits May 20, 2022 14:31
Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
@VinozzZ VinozzZ requested a review from carolynvs May 20, 2022 19:27
Copy link
Member

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

Just one thing to clear up and then this is ready to merge! 🚀

pkg/storage/grpc_test.go Show resolved Hide resolved
@VinozzZ VinozzZ requested a review from carolynvs May 23, 2022 14:33
Copy link
Member

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

🤞

@carolynvs carolynvs merged commit da65f4e into getporter:release/v1 May 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.

2 participants