[vlc-devel] [PATCH] Prepared statements for SQL

[Srikanth Raju]

Hi,

On Tue, Oct 20, 2009 at 10:48 PM, Laurent Aimar <fenrir at via.ecp.fr> wrote:

> Hi,
>
> > +#define VLC_SQL_ROW 1
> > +#define VLC_SQL_DONE 2
>  What are they used for? A doxy comment would be usefull.
>
> They're used as return values for sql_Run(). Documented.

>  What are they used for? A doxy comment would be usefull.
> Also: enums.
>
Really necessary?

> +typedef enum {
> > +    SQL_INT,
> > +    SQL_DOUBLE,
> > +    SQL_TEXT,
> > +    SQL_BLOB,
> > +    SQL_NULL
> > +} sql_type_e;
>  sql_type_t for the typedef.
>  What are BLOB and NULL?
>
> BLOBs are binary objects. NULLs are treated specially in databases, as 0 is
not NULL in a database


> > +struct sql_value_t
> > +{
> > +    int length;
> > +    union
> > +    {
> > +        int i;
> > +        double dbl;
> > +        char* psz;
> > +        void* ptr;
> > +    } value;
> > +};
>  You could merge it with:
> > +typedef struct sql_value_t sql_value_t;
>
> I didn't because the other typedefs arent.


>
>
> > +struct sql_stmt_t
> > +{
> > +    sql_t* p_sql;
> > +    sql_stmt_sys_t *p_sys;
> > +};
>  I don't think that explicitly declaring sql_stmt_t is needed.
> It seems that is only needed by the plugin implementation.
>
Needed only by the plugin implementation meaning? It's required by users of
the sql module. They hold references to an sql_stmt_t object and the
internals
vary with the different modules. I think I'm missing something?


>

> +/**
> > + * @brief Prepare an sql statement
> > + * @param p_sql The SQL object
> > + * @param psz_fmt SQL query string
> > + * @param i_length length of the string
> > + * @return a sql_stmt_t pointer or NULL on failure
> > + */
> > +static inline sql_stmt_t* sql_Prepare( sql_t* p_sql, const char*
> psz_fmt,
> > +        int i_length )
> > +{
> > +    return p_sql->pf_prepare( p_sql, psz_fmt, i_length );
> > +}
>  I don't understand what i_length is. If it is strlen(psz_fmt), then it is
> useless, otherwise a more explicit documentation is needed.
>

I've documented it. jpd and Paul Corke are right. It is perfectly ok to put
0s in a database because they are treated differently from NULL. Using a -ve
length will select the psz_fmt upto the null terminator though.


>
> > +/**
> > + * @brief Bind arguments to a sql_stmt_t object
> > + * @param p_sql The SQL object
> > + * @param p_stmt Statement Object
> > + * @param type Data type of the value
> > + * @param p_value Value to be bound
> > + * @param i_length For values which require a length
> > + * @param i_pos Position at which the parameter should be bound
> > + * @return VLC_SUCCESS or VLC_EGENERIC
> > + */
> > +static inline int sql_BindGeneric( sql_t* p_sql, sql_stmt_t* p_stmt,
> > +        sql_type_e type, sql_value_t* p_value, int i_pos )
>  I think using int instead of sql_type_e is better for binary
> compatibility.
>
Type safety?


> Is p_value modified by the function? (Ifnot, a const is needed).
>
No. Added.


>
>  I think more advanced wrapper would be helpful, they could completly
> hide sql_value_t.
>
> > +/**
> > + * @brief Reset the SQL statement
> > + * @param p_sql The SQL object
> > + * @param p_stmt The sql statement object
> > + * @return VLC_SUCCESS or VLC_EGENERIC
> > + */
> > +static inline int sql_Reset( sql_t* p_sql, sql_stmt_t* p_stmt )
> > +{
> > +    return p_sql->pf_reset( p_sql, p_stmt );
> > +}
>  What exactly does this function? Is it the same than finalize+prepare?
>
No. It unbinds the values that were "bound" to the statement. Documented.

>

> +/**
> > + * @brief Get the datatype of the result of the column
> > + * @param p_sql The SQL object
> > + * @param p_stmt The sql statement object
> > + * @param i_col The column
> > + * @param type pointer to datatype of the given column
> > + * @return VLC_SUCCESS or VLC_EGENERIC
> > + */
> > +static inline int sql_GetColumnType( sql_t* p_sql, sql_stmt_t* p_stmt,
> > +        int i_col, sql_type_e* type )
> > +{
> > +    return p_sql->pf_gettype( p_sql, p_stmt, i_col, type );
> > +}
> > +
> > +/**
> > + * @brief Get the column data
> > + * @param p_sql The SQL object
> > + * @param p_stmt The statement object
> > + * @param i_col The column number
> > + * @param type Datatype of result
> > + * @param p_res The structure which contains the value of the result
> > + * @return VLC_SUCCESS or VLC_EGENERIC
> > + */
> > +static inline int sql_GetColumn( sql_t* p_sql, sql_stmt_t* p_stmt,
> > +        int i_col, sql_type_e type, sql_value_t *p_res )
> > +{
> > +    return p_sql->pf_getcolumn( p_sql, p_stmt, i_col, type, p_res );
> > +}
>  Why is type needed? (Especially when sql_GetColumnType seems to be able
> to retreive it).
>
Because databases might do type casting. If there is an INT at i_col = 1,
but you ask for a string, it will perform an itoa() and return the value.
Atleast, that is how the sqlite interface is implemented.


> > +#define sql_GetIntegerColumn( A, B, C, D ) sql_GetColumn( A, B, C,
> SQL_INT, D )
> > +#define sql_GetDoubleColumn( A, B, C, D ) sql_GetColumn( A, B, C,
> SQL_DOUBLE, D )
> > +#define sql_GetTextColumn( A, B, C, D ) sql_GetColumn( A, B, C,
> SQL_TEXT, D )
> > +#define sql_GetBlobColumn( A, B, C, D ) sql_GetColumn( A, B, C,
> SQL_BLOB, D )
>  I think more advanced wrapper would be helpful, they could completly hide
> sql_value_t.
>

I'm not sure why one would want to hide the type of the value, because that
was one of the reasons the prepared statement interface is being included.
So that there may be stronger type checking.


> I am not sure how to improve this, but it begins to make sql_t having
> > an awful lot of functions, and also to be hard to know which ones work
> > together. Anyone has an idea?
>
> Prepared statements take a few extra functions, but perhaps one can
> merge functionality with the non-prepared versions. Though if that
> causes lots of extra complexity, then perhaps not.
>
By non-prepared versions I assume you mean the Query(sqlquery,...)
interface.
The non-prepared versions utilise the prepared statement interface
internally. The point of having the prepared statement interface was to have
control over values being bound. Trying to do this with the the non-prepared
versions doesn't make sense, because it brings back the sql injection
problem.
I'm not sure how you can combine "binding" with sqlquery. Functionality very
close to this already exists in the sql_Printf() interface.

-- 
Regards,
Srikanth Raju
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20091027/54b275a9/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Sql-Include-interface-for-prepared-statements.patch
Type: text/x-diff
Size: 6735 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20091027/54b275a9/attachment.patch>