WIP: Add support for running raw SQL files #729

rolandboon · 2020-11-24T14:17:14Z

Work in progress on this issue: #422

Is this the direction you're looking for?

While setting this up I had some questions/concerns:

On which routes do we want to serve these SQL files?
How do we distinguish between GET, POST, PUT etc.? Maybe some sort of comment/annotation on the top of the SQL file? Or do we introduce a PHP SQL parse dependency?
Do we need a root JSON key for the result? Similar to records?
Do we always serialize the result to an array? Or do we want to annotate SQL files for "single" record responses and serialize the first result into an object?
Generating an OpenAPI spec for these endpoints is a bit hard. Do we want this? Maybe it's possible with a SQL parser package.
I haven't read the code of all the middlewares, but I guess middlewares such as Authorization, MultiTenancy, PageLimits and Validation are pretty much tight to database tables, so those won't be usable on these "views"
I discovered actual views can also be queried through this package. What is the exact "business case" of this feature? To support complex insert/updates?

Todos on this PR:

mevdschee · 2020-11-24T15:34:19Z

This is so awesome! Thank you. Let me try to answer your questions/concerns

On which routes do we want to serve these SQL files?

I like the /procedures endpoint.

How do we distinguish between GET, POST, PUT etc.? Maybe some sort of comment/annotation on the top of the SQL file? Or do we introduce a PHP SQL parse dependency?

Other projects (like prest) do it in the filename/path.

Do we need a root JSON key for the result? Similar to records?

I think we need to model the response after the results returned by the database (driver). I'm not sure we need a root key, maybe an array of result sets will do.

Do we always serialize the result to an array? Or do we want to annotate SQL files for "single" record responses and serialize the first result into an object?

I think we may need to iterate the result sets of the database driver, not making any assumptions of the contents of each result set.

Generating an OpenAPI spec for these endpoints is a bit hard. Do we want this? Maybe it's possible with a SQL parser package.

We may have some way of specifying input parameters, not sure how though.

I haven't read the code of all the middlewares, but I guess middlewares such as Authorization, MultiTenancy, PageLimits and Validation are pretty much tight to database tables, so those won't be usable on these "views"

I think the Authorization middleware can be mapped, the others may not be usable.

I discovered actual views can also be queried through this package. What is the exact "business case" of this feature? To support complex insert/updates?

Mainly to support an API to whatever is in your database.

rolandboon · 2020-11-25T17:49:50Z

Quick update:

I like the pREST approach for handling HTTP verbs, so I replicated that 👍
https://docs.postgres.rest/executing-sql-scripts/#scripts-templates-rules

And I've added input argument handling in 2 ways

Query and request body params are bound to the PDO statement if the SQL string contains these parameter indentifiers. This is the preferred approach to interpolate user input because PDO will properly escape the input and prevent SQL injections:
https://github.com/rolandboon/php-crud-api/blob/a43ae835e897c4dc17b4217839e7845ddeff838a/src/Tqdev/PhpCrudApi/Database/GenericDB.php#L327
The SQL string is parsed as PHP with all input arguments extracted to global variables. It's a bit experimental and I'm not 100% sure we should keep this, but it offers far more powerful templating features. It's inspired by pREST's template functions. An alternative (more safe) solution would be to include a simple template parser.
https://github.com/rolandboon/php-crud-api/blob/a43ae835e897c4dc17b4217839e7845ddeff838a/src/Tqdev/PhpCrudApi/Procedure/ProcedureService.php#L26

mevdschee · 2020-11-27T13:48:22Z

I will review your parsing/template options shortly.

mevdschee · 2020-11-28T07:21:49Z

src/Tqdev/PhpCrudApi/Config.php

@@ -22,6 +22,7 @@ class Config
        'debug' => false,
        'basePath' => '',
        'openApiBase' => '{"info":{"title":"PHP-CRUD-API","version":"1.0.0"}}',
+        'proceduresDir' => './procedures/'


Could this be 'procedurePath' as key and 'procedures' as the value?

I've refactored this.

mevdschee · 2020-11-28T07:24:32Z

src/Tqdev/PhpCrudApi/Procedure/ProcedureService.php

+
+class ProcedureService {
+    private $db;
+    private $baseDir;


I would not rename procedureDir/procedurePath here to baseDir.

I've refactored this.

mevdschee · 2020-11-28T07:29:39Z

src/Tqdev/PhpCrudApi/Controller/ProcedureController.php

+    {
+        $file = RequestUtils::getPathSegment($request, 2);
+        $operation = RequestUtils::getOperation($request);
+        $queryParams = RequestUtils::getParams($request, false);


Shouldn't we also/primarily support Path parameters? The body parameters may be reserved for parameters of a table type (for multiple-inserts for instance).

To support path parameters we'd need to extend the SQL file with some annotations, so users can describe which path parameters are required for the file.

Annotations would allow to this feature to be a bit more "robust", we could use it to describe all user input and sanitize it before plugging it into the query. However, it would also make using this custom SQL feature more complex. Users need to learn the annotation format we've come up with to start using the feature.

mevdschee · 2020-11-28T07:31:02Z

src/Tqdev/PhpCrudApi/Procedure/ProcedureService.php

+    private function parseSqlTemplate(string $path, array $context) {
+        ob_start();
+        extract($context);
+        include($path);


Maybe we need to split the file into a head with some meta information and a body containing the sql query. The include is a risk as the $path variable should not contain (unchecked) user input to avoid path traversal.

I don't think path traversing is an issue at this point, because the path is limited to a single URL path segment. The value of $path at this location is defined as follows:

$path = './' . Config->getProcedurePath() . '/'. RequestUtils::getPathSegment($request, 2) . '.' . $operation . '.sql';

The getPathSegment() blocks path traversing.

However, some meta info would allow us to handle user input and server responses better. We could define the files like this:

./procedures/example.GET.php

<?php $procedure = [ /* * Define user input from path * In this example the route will be GET /procedure/example/:id * The value of :id will be as :id in the PDO statement */ 'path': ['id'], /* * Define user input from query string * Route will be /procedure/example?foo=hello&bar=world / The values will be available as :foo and :bar in the PDO statement */ 'query': ['foo', 'bar'], /* * Define user input from the request body */ 'body': ['buz'], 'statement': ' SELECT p.id, p.content, c.name FROM posts p INNER JOIN categories c ON c.id = p.category_id WHERE p.id = :id AND categories.name = :foo ', ];

mevdschee · 2020-11-28T07:32:43Z

src/Tqdev/PhpCrudApi/RequestUtils.php

@@ -19,12 +19,14 @@ public static function getHeader(ServerRequestInterface $request, string $header
        return isset($headers[0]) ? $headers[0] : '';
    }

-    public static function getParams(ServerRequestInterface $request): array
+    public static function getParams(ServerRequestInterface $request, bool $forceArray = true): array


I don't like this addition here. I feel it could also be solved in the controller by getting the first/last element of all the entries returned.

I've refactored this.

mevdschee · 2020-11-28T07:34:47Z

src/Tqdev/PhpCrudApi/RequestUtils.php

@@ -63,6 +65,17 @@ public static function getOperation(ServerRequestInterface $request): string
                    case 'PATCH':
                        return 'increment';
                }
+            case 'procedures':


Can't we merge this with the 'records' way of determining the operation? Or we implement that the operation is set to the verb in case of procedures. We could also support: /procedures/{table_name}/{verb}.sql, where {table_name} is a name you give this group of stored procedures (could be related to a table).

Verb or 'records' are fine with me. If we go for 'records', how do we distinguish between read and list?

mevdschee · 2020-11-28T07:36:14Z

src/procedures/example.write.sql

+SELECT p.id, p.content, c.name 
+FROM posts p
+INNER JOIN categories c ON c.id = p.category_id
+WHERE p.id = <?= $id ?>


This is a serious security issue, string concatenation in SQL with user input. Judging from your earlier comments and the different approach above, it seems that you are already aware that this is not the way to go.

Yeah, of course this is a particular bad example. But this feature (ob_start, include, ob_get_clean instead of file_get_contents) does give a lot of flexibility, for example:

INSERT INTO users SET username = :username, password = "<?= password_hash($password, PASSWORD_DEFAULT) ?>"

Or

INSERT INTO logs SET foo = :foo, user_ip = "<?= $_SERVER['REMOTE_ADDR'] ?>"

mevdschee

These changes are a great start, let's continue to make this functionality even better! Thank you.

…procedures

mevdschee reviewed Nov 28, 2020

View reviewed changes

mevdschee self-assigned this Nov 28, 2020

mevdschee added the enhancement label Nov 28, 2020

mevdschee mentioned this pull request Dec 11, 2020

Fediverse SNS #735

Closed

rolandboon added 2 commits December 28, 2020 14:17

wip: Add support for running raw SQL files

7c3553a

Add input argument handling, template parsing and http verbs for SQL …

0783de5

…procedures

rolandboon force-pushed the master branch from a43ae83 to 0783de5 Compare December 28, 2020 13:17

Refactor for PR feedback

46c6572

mevdschee mentioned this pull request May 25, 2021

stand alone or use in php-crud-api api.php? mevdschee/php-sp-api#3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Add support for running raw SQL files #729

WIP: Add support for running raw SQL files #729

rolandboon commented Nov 24, 2020 •

edited

mevdschee commented Nov 24, 2020

rolandboon commented Nov 25, 2020

mevdschee commented Nov 27, 2020

mevdschee Nov 28, 2020 •

edited

rolandboon Dec 28, 2020

mevdschee Nov 28, 2020

rolandboon Dec 28, 2020

mevdschee Nov 28, 2020

rolandboon Dec 28, 2020

mevdschee Nov 28, 2020 •

edited

rolandboon Dec 28, 2020

mevdschee Nov 28, 2020 •

edited

rolandboon Dec 28, 2020

mevdschee Nov 28, 2020 •

edited

rolandboon Dec 28, 2020

mevdschee Nov 28, 2020 •

edited

rolandboon Dec 28, 2020

mevdschee left a comment •

edited

WIP: Add support for running raw SQL files #729

Are you sure you want to change the base?

WIP: Add support for running raw SQL files #729

Conversation

rolandboon commented Nov 24, 2020 • edited

mevdschee commented Nov 24, 2020

rolandboon commented Nov 25, 2020

mevdschee commented Nov 27, 2020

mevdschee Nov 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mevdschee Nov 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mevdschee Nov 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mevdschee Nov 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mevdschee Nov 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mevdschee left a comment • edited

Choose a reason for hiding this comment

rolandboon commented Nov 24, 2020 •

edited

mevdschee Nov 28, 2020 •

edited

mevdschee Nov 28, 2020 •

edited

mevdschee Nov 28, 2020 •

edited

mevdschee Nov 28, 2020 •

edited

mevdschee Nov 28, 2020 •

edited

mevdschee left a comment •

edited