[fpc-pascal] Troubles with SQLDBRESTBridge

[Michael Van Canneyt]

On Fri, 21 Jun 2019, Sven Barth via fpc-pascal wrote:

> Hello together!
>
> I've played around a bit with the SQLDBRESTBridge and encountered some
> troubles which I wanted to check up here on the list whether those are
> indeed bugs or not before reporting them to Mantis.
>
> For the tests I've used FPC 3.0.4 and Lazarus 2.0 with the
> SQLDBRESTBridge and the Lazarus examples being from trunk on Friday 14th
> June.
>
> As server I used the restmodule demo in
> $lazarus/components/sqldbrest/demo/restmodule and changed it to use
> SQLite3 instead of PostgreSQL.
> For the client I used the jsonclient demo in
> $lazarus/components/sqldbrest/demo/jsonclient.
>
> I created the database using the expenses-sqlite.sql script in
> $fpc/packages/fcl-web/examples/restbridge/expenses-sqlite.sql (and no I
> didn't execute expenses-data.sql at first which will be important
> further down ;) ).
>
> I started the server and tried to access localhost:7331/REST/metadata (I
> changed the port from 8080), but this resulted in a 404 error which was
> displayed as an empty page (maybe something isn't correctly working
> here?). Using localhost:7331/metadata resulted in a "Not found" exception.

This I fixed. I had noticed the same, but didn't have time to fix.
It is fixed now.

>
> So I started to research what went wrong and found various points:
> - the metadata was coupled with the rdoConnectionInURL flag in
> TSQLDBRestDispatcher.DoRegisterRoutes instead of rdoExposeMetadata (see
> patch exposematadata.patch)

This is not quite correct. The rdoConnectionInURL check is also necessary:

Both connection/metadata and /metadata must work.

connection/metadata is handled by HandleRequest (FindSpecialResource)
So a special registration for metadata must be handled here.

But there was indeed a bug in the code, I fixed it properly.


> - then it turned out that the routes are only registered once the module
> was loaded and thus they would never be found so I adjusted
> TSQLDBRestModule.HandleRequest to call the router again (this of course
> required to change the BasePath of the TSQLDBDispatcher in the example
> to 'REST' instead of '') (see patch reroute.patch) - this point I'm
> definitely not sure whether it is the right solution (but in the end it
> works)

This is not the correct solution.

The module itself handles the routes: the module is
registered e.g. under /REST and then the request is dispatched to the
router, and the dispatcher will take care of the rest.

This means of course that when you use a restmodule, the dispatcher
'RegisterRoutes' must not be called at all, and this can be achieved by
setting Active to False.

Maybe the meaning of active is a bit obtuse. I put a comment in when I made
it, but if that goes unnoticed...

In general, I added the rest module only for completeness. I think the
simple datamodule approach (as demoed in restbridge demo) is the better way:
this keeps the schema in memory. for cases where you want to control the
connections through the REST api, this is needed anyway.

The REst Module has the additional disadvantage that you must have an
initial /REST/ or whatever part in your URL. With the dispatcher on a
datamodule, you can skip this if so desired...

> - then the problem was that the module's route ('REST/*/') took
> precedence over the routes added by the TSQLDBRestDispatcher, so I
> implemented a sorting of the routes so that more specific ones are
> encountered first (see patch sort-routes.patch) - don't know whether
> this is the right approach either, but this *is* a problem

No, see above.

> - then the fact that TSQLDBRestModule is by default set to wkOneShot
> lead to the routes being registered again or more precisely the old
> routes being left over and pointing to a freed TSQLDBRestDispatcher
> instance; thus I added a call to UnRegisterRoutes to
> TSQLDBRestDispatcher.Destroy (see unregister-routes.patch)

Well, since they are not supposed to be registered in the first place...

>
> With these changes the server worked, but then I encountered two
> problems in the client:
> - if the database table is empty an empty JSON object ('{}') is returned
> resulting in the client complaining about the missing metadata (after
> this I executed the expenses-data.sql script)

Indeed. 
I fixed that, an error on my end, probably got in after some refactoring :(

> - when changing from one resource to another I got an exception that the
> operation is not possible on an active dataset; I solved this for me by
> closing the dataset before executing LoadFromStream (see patch
> jsonclient.patch)

Applied, thanks !

>
> And an additional note regarding the expenses-sqlite.sql script. The
> intention appears to be that the ID columns are auto increment. This
> will however *only* work if the type of the columns is "integer primary
> key". "bigint primary key" won't work here. See also:
> https://www.sqlite.org/autoinc.html
> Thus as with patch expenses-sqlite.sql I updat the SQL script for that
> database.

Yes, the autoincrement in sqlite is rather weird, it seems I forgot to adapt
the script for this :(

I applied your patch, thank you !

>
> If these are indeed bugs I'll either commit the patches as is (e.g. the
> one for the SQLite script is rather surely a bug) or open a bug report
> for the more complex ones.

Well, I have applied patches where necessary.

>
> I haven't tested more yet, so I don't know whether there are any further
> problems. :)

Well, I yesterday created a demo for TMS Software's TMS Web core using the 
SQLDB Rest bridge as a backend, so I'm pretty sure it works 99%. 
They will include a component that can be dropped on a web form which 
will allow you to enter SQL etc.

With your patches, we got closer to 100%, so many thanks for that !! :-)

Michael.