Fix parseToken array popping #13620

naomichi-y · 2024-05-10T04:39:40Z

Description

Fixed an issue in the flat_object where array field names were incorrectly removed after processing the first object, leading to inaccurate key-paths in flattened JSON data.

Related Issues

#13351

Check List

New functionality includes testing.
- All tests pass
~~New functionality has been documented.~~
- ~~New functionality has javadoc added~~
Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)
Public documentation issue/PR created

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.

github-actions · 2024-05-10T04:49:26Z

❌ Gradle check result for 51d6d10: 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?

github-actions · 2024-05-10T04:59:54Z

❌ Gradle check result for 3862517: 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?

naomichi-y · 2024-05-10T05:07:15Z

@msfroh
I tried to fix the issue where array fields disappear in flat_object Is this content correct?
#13259 (comment)

github-actions · 2024-05-10T05:09:41Z

❌ Gradle check result for fe60f0d: 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?

github-actions · 2024-05-17T03:41:37Z

❌ Gradle check result for c3adeb4: 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?

github-actions · 2024-05-17T03:51:38Z

❌ Gradle check result for 7f7ee10: 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?

server/src/main/java/org/opensearch/common/xcontent/JsonToStringXContentParser.java

opensearch-trigger-bot · 2024-06-29T15:20:35Z

This PR is stalled because it has been open for 30 days with no activity.

This method was pretty complicated, hard to follow, and therefore prone to bugs. This change introduces a proper stack (rather than a StringBuilder) to keep track of fields under fields. When ever we parse a field name, we push it onto the stack, recursively parse its value (an object, an array, or a primitive value), then we pop (guaranteeing pushes and pops are balanced). Signed-off-by: Michael Froh <froh@amazon.com>

Signed-off-by: Michael Froh <froh@amazon.com>

There is logic in DocumentParser that expects any object parser to start with currentToken() pointing to START_OBJECT, and return with currentToken() pointing to END_OBJECT. My previous commits were stepping over the start/end, and returning on the token following the end. Signed-off-by: Michael Froh <froh@amazon.com>

msfroh · 2024-08-13T18:06:39Z

Oof... that was complicated. Even debugging the yamlRestTests, the cause of failure wasn't obvious.

It turns out that it was all because I was stepping past the final END_OBJECT token, which DocumentParser doesn't like.

naomichi-y · 2024-08-13T18:12:28Z

@msfroh
Sorry for the late response. Thank you for your help!

github-actions · 2024-08-13T18:28:30Z

❌ Gradle check result for 02c12c6: 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?

github-actions · 2024-08-13T19:18:19Z

❌ Gradle check result for 02c12c6: 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?

msfroh · 2024-08-13T19:39:46Z

java.lang.AssertionError
	at __randomizedtesting.SeedInfo.seed([D25DBB2E808359FD:17E94777B0CD7A53]:0)
	at org.opensearch.common.xcontent.JsonToStringXContentParser.parseObject(JsonToStringXContentParser.java:69)
	at org.opensearch.index.mapper.FlatObjectFieldMapper.parseCreateField(FlatObjectFieldMapper.java:582)
	at org.opensearch.index.mapper.FieldMapper.parse(FieldMapper.java:269)
	at org.opensearch.index.mapper.DocumentParser.parseObjectOrField(DocumentParser.java:527)
	at org.opensearch.index.mapper.DocumentParser.parseNullValue(DocumentParser.java:725)
	at org.opensearch.index.mapper.DocumentParser.innerParseObject(DocumentParser.java:451)
	at org.opensearch.index.mapper.DocumentParser.parseObjectOrNested(DocumentParser.java:419)
	at org.opensearch.index.mapper.DocumentParser.internalParseDocument(DocumentParser.java:138)
	at org.opensearch.index.mapper.DocumentParser.parseDocument(DocumentParser.java:93)
	at org.opensearch.index.mapper.DocumentMapper.parse(DocumentMapper.java:251)

Hmm... apparently I missed the null case.

The existing behavior throws an exception on null flat object by advancing the parser beyond the end of the flat object input. We could simply skip a null flat_object field, but this would be a behavioral change from 2.7. Signed-off-by: Michael Froh <froh@amazon.com>

github-actions · 2024-08-13T20:53:42Z

✅ Gradle check result for cac0d94: SUCCESS

codecov · 2024-08-13T21:00:13Z

Codecov Report

Attention: Patch coverage is 90.90909% with 4 lines in your changes missing coverage. Please review.

Project coverage is 71.85%. Comparing base (ea1ab7c) to head (cac0d94).
Report is 2 commits behind head on main.

Files	Patch %	Lines
...ch/common/xcontent/JsonToStringXContentParser.java	90.00%	1 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #13620      +/-   ##
============================================
- Coverage     71.90%   71.85%   -0.06%     
+ Complexity    63045    63029      -16     
============================================
  Files          5197     5197              
  Lines        295313   295309       -4     
  Branches      42677    42675       -2     
============================================
- Hits         212341   212189     -152     
- Misses        65558    65748     +190     
+ Partials      17414    17372      -42

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

* Fix parseToken array popping Signed-off-by: naomichi-y <n.yamakita@gmail.com> * Add fix description Signed-off-by: naomichi-y <n.yamakita@gmail.com> * Refactor JsonToStringXContentParser#parseToken This method was pretty complicated, hard to follow, and therefore prone to bugs. This change introduces a proper stack (rather than a StringBuilder) to keep track of fields under fields. When ever we parse a field name, we push it onto the stack, recursively parse its value (an object, an array, or a primitive value), then we pop (guaranteeing pushes and pops are balanced). Signed-off-by: Michael Froh <froh@amazon.com> * Remove use of org.apache.logging.log4j.util.Strings Signed-off-by: Michael Froh <froh@amazon.com> * Make sure JsonToStringXContentParser ends on END_OBJECT There is logic in DocumentParser that expects any object parser to start with currentToken() pointing to START_OBJECT, and return with currentToken() pointing to END_OBJECT. My previous commits were stepping over the start/end, and returning on the token following the end. Signed-off-by: Michael Froh <froh@amazon.com> * Throw exception when flat object is null The existing behavior throws an exception on null flat object by advancing the parser beyond the end of the flat object input. We could simply skip a null flat_object field, but this would be a behavioral change from 2.7. Signed-off-by: Michael Froh <froh@amazon.com> --------- Signed-off-by: naomichi-y <n.yamakita@gmail.com> Signed-off-by: Michael Froh <froh@amazon.com> Co-authored-by: Michael Froh <froh@amazon.com> (cherry picked from commit 6e27e5a) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* Fix parseToken array popping * Add fix description * Refactor JsonToStringXContentParser#parseToken This method was pretty complicated, hard to follow, and therefore prone to bugs. This change introduces a proper stack (rather than a StringBuilder) to keep track of fields under fields. When ever we parse a field name, we push it onto the stack, recursively parse its value (an object, an array, or a primitive value), then we pop (guaranteeing pushes and pops are balanced). * Remove use of org.apache.logging.log4j.util.Strings * Make sure JsonToStringXContentParser ends on END_OBJECT There is logic in DocumentParser that expects any object parser to start with currentToken() pointing to START_OBJECT, and return with currentToken() pointing to END_OBJECT. My previous commits were stepping over the start/end, and returning on the token following the end. * Throw exception when flat object is null The existing behavior throws an exception on null flat object by advancing the parser beyond the end of the flat object input. We could simply skip a null flat_object field, but this would be a behavioral change from 2.7. --------- (cherry picked from commit 6e27e5a) Signed-off-by: naomichi-y <n.yamakita@gmail.com> Signed-off-by: Michael Froh <froh@amazon.com> 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> Co-authored-by: Michael Froh <froh@amazon.com>

nicklasmoeller · 2024-08-20T17:52:07Z

Hi - not sure if this is the place to ask, but can't find anything in the release docs.

When can this be expected to be tagged in a release? We're running into this on AWS OpenSearch

dblock · 2024-08-21T13:50:06Z

Hi - not sure if this is the place to ask, but can't find anything in the release docs.

When can this be expected to be tagged in a release? We're running into this on AWS OpenSearch

This was backported to 2.x in #15235, so it will be in the next open source 2.17 release (https://opensearch.org/releases.html).

For Amazon OpenSearch this is really a question for Amazon support. The service deploys every other release, see https://docs.aws.amazon.com/opensearch-service/latest/developerguide/release-notes.html and you can see the past dates to get an idea of when you can expect something (2.9: October 2, 2023, 2.11: November 17, 2023, 2.13: May 21, 2024, 2.15 would be next).

* Fix parseToken array popping Signed-off-by: naomichi-y <n.yamakita@gmail.com> * Add fix description Signed-off-by: naomichi-y <n.yamakita@gmail.com> * Refactor JsonToStringXContentParser#parseToken This method was pretty complicated, hard to follow, and therefore prone to bugs. This change introduces a proper stack (rather than a StringBuilder) to keep track of fields under fields. When ever we parse a field name, we push it onto the stack, recursively parse its value (an object, an array, or a primitive value), then we pop (guaranteeing pushes and pops are balanced). Signed-off-by: Michael Froh <froh@amazon.com> * Remove use of org.apache.logging.log4j.util.Strings Signed-off-by: Michael Froh <froh@amazon.com> * Make sure JsonToStringXContentParser ends on END_OBJECT There is logic in DocumentParser that expects any object parser to start with currentToken() pointing to START_OBJECT, and return with currentToken() pointing to END_OBJECT. My previous commits were stepping over the start/end, and returning on the token following the end. Signed-off-by: Michael Froh <froh@amazon.com> * Throw exception when flat object is null The existing behavior throws an exception on null flat object by advancing the parser beyond the end of the flat object input. We could simply skip a null flat_object field, but this would be a behavioral change from 2.7. Signed-off-by: Michael Froh <froh@amazon.com> --------- Signed-off-by: naomichi-y <n.yamakita@gmail.com> Signed-off-by: Michael Froh <froh@amazon.com> Co-authored-by: Michael Froh <froh@amazon.com>

naomichi-y marked this pull request as ready for review May 17, 2024 04:56

naomichi-y requested review from anasalkouz, andrross, Bukhtawar, CEHENKLE, dblock, dbwiddis, dreamer-89, gbbafna, kotwanikunal, mch2, msfroh, nknize, owaiskazi19, reta, Rishikesh1159, sachinpkale, saratvemulapalli, shwetathareja, sohami, tlfeng and VachaShah as code owners May 17, 2024 04:56

reta reviewed May 21, 2024

View reviewed changes

server/src/main/java/org/opensearch/common/xcontent/JsonToStringXContentParser.java Outdated Show resolved Hide resolved

msfroh added 3 commits August 13, 2024 09:42

Remove use of org.apache.logging.log4j.util.Strings

7f176b3

Signed-off-by: Michael Froh <froh@amazon.com>

msfroh force-pushed the feature/fix-parseToken-array-popping branch from 8b14531 to 02c12c6 Compare August 13, 2024 18:00

msfroh requested a review from jed326 as a code owner August 13, 2024 18:00

reta approved these changes Aug 13, 2024

View reviewed changes

reta merged commit 6e27e5a into opensearch-project:main Aug 13, 2024
34 checks passed

reta added the bug Something isn't working label Aug 13, 2024

opensearch-trigger-bot bot mentioned this pull request Aug 13, 2024

[Backport 2.x] Fix parseToken array popping #15235

Merged

opensearch-ci-bot mentioned this pull request Sep 6, 2024

[AUTOCUT] Gradle Check Flaky Test Report for MasterServiceTests #15809

Open

BrewTestBot mentioned this pull request Sep 18, 2024

opensearch 2.17.0 Homebrew/homebrew-core#191052

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix parseToken array popping #13620

Fix parseToken array popping #13620

naomichi-y commented May 10, 2024 •

edited

Loading

github-actions bot commented May 10, 2024

github-actions bot commented May 10, 2024

naomichi-y commented May 10, 2024

github-actions bot commented May 10, 2024

github-actions bot commented May 17, 2024

github-actions bot commented May 17, 2024

opensearch-trigger-bot bot commented Jun 29, 2024

msfroh commented Aug 13, 2024

naomichi-y commented Aug 13, 2024

github-actions bot commented Aug 13, 2024

github-actions bot commented Aug 13, 2024

msfroh commented Aug 13, 2024

github-actions bot commented Aug 13, 2024

codecov bot commented Aug 13, 2024

nicklasmoeller commented Aug 20, 2024

dblock commented Aug 21, 2024

Fix parseToken array popping #13620

Fix parseToken array popping #13620

Conversation

naomichi-y commented May 10, 2024 • edited Loading

Description

Related Issues

Check List

github-actions bot commented May 10, 2024

github-actions bot commented May 10, 2024

naomichi-y commented May 10, 2024

github-actions bot commented May 10, 2024

github-actions bot commented May 17, 2024

github-actions bot commented May 17, 2024

opensearch-trigger-bot bot commented Jun 29, 2024

msfroh commented Aug 13, 2024

naomichi-y commented Aug 13, 2024

github-actions bot commented Aug 13, 2024

github-actions bot commented Aug 13, 2024

msfroh commented Aug 13, 2024

github-actions bot commented Aug 13, 2024

codecov bot commented Aug 13, 2024

Codecov Report

nicklasmoeller commented Aug 20, 2024

dblock commented Aug 21, 2024

naomichi-y commented May 10, 2024 •

edited

Loading