Fix for #380. #503

DumboJetEngine · 2024-01-08T10:44:33Z

Description

Fix for #380.

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

Thanks. Will need a unit test as well.

dblock · 2024-01-08T16:35:09Z

src/OpenSearch.Client/Mapping/Visitor/PropertyWalker.cs


 			return type;
+
+			bool ShouldUnwrapType(Type ty)


Is this a pattern elsewhere in the codebase to write a function within another function? Otherwise make this private static?

Apparently not.
Although, it might be because local functions are a relatively new language feature.
I can change this.

For the test, I saw that there were no tests at all for the PropertyWalker and the PropertiesExtensions that was using it was marked as internal. I will add some tests now.

I guess I also need to update the change-log too?
Anything special I should pay attention to?

Nope, keep it short! :) Thanks much.

DumboJetEngine · 2024-01-08T17:04:14Z

Thanks. Will need a unit test as well.

Shall I put it at Tests.Reproduce.GitHubIssue380 or this is some code area that is inherited by ElasticSearch?
I see it contains 4-digit issues in there, while OpenSearch is still using 3-digit issues. That's why I ask -- it feels weird.
Perhaps I can put it as Tests.Reproduce.PropertyWalkerTests instead...

DumboJetEngine · 2024-01-08T17:43:59Z

I have mixed the two issues ( #380, #375 ) a bit and messed up the history. 🤦‍♂️
Shall I perhaps create the PR from scratch?
Other than that, I think the changes should be OK now.

Update: Perhaps I should move my tests inside the Tests project, and out of Tests.Reproduce.

Xtansia · 2024-01-10T01:01:17Z

I have mixed the two issues ( #380, #375 ) a bit and messed up the history. 🤦‍♂️ Shall I perhaps create the PR from scratch? Other than that, I think the changes should be OK now.

Update: Perhaps I should move my tests inside the Tests project, and out of Tests.Reproduce.

@DumboJetEngine You don't need to completely recreate the PR, you can amend and drop the commits on your local branch as you see fit and then force push your feature branch and the content of the PR will update to match. This is preferred over closing and recreating the PR, as otherwise it can create a lot of notification spam and churn in the closed PR history.

DumboJetEngine · 2024-01-10T10:20:10Z

@Xtansia
OK.
Done. :)
Let me know if all is OK.

Xtansia

Thanks @DumboJetEngine, code-wise all looks good to me, just need to correct the PR number in the changelog item

Xtansia · 2024-01-10T21:19:33Z

CHANGELOG.md

@@ -14,6 +14,11 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 ### Removed
 - Removed the `Features` API which is not supported by OpenSearch from the low-level client ([#331](https://github.com/opensearch-project/opensearch-net/pull/331))

+
+### Fixed
+- Fixed `IEnumerable<int?>` property mapping. ([#380](https://github.com/opensearch-project/opensearch-net/pull/375))


The changelog item should reference this PR's number: 503

Suggested change

- Fixed `IEnumerable<int?>` property mapping. ([#380](https://github.com/opensearch-project/opensearch-net/pull/375))

- Fixed `IEnumerable<int?>` property mapping. ([#503](https://github.com/opensearch-project/opensearch-net/pull/503))

Xtansia

Sorry just noticed a couple other things, the test file needs a license header added, and it appears you have a space in the file name of the test file.

Xtansia · 2024-01-10T21:22:59Z

tests/Tests/Mapping/PropertyWalkerTests .cs

@@ -0,0 +1,77 @@
+using System;


This file needs a license header like so:

Suggested change

using System;

/* SPDX-License-Identifier: Apache-2.0

*

* The OpenSearch Contributors require contributions made to

* this file be licensed under the Apache-2.0 license or a

* compatible open source license.

*/

using System;

Fixed. Anything more I need to change?

Signed-off-by: Kostas <[email protected]>

opensearch-trigger-bot · 2024-01-10T22:15:22Z

The backport to 1.x failed:

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

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-1.x 1.x
# Navigate to the new working tree
cd .worktrees/backport-1.x
# Create a new branch
git switch --create backport/backport-503-to-1.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 afa7c7c928decdeac66feabd64c04732e1b57fe7
# Push it to GitHub
git push --set-upstream origin backport/backport-503-to-1.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.x

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

Xtansia · 2024-01-10T22:17:03Z

@DumboJetEngine Would you be able to follow the above instructions fixing any merge conflicts (almost certainly only something minor in the CHANGELOG) to backport this fix to the 1.x version line?

DumboJetEngine · 2024-01-10T22:21:23Z

@Xtansia Yes, I think I can do this.
Tomorrow though.
I need to leave my computer now.

DumboJetEngine requested review from anirudha, dblock, joshuali925, Xtansia, VachaShah, MaxKsyunz and Yury-Fridlyand as code owners January 8, 2024 10:44

dblock reviewed Jan 8, 2024

View reviewed changes

DumboJetEngine force-pushed the fix_380 branch from ea09f96 to d216eef Compare January 10, 2024 10:17

DumboJetEngine force-pushed the fix_380 branch from d216eef to ae48981 Compare January 10, 2024 12:06

Xtansia requested changes Jan 10, 2024

View reviewed changes

Fix for IEnumerable<int?> property mapping ( opensearch-project#380 )

14c8af6

Signed-off-by: Kostas <[email protected]>

DumboJetEngine force-pushed the fix_380 branch from ae48981 to 14c8af6 Compare January 10, 2024 21:33

DumboJetEngine requested a review from Xtansia January 10, 2024 21:38

Xtansia approved these changes Jan 10, 2024

View reviewed changes

Xtansia merged commit afa7c7c into opensearch-project:main Jan 10, 2024
32 of 35 checks passed

Xtansia added the backport 1.x Backport to 1.x branch label Jan 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for #380. #503

Fix for #380. #503

DumboJetEngine commented Jan 8, 2024

dblock left a comment

dblock Jan 8, 2024

DumboJetEngine Jan 8, 2024 •

edited

Loading

DumboJetEngine Jan 8, 2024

dblock Jan 8, 2024

DumboJetEngine commented Jan 8, 2024 •

edited

Loading

DumboJetEngine commented Jan 8, 2024 •

edited

Loading

Xtansia commented Jan 10, 2024

DumboJetEngine commented Jan 10, 2024

Xtansia left a comment

Xtansia Jan 10, 2024

Xtansia left a comment

Xtansia Jan 10, 2024

DumboJetEngine Jan 10, 2024

opensearch-trigger-bot bot commented Jan 10, 2024

Xtansia commented Jan 10, 2024

DumboJetEngine commented Jan 10, 2024

	- Fixed `IEnumerable<int?>` property mapping. ([#380](https://github.com/opensearch-project/opensearch-net/pull/375))
	- Fixed `IEnumerable<int?>` property mapping. ([#503](https://github.com/opensearch-project/opensearch-net/pull/503))

-using System;
+/* SPDX-License-Identifier: Apache-2.0
+*
+* The OpenSearch Contributors require contributions made to
+* this file be licensed under the Apache-2.0 license or a
+* compatible open source license.
+*/
+using System;

Fix for #380. #503

Fix for #380. #503

Conversation

DumboJetEngine commented Jan 8, 2024

Description

Issues Resolved

dblock left a comment

Choose a reason for hiding this comment

dblock Jan 8, 2024

Choose a reason for hiding this comment

DumboJetEngine Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

DumboJetEngine Jan 8, 2024

Choose a reason for hiding this comment

dblock Jan 8, 2024

Choose a reason for hiding this comment

DumboJetEngine commented Jan 8, 2024 • edited Loading

DumboJetEngine commented Jan 8, 2024 • edited Loading

Xtansia commented Jan 10, 2024

DumboJetEngine commented Jan 10, 2024

Xtansia left a comment

Choose a reason for hiding this comment

Xtansia Jan 10, 2024

Choose a reason for hiding this comment

Xtansia left a comment

Choose a reason for hiding this comment

Xtansia Jan 10, 2024

Choose a reason for hiding this comment

DumboJetEngine Jan 10, 2024

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Jan 10, 2024

Xtansia commented Jan 10, 2024

DumboJetEngine commented Jan 10, 2024

DumboJetEngine Jan 8, 2024 •

edited

Loading

DumboJetEngine commented Jan 8, 2024 •

edited

Loading

DumboJetEngine commented Jan 8, 2024 •

edited

Loading