Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Illegal SQL generated with empty arrayref on IN clause #20

Open
jjatria opened this issue Feb 9, 2022 · 4 comments
Open

Illegal SQL generated with empty arrayref on IN clause #20

jjatria opened this issue Feb 9, 2022 · 4 comments

Comments

@jjatria
Copy link

jjatria commented Feb 9, 2022 •
edited
Loading

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
Copy link
Owner

damil commented Feb 9, 2022

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
Copy link
Author

jjatria commented Apr 20, 2022

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

@damil
Copy link
Owner

damil commented Apr 21, 2022

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
Copy link
Author

jjatria commented Apr 21, 2022

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 🍿

jjatria added a commit to jjatria/sql-abstract that referenced this issue Sep 7, 2022
@jjatria
Handle empty array argument to in in subclasses
0f94bfb 
Subclasses that override `_where_field_IN` and `_where_field_BETWEEN`
and conditionally re-dispatch to the default handlers in SQL::Abstract
were not correctly handling empty array references passed as arguments
to `-in`, `-between`, and their negated counterparts.

This behaviour was broken in the refactor that lead to 2.000000, and
more specifically in 1.90_03, going from

    $ perl -Ilib -MSQL::Abstract -E '
      say "version " . $SQL::Abstract::VERSION;
      package X {
        use parent "SQL::Abstract";
        use mro "c3";
        sub _where_field_IN { shift->next::method(@_) }
      }
      say X->new->select( table => ["*"], { foo => { -in => [] } } )
    '
    version 1.9002
    SELECT * FROM table WHERE 0=1

to

    $ perl -Ilib -MSQL::Abstract -E '
      say "version " . $SQL::Abstract::VERSION;
      package X {
        use parent "SQL::Abstract";
        use mro "c3";
        sub _where_field_IN { shift->next::method(@_) }
      }
      say X->new->select( table => ["*"], { foo => { -in => [] } } )
    '
    version 1.9003
    SELECT * FROM table WHERE foo IN ( )

and resulting in illegal SQL.

This was breaking behaviour that resulted in downstream issues affecting
at least SQL::Abstract::More
(see damil/SQL-Abstract-More#20).

Likewise, a class overriding `_where_field_BETWEEN` was not triggering
the parameter validation that disallowed empty array references or
undefined values to be used as parameters to `-between` and its
negation:

    $ perl -Ilib -MSQL::Abstract -E '
      say "version " . $SQL::Abstract::VERSION;
      package X {
        use parent "SQL::Abstract";
        use mro "c3";
        sub _where_field_BETWEEN { shift->next::method(@_) }
      }
      say X->new->select( table => ["*"], { foo => { -between => [] } } )
    '
    version 1.9003
    SELECT * FROM table WHERE ( foo BETWEEN AND )

This change ensures that the path taken by subclasses that override
those methods triggers equivalent code paths to those that don't.

Fixes https://rt.cpan.org/Ticket/Display.html?id=142341
@jjatria jjatria mentioned this issue Sep 7, 2022
Open
jjatria added a commit to jjatria/sql-abstract that referenced this issue Sep 7, 2022
@jjatria
Handle empty arrays in -in and -between in subclasses
6c27863 
Subclasses that override `_where_field_IN` and `_where_field_BETWEEN`
and conditionally re-dispatch to the default handlers in SQL::Abstract
were not correctly handling empty array references passed as arguments
to `-in`, `-between`, and their negated counterparts.

This behaviour was broken in the refactor that lead to 2.000000, and
more specifically in 1.90_03, going from

    $ perl -Ilib -MSQL::Abstract -E '
      say "version " . $SQL::Abstract::VERSION;
      package X {
        use parent "SQL::Abstract";
        use mro "c3";
        sub _where_field_IN { shift->next::method(@_) }
      }
      say X->new->select( table => ["*"], { foo => { -in => [] } } )
    '
    version 1.9002
    SELECT * FROM table WHERE 0=1

to

    $ perl -Ilib -MSQL::Abstract -E '
      say "version " . $SQL::Abstract::VERSION;
      package X {
        use parent "SQL::Abstract";
        use mro "c3";
        sub _where_field_IN { shift->next::method(@_) }
      }
      say X->new->select( table => ["*"], { foo => { -in => [] } } )
    '
    version 1.9003
    SELECT * FROM table WHERE foo IN ( )

and resulting in illegal SQL.

This was breaking behaviour that resulted in downstream issues affecting
at least SQL::Abstract::More
(see damil/SQL-Abstract-More#20).

Likewise, a class overriding `_where_field_BETWEEN` was not triggering
the parameter validation that disallowed empty array references or
undefined values to be used as parameters to `-between` and its
negation:

    $ perl -Ilib -MSQL::Abstract -E '
      say "version " . $SQL::Abstract::VERSION;
      package X {
        use parent "SQL::Abstract";
        use mro "c3";
        sub _where_field_BETWEEN { shift->next::method(@_) }
      }
      say X->new->select( table => ["*"], { foo => { -between => [] } } )
    '
    version 1.9003
    SELECT * FROM table WHERE ( foo BETWEEN AND )

This change ensures that the path taken by subclasses that override
those methods triggers equivalent code paths to those that don't.

Fixes https://rt.cpan.org/Ticket/Display.html?id=142341
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@damil @jjatria