Details

    • Type: Bug
    • Status: Resolved
    • Priority: Trivial
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.4, 7.0
    • Component/s: None
    • Security Level: Public (Default Security Level. Issues are Public)
    • Labels:
      None

      Description

      In recent code snapshot (Github mirror's commit id: c8542b2bd0470af9f8d64bb8133f31828b342604 as today), there is an illogical condition that can be a code smell or a potential bug:

      ReversedWildcardFilterFactory fac = leadingWildcards.get(fieldType);
          if (fac != null || leadingWildcards.containsKey(fac)) {
            return fac;
          }
      

      In SOLR-3492, it said there was a fix in SOLR-4093. However, the fix still has an issue as above: containsKey will always have null in this if statement. The second condition could be unnecessary. Does leadingWildcards allow a null object as a key? If so, it will return null that might cause NPE in some other locations.

      Patch could be just like in SOLR-3492?:

      if (fac != null)
      
      1. SOLR-9878.patch
        4 kB
        Mikhail Khludnev

        Issue Links

          Activity

          Hide
          tomasflobbe Tomás Fernández Löbbe added a comment -

          Yes, that doesn't look right. Also, leadingWildcards is defined as:

          private Map<FieldType, ReversedWildcardFilterFactory> leadingWildcards;
          

          there is no way to have a ReversedWildcardFilterFactory as key

          Show
          tomasflobbe Tomás Fernández Löbbe added a comment - Yes, that doesn't look right. Also, leadingWildcards is defined as: private Map<FieldType, ReversedWildcardFilterFactory> leadingWildcards; there is no way to have a ReversedWildcardFilterFactory as key
          Hide
          mkhludnev Mikhail Khludnev added a comment -

          what about SOLR-9878.patch? this fixes caching of nulls.

          Show
          mkhludnev Mikhail Khludnev added a comment - what about SOLR-9878.patch ? this fixes caching of nulls.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit f1e636f5611abea66efed896a95eebbb6d765300 in lucene-solr's branch refs/heads/master from Mikhail Khludnev
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=f1e636f ]

          SOLR-9878: fix ReversedWildcardFilterFactory caching in query parser

          Show
          jira-bot ASF subversion and git services added a comment - Commit f1e636f5611abea66efed896a95eebbb6d765300 in lucene-solr's branch refs/heads/master from Mikhail Khludnev [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=f1e636f ] SOLR-9878 : fix ReversedWildcardFilterFactory caching in query parser
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 1e22dd25bf20844c71648c39028c48d2a9aa4e58 in lucene-solr's branch refs/heads/branch_6x from Mikhail Khludnev
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=1e22dd2 ]

          SOLR-9878: fix ReversedWildcardFilterFactory caching in query parser

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1e22dd25bf20844c71648c39028c48d2a9aa4e58 in lucene-solr's branch refs/heads/branch_6x from Mikhail Khludnev [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=1e22dd2 ] SOLR-9878 : fix ReversedWildcardFilterFactory caching in query parser
          Hide
          lifove JC added a comment -

          I'm wondering why the second condition, "|| leadingWildcards.containsKey(fieldType)" still required.
          The second condition can be reached only when fac is null. Can leadingWildcards.get(fieldType) have null when leadingWildcards.containsKey(fieldType) is true? If yes, this method will return null in this case. (Is this intended?) Since I don't have domain knowledge about this project, I just wanted to confirm this. If no, the second condition might not be necessary since when fac is null, leadingWildcards.containsKey(fieldType) is always false. It's quite confusing.

          Show
          lifove JC added a comment - I'm wondering why the second condition, "|| leadingWildcards.containsKey(fieldType)" still required. The second condition can be reached only when fac is null. Can leadingWildcards.get(fieldType) have null when leadingWildcards.containsKey(fieldType) is true? If yes, this method will return null in this case. (Is this intended?) Since I don't have domain knowledge about this project, I just wanted to confirm this. If no, the second condition might not be necessary since when fac is null, leadingWildcards.containsKey(fieldType) is always false. It's quite confusing.
          Hide
          mkhludnev Mikhail Khludnev added a comment -

          when a fieldType doesn't have RWFF it's cached as null value and isn't checked anymore. I agree it's not obvious and probably over-engineered, but I decided to fix as is. You can check the test which demonstrate this invariant.
          Thanks for reporting.

          Show
          mkhludnev Mikhail Khludnev added a comment - when a fieldType doesn't have RWFF it's cached as null value and isn't checked anymore. I agree it's not obvious and probably over-engineered, but I decided to fix as is. You can check the test which demonstrate this invariant. Thanks for reporting.

            People

            • Assignee:
              Unassigned
              Reporter:
              lifove JC
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development