Doctrine DBAL

OCI8 Adapter (convertPositionalToNamedPlaceholders) Statemachine doesn't consider comments and strings

Details

  • Type: Improvement Improvement
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: 2.0.5
  • Fix Version/s: None
  • Component/s: Drivers
  • Labels:
    None

Description

SQL statements may contain question marks in strings, inline comments or comment blocks. The current implementation of the method "convertPositionalToNamedPlaceholders()" doesn't consider these implications and falsely replaces them by named bind variables.

Replacement code with example:

<?php
$s = '-- Testkomm?ntar
	select	/* ? *//* ??? */ ?||\'H"al?l"o?\' as "h?""llo" union /* "? Kommentar" \' */
	/* ?" */ select \'/*Hallo\'||to_char( ? ) union select \'--Welt\'
	union--
	select ?';

echo 'In: ' . $s . PHP_EOL;

$bind = 0;
$skip = array( '--' => PHP_EOL, '/*' => '*/', '"' => '"', "'" => "'" );
for( $i = 0; $i < strlen( $s ) /* size of string might change! */; /* yes, no increment here! */ )
{
	// Skipping comments and literals
	foreach( $skip as $begin => $end )
	{
		$matches = substr_compare( $s, $begin, $i, strlen( $begin ) );
		if( $matches !== false && $matches == 0 )
		{
			$pos = strpos( $s, $end, $i+strlen( $begin ) );
			// echo "Found $begin, skipping at $i to $end at $pos" . PHP_EOL;
			if( $pos === false )
			{
				// No more data or illegal statement - anyway: no more replacements!
				// echo "EOD" . PHP_EOL;
				break 2;
			}
			$i = $pos + strlen( $end );
			continue 2; // Ensure we match /*..*//*..*/, '''' or """" - that's why we don't ++$i in the for-loop!
		}
	}
	if( $s[$i] == "?" )
	{
		// Positional to named
		// echo "Replace $bind" . PHP_EOL;
		$r = ':name' . ++$bind;
		$s = substr_replace( $s, $r, $i, 1 );
		$i += strlen( $r );
	}
	++$i;
}

echo 'Out: ' . $s . PHP_EOL;

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Benjamin Eberlei added a comment -

This algorithmus is painfully slow. There has to be something better, why do you need to foreach the loop inside the for?

Show
Benjamin Eberlei added a comment - This algorithmus is painfully slow. There has to be something better, why do you need to foreach the loop inside the for?
Hide
Permalink
Carsten Hetzel added a comment - - edited

Ok, sent you a PM but I'll put my response here, too:

"Painfully slow" at what circumstances? Benchmarks?

This was meant as an example implementation to show the problems of the current implementation - there's always room for improvement.

And never forget: Premature optimization is the root of all evil! ;-p

You want speed - try this one:

<?php
$sql = '-- Testkomm?ntar
        select  /* ? *//* ??? */ ?||\'H"al?l"o?\' as "h?""llo" union /* "? Kommentar" \' */
        /* ?" */ select \'/*Hallo\'||to_char( ? ) union select \'--Welt\'
        union--
        select ?';

echo 'In: ' . $sql . PHP_EOL;

$map = array();
for( $cnt = 0; $cnt < 1000; ++$cnt )
{
        $bind = 0;
        $s = $sql;

        if( isset( $map[$s] ) )
                continue;

        $strlen = strlen( $s );
        for( $i = 0; $i < $strlen /* size of string might change! */; /* yes, no increment here! */ )
        {
                $c1 = $s[$i];
                $c2 = isset( $s[$i+1] ) ? $s[$i+1] : '';
                if( $c1 == '-' && $c2 == '-')
                {
                        $pos = strpos( $s, PHP_EOL, $i+2 );
                        if( $pos === false )
                        {
                                break 2;
                        }
                        $i = $pos + 2;
                }
                elseif( $c1 == '/' && $c2 == '*' )
                {
                        $pos = strpos( $s, '*/', $i+2 );
                        if( $pos === false )
                        {
                                break 2;
                        }
                        $i = $pos + 2;
                }
                elseif( $c1 == '"' )
                {
                        $pos = strpos( $s, '"', $i+1 );
                        if( $pos === false )
                        {
                                break 2;
                        }
                        $i = $pos + 1;
                }
                elseif( $c1 == "'" )
                {
                        $pos = strpos( $s, "'", $i+1 );
                        if( $pos === false )
                        {
                                break 2;
                        }
                        $i = $pos + 1;
                }
                elseif( $c1 == "?" )
                {
                        // Positional to named
                        // echo "Replace $bind" . PHP_EOL;
                        $r = ':name' . ++$bind;
                        $s = substr_replace( $s, $r, $i, 1 );
                        $rLen = strlen( $r );
                        $i += $rLen;
                        $strlen += $rLen - 1;
                }
                else
                        ++$i;
        }
        $map[$sql] = $s;
}
echo 'Out: ' . $map[$sql] . PHP_EOL;

ATTENTION: Each "break 2;" means, you have an invalid statement! Do, whatever the statement class is supposed to do in this case!

Regards, Carsten

Show
Carsten Hetzel added a comment - - edited Ok, sent you a PM but I'll put my response here, too: "Painfully slow" at what circumstances? Benchmarks? This was meant as an example implementation to show the problems of the current implementation - there's always room for improvement. And never forget: Premature optimization is the root of all evil! ;-p You want speed - try this one: <?php $sql = '-- Testkomm?ntar select /* ? * //* ??? */ ?||\'H "al?l" o?\' as "h?" "llo" union /* "? Kommentar" \' */ /* ?" */ select \'/*Hallo\'||to_char( ? ) union select \'--Welt\' union-- select ?'; echo 'In: ' . $sql . PHP_EOL; $map = array(); for ( $cnt = 0; $cnt < 1000; ++$cnt ) { $bind = 0; $s = $sql; if ( isset( $map[$s] ) ) continue ; $strlen = strlen( $s ); for ( $i = 0; $i < $strlen /* size of string might change! */; /* yes, no increment here! */ ) { $c1 = $s[$i]; $c2 = isset( $s[$i+1] ) ? $s[$i+1] : ''; if ( $c1 == '-' && $c2 == '-') { $pos = strpos( $s, PHP_EOL, $i+2 ); if ( $pos === false ) { break 2; } $i = $pos + 2; } elseif( $c1 == '/' && $c2 == '*' ) { $pos = strpos( $s, '*/', $i+2 ); if ( $pos === false ) { break 2; } $i = $pos + 2; } elseif( $c1 == '"' ) { $pos = strpos( $s, '"', $i+1 ); if ( $pos === false ) { break 2; } $i = $pos + 1; } elseif( $c1 == "'" ) { $pos = strpos( $s, "'" , $i+1 ); if ( $pos === false ) { break 2; } $i = $pos + 1; } elseif( $c1 == "?" ) { // Positional to named // echo "Replace $bind" . PHP_EOL; $r = ':name' . ++$bind; $s = substr_replace( $s, $r, $i, 1 ); $rLen = strlen( $r ); $i += $rLen; $strlen += $rLen - 1; } else ++$i; } $map[$sql] = $s; } echo 'Out: ' . $map[$sql] . PHP_EOL; ATTENTION: Each "break 2;" means, you have an invalid statement! Do, whatever the statement class is supposed to do in this case! Regards, Carsten
Benjamin Eberlei made changes -
Field Original Value New Value
Workflow jira [ 12654 ] jira-feedback2 [ 17574 ]
Benjamin Eberlei made changes -
Workflow jira-feedback2 [ 17574 ] jira-feedback3 [ 19928 ]
This list may be incomplete, as errors occurred whilst retrieving source from linked applications:
  • Request to http://www.doctrine-project.org/fisheye/ failed: Error in remote call to 'FishEye 0 (http://www.doctrine-project.org/fisheye/)' (http://www.doctrine-project.org/fisheye) [AbstractRestCommand{path='/rest-service-fe/search-v1/crossRepositoryQuery', params={query=DBAL-124, expand=changesets[-21:-1].revisions[0:29],reviews}, methodType=GET}] : Received status code 503 (Service Temporarily Unavailable)

People

  • Assignee:
    Benjamin Eberlei
    Reporter:
    Carsten Hetzel
Vote (0)
Watch (1)

Dates

  • Created:
    Updated: