pBlueG / SA-MP-MySQL

MySQL plugin for San Andreas Multiplayer
BSD 3-Clause "New" or "Revised" License
196 stars 80 forks source link

Proposal for file-based query templates with format specifier support #210

Open Southclaws opened 5 years ago

Southclaws commented 5 years ago

Storing queries in files is common in other languages, it also provides things like syntax highlighting, no more writing queries with \ line endings across horrible string literals and better git history for data model changes.

Unfortunately, the current API that reads SQL queries from files does not support format specifiers in the actual query because the variadic parameters for the function call are used for passing locally accessible data to the callback function, such as identifiers etc.

This proposal includes two possible solutions for this problem:

Solution 1: Extend the variadic parameters

This one is the simplest to implement and merely involves using the remaining variadic parameters for the query, for example:

get_admins_level_date.sql:

select
    players.id,
    players.name,
    admins.level,
    admins.date_assigned
from players
join admins on players.id = admins.player_id
where
    admins.level > ? and
    admins.date_assigned > ?

get_admins_level_date.pwn:

stock DB_GetAdminsByLevelAfterDate(MySQL:handle, level, Timestamp:date, playerid) {
    mysql_tquery_file(
        handle,
        "sql/get_admins_level_date.sql",
        "OnGetAdmins",
        "d",
        playerid, // for the "d" callback parameter specifier
        level,    // for the SQL query
        _:date    // for the SQL query
    );
}

My concern with this approach is that it could result in non-obvious bugs as it requires counting the parameters in two separate places.

Solution 2: New API for file-based queries

This would require a bit more work and testing since it introduces a new API but I feel like this is a better approach overall since it separates the variadics into two different function calls.

get_admins_level_date.sql:

select
    players.id,
    players.name,
    admins.level,
    admins.date_assigned
from players
join admins on players.id = admins.player_id
where
    admins.level > ? and
    admins.date_assigned > ?

get_admins_level_date.pwn:

stock DB_GetAdminsByLevelAfterDate(MySQL:handle, level, Timestamp:date, playerid) {
    mysql_tquery_file_exec(
        handle,
        mysql_tquery_file_load(
            "sql/get_admins_level_date.sql",
            level,
            _:date
        ),
        "OnGetAdmins",
        "d",
        playerid
    );
}

The signatures for the new API would be:

This would require a very simple pool of FileQuery: objects which would be instantiated by mysql_tquery_file_load and then immediately garbage collected inside mysql_tquery_file_exec.


There also may be another better way to do this so feel free to propose an API design for this in this issue!

Vince0789 commented 5 years ago

Not a fan of the nested structure, to be honest. If a a statement needs to be executed more than once within the same method (e.g. multiple concurrent inserts) then it should only be loaded once, and before entering the loop, akin to how one would use a prepared statement. Garbage collection should then only happen after said loop. Not too sure how that should be implemented, though. Explicit garbage collection opens the doors to memory leaks.

PatrickGTR commented 5 years ago

This addition would be so nice. Readability with be so much better without having so many backslash in your code, for a long queries. This requires more attention tbh.

Alasnkz commented 4 years ago

You could always just add a delete function so you can manage it.

Something like:

new QueryFile:query = mysql_query_file_load("sql/my_admin.sql");

mysql_tquery_file_exec(handle, query, "OnAdmin", "sd", "Josh", 50, "i", playerid);

mysql_query_file_delete(query);

The issue is the function header for file_exec will be a bit daunting because the callback specifiers will be crammed in with the variadic arguments so the plugin will count the specifiers for the query "sd" (so 2) read them, then expect a callback specifier if there's any.

Southclaws commented 4 years ago

Or just implement the operator~ function for QueryFile

Alasnkz commented 4 years ago

I forgot that existed, is there any reference to that in any other libraries that I can take a peek at?

Southclaws commented 4 years ago

Check JSON plugin or pawn requests

Alasnkz commented 4 years ago
new MySQL:handle = mysql_connect("localhost", "root", "", "test");
new QueryFile:query_file = mysql_query_file_load("find_ip_from_name.sql");
mysql_tquery_file_exec(handle, query_file, "e", "josh", "OnPlayerIPFound", "i", 255);

Here's find_ip_from_name.sql

select *
    from players
where
    name = ?
LIMIT 1;

Here's a current example of the API i have right now,trouble is the callback name and specifiers are at the end with the other varargs so you can't really tell if it is a callback if you don't have the On keyword.

Also if you have a better naming scheme I'm all ears. It is too close to mysql_query_file for my liking.

Southclaws commented 4 years ago

suggestion 2 best .

Alasnkz commented 4 years ago

If we wanted it to be GC'd every time that is executed bro

Southclaws commented 4 years ago

you're never going to have more than one active anyway though so it doesn't even need a "GC" it just needs a single place to store these and each time you call it, it overwrites the previous data.