Fix for broken mssql-jdbc.properties location logic
#2579
Conversation
…utOfBoundsExcelption
Jeffery-Wasty
requested review from
lilgreenbird,
divang,
Ananya2,
tkyc,
muskan124947 and
machavan
| @@ -283,16 +282,25 @@ private static void createConnectionRules(LinkedList<String> listOfRules) throws | |||
| private static String getCurrentClassPath() throws SQLServerException { | |||
| String location = ""; | |||
| String className = ""; | |||
| String locationSuffix = "target/classes/"; | |||
There was a problem hiding this comment.
What about C:\target\classes\mssql-jdbc.jar + C:\target\classes\mssql-jdbc.properties?. Unlikely scenario I suppose.
There was a problem hiding this comment.
Unlikely scenario that I'm not sure how we could address. In that case, they would not be able to use CRL with the props file. They would have to use the connection string property instead.
| className = new Object() {}.getClass().getEnclosingClass().getName(); | ||
| location = Class.forName(className).getProtectionDomain().getCodeSource().getLocation().getPath(); |
There was a problem hiding this comment.
If I do a java -jar mssql-jdbc.jar with the above code, the location always includes the JAR name eg. /path/to/mssql-jdbc.jar. But, when I run it in my IDE I get /path/to/. Without the JAR name in the path.
Do you need to account for the cases mentioned?
There was a problem hiding this comment.
Class.forName(className).getProtectionDomain().getCodeSource().getLocation().getPath(); will return the /path/to example you showed above, i.e. where the jar is without including the jar. This is the scenario I tested before pushing this PR.
| // the above will return with a suffix "target/classes/" which must be removed, as, with the first case, the | ||
| // properties file should be in the main directory. | ||
|
|
||
| if (location.endsWith(locationSuffix)) { |
There was a problem hiding this comment.
Do we need to handle this case of testing/building the driver?
Would it be fair to expect developers to place the the mssql-jdbc.properties file under <path>/tagret/classes themselves while tresting/debugging etc. ?
There was a problem hiding this comment.
IDEs may normally put things there from resources subdirectory, so probably yes?
There was a problem hiding this comment.
It's not that the properties file is in target/classes. It should be in the main directory, and the documentation says this (or it will when we update it for 12.10.0). Class.forName(className).getProtectionDomain().getCodeSource().getLocation().getPath(); returns the correct main directory when the driver is ran as a dependency or as a jar, when its ran while testing (i.e. running the source code for tests), the above snippet instead returns <main_directory>/target/classes which must be "corrected" to the main directory when the props file should be.
We can avoid this whole issue by only expected the props file in the main directory OR next to the jar, however this would mean we wouldn't be able to run internal testing for CRL using a props file. Also, since we're thinking of perhaps using the props file in the future for other connection properties, this would only be more of a limitation in the future.
Finally, I think this will be resolved when we institute custom locations for the props file as another connection string property (in our backlog). Where we only have to deal with "unusual" paths if they are specified in the prop.
The logic for finding the
mssql-jdbc.propertiesfile, used in Configurable Retry Logic was correct for test environments, and when building the driver manually, but not correct when using the driver as a jar or dependency. This fixes that issue by only removing thetarget/classessuffix if needed.