damil / SQL-Abstract-More

extension to SQL-Abstract with named parameters and support for several additional SQL clauses
https://metacpan.org/pod/SQL::Abstract::More
6 stars 10 forks source link

Illegal SQL generated with empty arrayref on IN clause #20

Open jjatria opened 2 years ago

jjatria commented 2 years ago

The following code:

perl -MSQL::Abstract::More -E '
    SQL::Abstract::More->new->select(
        -from => "table",
        -where => { account => { -in => [] } }
    )
'

generates the following SQL

SELECT * FROM table WHERE account IN ( )

which is illegal.

The equivalent code with SQL::Abstract:

perl -MSQL::Abstract -E '
    SQL::Abstract->new->select(
        table => ["*"] => { account => { -in => [] } }
    )
'

generates

SELECT * FROM table WHERE 0=1

which is legal (and is what used to be generated before the SQL::Abstract rewrite in 2.000000).

After tracking this for a while, this seems to be down to the _where_field_IN override in SQL::Abstract::More. When SQL::Abstract runs on its own (or finds no override) it uses its own handler generated by make_binop_expander, and that generates the correct output. The override from SQL::Abstract::More finishes with

$self->next::method($k, $op, $vals)

but that passes the buck to the _where_field_IN method in SQL::Abstract, which does not do the right thing:

perl -MSQL::Abstract -E'
    SQL::Abstract->new->_where_field_IN( foo => in => []  )
'
# foo IN ( )

Further demonstration of this is that deleting the override in SQL::Abstract::More makes this go back to the old behaviour:

perl -MSQL::Abstract::More -E '
    delete $SQL::Abstract::More::{_where_field_IN};
    SQL::Abstract::More->new->select(
        -from => "table",
        -where => { account => { -in => [] } }
    )
'    
# SELECT * FROM table WHERE 0=1

I don't know if this is an issue with SQL::Abstract::More, SQL::Abstract, or something else, though.

damil commented 2 years ago

Hi, Thanks for this report and for your detailed study of the interaction between SQLAM and SQLA. When Matt Trout worked on SQLA 2.0 he took great care of backwards compatibility, we had several exchanges about that ... but it does not necessarily include kind-of private internal methods like _where_field_IN. I still don't know which is the best way to solve this issue .. maybe SQLAM should just implement it directly instead of calling next::method ... or maybe it should directly call make_binop_expander. I'll reserve some time to consider the various options and then publish a new version. Thanks again, Laurent

jjatria commented 2 years ago

Any updates on this by any chance? Or should I maybe raise this up as an issue with SQL::Abstract?

damil commented 2 years ago

Hi, thanks for the heads up. Looking again at SQLA, I found this line in the Changes log :

1.90_03 - 2019-10-13

  • Add proof of concept DBIx::Class::SQLMaker::Role::SQLA2Passthrough
  • _where_field_IN/BETWEEN are documented as subclassable; feature restored

Indeed -where_field_IN is mentioned in the doc, so it makes sense to raise up the issue with SQL::Abstract instead of trying to fix it in SQLAM .. so please go ahead with your suggestion

jjatria commented 2 years ago

Thanks. I've raised it as an issue with SQL::Abstract: https://rt.cpan.org/Ticket/Display.html?id=142342

Let's see what happens :popcorn: