Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed scheme from URI before fetching path for CRL path check #2622

Merged
merged 6 commits into from
Mar 3, 2025

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
@Jeffery-Wasty Jeffery-Wasty self-assigned this Feb 28, 2025
Copilot

This comment was marked as resolved.

Jeffery-Wasty and others added 2 commits February 28, 2025 13:09
…gic.java

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

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());

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 /
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
@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
5 participants