[
https://issues.apache.org/jira/browse/SOLR-10235?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15899806#comment-15899806
]
Uwe Schindler edited comment on SOLR-10235 at 3/7/17 5:31 PM:
--------------------------------------------------------------
bq. 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).
bq. 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.
bq. 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.
was (Author: thetaphi):
bq. 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).
bq. 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 revert that. The original code used driver so I kept that. The
problem here was not the class name, I analyzed Java 9. 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 calls the methods in same order, which is undefined in the spec. Because
of that you have to implement a "real" driver, a mock is not enough. Thois
"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 a driver.
bq. 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.
> fix last TestJdbcDataSource / mock issue with java9
> ---------------------------------------------------
>
> Key: SOLR-10235
> URL: https://issues.apache.org/jira/browse/SOLR-10235
> Project: Solr
> Issue Type: Bug
> Security Level: Public(Default Security Level. Issues are Public)
> Reporter: Hoss Man
> Labels: java9
> Attachments: SOLR-10235.patch, SOLR-10235.patch, SOLR-10235.patch,
> SOLR-10235.patch
>
>
> 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.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
