Repository navigation
Conversation
…itched to List.of Collections.singletonList accepted the null from createField(). List.of does not, so the whole document was rejected.
|
Automated PR triage (LLM-generated; may be wrong). Checked once, when the PR was opened or marked ready; not updated on later pushes. No clear mismatches in the answered judgements.
|
|
I checked the other createFields() overrides that return List.of(createField()). BoolField, BinaryField, SortableTextField, CollationField, and ICUCollationField have the same shape, but only when indexed, stored, and docValues are all false. The ignored field type is a StrField, and that is the reported case, so this PR only changes StrField. |
735f5d9 to
00292cd
Compare
| fval = docval; | ||
| } | ||
| return List.of(fval); | ||
| return fval == null ? List.of() : List.of(fval); |
There was a problem hiding this comment.
that sucks that we have to do this check! I guess technically if fval is null, then the List.of is return a list with a single value, null instead of an empty list?
Out of curiosity, are we doing something wrong that we trigger this pattern? I'm wondering if our coding style conflicts with how Java thinks we should do things... Or is this just totally normal java code? If it isn't totally normal java code, I wonder if there are other places we follow this pattern that are latent bugs?
Lastly, if return List.of(fval); is bad form, is there a errorprone or other checker rule that we could enable?
There was a problem hiding this comment.
FieldType.createFields already does f == null ? List.of() : List.of(f), and SOLR-18164 kept that check. createField() returns null when the field is neither indexed nor stored, and List.of(null) throws, so StrField now uses the same check. I don't have a better idea for now.
I just saw this. So, are you saying that BoolField etc all have the same problem? |
Yes. Those overrides call List.of(createField()) with no null check. They throw only when indexed, stored, and docValues are all false. The ignored type is a StrField, which is the case this PR fixes. |
|
LGTM. |
Thanks, Nick! I’d be happy to let you handle the follow-up PR for the other field types. Thanks for preparing the branch and looking into this! |
|
Humm.. So, do the other field types have the same issue? If so, feels like this one PR should solve them unless it's a huge code change? whats the advantage to having multiple PRs? If the same problem affects multipole fields, then I'd rather review and test one PR... |
|
@linslee75, I’ve prepared the sibling fixes and tests on top of your current PR head. You’re welcome to take them into this PR: From your PR branch, you can bring in the commit with: The added tests cover ignored fields and include docValues-only checks. Linux verification of the revised commit is pending. |
|
Agreed we should all occurrences of the bug in this PR. Nick; thanks for the additions, although I feel the LOC added for the tests isn't warranted. |
One question about the changes that delegate to super.createFields(). Before Solr 10.1, these field types had their own createFields() implementations, including the DocValues handling. Since your changes move some of this logic to the parent implementation, do you think we need focused tests to verify that the existing behavior is preserved, particularly for DocValues and multi-valued fields? I just want to make sure this change does not introduce unintended behavior differences. |
|
The other field types are now covered in this PR. Each createFields() override uses the same null check as FieldType.createFields, and testIgnoredFields writes a document for each of them. Thanks, @nick-boss-tech , for finding those field types and preparing a branch. |
|
Thanks, it looks good to me. |
https://issues.apache.org/jira/browse/SOLR-18627
Dev list: subject "ignored fields throw NPE since Solr 10.1"
Description
Writing a document that contains an ignored field works on Solr 10.0 and throws a NullPointerException on 10.1. The whole update is rejected with 400, not just the ignored field.
The demo config has dynamicField ignored_* of type ignored:
Solr 10.0 returns status 0. Solr 10.1 returns status 400.
This was reported on users@:
https://lists.apache.org/thread.html/28pqz7gkvr3tb3mrj7f9npvs8f6slqq5
David Smiley confirmed on dev@ that this is not the expected behavior, and that a test is missing.
Cause
ignored is a StrField that is not indexed, not stored, and has no docValues. FieldType.createField() returns null for that case, and DocumentBuilder already skips null fields.
StrField.createFields() used to return Collections.singletonList(fval), which accepts null. Since 10.1 it returns List.of(fval), which does not. The NPE is thrown before the field is dropped.
Fix
Return an empty list when fval is null, matching FieldType.createFields().
Tests
BasicFunctionalityTest.testIgnoredFields: verifies that a document containing an ignored StrField can be indexed and queried successfully.
AI disclosure: I used an AI coding assistant for the investigation and this description. I reviewed and verified the change.