Skip to content

Conversation

@Jeffery-Wasty
Copy link
Contributor

@Jeffery-Wasty Jeffery-Wasty commented Feb 28, 2025

When attempting to get the Path for use in Configurable Retry Logic patch check, this could fail if the scheme was not file:. This is resolved by removing the scheme component from the URI before feeding it into Paths.get(). This is done by checking if there is a scheme, and if so, removing anything before the first forward slash, including that slash,

@Jeffery-Wasty Jeffery-Wasty added this to the 12.10.0 milestone Feb 28, 2025
@github-project-automation github-project-automation bot moved this to In progress in MSSQL JDBC Feb 28, 2025
@Jeffery-Wasty Jeffery-Wasty self-assigned this Feb 28, 2025

This comment was marked as resolved.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This PR refactors the logic for obtaining a usable file path for CRL path checks by removing the URI scheme from the path before calling Paths.get(). It introduces a more robust way to handle file URIs and adds an appropriate error message for invalid paths.

  • Extracts the scheme-specific part from the URI and removes its leading "/" before verifying if the path is a directory.
  • Adds handling for InvalidPathException with a corresponding error message update in SQLServerResource.java.

Reviewed Changes

File Description
src/main/java/com/microsoft/sqlserver/jdbc/ConfigurableRetryLogic.java Modifies the retrieval of the code source URI to remove the scheme component and adjusts path handling accordingly.
src/main/java/com/microsoft/sqlserver/jdbc/SQLServerResource.java Adds an error message for invalid paths.

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

src/main/java/com/microsoft/sqlserver/jdbc/ConfigurableRetryLogic.java:296

  • Ensure that schemeSpecificPart is non-empty and starts with '/' before removing the first character to avoid unexpected behavior or exceptions.
schemeSpecificPart = schemeSpecificPart.substring(1); // Remove leading /

src/main/java/com/microsoft/sqlserver/jdbc/ConfigurableRetryLogic.java:301

  • [nitpick] Consider extracting the hardcoded string 'target/classes/' into a constant to improve maintainability and clarity.
location = location.substring(0, location.length() - ("target/classes/").length());

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This PR fixes an issue where a URI scheme interferes with fetching the file path for the CRL path check by stripping the scheme before using Paths.get().

  • Remove the URI scheme using getSchemeSpecificPart and a new constant for the forward slash.
  • Update error handling and add a new resource error message for invalid paths.

Reviewed Changes

File Description
src/main/java/com/microsoft/sqlserver/jdbc/ConfigurableRetryLogic.java Adjusts path extraction from the CodeSource URI by removing the scheme and handling directory checks
src/main/java/com/microsoft/sqlserver/jdbc/SQLServerResource.java Adds a new error message resource for invalid path exceptions

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

src/main/java/com/microsoft/sqlserver/jdbc/ConfigurableRetryLogic.java:298

  • Removing the leading '/' may yield an incorrect relative path on Unix-based systems where an absolute path is required. Consider verifying that this transformation works correctly on all target platforms.
schemeSpecificPart = schemeSpecificPart.substring(1); // Remove leading /

@codecov
Copy link

codecov bot commented Feb 28, 2025

Codecov Report

Attention: Patch coverage is 53.84615% with 6 lines in your changes missing coverage. Please review.

Project coverage is 51.64%. Comparing base (7e795d4) to head (ade238f).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...crosoft/sqlserver/jdbc/ConfigurableRetryLogic.java 53.84% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2622      +/-   ##
============================================
+ Coverage     51.54%   51.64%   +0.10%     
- Complexity     3996     4026      +30     
============================================
  Files           147      147              
  Lines         33694    33704      +10     
  Branches       5630     5631       +1     
============================================
+ Hits          17366    17405      +39     
- Misses        13877    13886       +9     
+ Partials       2451     2413      -38     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Jeffery-Wasty
Copy link
Contributor Author

Passes tests with run id = 110147.

@Jeffery-Wasty Jeffery-Wasty merged commit b8be643 into main Mar 3, 2025
19 checks passed
@github-project-automation github-project-automation bot moved this from In progress to Closed/Merged PRs in MSSQL JDBC Mar 3, 2025
@Jeffery-Wasty Jeffery-Wasty deleted the crlWildFlyFix branch March 3, 2025 23:55
@lilgreenbird lilgreenbird changed the title Remove scheme from URI before fetching path for CRL path check Removed scheme from URI before fetching path for CRL path check Mar 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Closed/Merged PRs

Development

Successfully merging this pull request may close these issues.

WildFly server won't start using code after the 12.9.0 preview due to error with ConfigurableRetryLogic

6 participants