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 null values indexed as "null" strings in flat_object field #14069

Merged
merged 1 commit into from
Aug 30, 2024

Conversation

kkewwei
Copy link
Contributor

@kkewwei kkewwei commented Jun 7, 2024

Signed-off-by: kkewwei [email protected]

Description

The pr soloves three cases:

  • fix null value case, I have test too many cases about null value, all of the tests are passed.
  • deduplication the value of fieldName, valueList, valueAndPathList, there is no need to store the duplicate values.
  • fix infinite loop with null value (comment)

Related Issues

Resolves #13880

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

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.

Copy link
Contributor

github-actions bot commented Jun 7, 2024

❌ Gradle check result for 53b8006: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

github-actions bot commented Jun 7, 2024

❌ Gradle check result for 3a814c8: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

github-actions bot commented Jun 7, 2024

❌ Gradle check result for 687dbfb: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@kkewwei kkewwei force-pushed the fix_null_flat_object branch 2 times, most recently from c61403e to 67525ea Compare June 7, 2024 14:44
Copy link
Contributor

✅ Gradle check result for 3d59204: SUCCESS

Copy link
Contributor

❌ Gradle check result for 92c8ddd: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@kkewwei
Copy link
Contributor Author

kkewwei commented Aug 28, 2024

FAILURE

❌ Gradle check result for 92c8ddd: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

ReindexBasicTests.testMultipleSources #13913
MasterServiceTests.testClusterStateUpdateLoggingWithDebugEnabled #15117

Copy link
Contributor

❌ Gradle check result for 7023bd1: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@kkewwei
Copy link
Contributor Author

kkewwei commented Aug 28, 2024

❌ Gradle check result for 7023bd1: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

MasterServiceTests.testClusterStateUpdateLoggingWithDebugEnabled #15117

Copy link
Contributor

✅ Gradle check result for 53306f7: SUCCESS

Copy link
Collaborator

@msfroh msfroh left a comment

Choose a reason for hiding this comment

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

Hey @kkewwei -- thank you so much for really diving into this fix!

My earlier fix over at #15375 wasn't nearly so complete. With this fix, I believe we'll finally be null-safe.

I left a minor suggestion that should reduce the diff on JsonToStringXContentParserTests.

@msfroh msfroh added the backport 2.x Backport to 2.x branch label Aug 29, 2024
@msfroh
Copy link
Collaborator

msfroh commented Aug 30, 2024

Actually, while I still agree with my prior review comments, they don't affect the usefulness of this fix. I'm going to approve and merge, and I can take care of the cosmetic changes.

@msfroh msfroh merged commit 9194b7a into opensearch-project:main Aug 30, 2024
36 of 43 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 30, 2024
Signed-off-by: kkewwei <[email protected]>
(cherry picked from commit 9194b7a)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@kkewwei
Copy link
Contributor Author

kkewwei commented Aug 30, 2024

@msfroh, very thank you for your follow-up cosmetic changes.

@kkewwei kkewwei deleted the fix_null_flat_object branch August 30, 2024 05:54
msfroh pushed a commit that referenced this pull request Aug 30, 2024
(cherry picked from commit 9194b7a)

Signed-off-by: kkewwei <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
akolarkunnu pushed a commit to akolarkunnu/OpenSearch that referenced this pull request Sep 10, 2024
@cameronattard
Copy link

I think I'm encountering the infinite loop. Any chance of getting this backported and released into 2.15 any time soon?

@dblock
Copy link
Member

dblock commented Sep 19, 2024

I think I'm encountering the infinite loop. Any chance of getting this backported and released into 2.15 any time soon?

@cameronattard only high sev security fixes get released in patch releases, and only on the latest (2.17), so 2.15 is long gone

@cameronattard
Copy link

I think I'm encountering the infinite loop. Any chance of getting this backported and released into 2.15 any time soon?

@cameronattard only high sev security fixes get released in patch releases, and only on the latest (2.17), so 2.15 is long gone

Surely being able to DoS an entire cluster by indexing a single document is a high severity security issue. And 2.15 support was only just added to Amazon OpenSearch Service a week ago. Hardly long gone.

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 bug Something isn't working Indexing Indexing, Bulk Indexing and anything related to indexing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] In flat_object fields null values indexed as "null" strings
7 participants