SeaQL / sea-query

🔱 A dynamic SQL query builder for MySQL, Postgres and SQLite
https://www.sea-ql.org
Other
1.12k stars 182 forks source link

Exposition of Postgres functions `to_tsquery`, `to_tsvector`, `phraseto_tsquery`, `plainto_tsquery`, `websearch_to_tsquery` are unergonomic because they only accept the regconfig as an u32 #784

Open reivilibre opened 2 months ago

reivilibre commented 2 months ago

Description

The way that the Postgres functions to_tsquery, to_tsvector, phraseto_tsquery, plainto_tsquery, websearch_to_tsquery are exposed in sea-query makes them unergonomic.

Example:

    /// Call `TO_TSVECTOR` function. Postgres only.
    ///
    /// The parameter `regconfig` represents the OID of the text search configuration.
    /// If the value is `None` the argument is omitted from the query, and hence the database default used.
    ///
    /// # Examples
    ///
    /// ```
    /// use sea_query::{tests_cfg::*, *};
    ///
    /// let query = Query::select()
    ///     .expr(PgFunc::to_tsvector("a b", None))
    ///     .to_owned();
    ///
    /// assert_eq!(
    ///     query.to_string(PostgresQueryBuilder),
    ///     r#"SELECT TO_TSVECTOR('a b')"#
    /// );
    /// ```
    pub fn to_tsvector<T>(expr: T, regconfig: Option<u32>) -> FunctionCall
    where
        T: Into<SimpleExpr>,
    {
        match regconfig {
            Some(config) => {
                let config = SimpleExpr::Value(config.into());
                FunctionCall::new(Function::PgFunction(PgFunction::ToTsvector))
                    .args([config, expr.into()])
            }
            None => FunctionCall::new(Function::PgFunction(PgFunction::ToTsvector)).arg(expr),
        }
    }

The problem is that it accepts the regconfig as a u32, whereas in my experience from other code bases and from the code examples in Postgres' own manual, it is more conventional to pass this as a string such as 'english' or 'simple'.

Ref: https://www.postgresql.org/docs/current/textsearch-controls.html#TEXTSEARCH-PARSING-QUERIES

It looks like you'd have to issue queries such as the following to look it up before making the query:

postgres=# SELECT 'english'::regconfig::int; -- server 1
 int4  
-------
 14123
(1 row)

But it begs the question: why? This needs an extra round trip and makes the code harder to read.

I also note that the functions accept an Option<u32>, not a Option<SimpleExpr> or so, meaning there's no way to avoid the extra round trip — you can't build an expression equivalent to 'english'::regconfig::int and pass it in in lieu of the magic number.

Not only this, but the number is different on different database servers, which makes this a hazard for applications that may connect to multiple database servers (as a valid idea might be to perform the lookups on startup to avoid the extra round trip for every query, but then you have to remember which numbers are for which servers):

postgres=# SELECT 'english'::regconfig::int; -- server 2
 int4  
-------
 13169
(1 row)

I am not sure about whether backwards compatibility is needed or not or whether this would be accepted, but my suggestion would be to let people pass in strings here instead.

Versions

0.31.0-rc.7

tuanla-mirabo commented 3 weeks ago

@reivilibre You can use Func::cust and pass 'english'::regconfig::int as an argument.