Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-10235

fix last TestJdbcDataSource / mock issue with java9

    Details

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

      Description

      The way TestJdbcDataSource was converted to use Mockito in SOLR-9966 still left one outstanding test that was incompatible with Java9: testRetrieveFromDriverManager()

      The way this test worked with mock classes was also sketchy, but under java9 (even with Mockito) the attempt at using class names to resolve things in the standard SQL DriverManager isn't viable.

      It seems like any easy fix is to create real class (with a real/fixed classname) that acts as a wrapper around a mockito "Driver" instance just for the purposes of checking that the DriverManaer related code is working properly.

      1. SOLR-10235.patch
        5 kB
        Uwe Schindler
      2. SOLR-10235.patch
        4 kB
        Uwe Schindler
      3. SOLR-10235.patch
        5 kB
        Uwe Schindler
      4. SOLR-10235.patch
        5 kB
        Uwe Schindler
      5. SOLR-10235.patch
        4 kB
        Hoss Man

        Issue Links

          Activity

          Hide
          hossman Hoss Man added a comment -

          Attaching a patch showing what i had in mind.

          seems to work fine with both java8 & java9.

          Uwe Schindler & Cao Manh Dat: do you guys see any problems with this type of approach?

          Show
          hossman Hoss Man added a comment - Attaching a patch showing what i had in mind. seems to work fine with both java8 & java9. Uwe Schindler & Cao Manh Dat : do you guys see any problems with this type of approach?
          Hide
          thetaphi Uwe Schindler added a comment -

          Hi Hoss Man,
          thanks for converting it, but there are 2 issues: getPropertyInfo(String url, Properties info) and jdbcCompliant() miss to delegate to wrapper; they call themselves leading to stack overflow. It looks like they are not called, but if the test gets extended it may overflow.

          Otherwise, I'd make the Mock setup in the constructor of the wrapper. With the current code, if the JDBC driver would actually create a new instance of the wrapper based on the class name, the driver would not have the recorded behaviour. To do this reove "static" from the inner class. getMockitoDriver() is then also obsolete.

          Finally, add @Override.

          Show
          thetaphi Uwe Schindler added a comment - Hi Hoss Man , thanks for converting it, but there are 2 issues: getPropertyInfo(String url, Properties info) and jdbcCompliant() miss to delegate to wrapper; they call themselves leading to stack overflow. It looks like they are not called, but if the test gets extended it may overflow. Otherwise, I'd make the Mock setup in the constructor of the wrapper. With the current code, if the JDBC driver would actually create a new instance of the wrapper based on the class name, the driver would not have the recorded behaviour. To do this reove "static" from the inner class. getMockitoDriver() is then also obsolete. Finally, add @Override .
          Hide
          thetaphi Uwe Schindler added a comment -

          Here is a fixed patch. Actually I now understand the problem: JdbcDataSource tries to instantiate the class and create an instance if the driver is not already registered. But as the driver is registered manually before usage in our code, we don't want to have this. Because of this I added an empty ctor to disallow this. For "registering" the driver, we pass our mock connection to the driver and it passes.

          Show
          thetaphi Uwe Schindler added a comment - Here is a fixed patch. Actually I now understand the problem: JdbcDataSource tries to instantiate the class and create an instance if the driver is not already registered. But as the driver is registered manually before usage in our code, we don't want to have this. Because of this I added an empty ctor to disallow this. For "registering" the driver, we pass our mock connection to the driver and it passes.
          Hide
          thetaphi Uwe Schindler added a comment -

          In addition, I made the Driver class public, otherwise it's broken according to JDBC standards (Class.forName won't work).

          Show
          thetaphi Uwe Schindler added a comment - In addition, I made the Driver class public, otherwise it's broken according to JDBC standards (Class.forName won't work).
          Hide
          thetaphi Uwe Schindler added a comment -

          I changed the Driver a bit to only work on its "supported URL" (a ststic final String). This ensure that we do not pollute the JVM with a driver that "owns" every URL.

          Show
          thetaphi Uwe Schindler added a comment - I changed the Driver a bit to only work on its "supported URL" (a ststic final String). This ensure that we do not pollute the JVM with a driver that "owns" every URL.
          Hide
          thetaphi Uwe Schindler added a comment -

          Sorry for posting so many patches. I reimplemented the Driver interface without mocks. This ensures that it will also work with later Java versions. It will return a version number and properties (if the DriverManager requires that metainformation), but return the mocked Connection instance, if the URL matches.

          I think that's ready now.

          Show
          thetaphi Uwe Schindler added a comment - Sorry for posting so many patches. I reimplemented the Driver interface without mocks. This ensures that it will also work with later Java versions. It will return a version number and properties (if the DriverManager requires that metainformation), but return the mocked Connection instance, if the URL matches. I think that's ready now.
          Hide
          hossman Hoss Man added a comment -

          Uwe: i like this approach, three nits...

          1) the javadocs for MockDriver should explain the purpose of this class given how much of the rest of the outer class uses Mockito.

          2) I don't like that you're shadowing the variable named driver ... at first glance this could confuse people skimming code who have already seen the class level driver variable ... better to use mockDriver or fixedClassDriver or something like that.

          3) rather then returning null, I suggest MockDriver.connect(...) throw a new SQLException("attempted to use this driver with bogus url") if acceptsURL(...) is false – so if the day comes when someone breaks the code, they'll have a decent error msg to identify the bug instead of an NPE.

          Show
          hossman Hoss Man added a comment - Uwe: i like this approach, three nits... 1) the javadocs for MockDriver should explain the purpose of this class given how much of the rest of the outer class uses Mockito. 2) I don't like that you're shadowing the variable named driver ... at first glance this could confuse people skimming code who have already seen the class level driver variable ... better to use mockDriver or fixedClassDriver or something like that. 3) rather then returning null, I suggest MockDriver.connect(...) throw a new SQLException("attempted to use this driver with bogus url") if acceptsURL(...) is false – so if the day comes when someone breaks the code, they'll have a decent error msg to identify the bug instead of an NPE.
          Hide
          thetaphi Uwe Schindler added a comment - - edited

          1) the javadocs for MockDriver should explain the purpose of this class given how much of the rest of the outer class uses Mockito.

          The Javadocs already say that this is a simple Driver rimplementation, that returns a mock connection for any url that equals MY_JDBC_URL. I can add a note to the Javadocs why it is not a mock, but the purpose of this class is identical to the other "mock" classes in this package (there is a mock JNDI Data Source that behaves similar).

          2) I don't like that you're shadowing the variable named driver ... at first glance this could confuse people skimming code who have already seen the class level driver variable ... better to use mockDriver or fixedClassDriver or something like that.

          oh sorry, will fix that. The original code used driver so I kept that. FYI: The problem with Java 9 was not the class name! I analyzed Java 9's DriverManager: The problem was caused by another workflow used internally when trying to find the right driver. If you use a "Mock" Driver implementation (as Mockito class), it requires that JDBC runtime calls the methods in same order, which is not defined in the spec. Because of that you have to implement a "real" driver allowing several ways to get a connection, a mock is not enough. This "real" driver must be prepared that the runtime may call methods in different order or suddenly use different methods like "acceptURL" for the purpose of looking up the correct driver. It must also interact in a sane way with other drivers registered in the runtime. As the driver is JVM-Global, a mock is too risky.

          3) rather then returning null, I suggest MockDriver.connect(...) throw a new SQLException("attempted to use this driver with bogus url") if acceptsURL(...) is false – so if the day comes when someone breaks the code, they'll have a decent error msg to identify the bug instead of an NPE.

          I disagree with that, sorry. As there can be more than one driver installed in the system, DriverManager will ask all of them one after each other until one of them does not return null. The NPE is according to JDBC spec: it tells DriverManager to use next driver (RTFM). If all Drivers return null, DriverManager will finally say: "invalid URL". For historical reasons, DriverManager will not use the acceptsURL(), because older JDBC drivers may not implement that method and only return null.

          Show
          thetaphi Uwe Schindler added a comment - - edited 1) the javadocs for MockDriver should explain the purpose of this class given how much of the rest of the outer class uses Mockito. The Javadocs already say that this is a simple Driver rimplementation, that returns a mock connection for any url that equals MY_JDBC_URL. I can add a note to the Javadocs why it is not a mock, but the purpose of this class is identical to the other "mock" classes in this package (there is a mock JNDI Data Source that behaves similar). 2) I don't like that you're shadowing the variable named driver ... at first glance this could confuse people skimming code who have already seen the class level driver variable ... better to use mockDriver or fixedClassDriver or something like that. oh sorry, will fix that. The original code used driver so I kept that. FYI: The problem with Java 9 was not the class name! I analyzed Java 9's DriverManager: The problem was caused by another workflow used internally when trying to find the right driver. If you use a "Mock" Driver implementation (as Mockito class), it requires that JDBC runtime calls the methods in same order, which is not defined in the spec. Because of that you have to implement a "real" driver allowing several ways to get a connection, a mock is not enough. This "real" driver must be prepared that the runtime may call methods in different order or suddenly use different methods like "acceptURL" for the purpose of looking up the correct driver. It must also interact in a sane way with other drivers registered in the runtime. As the driver is JVM-Global, a mock is too risky. 3) rather then returning null, I suggest MockDriver.connect(...) throw a new SQLException("attempted to use this driver with bogus url") if acceptsURL(...) is false – so if the day comes when someone breaks the code, they'll have a decent error msg to identify the bug instead of an NPE. I disagree with that, sorry. As there can be more than one driver installed in the system, DriverManager will ask all of them one after each other until one of them does not return null. The NPE is according to JDBC spec: it tells DriverManager to use next driver (RTFM). If all Drivers return null, DriverManager will finally say: "invalid URL". For historical reasons, DriverManager will not use the acceptsURL() , because older JDBC drivers may not implement that method and only return null.
          Hide
          hossman Hoss Man added a comment -

          I can add a note to the Javadocs why it is not a mock, but the purpose of this class is identical to the other "mock" classes in this package (there is a mock JNDI Data Source that behaves similar).

          My point is that in this test class (TestJdbcDataSource) the new MockDriver class is the only "mock" class we explicitly define - all other code in this test uses Mockito based mocks, so it would be easy for someone down the road to look at this class and think "this MockDriver class looks like cruft we can replace with another one line Mockito call" ... basically i'm just suggesting that virtually the same verbage from the testRetrieveFromDriverManager method should be in MockDriver's javadocs.

          I disagree with that, sorry. As there can be more than one driver installed in the system, ...

          AUGH! ... yoy are completely correct, i wasn't thinking holisticly about the DriverManager as a whole ... agreed, ignore that suggestion completley.

          Show
          hossman Hoss Man added a comment - I can add a note to the Javadocs why it is not a mock, but the purpose of this class is identical to the other "mock" classes in this package (there is a mock JNDI Data Source that behaves similar). My point is that in this test class (TestJdbcDataSource) the new MockDriver class is the only "mock" class we explicitly define - all other code in this test uses Mockito based mocks, so it would be easy for someone down the road to look at this class and think "this MockDriver class looks like cruft we can replace with another one line Mockito call" ... basically i'm just suggesting that virtually the same verbage from the testRetrieveFromDriverManager method should be in MockDriver 's javadocs. I disagree with that, sorry. As there can be more than one driver installed in the system, ... AUGH! ... yoy are completely correct, i wasn't thinking holisticly about the DriverManager as a whole ... agreed, ignore that suggestion completley.
          Hide
          thetaphi Uwe Schindler added a comment -

          New patch.

          Show
          thetaphi Uwe Schindler added a comment - New patch.
          Hide
          hossman Hoss Man added a comment -

          +1

          Show
          hossman Hoss Man added a comment - +1
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 0d2c027857bfca3486399b0e6b19a5887081287a in lucene-solr's branch refs/heads/master from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=0d2c027 ]

          SOLR-10235: Fix DIH's TestJdbcDataSource to work with Java 9 and other Java runtimes that do not use the same DriverManager implementation like Oracle's original one

          Show
          jira-bot ASF subversion and git services added a comment - Commit 0d2c027857bfca3486399b0e6b19a5887081287a in lucene-solr's branch refs/heads/master from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=0d2c027 ] SOLR-10235 : Fix DIH's TestJdbcDataSource to work with Java 9 and other Java runtimes that do not use the same DriverManager implementation like Oracle's original one
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit d64920b5499ec1b95352dc520cb90d24f11c667d in lucene-solr's branch refs/heads/branch_6x from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d64920b ]

          SOLR-10235: Fix DIH's TestJdbcDataSource to work with Java 9 and other Java runtimes that do not use the same DriverManager implementation like Oracle's original one

          Show
          jira-bot ASF subversion and git services added a comment - Commit d64920b5499ec1b95352dc520cb90d24f11c667d in lucene-solr's branch refs/heads/branch_6x from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d64920b ] SOLR-10235 : Fix DIH's TestJdbcDataSource to work with Java 9 and other Java runtimes that do not use the same DriverManager implementation like Oracle's original one
          Hide
          mkhludnev Mikhail Khludnev added a comment -

          I have this commit locally. precommit just failed to me

          -check-forbidden-all:
          [forbidden-apis] Reading bundled API signatures: jdk-unsafe-1.8
          [forbidden-apis] Reading bundled API signatures: jdk-deprecated-1.8
          [forbidden-apis] Reading bundled API signatures: jdk-non-portable
          [forbidden-apis] Reading bundled API signatures: jdk-reflection
          [forbidden-apis] Reading bundled API signatures: commons-io-unsafe-2.5
          [forbidden-apis] Reading API signatures: /Users/khludnevm/lucene-solr/lucene/tools/forbiddenApis/base.txt
          [forbidden-apis] Reading API signatures: /Users/khludnevm/lucene-solr/lucene/tools/forbiddenApis/servlet-api.txt
          [forbidden-apis] Reading API signatures: /Users/khludnevm/lucene-solr/lucene/tools/forbiddenApis/solr.txt
          [forbidden-apis] Loading classes to check...
          [forbidden-apis] Scanning classes for violations...
          [forbidden-apis] Forbidden class/interface use: java.util.logging.Logger [Use slf4j classes instead]
          [forbidden-apis] in org.apache.solr.handler.dataimport.TestJdbcDataSource$MockDriver (TestJdbcDataSource.java, method declaration of 'getParentLogger()')

          beware

          Show
          mkhludnev Mikhail Khludnev added a comment - I have this commit locally. precommit just failed to me -check-forbidden-all: [forbidden-apis] Reading bundled API signatures: jdk-unsafe-1.8 [forbidden-apis] Reading bundled API signatures: jdk-deprecated-1.8 [forbidden-apis] Reading bundled API signatures: jdk-non-portable [forbidden-apis] Reading bundled API signatures: jdk-reflection [forbidden-apis] Reading bundled API signatures: commons-io-unsafe-2.5 [forbidden-apis] Reading API signatures: /Users/khludnevm/lucene-solr/lucene/tools/forbiddenApis/base.txt [forbidden-apis] Reading API signatures: /Users/khludnevm/lucene-solr/lucene/tools/forbiddenApis/servlet-api.txt [forbidden-apis] Reading API signatures: /Users/khludnevm/lucene-solr/lucene/tools/forbiddenApis/solr.txt [forbidden-apis] Loading classes to check... [forbidden-apis] Scanning classes for violations... [forbidden-apis] Forbidden class/interface use: java.util.logging.Logger [Use slf4j classes instead] [forbidden-apis] in org.apache.solr.handler.dataimport.TestJdbcDataSource$MockDriver (TestJdbcDataSource.java, method declaration of 'getParentLogger()') beware
          Hide
          risdenk Kevin Risden added a comment -

          I haven't looked too closely at this patch, but for the DriverImpl for SolrJ JDBC we had to add a suppress annotation.

          https://github.com/apache/lucene-solr/blob/master/solr/solrj/src/java/org/apache/solr/client/solrj/io/sql/DriverImpl.java#L97

            @SuppressForbidden(reason="Required by jdbc")
            public Logger getParentLogger() {
              return null;
            }
          
          Show
          risdenk Kevin Risden added a comment - I haven't looked too closely at this patch, but for the DriverImpl for SolrJ JDBC we had to add a suppress annotation. https://github.com/apache/lucene-solr/blob/master/solr/solrj/src/java/org/apache/solr/client/solrj/io/sql/DriverImpl.java#L97 @SuppressForbidden(reason= "Required by jdbc" ) public Logger getParentLogger() { return null ; }
          Hide
          thetaphi Uwe Schindler added a comment -

          Oh yeah. Sorry, I'll fix.

          Show
          thetaphi Uwe Schindler added a comment - Oh yeah. Sorry, I'll fix.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit d8442070cfbb0cec815a465a6c143e2b31870e34 in lucene-solr's branch refs/heads/master from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d844207 ]

          SOLR-10235: fix precommit

          Show
          jira-bot ASF subversion and git services added a comment - Commit d8442070cfbb0cec815a465a6c143e2b31870e34 in lucene-solr's branch refs/heads/master from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d844207 ] SOLR-10235 : fix precommit
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 2cc1c0ed29f696bf2ab5e72239f328c290c9101c in lucene-solr's branch refs/heads/branch_6x from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=2cc1c0e ]

          SOLR-10235: fix precommit

          Show
          jira-bot ASF subversion and git services added a comment - Commit 2cc1c0ed29f696bf2ab5e72239f328c290c9101c in lucene-solr's branch refs/heads/branch_6x from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=2cc1c0e ] SOLR-10235 : fix precommit

            People

            • Assignee:
              thetaphi Uwe Schindler
              Reporter:
              hossman Hoss Man
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development