[BUG] Parsing a GetResult returns NPE if "found" field is missing

[dbwiddis]

Describe the bug

GetResult.fromXContent(XcontentParser parser) throws a NullPointerException if the field "found" is missing.

This is an unexpected exception type for a parsing failure and can be confusing to developers. Other missing fields throw a ParsingException which developers are used to handling.

Related component

Libraries

To Reproduce

Run the following program.

import org.opensearch.action.get.GetResponse;
import org.opensearch.common.xcontent.LoggingDeprecationHandler;
import org.opensearch.common.xcontent.json.JsonXContent;
import org.opensearch.core.common.ParsingException;
import org.opensearch.core.xcontent.NamedXContentRegistry;
import org.opensearch.core.xcontent.XContentParser;

import java.io.IOException;

public class ReproTestCase {

    public static void main(String... args) {
        printExceptionType("{foo}");
        printExceptionType("{\"found\":false}");
        printExceptionType("{\"_index\":\"foo\",\"_id\":\"bar\"}");
    }

    static void printExceptionType(String json) {
        System.out.println("String to parse: " + json);
        try (
            XContentParser parser = JsonXContent.jsonXContent
                .createParser(NamedXContentRegistry.EMPTY, LoggingDeprecationHandler.INSTANCE, json)
        ) {
            GetResponse.fromXContent(parser);
        } catch (IOException io) {
            System.out.println("  Bad JSON, expected IOException: " + io.getMessage());
        } catch (ParsingException p) {
            System.out.println("  Missing fields, expected ParsingException: " + p.getMessage());
        } catch (Exception e) {
            System.out.println("  Missing \"found\", unexpected Exception: " + e.getClass() + ", " + e.getMessage());
        }
        System.out.println();
    }

}

Output:

String to parse: "{foo}"
  Bad JSON, expected IOException: Unexpected character ('f' (code 102)): was expecting double-quote to start field name
 at [Source: REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 1, column: 2]

String to parse: "{"found":false}"
  Missing fields, expected ParsingException: Missing required fields [_index,_id]

String to parse: "{"_index":"foo","_id":"bar"}"
  Missing "found", unexpected Exception: class java.lang.NullPointerException, null

Expected behavior

Code should either throw a ParsingException for a missing found field, or assume a sensible default (false) if it's omitted.

Additional Details

Additional context

The root cause of the problem is that found is initialized to null here:

OpenSearch/server/src/main/java/org/opensearch/index/get/GetResult.java

Line 354 in 47feca7

Boolean found = null;

And then only conditionally initialized:

OpenSearch/server/src/main/java/org/opensearch/index/get/GetResult.java

Lines 372 to 374 in 47feca7

	} else if (FOUND.equals(currentFieldName)) {
	found = parser.booleanValue();
	} else {

This value is passed to the GetResult constructor which expects a primitive boolean for the exists parameter. Unboxing the null Boolean results in the NPE.

Suggested Fix

Option 1: test the value for null before calling the GetResult constructor, and throw a ParsingException.

Option 2: initialize found to false

[imvtsl]

Hi @dbwiddis
I am looking to begin contributing to this repository.
Can I go ahead and create a PR with the fix? I am looking to go with option 1.

[dbwiddis]

Go for it, @imvtsl ! Let us know if you need any help!

[imvtsl]

Thank you!! Appreciate it.
I am working on it now.

[imvtsl]

I created a PR with fix. I added a unit test with this scenario. I also updated CHANGELOG.md.
Please let me know if anything is missing.
Thank you!

[imvtsl]

How do we go about from here?
When do we merge the PR? I also noticed "backport 2.x" label. How do we go about it?
Is there anything required from my end?

[dbwiddis]

How do we go about from here? When do we merge the PR?

We normally have two maintainers review the PR. The workflows allow one maintainer's review after the last push (which I have given), but there's no great rush on this one as we're several weeks away from a release.

There's also a need for the gradle checks to all pass. Often there are failures on flaky tests that are not associated with your PR, and sometimes we just have to retry those tests until they pass. There's not much you can do for the retries here; as a first-time committer any changes you make require approval to run the tests (otherwise a git amend/force-push can retrigger it) so you're sort of at the mercy of the maintainers. Not to worry, I'm checking on this PR regularly to get it through. It just may take a bit. :)

I also noticed "backport 2.x" label. How do we go about it?

TLDR: There's nothing you need to do.

The main branch tracks version 3.0.0 which has some breaking changes. We'll probably release it "soon" (a very flexible term currently meaning maybe sometime in 2025) but in the meantime our regular releases are on the 2.x branch. We just released 2.15.0, so the 2.x branch is currently tracking 2.16.0 which will be released in early August. Since your change is useful to the existing 2.x branch and doesn't require any new 3.x capability, I added that backport tag myself. When your PR gets merged, automation will trigger a PR against the 2.x branch cherry-picking the changes from this PR into that branch. Maintainers will approve and merge that backport PR when it happens.

We can also backport to the 1.x branch if needed as we are still doing patch releases there. However, since this is not a user-facing issue, and mostly a developer debugging annoyance (I say as someone who spent about an hour debugging to find this corner case when writing relative code) it's really not needed on 1.3.x (our maintenance branch for the 1.x series).

Is there anything required from my end?

I don't think so!

I've approved your PR with no more comments. You did a fantastic job and I really appreciate you contributing to this project!

I'm just waiting for a second maintainer to have eyes on it. They'll likely either approve and merge or maybe find some suggestion I missed in which case you might need another tweak.

If nobody else looks at it after a week or so, I'll just merge it (and the auto-generated backport).

Thanks again for your contribution!

[imvtsl]

Thank you for taking out time for the detailed response.
Maybe we can add the part about two maintainers to CONTRIBUTING.md?
Please feel free to let me know if there is any other issue/feature enhancement where I can contribute. I would be happy to do that.
Once again, thank you for your support.

[dbwiddis]

Maybe we can add the part about two maintainers to CONTRIBUTING.md?

Not sure if that's really standard; the official limit is 1. I think the section on reviews is probably fine as-is.

My hesitation in merging this with just my review is that so far I'm the only maintainer with eyes-on this from start to finish. I both created the issue and approved the PR so the number of people who even know about this is pretty small. I'd go fater if a release were imminent but we have a few weeks for that so there simply isn't a rush. I really appreciate your eagerness to get it merged, but it won't make it out until 2.16.0 anyway.

I really appreciate your eagerness to contribute, though! I know many other large projects (most ASF projects) use a 72-hour rule to make sure enough people have an idea to see/contribute to things, so I'll probably wait one more day.

Please feel free to let me know if there is any other issue/feature enhancement where I can contribute.

We have quite a few issues, many tagged good first issue, some slightly more challenging but still accessible. Is there a particular area of the codebase you're interested in working on?

[imvtsl]

Once again, I appreciate your prompt and detailed response.

My hesitation in merging this with just my review is that so far I'm the only maintainer with eyes-on this from start to finish. I both created the issue and approved the PR so the number of people who even know about this is pretty small. I'd go fater if a release were imminent but we have a few weeks for that so there simply isn't a rush. I really appreciate your eagerness to get it merged, but it won't make it out until 2.16.0 anyway.

It makes sense to wait to give others a chance to see the commit. Perhaps, I came across as too eager to merge. It's my first meaningful open source contribution and I wanted to see it through!!

We have quite a few issues, many tagged good first issue, some slightly more challenging but still accessible.

I did have a look at few issues tagged 'good first issue'. A lot of them seem to already have commits waiting to be merged.

Is there a particular area of the codebase you're interested in working on?

I started very recently with this repo so there's nothing in particular yet. Hopefully I have something in the near future. I am going through onboarding.md.
I am just starting with open source contributions and managed to create a commit in another repository too (spring-framework!!). So, I am looking to make meaningful contributions to the open source community.