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

Add Pdo abstraction layer #332

Closed
wants to merge 4 commits into from
Closed

Add Pdo abstraction layer #332

wants to merge 4 commits into from

Conversation

glensc
Copy link
Contributor

@glensc glensc commented Sep 19, 2020

To handle:

  • different identifier quote style for tables/columns
  • sqlite: double quote
  • pgsql: double quote
  • mysql: backtick
  • LIMIT operand: PdoSearcher: Use portable LIMIT/OFFSET #335
  • sqlite: LIMIT row_to_skip, row_count
  • mysql: LIMIT row_to_skip, row_count
  • pgsql: LIMIT row_count OFFSET row_to_skip

A solution for #322

@glensc glensc self-assigned this Sep 19, 2020
@glensc
Copy link
Contributor Author

glensc commented Sep 19, 2020

@perftools/maintainers perhaps it's better to use existing abstraction, than invent our own?

same question asked earlier:

Some choices:

the different pdo drivers need different approaches, see notes from #322

WDYT?

$pdoOptions
);

$pdo->quoteIdentifier = self::getQuoteIdentifier($options['dsn']);
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't https://www.php.net/manual/en/pdo.quote.php help you achieve the same thing?

PDO::quote() places quotes around the input string (if required) and escapes special characters within the input string, using a quoting style appropriate to the underlying driver.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lauripiisang, unfortunately, no, quote() is for values quoting ("a\""), but here need to quote identifiers (table names, column names).

in mysql identifiers are quoted with backticks and values with backslash. other systems use double quotes for identifiers and double quotes for an enoding double quote inside double quote separated value: """" is a double quote. actually mysql supports this encode style too.

Copy link
Contributor Author

@glensc glensc Sep 22, 2020

Choose a reason for hiding this comment

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

so to cut story short: can't reliably quote column "get" in these pdo drivers:

  • pgsql
  • mysql
  • sqlite

mysql example included:

15:13:36 mysql{2}> select get;
ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'get' at line 1
15:15:31 mysql{3}> select `get`;
ERROR 1054 (42S22): Unknown column 'get' in 'field list'
15:15:35 mysql{4}> select "get";
+-----+
| get |
+-----+
| get |
+-----+
1 row in set (0.00 sec)

15:15:45 mysql{5}>

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, missed that it was about quoting identifiers . yeah, that's unfortunate separate story.

Copy link
Contributor

Choose a reason for hiding this comment

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

According to https://stackoverflow.com/questions/214309/do-different-databases-use-different-name-quote/214344#214344

The standard SQL language uses double-quotes for delimited identifiers

MySQL uses back-quotes by default. MySQL can use standard double-quotes:

SELECT * FROM `my table`;
SET SQL_MODE=ANSI_QUOTES;
SELECT * FROM "my table";

Maybe it's possible to drop backticks in favor of "ansi quotes" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's a good find!

also need to quote the column named GET, seems it suits there as well, not selecting a string:

15:03:49 mysql{1}> SET SQL_MODE=ANSI_QUOTES;
Query OK, 0 rows affected, 1 warning (0.03 sec)

Warning (Code 3090): Changing sql mode 'NO_AUTO_CREATE_USER' is deprecated. It will be removed in a future release.
15:03:50 mysql{2}> select "GET";
ERROR 1054 (42S22): Unknown column 'GET' in 'field list'
15:03:55 mysql{3}>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good thing is that it works as low as mysql 5.0:


15:06:00 mysql@192.168.40.12:1> SET SQL_MODE=ANSI_QUOTES;
Query OK, 0 rows affected (0.00 sec)

15:06:06 mysql@192.168.40.12:2> select "GET";
ERROR 1054 (42S22): Unknown column 'GET' in 'field list'
15:06:11 mysql@192.168.40.12:3> select @@version;
+------------+
| @@version  |
+------------+
| 5.0.96-log |
+------------+
1 row in set (0.00 sec)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#336

altho maybe acceptable solution would had also been to rename "GET" column to to something else, and leave upgrade notitice that the column must be renamed when upgrading.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, I suppose that would be easier and less-hacky. reserved words probably should be avoided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the thing is that reserved new reserved words can appear.

stupid solution to that is to prefix all columns and tables with xhgui :)

@lauripiisang
Copy link
Contributor

as for limit and offset,

LIMIT x OFFSET y should be valid SQL regardless of engine

@glensc
Copy link
Contributor Author

glensc commented Sep 22, 2020

Fix limit: #335

I knew mysql suports LIMIT x OFFSET y, wasn't sure about sqlite, and didn't search for actual SQL specification to get confidence.

@glensc
Copy link
Contributor Author

glensc commented Sep 23, 2020

added PoC with needed changes to use quoteIdentifier.

it really looks like building another abstraction layer here...

@glensc
Copy link
Contributor Author

glensc commented Sep 24, 2020

superseded by #336

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