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

add toBuilder method on some models #766

Merged

Conversation

channel-dante
Copy link
Contributor

Description

  • added toBuilder method to all request model in core package & _types.query_dsl package
  • for consistent model management, add to all related models rather than just a specific model.

Issues Resolved

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.

@dblock
Copy link
Member

dblock commented Dec 12, 2023

This looks good. Any way we can get some test coverage here on these other classes? I understand it's laborious (hence we should be investing in a code generator).

@channel-dante
Copy link
Contributor Author

channel-dante commented Dec 13, 2023

This looks good. Any way we can get some test coverage here on these other classes? I understand it's laborious (hence we should be investing in a code generator).

@dblock I'm curious what you want to test. Can I just test whether toBuilder works (like this)? Or do you want to verify that all fields operate normally? If you want a second one, it won't be an easy task.

@dblock
Copy link
Member

dblock commented Dec 13, 2023

This looks good. Any way we can get some test coverage here on these other classes? I understand it's laborious (hence we should be investing in a code generator).

@dblock I'm curious what you want to test. Can I just test whether toBuilder works (like this)? Or do you want to verify that all fields operate normally? If you want a second one, it won't be an easy task.

I'm good with anything that just exercises the code to start.

@channel-dante channel-dante force-pushed the to-query-for-search-request branch 5 times, most recently from d897a81 to eea2804 Compare December 18, 2023 03:29
@channel-dante
Copy link
Contributor Author

@dblock Can you please review again? It was a very laborious task 😅

dblock
dblock previously approved these changes Dec 18, 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 to me! Nit on CHANGELOG.

Thank you for doing this. I recommend investing more time in code generation so we don't have to do this by hand :)

CHANGELOG.md Outdated
@@ -39,12 +39,15 @@ This section is for maintaining a changelog for all breaking changes for the cli
- Added support for icu_collation_keyword type ([#725](https://github.com/opensearch-project/opensearch-java/pull/725))
- Added support for flat_object field property ([#735](https://github.com/opensearch-project/opensearch-java/pull/735))
- Added toQuery method in Query and QueryVariant ([#760](https://github.com/opensearch-project/opensearch-java/pull/760)
- Added toBuilder method to all request model in core package & _types.query_dsl package
Copy link
Member

Choose a reason for hiding this comment

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

Add a link to the PR like other lines.

@dblock
Copy link
Member

dblock commented Dec 18, 2023

@VachaShah @reta Any ideas of how to do this kind of change better?

@VachaShah
Copy link
Collaborator

@VachaShah @reta Any ideas of how to do this kind of change better?

I think for changes like these which span almost all files, code generation is the way to go. If we have some code which can incrementally add such methods (even if it is not a complete code generator for the client), it would be beneficial to add features like toString and withJson.

VachaShah
VachaShah previously approved these changes Dec 18, 2023
Copy link
Collaborator

@VachaShah VachaShah left a comment

Choose a reason for hiding this comment

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

This is great! Thank you @channel-dante for adding this along with the tests!

@reta
Copy link
Collaborator

reta commented Dec 19, 2023

@VachaShah @reta Any ideas of how to do this kind of change better?

Agree with @VachaShah , having code generation in place would make it much simpler ....

@channel-dante channel-dante dismissed stale reviews from VachaShah and dblock via 7549084 December 19, 2023 01:59
@channel-dante channel-dante force-pushed the to-query-for-search-request branch 2 times, most recently from aebc5f5 to df61786 Compare December 19, 2023 04:35
@channel-dante channel-dante requested a review from dblock December 19, 2023 05:29
dblock
dblock previously approved these changes Dec 20, 2023
Signed-off-by: channel-dante <[email protected]>
Signed-off-by: channel-dante <[email protected]>
Signed-off-by: channel-dante <[email protected]>
Signed-off-by: channel-dante <[email protected]>
Signed-off-by: channel-dante <[email protected]>
Signed-off-by: channel-dante <[email protected]>
Signed-off-by: channel-dante <[email protected]>
Signed-off-by: channel-dante <[email protected]>
Signed-off-by: channel-dante <[email protected]>
Signed-off-by: channel-dante <[email protected]>
Signed-off-by: channel-dante <[email protected]>
Signed-off-by: channel-dante <[email protected]>
Signed-off-by: channel-dante <[email protected]>
@channel-dante channel-dante force-pushed the to-query-for-search-request branch from cbceb9d to 66c2454 Compare January 2, 2024 00:07
@channel-dante channel-dante requested a review from dblock January 2, 2024 00:17
Copy link
Collaborator

@VachaShah VachaShah left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @channel-dante!

@dblock dblock merged commit a8fea24 into opensearch-project:main Jan 2, 2024
46 checks passed
@dblock dblock added the backport 2.x Backport to 2.x branch label Jan 2, 2024
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-766-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 a8fea24a626485173c870e220731e15482f1ab94
# Push it to GitHub
git push --set-upstream origin backport/backport-766-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-766-to-2.x.

@dblock
Copy link
Member

dblock commented Jan 2, 2024

@channel-dante you should backport this manually to 2.x so we can release it with the next minor

channel-dante added a commit to channel-dante/opensearch-java that referenced this pull request Jan 3, 2024
* add toQuery() on SearchRequest

Signed-off-by: channel-dante <[email protected]>

* add toBuilder() on all Request in org.opensearch.client.opensearch.core

Signed-off-by: channel-dante <[email protected]>

* add toBuilder() on all Builder in org.opensearch.client.opensearch._types.query_dsl

Signed-off-by: channel-dante <[email protected]>

* update CHANGELOG.md

Signed-off-by: channel-dante <[email protected]>

* apply spotlessApply

Signed-off-by: channel-dante <[email protected]>

* fix CommonTermsQuery.toBuilder()

Signed-off-by: channel-dante <[email protected]>

* fix FuzzyQuery.toBuilder()

Signed-off-by: channel-dante <[email protected]>

* fix toBuilders

Signed-off-by: channel-dante <[email protected]>

* add test for _types/query_dsl package

Signed-off-by: channel-dante <[email protected]>

* add test for core package

Signed-off-by: channel-dante <[email protected]>

* spotlessApply

Signed-off-by: channel-dante <[email protected]>

* fix test

Signed-off-by: channel-dante <[email protected]>

* revert ModelTestCase

Signed-off-by: channel-dante <[email protected]>

* revert ModelTestCase

Signed-off-by: channel-dante <[email protected]>

* add pr link on CHANGELOG.md

Signed-off-by: channel-dante <[email protected]>

* add pr link on CHANGELOG.md

Signed-off-by: channel-dante <[email protected]>

* apply spotless

Signed-off-by: channel-dante <[email protected]>

---------

Signed-off-by: channel-dante <[email protected]>

(cherry picked from commit a8fea24)
Signed-off-by: channel-dante <[email protected]>
@channel-dante channel-dante deleted the to-query-for-search-request branch January 3, 2024 04:06
VachaShah pushed a commit that referenced this pull request Jan 3, 2024
* add toBuilder method on some models (#766)

* add toQuery() on SearchRequest

Signed-off-by: channel-dante <[email protected]>

* add toBuilder() on all Request in org.opensearch.client.opensearch.core

Signed-off-by: channel-dante <[email protected]>

* add toBuilder() on all Builder in org.opensearch.client.opensearch._types.query_dsl

Signed-off-by: channel-dante <[email protected]>

* update CHANGELOG.md

Signed-off-by: channel-dante <[email protected]>

* apply spotlessApply

Signed-off-by: channel-dante <[email protected]>

* fix CommonTermsQuery.toBuilder()

Signed-off-by: channel-dante <[email protected]>

* fix FuzzyQuery.toBuilder()

Signed-off-by: channel-dante <[email protected]>

* fix toBuilders

Signed-off-by: channel-dante <[email protected]>

* add test for _types/query_dsl package

Signed-off-by: channel-dante <[email protected]>

* add test for core package

Signed-off-by: channel-dante <[email protected]>

* spotlessApply

Signed-off-by: channel-dante <[email protected]>

* fix test

Signed-off-by: channel-dante <[email protected]>

* revert ModelTestCase

Signed-off-by: channel-dante <[email protected]>

* revert ModelTestCase

Signed-off-by: channel-dante <[email protected]>

* add pr link on CHANGELOG.md

Signed-off-by: channel-dante <[email protected]>

* add pr link on CHANGELOG.md

Signed-off-by: channel-dante <[email protected]>

* apply spotless

Signed-off-by: channel-dante <[email protected]>

---------

Signed-off-by: channel-dante <[email protected]>

(cherry picked from commit a8fea24)
Signed-off-by: channel-dante <[email protected]>

* removed not related test code

Signed-off-by: channel-dante <[email protected]>

---------

Signed-off-by: channel-dante <[email protected]>
@BrendonFaleiro BrendonFaleiro mentioned this pull request Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants