sql: create common SQL parsing library to be used by mysql and postgres by cpakulski · Pull Request #11368 · envoyproxy/envoy

cpakulski · 2020-05-29T18:04:16Z

Description:
Created sqlutils library to be shared for common functionality between SQL filters.
mysql and postgres filters will use that library to create filter metadata based on SQL query.
mysql filter was already producing metadata but postges will use the new library as described in #11065.

Risk Level:
Low: No new functionality has been added and only mysql_proxy filter is affected

Testing:
Added unit tests.

Docs Changes:
No.

Release Notes:
No.

Fixes: #11320

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Fixed a bug in mysql testing routines which masked wrong results returned by SQL parser. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

dio

Looks good, couple of notes:

dio · 2020-06-04T04:50:29Z

+
+class SQLutils {
+public:
+  static bool setMetadata(const std::string& query, ProtobufWkt::Struct& metadata);


It seems like we need to add docs on this.

I see what you mean. Actually at this moment nothing changed from metadata point of view. Documentation for mysql is still valid:https://www.envoyproxy.io/docs/envoy/v1.14.1/configuration/listeners/network_filters/mysql_proxy_filter.html?highlight=metadata#dynamic-metadata.
When I link this setMetadata to Postgres filter I will create similar docs describing what metadata is created by Postgres filter, but this will happen once this PR is merged.

I think @dio meant doxygen style doc for this method.

dio · 2020-06-04T04:52:07Z

+    ::testing::Values(
+        TEST_VALUE("blahblah;", false, {}),
+
+        TEST_VALUE("CREATE TABLE IF NOT EXISTS table1(Usr VARCHAR(40),Count INT);", true,


Can we check for all lowercased queries?

create tabel if not exists table1(usr varchar(40),count int);

Changed testing routine to convert queries to lowercase and uppercase, so all different variants are tested.

test queries. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

cpakulski · 2020-06-05T19:11:59Z

/azp run

azure-pipelines · 2020-06-05T19:12:04Z

Commenter does not have sufficient privileges for PR 11368 in repo envoyproxy/envoy

lizan · 2020-06-05T20:39:52Z

+
+class SQLutils {
+public:
+  static bool setMetadata(const std::string& query, ProtobufWkt::Struct& metadata);


I think @dio meant doxygen style doc for this method.

lizan · 2020-06-05T22:12:10Z


 /*
 * Test query: "DROP DATABASE <DB>"
+ * Test is disabled because of a bug in SQL parsing library.


wouldn't this be a regression?

The bug was always there and this PR does not change anything. The old behaviour is preserved. The SQL parser has a bug and produces wrong output for DROP DATABASE. The old unit tests in mysql had another bug which masked SQL parser's bug. In https://github.com/envoyproxy/envoy/blob/release/v1.14/test/extensions/filters/network/mysql_proxy/mysql_command_test.cc#L205-L209 if expected_table_access_map is empty, the routine does NOT check what was parser's result, which is stored in table_access_map. In case of DROP DATABASE the table_access_map is not empty, but old unit test never checked that.

I changed how parsing results are accessed and this uncovered the bug.

then can we either delete these tests or fix them? I don't think we want permanently disabled test live in the code base.

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

lizan

LGTM except the test.

lizan · 2020-06-10T04:31:09Z


 /*
 * Test query: "DROP DATABASE <DB>"
+ * Test is disabled because of a bug in SQL parsing library.


then can we either delete these tests or fix them? I don't think we want permanently disabled test live in the code base.

Enabled unit tests for DROP DATABASE after fix in sql-parser library. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

cpakulski · 2020-06-10T19:19:08Z

Fixed issue with DROP DATABASE: envoyproxy/sql-parser#3.

Now all tests are enabled and they pass.

cpakulski · 2020-06-10T20:25:27Z

/retest

repokitteh-read-only · 2020-06-10T20:25:30Z

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #11368 (comment) was created by @cpakulski.

see: more, trace.

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

dio is OOO

…es (envoyproxy#11368) Description: Created _sqlutils_ library to be shared for common functionality between SQL filters. mysql and postgres filters will use that library to create filter metadata based on SQL query. mysql filter was already producing metadata but postges will use the new library as described in envoyproxy#11065. Risk Level: Low: No new functionality has been added and only mysql_proxy filter is affected Testing: Added unit tests. Docs Changes: No. Release Notes: No. Fixes: envoyproxy#11320 Signed-off-by: Christoph Pakulski <christoph@tetrate.io> Signed-off-by: Arthur Yan <arthuryan@google.com>

…es (envoyproxy#11368) Description: Created _sqlutils_ library to be shared for common functionality between SQL filters. mysql and postgres filters will use that library to create filter metadata based on SQL query. mysql filter was already producing metadata but postges will use the new library as described in envoyproxy#11065. Risk Level: Low: No new functionality has been added and only mysql_proxy filter is affected Testing: Added unit tests. Docs Changes: No. Release Notes: No. Fixes: envoyproxy#11320 Signed-off-by: Christoph Pakulski <christoph@tetrate.io> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>

…es (envoyproxy#11368) Description: Created _sqlutils_ library to be shared for common functionality between SQL filters. mysql and postgres filters will use that library to create filter metadata based on SQL query. mysql filter was already producing metadata but postges will use the new library as described in envoyproxy#11065. Risk Level: Low: No new functionality has been added and only mysql_proxy filter is affected Testing: Added unit tests. Docs Changes: No. Release Notes: No. Fixes: envoyproxy#11320 Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

…es (envoyproxy#11368) Description: Created _sqlutils_ library to be shared for common functionality between SQL filters. mysql and postgres filters will use that library to create filter metadata based on SQL query. mysql filter was already producing metadata but postges will use the new library as described in envoyproxy#11065. Risk Level: Low: No new functionality has been added and only mysql_proxy filter is affected Testing: Added unit tests. Docs Changes: No. Release Notes: No. Fixes: envoyproxy#11320 Signed-off-by: Christoph Pakulski <christoph@tetrate.io> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>

cpakulski added 3 commits May 27, 2020 21:52

Create common SQL library to be used by SQL filters.

ba888f1

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Added unit tests to sqlutils library.

c4f5a6a

Fixed a bug in mysql testing routines which masked wrong results returned by SQL parser. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Small corrections to code comments.

5dcc004

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

cpakulski requested a review from mattklein123 as a code owner May 29, 2020 18:04

cpakulski added 4 commits May 29, 2020 18:15

Updated CODEOWNERS file.

6f09ee9

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Clang-tidy changes.

505d007

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Merge branch 'master' into issue/11320

e83cdf4

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Merge branch 'master' into issue/11320

c1a3aa6

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

mattklein123 assigned dio May 30, 2020

dio previously requested changes Jun 4, 2020

View reviewed changes

cpakulski added 3 commits June 4, 2020 18:30

Changed unit tests to verify uppercase and lowercse versions of SQL

119c517

test queries. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Merge branch 'master' into issue/11320

087508c

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Corrected format in BUILD files.

1bc2356

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

lizan reviewed Jun 5, 2020

View reviewed changes

Changed test routines to use abseil utilities for string manipulation.

9314508

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

cpakulski requested a review from lizan June 8, 2020 17:15

lizan reviewed Jun 10, 2020

View reviewed changes

Moved sql-parser library to new revision.

ff93e52

Enabled unit tests for DROP DATABASE after fix in sql-parser library. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

cpakulski requested a review from lizan June 10, 2020 19:42

lizan previously approved these changes Jun 11, 2020

View reviewed changes

Removed obsolete comment.

a80b087

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

cpakulski dismissed lizan’s stale review via a80b087 June 11, 2020 01:54

cpakulski requested a review from lizan June 11, 2020 03:47

lizan approved these changes Jun 11, 2020

View reviewed changes

lizan merged commit 8b4c943 into envoyproxy:master Jun 11, 2020

cpakulski deleted the issue/11320 branch June 12, 2020 18:33

Conversation

cpakulski commented May 29, 2020

Uh oh!

dio 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpakulski commented Jun 5, 2020

Uh oh!

azure-pipelines Bot commented Jun 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpakulski commented Jun 10, 2020

Uh oh!

cpakulski commented Jun 10, 2020

Uh oh!

repokitteh-read-only Bot commented Jun 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants