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.

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

Do not generate incorrect SQL in presence of -and=>[] #15

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fabzzap
Copy link
Contributor

@fabzzap fabzzap commented Aug 29, 2018

An empty array ref as value for -and can cause incorrect SQL. Example:

$ perl -Mv5.12 -MSQL::Abstract -e 'my ($sql,@bind)=SQL::Abstract->new->where({-and=>[],-or=>[a=>1]});say $sql'
Use of uninitialized value in join or string at /usr/share/perl5/SQL/Abstract.pm line 1482.
 WHERE ( (  AND a = ? ) )

This is a proposed fix to that bug

@codecov-io
Copy link

codecov-io commented Aug 29, 2018

Codecov Report

Merging #15 into master will increase coverage by 0.12%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #15      +/-   ##
==========================================
+ Coverage   83.06%   83.18%   +0.12%     
==========================================
  Files           3        3              
  Lines         927      928       +1     
  Branches      189      189              
==========================================
+ Hits          770      772       +2     
+ Misses        107      106       -1     
  Partials       50       50
Impacted Files Coverage Δ
lib/SQL/Abstract.pm 82.47% <100%> (+0.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4805666...3e759b1. Read the comment docs.

@coveralls
Copy link

coveralls commented Aug 29, 2018

Pull Request Test Coverage Report for Build 28

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 88.414%

Totals Coverage Status
Change from base Build 26: -0.2%
Covered Lines: 786
Relevant Lines: 889

💛 - Coveralls

@mohawk2
Copy link

mohawk2 commented Jan 29, 2019

Surely there should be tests? And rather than just silently dropping things, this should cause an exception?

@fabzzap
Copy link
Contributor Author

fabzzap commented Feb 5, 2019

I just pushed a test for the patch. I don't know what "silently drop[s] things" here. -and=>[] is supposed to have no effect, but the main code instead generates a warning and incorrect SQL. With the patch, it behaves correctly: it does nothing.

@dbsrgits-sync dbsrgits-sync force-pushed the master branch 2 times, most recently from cd96b8a to d291f33 Compare January 21, 2021 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants