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

Proper escaping of TNS_ADMIN path. Select JDBC driver with defined implementation. #5363

Merged
merged 2 commits into from
Jan 27, 2023

Conversation

sdedic
Copy link
Member

@sdedic sdedic commented Jan 25, 2023

This PR fixes two issues with OCI database support:

The Download Wallet action writes a JDBC connection string with a pathname in it, but the pathname is not properly escaped. This is not an issue on MacOS / Linux, but on Windows that uses backslashes as dir separators, the backslashes in string content are interpreted as escapes by the JDBC driver, and the resulting path will be broken.

Second, the action tries to find the appropriate JDBC driver, but JDBCDriverManager.getDefault().getDrivers() call may reeturn half-configured drivers, that have no JAR files associated with them. This PR makes the action to prefer such drivers that define jars with the actual driver code.
The original behaviour had the effect of selecting a wrong driver, i.e. OracleThin driver (without JARs) was persisted while there was full Oracle jdbc driver present (that was configured with appropriate code jars).

@sdedic sdedic added VSCode Extension [ci] enable VSCode Extension tests enterprise [ci] enable enterprise job labels Jan 25, 2023
MartinBalin
MartinBalin previously approved these changes Jan 25, 2023
@sdedic sdedic self-assigned this Jan 26, 2023
@sdedic
Copy link
Member Author

sdedic commented Jan 26, 2023

This will be also needed for vsnetbeans, I guess I should rebase to delivery (@neilcsmith-net) ?

@neilcsmith-net
Copy link
Member

neilcsmith-net commented Jan 26, 2023

If it needs to go into NB17, then yes - base branch of PR needs changing to delivery, and commit rebasing on top of delivery if necessary. Also add the milestone when you're ready.

@sdedic sdedic added this to the NB17 milestone Jan 26, 2023
@sdedic
Copy link
Member Author

sdedic commented Jan 26, 2023

(marked as NB17, will rebase after @jhorvath's review, should be clean)

jhorvath
jhorvath previously approved these changes Jan 26, 2023
Copy link
Contributor

@jhorvath jhorvath left a comment

Choose a reason for hiding this comment

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

looks good

@sdedic sdedic changed the base branch from master to delivery January 27, 2023 07:30
@sdedic sdedic dismissed stale reviews from jhorvath and MartinBalin January 27, 2023 07:30

The base branch was changed.

@sdedic
Copy link
Member Author

sdedic commented Jan 27, 2023

Rebased onto delivery

@apache apache locked and limited conversation to collaborators Jan 27, 2023
@apache apache unlocked this conversation Jan 27, 2023
@MartinBalin MartinBalin merged commit d751a93 into apache:delivery Jan 27, 2023
sdedic added a commit that referenced this pull request Jan 27, 2023
…plementation. (#5363)

* Proper escaping of TNS_ADMIN path. Select JDBC driver with defined implementation.

* Issue warning if driver w/o code locations is selected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enterprise [ci] enable enterprise job VSCode Extension [ci] enable VSCode Extension tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants