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

driver loading: provide loaded driver to Sequel::connect #34

Merged
merged 1 commit into from
Jun 11, 2020

Conversation

yaauie
Copy link
Contributor

@yaauie yaauie commented Jun 8, 2020

Sequel::connect will first attempt to auto-detect the driver based on the
jdbc connection URI, and will fall-back to the driver given as opts[:driver]
when auto-detection fails.

By memoizing the loaded driver from our previous Sequel::JDBC::load_driver
and providing it to Sequel::connect, we provide one more chance for loading
to work.

This specifically seems to be the only path forward for Sybase drivers, whose
jdbc connection strings begin with jdbc:sybase:Tds: but use the driver with
path com.sybase.jdbc4.jdbc.SybDriver.

Copy link
Contributor

@robbavey robbavey left a comment

Choose a reason for hiding this comment

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

LGTM

@yaauie
Copy link
Contributor Author

yaauie commented Jun 9, 2020

Fixes: #30
Fixes: #23

@yaauie yaauie force-pushed the improved-driver-loading branch 2 times, most recently from 14049f6 to e12b7d0 Compare June 10, 2020 17:09
@yaauie yaauie requested a review from robbavey June 10, 2020 17:13
@yaauie
Copy link
Contributor Author

yaauie commented Jun 10, 2020

@robbavey I've gone back and added the driver fallback logic to all three plugins, and adjusted their specs to ensure that the result of Sequel::JDBC.load_driver is passed through to opts[:driver]. Could you re-review?

Copy link
Contributor

@robbavey robbavey left a comment

Choose a reason for hiding this comment

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

LGTM. Integration test looks like unrelated

`Sequel::connect` will first attempt to auto-detect the driver based on the
jdbc connection string, and will [fall-back][] to the driver given as
opts[:driver] when auto-detection fails.

By memoizing the loaded driver from our previous `Sequel::JDBC::load_driver`
and providing it to `Sequel::connect`, we provide one more chance for loading
to work.

This specifically seems to be the only path forward for Sybase drivers, whose
jdbc connection strings begin with `jdbc:sybase:Tds:` but use the driver with
path `com.sybase.jdbc4.jdbc.SybDriver`.

[fall-back]: https://github.com/jeremyevans/sequel/blob/5.19.0/lib/sequel/adapters/jdbc.rb#L211-L213

Fixes: logstash-plugins#23
Fixes: logstash-plugins#30
@yaauie yaauie merged commit 94821e5 into logstash-plugins:master Jun 11, 2020
@yaauie yaauie deleted the improved-driver-loading branch June 11, 2020 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants