Solr
  1. Solr
  2. SOLR-5526

Query parser extends standard cause NPE on Solr startup

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 4.5.1, 4.6, 6.0
    • Fix Version/s: 4.7, 6.0
    • Component/s: query parsers
    • Labels:
      None
    • Environment:

      Linux, Java 1.7.0_45

      Description

      Adding any custom query parser extending standard one with non-final field NAME lead to messy NullPointerException during Solr startup.

      Definition of standard parsers is located in QParserPlugin.standardPlugins static array. The array contains names from static NAME fields and classes for each plugin.

      But all of listed parsers are derived from QParserPlugin, so we have circular dependency of static fields.

      Normally, class loader start initializing QParserPlugin before all listed plugins in SolrCore.initQParsers, and array elements referenced to NAME plugins' fields are filled properly.

      Custom parsers are instantiated before standard parsers. And when we subclass plugin with non-final NAME field and add it to Solr via solrconfig.xml, class loader start loading our class before QParserPlugin. Because QParserPlugin is a superclass for plugin, it must be initialized before subclasses, and static dereferencing cause null elements in standardPlugins array because it filled before NAME field of loading plugin's superclass.

      How to reproduce:

      1. Checkout Solr (trunk or stable)
      2. Add the following line to solr/example/solr/collection1/conf/solrconfig.xml
        <queryParser name="fail" class="solr.search.LuceneQParserPlugin"/>
      3. Call ant run-example in solr folder

      Possible workarounds:

      • dev-workaround: add int workaround = QParserPlugin.standardPlugins.length; as a first line to
        SolrCore.initQParsers
      • user-workaround: add plugin with final NAME field (edismax) to solrconfig.xml before subclasses of standard plugins.
        <queryParser name="workaround" class="solr.search.ExtendedDismaxQParserPlugin"/>

      Possible fix:
      Move standardPlugins to new final class to break circular dependency.

      1. NPE_load_trace
        3 kB
        Vitaliy Zhovtyuk
      2. SOLR-5526.patch
        23 kB
        Hoss Man
      3. SOLR-5526.patch
        23 kB
        Hoss Man
      4. SOLR-5526.patch
        22 kB
        Vitaliy Zhovtyuk
      5. SOLR-5526.patch
        20 kB
        Vitaliy Zhovtyuk
      6. SOLR-5526.patch
        9 kB
        Nikolay Khitrin
      7. SOLR-5526-final-names.patch
        12 kB
        Vitaliy Zhovtyuk
      8. SOLR-5526-final-names.patch
        13 kB
        Nikolay Khitrin
      9. SOLR-5526-tests.patch
        7 kB
        Vitaliy Zhovtyuk

        Issue Links

          Activity

          Hide
          Nikolay Khitrin added a comment -

          I've attached patch where all static variables from QParserPlugin were moved to inner static final class QParserPlugin.Defaults.

          Show
          Nikolay Khitrin added a comment - I've attached patch where all static variables from QParserPlugin were moved to inner static final class QParserPlugin.Defaults .
          Hide
          Hoss Man added a comment -

          Ouch!

          Nikolay: Nice diagnoses! thanks for digging into this.

          Maybe i'm missing something, but isn't the root level bug really that the standard parsers should all be declaring the static NAME as final? ... even with your proposed change, it seems dangerous to me that those NAME variables are mutable.

          If we fix them all to be "final" then the problem goes away w/o needing the rest of your patch, correct?

          We should also be able to whip up a simple test using reflection to fail the build if anyone ever adds a class to QParserPlugin.standardPlugins w/o static final NAME.

          You mentioned discovering this problem because you have custom query parsers extending some standard one where NAME isn't final –
          Is there an aspect of this bug that you're seeing with your custom parser that wouldn't be fixed with the solution i'm suggesting?

          Show
          Hoss Man added a comment - Ouch! Nikolay: Nice diagnoses! thanks for digging into this. Maybe i'm missing something, but isn't the root level bug really that the standard parsers should all be declaring the static NAME as final? ... even with your proposed change, it seems dangerous to me that those NAME variables are mutable. If we fix them all to be "final" then the problem goes away w/o needing the rest of your patch, correct? We should also be able to whip up a simple test using reflection to fail the build if anyone ever adds a class to QParserPlugin.standardPlugins w/o static final NAME. You mentioned discovering this problem because you have custom query parsers extending some standard one where NAME isn't final – Is there an aspect of this bug that you're seeing with your custom parser that wouldn't be fixed with the solution i'm suggesting?
          Hide
          Nikolay Khitrin added a comment -

          I think making all NAMEs final will fix the issue.

          There is extremely artificial case with non compile time constant final NAME field...
          (How to reproduce:
          on edismax parser: public static final String NAME = QParserPlugin.DEFAULT_QTYPE + "edismax";
          solrconfig.xml: <queryParser name="fail" class="solr.search.ExtendedDismaxQParserPlugin"/> )
          But I absolutely cannot imagine this in real life.

          So I propose to apply your suggestion as a hotfix and do calmly think about plugins list structure and location.
          Will it be located in abstract supeclass field? Is it good?

          Show
          Nikolay Khitrin added a comment - I think making all NAMEs final will fix the issue. There is extremely artificial case with non compile time constant final NAME field... (How to reproduce: on edismax parser: public static final String NAME = QParserPlugin.DEFAULT_QTYPE + "edismax"; solrconfig.xml: <queryParser name="fail" class="solr.search.ExtendedDismaxQParserPlugin"/> ) But I absolutely cannot imagine this in real life. So I propose to apply your suggestion as a hotfix and do calmly think about plugins list structure and location. Will it be located in abstract supeclass field? Is it good?
          Hide
          Nikolay Khitrin added a comment -

          Attached patch adding final modifiers to all names for standard parsers, as mentioned in your suggestion.

          Show
          Nikolay Khitrin added a comment - Attached patch adding final modifiers to all names for standard parsers, as mentioned in your suggestion.
          Hide
          Vitaliy Zhovtyuk added a comment -

          NPE stacktrace during solr load

          Show
          Vitaliy Zhovtyuk added a comment - NPE stacktrace during solr load
          Hide
          Vitaliy Zhovtyuk added a comment -

          Missed final NAME field in org.apache.solr.search.SimpleQParserPlugin. Fixed.

          Show
          Vitaliy Zhovtyuk added a comment - Missed final NAME field in org.apache.solr.search.SimpleQParserPlugin. Fixed.
          Hide
          Vitaliy Zhovtyuk added a comment -

          Test reproducing NPE on Solr start-up
          Test checking final and static NAME field for all standard parsers

          Show
          Vitaliy Zhovtyuk added a comment - Test reproducing NPE on Solr start-up Test checking final and static NAME field for all standard parsers
          Hide
          Shalin Shekhar Mangar added a comment -

          Thanks Vitaliy. Can you please combine the two patches i.e. the test and making the NAME fields final?

          Just a tip for the future: Just have the same names for each iteration of the patch. Jira will automatically grey out old patches and keep the latest one highlighted. It also helps reviewers in figuring out which is the the latest patch.

          Show
          Shalin Shekhar Mangar added a comment - Thanks Vitaliy. Can you please combine the two patches i.e. the test and making the NAME fields final? Just a tip for the future: Just have the same names for each iteration of the patch. Jira will automatically grey out old patches and keep the latest one highlighted. It also helps reviewers in figuring out which is the the latest patch.
          Hide
          Vitaliy Zhovtyuk added a comment -

          Combined:
          Missed final NAME field fixes.
          Test reproducing NPE on Solr start-up
          Test checking final and static NAME field for all standard parsers

          Show
          Vitaliy Zhovtyuk added a comment - Combined: Missed final NAME field fixes. Test reproducing NPE on Solr start-up Test checking final and static NAME field for all standard parsers
          Hide
          Hoss Man added a comment -

          Hey Vitaliy,

          For the most part, these tests look great – just a few minor things i think we should try to clean up...

          • QParserPlugin.standardPlugins's javadoc needs to point out the importance of these names being static & final so people aren't surprised by these tests when new parsers are added in the future.
          • TestStandardQParsers is doing something sufficiently odd that it really needs some javadocs explaining why it exists (ie: mention the class loading problems associated if there is a standardPlugin that has a non-static, non-final name, with an @see this issue, @see QParserPlugin.standardPlugins, etc...)
          • we should probably make TestStandardQParsers assert that the static & final name it finds in each class matches the name associated in QParserPlugin.standardPlugins.
          • solrconfig-query-parser-init.xml has a cut & paste comment referring to an unrelated test.
          • TestInitQParser should have a javadoc comment explaining what the point of the test is
          • TestInitQParser should actaully do a query using the "fail" parser registered in the config, to help future-proof us against someone unwittingly changing the test config in a way that defeats the point of the test.

          ...if you want to take a crack at this that would be great, if not i'll try to make some time later this week.

          Show
          Hoss Man added a comment - Hey Vitaliy, For the most part, these tests look great – just a few minor things i think we should try to clean up... QParserPlugin.standardPlugins's javadoc needs to point out the importance of these names being static & final so people aren't surprised by these tests when new parsers are added in the future. TestStandardQParsers is doing something sufficiently odd that it really needs some javadocs explaining why it exists (ie: mention the class loading problems associated if there is a standardPlugin that has a non-static, non-final name, with an @see this issue, @see QParserPlugin.standardPlugins , etc...) we should probably make TestStandardQParsers assert that the static & final name it finds in each class matches the name associated in QParserPlugin.standardPlugins. solrconfig-query-parser-init.xml has a cut & paste comment referring to an unrelated test. TestInitQParser should have a javadoc comment explaining what the point of the test is TestInitQParser should actaully do a query using the "fail" parser registered in the config, to help future-proof us against someone unwittingly changing the test config in a way that defeats the point of the test. ...if you want to take a crack at this that would be great, if not i'll try to make some time later this week.
          Hide
          Vitaliy Zhovtyuk added a comment -

          minor fixes:

          QParserPlugin.standardPlugins's javadoc needs to point out the importance of these names being static & final so people aren't surprised by these tests when new parsers are added in the future.

          Added relevant javadocs.


          TestStandardQParsers is doing something sufficiently odd that it really needs some javadocs explaining why it exists (ie: mention the class loading problems associated if there is a standardPlugin that has a non-static, non-final name, with an @see this issue, @see QParserPlugin.standardPlugins, etc...)

          Added javadocs

          we should probably make TestStandardQParsers assert that the static & final name it finds in each class matches the name associated in QParserPlugin.standardPlugins.

          Thats actually what TestStandardQParsers does. Unit test takes classes registered in QParserPlugin.standardPlugins and ensure that each class has final and static NAME field.
          Added relevant javadocs to TestStandardQParsers.

          solrconfig-query-parser-init.xml has a cut & paste comment referring to an unrelated test.

          Fixed, added relevant comments.

          TestInitQParser should have a javadoc comment explaining what the point of the test is

          Fixed, added relevant comments.

          TestInitQParser should actaully do a query using the "fail" parser registered in the config, to help future-proof us against someone unwittingly changing the test config in a way that defeats the point of the test.

          This test alerady does query using defType=fail, so i expect this registered QParser used and return result.

          Show
          Vitaliy Zhovtyuk added a comment - minor fixes: QParserPlugin.standardPlugins's javadoc needs to point out the importance of these names being static & final so people aren't surprised by these tests when new parsers are added in the future. Added relevant javadocs. TestStandardQParsers is doing something sufficiently odd that it really needs some javadocs explaining why it exists (ie: mention the class loading problems associated if there is a standardPlugin that has a non-static, non-final name, with an @see this issue, @see QParserPlugin.standardPlugins , etc...) Added javadocs we should probably make TestStandardQParsers assert that the static & final name it finds in each class matches the name associated in QParserPlugin.standardPlugins. Thats actually what TestStandardQParsers does. Unit test takes classes registered in QParserPlugin.standardPlugins and ensure that each class has final and static NAME field. Added relevant javadocs to TestStandardQParsers. solrconfig-query-parser-init.xml has a cut & paste comment referring to an unrelated test. Fixed, added relevant comments. TestInitQParser should have a javadoc comment explaining what the point of the test is Fixed, added relevant comments. TestInitQParser should actaully do a query using the "fail" parser registered in the config, to help future-proof us against someone unwittingly changing the test config in a way that defeats the point of the test. This test alerady does query using defType=fail, so i expect this registered QParser used and return result.
          Hide
          Hoss Man added a comment -

          This test alerady does query using defType=fail, so i expect this registered QParser used and return result.

          Yep yep ... i must have misread that, sorry.

          Thats actually what TestStandardQParsers does. Unit test takes classes registered in QParserPlugin.standardPlugins and ensure that each class has final and static NAME field.

          Correct, but that's not the point i was making in that bullet point – what i said was as long as we were doing that, we should also make sure the NAME field matches the registered name in QParserPlugin.standardPlugins.

          I'm attaching a patch that eliminates some existing duplication in your TestStandardQParsers and adds the type of check i was refering to ... running full tests & precommit now, and unless something weird pops up i'll commit.

          Show
          Hoss Man added a comment - This test alerady does query using defType=fail, so i expect this registered QParser used and return result. Yep yep ... i must have misread that, sorry. Thats actually what TestStandardQParsers does. Unit test takes classes registered in QParserPlugin.standardPlugins and ensure that each class has final and static NAME field. Correct, but that's not the point i was making in that bullet point – what i said was as long as we were doing that, we should also make sure the NAME field matches the registered name in QParserPlugin.standardPlugins. I'm attaching a patch that eliminates some existing duplication in your TestStandardQParsers and adds the type of check i was refering to ... running full tests & precommit now, and unless something weird pops up i'll commit.
          Hide
          Hoss Man added a comment -

          fixed a javadoc mistake caught by "ant precommit"

          Going to commit & backport now

          Show
          Hoss Man added a comment - fixed a javadoc mistake caught by "ant precommit" Going to commit & backport now
          Hide
          ASF subversion and git services added a comment -

          Commit 1564588 from hossman@apache.org in branch 'dev/trunk'
          [ https://svn.apache.org/r1564588 ]

          SOLR-5526: Fixed NPE that could arrise when explicitly configuring some built in QParserPlugins

          Show
          ASF subversion and git services added a comment - Commit 1564588 from hossman@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1564588 ] SOLR-5526 : Fixed NPE that could arrise when explicitly configuring some built in QParserPlugins
          Hide
          ASF subversion and git services added a comment -

          Commit 1564606 from hossman@apache.org in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1564606 ]

          SOLR-5526: Fixed NPE that could arrise when explicitly configuring some built in QParserPlugins (merge r1564588)

          Show
          ASF subversion and git services added a comment - Commit 1564606 from hossman@apache.org in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1564606 ] SOLR-5526 : Fixed NPE that could arrise when explicitly configuring some built in QParserPlugins (merge r1564588)
          Hide
          Hoss Man added a comment -

          Thanks Nikolay & Vitaliy!

          Show
          Hoss Man added a comment - Thanks Nikolay & Vitaliy!

            People

            • Assignee:
              Unassigned
              Reporter:
              Nikolay Khitrin
            • Votes:
              2 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development