OAK-12051: Fix NPE when ordering union query by jcr:score#2746
OAK-12051: Fix NPE when ordering union query by jcr:score#2746ChlineSaurus wants to merge 3 commits intoapache:OAK-12051from
Conversation
| PropertyValue[] values = new PropertyValue[] { | ||
| PropertyValues.newString(path), | ||
| PropertyValues.newDouble(score) | ||
| score == null ? null : PropertyValues.newDouble(score) |
There was a problem hiding this comment.
This looks wrong to me. What about:
| score == null ? null : PropertyValues.newDouble(score) | |
| PropertyValues.newDouble(score == null ? 0.0 : score) |
There was a problem hiding this comment.
This is only for mocking behavior in tests. The goal is that it returns null, so the change would defeat the purpose in my opinion. The mock should be able to return null to test if the actual code can handle a null return.
What this (somewhat weird) line does is to ensure, that we only try to turn the value to be returned by the mock into a Double if it isn't null, with the goal that the same mocking code can be used for tests with null and actual scores.
There was a problem hiding this comment.
Yes, I see, you are right! Sorry I got confused.
| } | ||
| return scoreValue.getValue(Type.DOUBLE); | ||
| } catch (IllegalArgumentException e) { | ||
| LOG.warn("Failed to get jcr:score for path={}", row.getPath(), e); |
There was a problem hiding this comment.
Okey, I can change it. Two questions:
- Should I reduce it to an Info/Debug log (in my understanding this could happen quite often)
- Is path always given, or can it also be null (I don't want to introduce a new NPE when fixing one 😅)
There was a problem hiding this comment.
If it fails quite often, then we should probably change the code so that it first checks if the column is available, in order to not throw an exception in the first place.
| if (scoreValue == null) { | ||
| return 0.0; | ||
| } | ||
| return scoreValue.getValue(Type.DOUBLE); |
There was a problem hiding this comment.
I wonder if we should also check the type... maybe there are nodes with a "jcr:score" property but it's text? Probably not, but I guess we should be conservative.
2 participants
Fixes the NullPointerException during UnionQuery sorting by the JCR score. The problem occurs if there is a score column, but accessing a score of that column returns Null. This can happen if, for example, there is a union between three queries, of which at least one has scores, and at least one doesn't have, and those two results are merged first.
Issue: https://issues.apache.org/jira/browse/OAK-12051
New behavior: If null values occur in such a column, we replace it with a zero. The warning from the illegalArgumentException was removed, as returning 0 occurs frequently and is nothing to worry about.
Original PR: #2687
Moved to merging from trunk to new branch to run evaluation