Fix flat_object long field error

[naomichi-y]

Description

Fixed an error 'Preview of field's value: 'null' that occurred when using flat_object with long field names in OpenSearch version 2.12 and later.

Related Issues

#13184

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]

❌ Gradle check result for 5aa3812: 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]

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

[dblock]

What are the values of path and currentFieldName that we end up with in the changed code and what is that code trying to do? I am just wondering what the side effects of setting path to blank are.

@msfroh take a look pls

[dblock]

* What went wrong:
Execution failed for task ':server:spotlessJavaCheck'.
> The following files had format violations:
      src/test/java/org/opensearch/index/mapper/FlatObjectFieldDataTests.java
          @@ -73,8 +73,12 @@
           ············.startObject("field")
           ············.startObject("detail")
           ············.startArray("fooooooooooo")
          -············.startObject().field("name",·"baz").endObject()
          -············.startObject().field("name",·"baz").endObject()
          +············.startObject()
          +············.field("name",·"baz")
          +············.endObject()
          +············.startObject()
          +············.field("name",·"baz")
          +············.endObject()
           ············.endArray()
           ············.endObject()
           ············.endObject()
          @@ -93,7 +97,6 @@
           ········assertEquals(1,·valueReaders.size());
           ····}
           
          -
           ····@Override
           ····protected·String·getFieldDataType()·{
           ········return·FIELD_TYPE;
  Run './gradlew :server:spotlessApply' to fix these violations.

^

[github-actions]

✅ Gradle check result for 85dafbf: SUCCESS

[msfroh]

I will take a look soon. I'm getting on a plane in an hour and a half and should be able to review once the in-flight wifi kicks in.

Grr... the in-flight wifi is too slow to connect to the EC2 instance where I usually work on OpenSearch, and the connection is too flaky to clone the repo onto my laptop. I'd like to try debugging the added unit test to wrap my head around the specifics of the bug. I'll try again once I'm checked into my hotel this evening. Nevermind -- I managed to get it to clone to my laptop. As they say, seventh time's the charm.

[msfroh]

I played around with the unit test and added the following unit test to JsonToStringXContentParserTest:

    public void testArrayOfObjects() throws IOException {
        String jsonExample ="{" +
            "\"field\": {" +
            "  \"detail\": {" +
            "    \"foooooooooooo\": [" +
            "      {\"name\":\"baz\"}," +
            "      {\"name\":\"baz\"}" +
            "    ]" +
            "  }" +
            "}}";

        assertEquals("{" +
            "\"flat\":[\"field\",\"detail\",\"foooooooooooo\",\"name\",\"name\"]," +
            "\"flat._value\":[\"baz\",\"baz\"]," +
            "\"flat._valueAndPath\":[" +
            "\"flat.field.detail.foooooooooooo.name=baz\"," +
            "\"flat.field.detail.foooooooooooo.name=baz\"" +
            "]}",
            flattenJsonString("flat", jsonExample)
        );
    }

The bug is in the use of path.lastIndexOf(). On a StringBuilder, it goes from the end of the buffer, regardless of the assigned length of the buffer. It can be fixed by changing it to path.lastIndexOf(DOT_SYMBOL, path.length()).

Unfortunately, there's another problem -- with that fix (as well as yours), the above unit test fails, because the second flat._valueAndPath becomes flat.field.detail.name=baz. That's because of another bug: we pop foooooooooooo off after processing the first object in the array, so it's no longer there when we process the second object.

I was able to fix that by adding a boolean popName argument to parseToken and wrapped the name-popping logic in a check. For startObject, we pass true, but for startArray, we pass false. Here's the modified popping logic:

                if (popName) {
                    int dotIndex = path.lastIndexOf(DOT_SYMBOL, path.length());
                    if (dotIndex != -1) {
                        path.setLength(path.length() - currentFieldName.length() - 1);
                    }
                }

[github-actions]

❌ Gradle check result for 5c276f0: 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]

❌ Gradle check result for 4b58495: 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]

@msfroh
Thank you. I have changed the path.lastIndexOf argument, please confirm.
The other bug was confirmed in my environment. I guess a new Issue needs to be made.

[dblock]

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

org.opensearch.indices.store.IndicesStoreIntegrationIT.testShardActiveElseWhere {p0={"cluster.remote_store.state.enabled":"true"}}

#12788

[dblock]

@msfroh Thank you. I have changed the path.lastIndexOf argument, please confirm. The other bug was confirmed in my environment. I guess a new Issue needs to be made.

Please do open an issue or add a fix/test to this PR @naomichi-y. Appreciate it.

I propose we get this change in first as is? @msfroh wdyt?

[naomichi-y]

I've opened a new issue.
#13351

[github-actions]

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

[dblock]

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

Test Result (4 failures / +4)
org.opensearch.cluster.coordination.AwarenessAttributeDecommissionIT.testConcurrentDecommissionAction
org.opensearch.cluster.coordination.AwarenessAttributeDecommissionIT.testConcurrentDecommissionAction
org.opensearch.cluster.coordination.AwarenessAttributeDecommissionIT.testConcurrentDecommissionAction
org.opensearch.cluster.coordination.AwarenessAttributeDecommissionIT.testConcurrentDecommissionAction

#12197

[github-actions]

❕ Gradle check result for 4b58495: UNSTABLE

• TEST FAILURES:

      1 org.opensearch.cluster.MinimumClusterManagerNodesIT.testThreeNodesNoClusterManagerBlock
      1 org.opensearch.action.admin.indices.create.CreateIndexIT.testCreateAndDeleteIndexConcurrently
      1 org.opensearch.action.admin.indices.create.CreateIndexIT.classMethod

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[msfroh]

Thanks for tackling this, @naomichi-y! If you're willing to address the follow-up issue, that would be great!

Please feel free to steal the unit test that I posted above.