php-crud-api icon indicating copy to clipboard operation
php-crud-api copied to clipboard

WIP: Add support for running raw SQL files

Open rolandboon opened this issue 4 years ago • 3 comments

Work in progress on this issue: https://github.com/mevdschee/php-crud-api/issues/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:

  • [x] Accept/handle input arguments
  • [ ] Tests
  • [ ] Readme
  • [x] Additional routes
  • [ ] ~~JSON root key?~~
  • [ ] OpenAPI spec generation?

rolandboon avatar Nov 24 '20 14:11 rolandboon

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.

mevdschee avatar Nov 24 '20 15:11 mevdschee

Quick update:

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

And I've added input argument handling in 2 ways

  1. 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

  2. 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

rolandboon avatar Nov 25 '20 17:11 rolandboon

I will review your parsing/template options shortly.

mevdschee avatar Nov 27 '20 13:11 mevdschee