[Dbix-class] Incorrect SQL being generated after DBIC library upgrade

[demerphq]

2009/3/20 Dami Laurent (PJ) <laurent.dami at justice.ge.ch>:
> Hi all,
>
> I am responsible for those changes in -and/-or behaviour between 1.24
> and 1.50.
>
> The point is: it was impossible to keep 1.24 behaviour on every detail,
> because 1.24 had several internal inconsistencies, and also was
> inconsistent with its own documentation. So understanding the old arcane
> behaviour, finding out where it
> differed from the doc, and deciding what to do was really a tough task.
> Please read carefully the CHANGES Pod section in 1.50, which starts by
> saying :
>
>   Great care has been taken to preserve the I<published> behavior
>   documented in previous versions in the 1.* family; however,
>   some features that were previously undocumented, or behaved
>   differently from the documentation, had to be changed in order
>   to clarify the semantics. Hence, client code that was relying
>   on some dark areas of C<SQL::Abstract> v1.*
>   B<might behave differently> in v1.50.
>
>
> If you want more details, also read the notes marked LDNOTE in the
> source code.
>
> In particular :
>
> - {-and/-or  => [...]} were indeed supported in older versions, but
> meanwhile the
>  old doc always said :
>
>   B<Important:> Note that the C<-modifier> goes
>   B<INSIDE> the arrayref, as an extra first element. This will
>   B<NOT> do what you think it might:
>
>       priority => -and => [{'!=', 2}, {'!=', 1}]   # WRONG!
>
> - nesting of -and / -or was implemented through a global variable, with
> silly
>  side-effects
>
>           # LDNOTE : previous SQLA code for hashrefs was creating a
> dirty
>           # side-effect: the first hashref within an array would change
>           # the global logic to 'AND'. So [ {cond1, cond2}, [cond3,
> cond4] ]
>           # was interpreted as "(cond1 AND cond2) OR (cond3 AND
> cond4)",
>           # whereas it should be "(cond1 AND cond2) OR (cond3 OR
> cond4)".


I dont mean to be a hard ass about this, but you cant change this and
keep the name SQL::Abstract without wreaking havoc on the
SQL::Abstract ecosystem.

There are acres and acres and acres of code depending on the behaviour
i illustrated.

Changing it is a serious problem.


Also your example doesnt suport this change. Note what is said:

priority => -and => [{'!=', 2}, {'!=', 1}]   # WRONG!

thats because the right thing would be either of the following:

$query = ['-and',[{'priority' => {'!=' => 2}},{'priority' => {'!=' => 1}}]];
$sql = ' WHERE ( ( ( ( priority != ? ) AND ( priority != ? ) ) ) )';
@args = (2,1);
####
$query = ['-and',['priority',{'!=' => 2},'priority',{'!=' => 1}]];
$sql = ' WHERE ( ( ( ( priority != ? ) AND ( priority != ? ) ) ) )';
@args = (2,1);
####

The crucial point here is that the docs are pointing out that you cant say:

COLUMN => OPER => [ {OP => VALUE}, {OP => VALUE}

Which is no big surprise really if you grok how hashes work.

However we arent talking about this case, we are talking about the
case where it is

OPER => [ CLAUSES ]

which i think IS documented to work as i detailed (see the bit on the
-nest operator).

And yes the docs are crap, and sometimes contradictory, however I
think this one isnt open to debate. This breakage is signifigant
enough that it should be fixed, or SQL::A 1.50 should be withdrawn and
renamed.  And to be absolutely clear i take absolutely NO pleasure in
saying that. I really hope that its fixable.

Incidentally, im speaking here as a person who has written at least
one SQL::A drop in replacement that passed all of the SQL::A tests.
So i have some experience on the subject and quite a bit of sympathy
for you in regard to this issue and a lot of respect that you took it
on. ++ to you.

I didnt publish my version for various reasons, however Matt Trout saw
it, and I still have a copy if you want a look.

Cheers,
Yves


-- 
perl -Mre=debug -e "/just|another|perl|hacker/"