database icon indicating copy to clipboard operation
database copied to clipboard

Prepared statements seem not to trigger an error if parameter typ is wrong

Open HLeithner opened this issue 6 years ago • 12 comments
trafficstars

While rewriting the cms queryies, I tested the affect of wrong input type and it seam that mysql don't care.

Is this normal? Or maybe I did something wrong...

If this expected i would like to suggest to test the type in our db layer and throw an exception if it's wrong.

HLeithner avatar May 29 '19 22:05 HLeithner

It instructs PDO or the mysqli API how to handle the data, it isn’t doing a type validation that what you passed is a certain PHP scalar. Nothing to fix here IMO.

On Wed, May 29, 2019 at 5:13 PM Harald Leithner [email protected] wrote:

While rewriting the cms queryies, I tested the affect of wrong input type and it seam that mysql don't care.

Is this normal? Or maybe I did something wrong...

If this expected i would like to suggest to test the type in our db layer and throw an exception if it's wrong.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/joomla-framework/database/issues/159?email_source=notifications&email_token=AACZ7IIF666GYL32ITSYRK3PX35XLA5CNFSM4HQWSYG2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4GWTD2VQ, or mute the thread https://github.com/notifications/unsubscribe-auth/AACZ7INK4SX6YCSDGA4ILHDPX35XLANCNFSM4HQWSYGQ .

--

  • Michael Please pardon any errors, this message was sent from my iPhone.

mbabker avatar May 29 '19 22:05 mbabker

Hmm but if I give an string to mysql and say its and integer it should give me a warning at least? Isn't this the whole point telling mysql what type the variable is?

HLeithner avatar May 30 '19 09:05 HLeithner

mysql is a little bit lighthearted on this (pdo or not), the opposite of pgsql that is much more strict

alikon avatar May 30 '19 09:05 alikon

@alikon Can you test the behavior on pgsql?

HLeithner avatar May 30 '19 10:05 HLeithner

i've already experienced that see https://github.com/joomla/joomla-cms/pull/24149

alikon avatar May 30 '19 10:05 alikon

Ok but this error only tells us that the target field is not an boolean but pdo still except an integer as input. Is this valid for string and integer too?

HLeithner avatar May 30 '19 10:05 HLeithner

I can't say I've run into these type of type checking problems you're seeing, but this isn't a behavior that belongs to a DBAL IMO (and if it does you need to convince Doctrine and Laravel to do the same).

Databases don't have a scalar boolean type, true/false is generally stored in a TINYINT column as 1/0. Unless I've completely misunderstood something, specifying the parameter type signals to PDO and the mysqli APIs the data should be internally processed to convert whatever you've given it to a representation that matches the type you've given (so those APIs handle converting boolean true to integer 1 when writing the query, because obviously you aren't getting a boolean as your query result without an ORM). Specifying a parameter type does not equate to requiring that data pass a is_(array|bool|float|int|string) check, and that would probably be a performance bottleneck if it started to be executed for every parameter on every query.

mbabker avatar May 30 '19 12:05 mbabker

atm I try to implement prepared statements into the sqlsrv driver and sqlsrv has a bool for example but still I didn't found out if all drivers silently convert input variables to it needs.

btw. would be really awesome to have declare strict but thats another topic

HLeithner avatar May 30 '19 13:05 HLeithner

Before I gave up putting too much of my time into things, I wanted to just rip apart and completely rebuild the database test suites. Because right now there are a lot of janky and goofy tests and not a lot of good functional/integration tests. Plus, with all the changes I've made to the Travis and AppVeyor configs, everything's in a better spot to be able to test behaviors on specific versions of specific database platforms whereas in the CMS repo everything's testing on one MySQL version.

mbabker avatar May 30 '19 13:05 mbabker

mssqlsrv doesn't like different datatypes: https://ci.appveyor.com/project/joomla/database/builds/24926396/job/7mq3farm0a2krxaq

HLeithner avatar May 30 '19 15:05 HLeithner

*sigh* I forgot that wasn't a PDO driver.

So if SQL Server is that picky about it then add type checks in that query/statement class. Either way it doesn't come across to me as something that belongs to DatabaseQuery::bind() as a default behavior to either be type checking or typecasting data given to it (none of the Framework apps in production have as high query usage as the CMS, the CMS easily gets into 25-30 queries per request and most queries have at least one parameter, stuff like modules and plugins can get into 4 or 5 params easily, this sounds like it could easily start to create a performance bottleneck). To me it feels like what is happening now is exactly what should happen; let the PHP extension internally decide what to do with the data when converting it into the structure needed to communicate with the database as it will know best and raise an error if it has a problem, otherwise our DBAL is getting into the business of making arbitrary decisions because of the behavior of one adapter.

mbabker avatar May 30 '19 16:05 mbabker

In this case it's my fault I think, I miss interpreted the 4 sqltype parameter. it seams it expects the target column type and not the variable type. Our bind system doesn't provide this information so I removed it.

I think for the bind function I would only check if int is expected and is given or type cast to int if set... but I would prefer to throw an exception in this case.

HLeithner avatar May 31 '19 08:05 HLeithner