Feature/support prepared statements #53

andsel · 2019-11-25T10:05:30Z

Add the support to prepared statements in local_lookup solves #50

kares · 2019-11-25T11:14:08Z

docs/index.asciidoc

 returns multiple columns, the data is stored as a JSON object within the field.
 <5> Takes data from the JSON object and stores it in top-level event fields for
 easier analysis in Kibana.
+<6> Local lookup queries could be use also prepared statements.


"Local lookup queries could also use prepared statements."
... but @karenzone should double check on us 😉

What about, "Local lookup queries can also use prepared statements."?

I like it more, thanks

kares · 2019-11-25T11:16:05Z

lib/logstash/filters/jdbc/lookup.rb

+      begin
+        logger.debug? && logger.debug("Executing Jdbc query", :lookup_id => @id, :statement => query, :parameters => params)
+        @prepared_statement.call(params).each do |row|
+          stringified = row.inject({}){|hash,(k,v)| hash[k.to_s] = v; hash} #Stringify row keys


Q: why do we need to stringify (just curious) ?

I've taken suggestion from the no prepared part, I think the stringify is need to avoid errors of type conversion error when the JDBC driver loads a BigInteger or the driver does ugly conversions.

kares · 2019-11-25T11:22:57Z

docs/index.asciidoc

 A SQL SELECT statement that is executed to achieve the lookup. To use
 parameters, use named parameter syntax, for example
-`"SELECT * FROM MYTABLE WHERE ID = :id"`.
+`"SELECT * FROM MYTABLE WHERE ID = :id"`. Alternatively could be also use


"Alternatively the ? sign could be used as prepared statement parameters, in which case the prepared_parameters is used to populate the values."
(maybe "... provide the values") sounds better - not sure.

What about, "Alternatively, the ? sign can be used as a prepared statement parameter, in which case the prepared_parameters array is used to populate the values

karenzone

The callout list is rendering incorrectly. I will research.

karenzone · 2019-11-25T15:38:23Z

docs/index.asciidoc

 returns multiple columns, the data is stored as a JSON object within the field.
 <5> Takes data from the JSON object and stores it in top-level event fields for
 easier analysis in Kibana.
+<6> Local lookup queries could be use also prepared statements.


What about, "Local lookup queries can also use prepared statements."?

karenzone · 2019-11-25T15:39:52Z

docs/index.asciidoc

 returns multiple columns, the data is stored as a JSON object within the field.
 <5> Takes data from the JSON object and stores it in top-level event fields for
 easier analysis in Kibana.
+<6> Local lookup queries could be use also prepared statements.


The callout list is acting up for no obvious reason. I will figure out why.

@karenzone Is this an issue with out of order callouts? The <6> is defined before the <5> and I seem to recall that callouts are auto-numbered(?), and ignored the number given in the table other than as a label to refer back to

Yes, @robbavey. You are correct!
@andsel The callouts must be issued in numerical order. Also, since our conversion to asciidoctor, we can't reuse a number (see <5> and <5>, for example). In other words, what was there worked before we converted to asciidoctor. It hasn't been quite right since we converted. (The repeated number is not related to any work you did.) Please let me know if you have questions.

If you want entries for both <5>, you can define once and assume the user will get it. Or, you can define as <5> and <6>, and reorder the other items accordingly.

As long as you define your callouts in order in your code sample, and don't repeat numbers, you should be fine.

@karenzone I've fixed the number ordering, but I don't know how to fix the issue related to double use of same callout

@andsel @karenzone How about having a single callout in the comment above the add_field settings - I'm not quite sure why the docs originally only had callouts on two of the three add_field, and putting it in the comment might work as a way of grouping the settings

I agree with you, and I would add the callout to the first of the add_field definitions

karenzone · 2019-11-25T16:00:51Z

docs/index.asciidoc

 A SQL SELECT statement that is executed to achieve the lookup. To use
 parameters, use named parameter syntax, for example
-`"SELECT * FROM MYTABLE WHERE ID = :id"`.
+`"SELECT * FROM MYTABLE WHERE ID = :id"`. Alternatively could be also use


What about, "Alternatively, the ? sign can be used as a prepared statement parameter, in which case the prepared_parameters array is used to populate the values

robbavey · 2019-11-25T20:38:47Z

spec/filters/jdbc/lookup_spec.rb

+      let(:local_db) { double("local_db") }
+      let(:lookup_hash) do
+        {
+          "query" => "select * from servers WHERE ip LIKE ?",


Can we have a test with >1 parameters?

Done, thanks for suggestion, because spotted a test that didn'tt do its job:-)

robbavey · 2019-11-25T20:52:05Z

@andsel, @jsvd I think this is a nicely done PR, with my only issue being a nagging question as to whether we need to expose this option to the user - given that the Derby database used for local lookups is somewhat an implementation detail, and we are asking the user to understand this implementation detail to determine whether or not to use prepared statements.

If we do expose this detail, we should probably document that the use of prepared statements in this plugin is preferred.
(I do realise that they way we currently use queries and parameters complicates this question somewhat)

andsel · 2019-11-26T10:15:07Z

@robbavey I think we expose the concept that the local DB could use prepared statements, not really an internal detail of Derby. But your point is correct, if we prefer the prepared statements we should avoid or discourage the use of the normal statements, but we can't drop that support because it's already used by pipelines out in the wild

robbavey · 2019-11-26T14:00:30Z

@andsel I'm ok with that - if we add a documentation note that the use of prepared statements is preferred/encouraged, and explain the reasons why, then I think this is good

cc @karenzone

robbavey

Nearly there, I think - can we add an integration test, and an "abnormal operation" test?

Also, there a few leftover debug statements that should be removed before merging.

robbavey · 2019-11-26T14:41:43Z

docs/index.asciidoc

-        parameters => {id => "[loggedin_userid]"}
-        target => "user" <4>
+        query => "select firstname, lastname from users WHERE userid = ?"
+        prepared_parameters => ["[loggedin_userid]"] <4>


How about having an example with > 1 prepared_parameters? There could be confusion as to what the correct syntax here (is it ["[param1, param2]"] or ["[param1]", "[param2]"]? Or does it not matter?) It might also help to reiterate here that the order of the parameters in prepared_parameters is important

robbavey · 2019-11-26T14:42:08Z

lib/logstash/filters/jdbc/lookup.rb

    end

    def fetch(local, event)
+      puts "Lookup::fetch >> invoked"


Leftover debug statement?

robbavey · 2019-11-26T14:42:19Z

lib/logstash/filters/jdbc/lookup.rb

    end

+    def call_prepared(local, event)
+      puts "Lookup::call_prepared >> invoked"


Leftover debug statement?

robbavey · 2019-11-26T14:42:47Z

lib/logstash/filters/jdbc/lookup.rb

+      begin
+        logger.debug? && logger.debug("Executing Jdbc query", :lookup_id => @id, :statement => query, :parameters => params)
+        @prepared_statement.call(params).each do |row|
+          puts "Lookup::call_prepared >> retrieved rw: #{row}"


Leftover debug statement?

karenzone

Docs build cleanly and render properly. Docs LGTM.

robbavey

Getting very close, just an integration test failure to fix, and I think given that we are introducing a new feature, we should be incrementing the minor of the plugin, rather than just the patch.

robbavey · 2019-11-26T19:13:44Z

CHANGELOG.md

@@ -1,3 +1,6 @@
+## 1.0.8


Maybe change this to a minor upgrade, as we are adding a new feature.

robbavey · 2019-11-26T19:20:45Z

logstash-filter-jdbc_static.gemspec

 Gem::Specification.new do |s|
  s.name            = 'logstash-filter-jdbc_static'
-  s.version         = '1.0.7'
+  s.version         = '1.0.8'


Maybe change this to a minor upgrade, as we are adding a new feature.

robbavey · 2019-11-26T19:35:43Z

spec/filters/integration/jdbc_static_spec.rb

+      context "under normal conditions with prepared statement" do
+        let(:lookup_statement) { "SELECT * FROM servers WHERE ip LIKE ?" }
+        it "enhances an event" do
+          settings.local_lookups.prepared_parameters = [parameters_rhs]


This currently fails testing - https://travis-ci.org/logstash-plugins/logstash-filter-jdbc_static/builds/617288523

To replace the value of parameters with prepared_parameters for this test case, I think you will need to replace the whole local_lookups block in :settings, rather than just the lookup statement: you will need to remove the existing parameters setting as well as include a new prepared_parameters setting.

Done :-) thanks @robbavey now Travis is green

robbavey

LGTM. Nice job @andsel

elasticsearch-bot · 2019-11-27T08:33:36Z

Andrea Selva merged this into the following branches!

Branch	Commits
master	`02c9ec2`

elasticsearch-bot self-assigned this Nov 25, 2019

kares reviewed Nov 25, 2019

View reviewed changes

karenzone reviewed Nov 25, 2019

View reviewed changes

robbavey reviewed Nov 25, 2019

View reviewed changes

robbavey unassigned elasticsearch-bot Nov 25, 2019

robbavey requested changes Nov 26, 2019

View reviewed changes

andsel requested a review from robbavey November 26, 2019 16:15

karenzone self-requested a review November 26, 2019 17:51

karenzone approved these changes Nov 26, 2019

View reviewed changes

robbavey requested changes Nov 26, 2019

View reviewed changes

robbavey approved these changes Nov 26, 2019

View reviewed changes

robbavey mentioned this pull request Nov 26, 2019

Meta Issue: Logstash JDBC Integration plugin logstash-plugins/logstash-integration-jdbc#2

Open

27 tasks

Updated the local lookup to use prepared statements, closes 50

91432a8

andsel force-pushed the feature/support_prepared_statements branch from 9aea0d0 to 91432a8 Compare November 27, 2019 08:17

elasticsearch-bot closed this in 02c9ec2 Nov 27, 2019

This was referenced Nov 27, 2019

support prepared statements #50

Closed

Porting of prepared statement usage from jdbc static filter logstash-plugins/logstash-integration-jdbc#7

Closed

karenzone mentioned this pull request Jan 29, 2020

[Doc][Meta]Logstash 7.6 documentation roadmap elastic/logstash#11527

Closed

18 tasks

Feature/support prepared statements #53

Feature/support prepared statements #53

Uh oh!

Conversation

andsel commented Nov 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karenzone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karenzone Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robbavey commented Nov 25, 2019

Uh oh!

andsel commented Nov 26, 2019

Uh oh!

robbavey commented Nov 26, 2019

Uh oh!

robbavey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karenzone left a comment

Choose a reason for hiding this comment

Uh oh!

robbavey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robbavey left a comment

Choose a reason for hiding this comment

karenzone Nov 25, 2019 •

edited

Loading