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

Adds dynamic type to _source field #285

Merged
merged 12 commits into from
Apr 14, 2023
Merged

Conversation

zethuman
Copy link
Contributor

@zethuman zethuman commented Apr 6, 2023

Description

This change adds a dynamic type to the _source field, as this field can be a boolean or string array according to the documentation. Test cases have also been added that will test this changes.

Issues Resolved

Closes [#158] .

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

zethuman and others added 6 commits April 1, 2023 07:01
Signed-off-by: Rakhat Zhuman <zhumanrakhat01@gmail.com>
Signed-off-by: Rakhat Zhuman <zhumanrakhat01@gmail.com>
Signed-off-by: Rakhat Zhuman <zhumanrakhat01@gmail.com>
Signed-off-by: Rakhat Zhuman <zhumanrakhat01@gmail.com>
Signed-off-by: Rakhat Zhuman <zhumanrakhat01@gmail.com>
Copy link
Collaborator

@Jakob3xD Jakob3xD left a comment

Choose a reason for hiding this comment

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

I am not yet sure what to think about the tests. I assume you create the struct to avoid duplicate code but we will lose the option to customize tests if needed, as they are static in the way how the loop runs. Maybe @dblock has a opinion on that.

Edit: Just found that such a test stile is defined in the google style guid. https://google.github.io/styleguide/go/decisions#table-driven-tests

Moreover the require does not print the error which makes it difficulty to see why a test fails. Maybe I overlooked it but I can see it locally nor in the previous failed checks.

Beside that, I would like to see a cleanup job, so you can re-run the test locally without cleaning the cluster by hand.

opensearchapi/api.bulk.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
opensearchapi/api.delete_by_query.go Outdated Show resolved Hide resolved
opensearchapi/api.exists.go Outdated Show resolved Hide resolved
opensearchapi/api.exists_source.go Outdated Show resolved Hide resolved
opensearchapi/api.search.go Outdated Show resolved Hide resolved
opensearchapi/api.update.go Outdated Show resolved Hide resolved
opensearchapi/api.update_by_query.go Outdated Show resolved Hide resolved
opensearchapi/api.document_test.go Outdated Show resolved Hide resolved
opensearchapi/api.document_test.go Outdated Show resolved Hide resolved
@zethuman
Copy link
Contributor Author

zethuman commented Apr 7, 2023

Moreover the require does not print the error which makes it difficulty to see why a test fails.

I think this amount of information is enough for debugging?

buffer := new(bytes.Buffer)
err = json.Compact(buffer, []byte("{sadsad"))
require.NoError(t, err)

image

@zethuman
Copy link
Contributor Author

zethuman commented Apr 7, 2023

we will lose the option to customize tests if needed, as they are static in the way how the loop runs.

All operations associated with api documents are approximately the same, both in the implementation example and in the fields, that is, we don’t have to come up with a lot of customization, even if we have to, it will be possible to test a separate function in the same format with this customization, or add a new one to the structure parameter if you need to test the entire api pool.

Also, I want to say that these are not finished test cases, but basic ones, in case of testing new functionality, you will not need to come up with guesses, but write cases in a table and check it, which is very convenient, since the verification logic is written. In my case, I wanted to check the work of the _source field, but since I did not find ready-made test functions, I had to write tests for the entire document. I hope this helps to further test different cases in other issues

@Jakob3xD
Copy link
Collaborator

Jakob3xD commented Apr 7, 2023

I think this amount of information is enough for debugging?

Yes, I overlooked the error when testing locally.

Your tests are okay and valid. I just haven't seen this test stile before.
So no further discussion needed about the test from my side.

Signed-off-by: Rakhat Zhuman <zhumanrakhat01@gmail.com>
@zethuman
Copy link
Contributor Author

zethuman commented Apr 8, 2023

@Jakob3xD @dblock Can someone merge this PR if there is no questions?

@zethuman zethuman requested a review from Jakob3xD April 9, 2023 02:49
dblock
dblock previously approved these changes Apr 10, 2023
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Looks good, resolve conflicts in CHANGELOG and let's merge!

opensearchapi/api.document_test.go Outdated Show resolved Hide resolved
Signed-off-by: Rakhat Zhuman <zhumanrakhat01@gmail.com>
CHANGELOG.md Outdated Show resolved Hide resolved
opensearchapi/api.document_test.go Outdated Show resolved Hide resolved
Signed-off-by: Rakhat Zhuman <zhumanrakhat01@gmail.com>
Signed-off-by: Rakhat Zhuman <zhumanrakhat01@gmail.com>
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

@VachaShah VachaShah merged commit 27a2f09 into opensearch-project:main Apr 14, 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.

4 participants