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 NullPointerException in Monitor.getQuery when query is not present #12736

Conversation

daviscook477
Copy link
Contributor

Description

The javadoc for Monitor.getQuery suggests that it will return null when the query is not present but instead it throws a NullPointerException.

This is because bytesHolder[0] will still be null after the call to search if it does not find any query matching the id in the monitor's index. Then when the serializer.deserialize call is made, any attempts to index the BytesRef will throw the NullPointerException. This will happen here when using the serializer/deserializer created with MonitorQuerySerializer.fromParser.

Alternative Approach

This could instead be fixed just for the case of MonitorQuerySerializer.fromParser by making the serializer/deserializer returned from it null-safe (i.e. it checks if the input is null and returns null). But I went with this approach since it would be robust against custom implementations of the MonitorQuerySerializer interface that also do not check for null like the fromParser implementation.

Tests

I introduced testGetQueryNotPresent which fails on master with the NullPointerException, but passes on this branch. I also added a corresponding testGetQueryPresent which passes on both master and this branch. Since I needed a monitor with persistence for these two new tests I refactored creating a monitor with persistence into a helper function newMonitorWithPersistence that gets reused throughout the TestMonitorPersistence file.

@romseygeek
Copy link
Contributor

This looks great, thank you @daviscook477! Would you be able to add an entry to CHANGES.txt under the 9.9.0 release?

@daviscook477 daviscook477 force-pushed the upstream-fix-monitor-get-query-null-pointer-exception branch from 80bbdfa to c3762c4 Compare October 31, 2023 13:32
@daviscook477
Copy link
Contributor Author

@romseygeek I have added a CHANGES.txt entry under the 9.9.0 release describing the bugfix in this PR.

@romseygeek romseygeek self-assigned this Oct 31, 2023
@romseygeek romseygeek merged commit bbf3221 into apache:main Oct 31, 2023
4 checks passed
@romseygeek
Copy link
Contributor

Thanks @daviscook477!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants