Two TZParams bugs

The offical for ZeosLib 7.3 Report problems, ask for help, post proposals for the new version of Zeoslib 7.3/v8
Quick Info:
-We made two new drivers: odbc(raw and unicode version) and oledb
-GUID domain/field-defined support for FB
-extended error infos of Firebird
-performance ups are still in queue
In future some more feature will arrive, so stay tuned and don't hassitate to help
Post Reply
User avatar
aehimself
Zeos Dev Team
Zeos Dev Team
Posts: 765
Joined: 18.11.2018, 17:37
Location: Hungary

Two TZParams bugs

Post by aehimself »

Hello,

I found two bugs with the TZparams handling. Imagine the following code:

Code: Select all

 ZQuery1.SQL.Text := ':pParam1 :pParam2 :pParam3';
 ZQuery1.Params[0].AsDateTime := Now;
 ZQuery1.Params[1].AsDateTime := Now;
 ZQuery1.Params[2].AsString := 'Hello, world';

 ZQuery1.SQL.Text := ':pParam5';
 If Not ZQuery1.Params[0].IsNull Then
   ShowMessage(ZQuery1.Params[0].AsString);
As it is a new parameter, I expect it to be empty but a message box will pop up, containing the date. I don't think this is correct.

Second one is an access violation and I have no clue what is going on...

Code: Select all

Var
 dict: TDictionary<String, Variant>;
 a: Integer;
begin
 dict := TDictionary<String, Variant>.Create;
 Try
  ZQuery1.SQL.Text := ':pParam1 :pParam2 :pParam3';
  ZQuery1.Params[0].AsDateTime := Now;
  ZQuery1.Params[1].AsDateTime := Now;
  ZQuery1.Params[2].AsString := 'Hello, world';

  For a := 0 To ZQuery1.Params.Count - 1 Do
   dict.AddOrSetValue(ZQuery1.Params[a].Name, ZQuery1.Params[a].Value);
 Finally
  FreeAndNil(dict);
 End;
End;
Error is raised at the string parameter, call stack is ZParam.GetAsVariant, line 2336:

UnicodeString(TVarData(Result).{$IF declared(VUString)}VUString{$ELSE}VAny{$IFEND}) := UnicodeString(FData.pvPointer);

Using Delphi 10.4.1, Zeos 8.0.0-015f7868e:
egonhugeist on 1/20/2021, 5:58:11 PM
Copy&Paste documentations Fix lately introduced old PG-Bug and IB/FB value logs
Delphi 12.1, Zeos 8 from latest GIT snapshot
Using:
- MySQL server 8.0.18; libmariadb.dll 3.3.8
- Oracle server 11.2.0, 12.1.0, 19.0.0; oci.dll 21.13
- MSSQL 2012, 2019; sybdb.dll FreeTDS_2435
- SQLite 3.45.2
User avatar
aehimself
Zeos Dev Team
Zeos Dev Team
Posts: 765
Joined: 18.11.2018, 17:37
Location: Hungary

Re: Two TZParams bugs

Post by aehimself »

For the first issue (old parameter values are assigned to the new ones) I posted a pull request on GitHub. I modified the code that it does not assign old parameter 0 to new parameter 0, but checks by name instead... I guess that was the originating logic. Now, if you have

Code: Select all

ZQuery.SQL.Text := ':param1';
ZQuery.Params[0].AsDateTime := Now;

ZQuery.SQL.Text := ':param2';
ShowMessage(ZQuery.Params[0].AsString);
this will result an empty string. However,

Code: Select all

ZQuery.SQL.Text := ':param1 :param2';
ZQuery.Params[0].AsDateTime := Now;
ZQuery.Params[1].AsString := 'Hello, world!'

ZQuery.SQL.Text := ':param2';
ShowMessage(ZQuery.Params[0].AsString);
This will show 'Hello, world!'. I guess this was the original logic behind assigning old values. Makes sense to me, anyway :)
Delphi 12.1, Zeos 8 from latest GIT snapshot
Using:
- MySQL server 8.0.18; libmariadb.dll 3.3.8
- Oracle server 11.2.0, 12.1.0, 19.0.0; oci.dll 21.13
- MSSQL 2012, 2019; sybdb.dll FreeTDS_2435
- SQLite 3.45.2
User avatar
EgonHugeist
Zeos Project Manager
Zeos Project Manager
Posts: 1936
Joined: 31.03.2011, 22:38

Re: Two TZParams bugs

Post by EgonHugeist »

ae,
according the AV: Should be resolved. Thanks for pointing this out.

@Jan
According your pull request(s):
Please do not apply the patches yet. I've been testing it and we've to many fails then.
Guys check https://sourceforge.net/p/zeoslib/tickets/469/

The ticket of Jan is resolved but i totaly agree with Fr0sT's statement. I just solved the ticket because of backward compatibility. The behavior of the param assignment if SQL changes is wrong from my POV.

I would propose an optional way of doing that:
1. assign param by index as it's implemented since ages.
2. Assign param by name as aehimself proposes.
3. Do not assign the values if the SQL changes as Fr0sT states.

IMO point 3 should be default. I would propose to add an puplished enumerator on the DataSets and/or the Connection component or an dataset event could be considered instead..
TOnUpdateSQLParamAssign = procedure(Sender: TDataSet; var AssginOption: (on of the enums representing my 3 points)
I'm open for proposals.
Best regards, Michael

You want to help? http://zeoslib.sourceforge.net/viewtopic.php?f=4&t=3671
You found a (possible) bug? Use the new bugtracker dude! http://sourceforge.net/p/zeoslib/tickets/

Image
User avatar
aehimself
Zeos Dev Team
Zeos Dev Team
Posts: 765
Joined: 18.11.2018, 17:37
Location: Hungary

Re: Two TZParams bugs

Post by aehimself »

EgonHugeist wrote: 22.01.2021, 07:09I've been testing it and we've to many fails then.
To be honest this is expected if the test relies on the previous values of different parameters. In this case the test is optimized for an incorrect behavior in my opinion.
EgonHugeist wrote: 22.01.2021, 07:09I would propose an optional way of doing that:
1. assign param by index as it's implemented since ages.
2. Assign param by name as aehimself proposes.
3. Do not assign the values if the SQL changes as Fr0sT states.
I would prefer #3. If we must keep old values, #2. Checking for indexes only will behave unexpectedly in several cases:

UPDATE MyTable SET FIELD1 = :pField1Value, FIELD2 = :pField2Value WHERE ID = :pID
UPDATE OtherTable SET ForeignKey = :pID

or

UPDATE MyTable SET FIELD = NULL WHERE ID = :pID
DELETE FROM MyOtherTable WHERE ID = :pOtherID

I'll run some tests with the built-in DataSet component to verify.
Delphi 12.1, Zeos 8 from latest GIT snapshot
Using:
- MySQL server 8.0.18; libmariadb.dll 3.3.8
- Oracle server 11.2.0, 12.1.0, 19.0.0; oci.dll 21.13
- MSSQL 2012, 2019; sybdb.dll FreeTDS_2435
- SQLite 3.45.2
marsupilami
Platinum Boarder
Platinum Boarder
Posts: 1918
Joined: 17.01.2011, 14:17

Re: Two TZParams bugs

Post by marsupilami »

EgonHugeist wrote: 22.01.2021, 07:09 1. assign param by index as it's implemented since ages.
Doesn't make sense - parameters have names for a reason. Humans like working with names and not with numbers.
EgonHugeist wrote: 22.01.2021, 07:09 2. Assign param by name as aehimself proposes.
This possiblity makes the most sense. If people expect parameters to keep their value (i.e. me) it is what they will expect.
EgonHugeist wrote: 22.01.2021, 07:09 3. Do not assign the values if the SQL changes as Fr0sT states.
Why would we implement this? What is the gain or the win in this behavior? People who expext this will assign values to all paramaters anyway - wether we implement it like this or not.

One example - I do have a program where I need to get a list of people who celebrated their birth days in the last days or who will celebrate their birth days in the next days. This is the (simplified) implementation:

1) Have a query sitting on my form with the following sql:

Code: Select all

select X.* from (
  SELECT ID, Name, Surname, birthday, cast(EXTRACT(month from birthday) * 100 + Extract(day from birthday) as integer) as birthday_c
  FROM clients C
  WHERE (remindme = 'y') AND ((state <> 'dead') or (state is null)) and ((cast(:USERID as idtype) in (Advisor1, Advisor2)) or (cast(:CanSeeAllClients as yesno) = 'y'))
)
AS X
/* has to be added / changed by code: */
/* where birthday_c >= :MINDAY and birthday_c <= :MAXDAY */
Please remember that this code is more complex in the real world example as it has more parameters and fetches birthdays from different tables. (@Egonhugeist: FrameStartformularGeburtstage)

2) The folowing code in the initialization of the form:

Code: Select all

    BirthdaysQ.ParamByName('USERID').AsString := frmDatamodule.UserID;
    BirthdaysQ.ParamByName('CanSeeAllClients').AsString := frmDatamodule.CheckUserPermissionDB(psSeeAllClients);
3) The following code to react to user input:

Code: Select all

procedure TStartformularGeburtstageFrame.ApplyChangedBirthdayParams;
var
  StartDate, EndDate: TDate;
  from, until: Integer;
  y, m, d: Word;
begin
  StartDate := IncDay(date, BackDaysEdt.Value * (-1));
  EndDate := IncDay(date, NextDaysEdt.Value);

  DecodeDate(StartDate, y, m, d);
  from := m * 100 + d;
  DecodeDate(EndDate, y, m, d);
  until := m * 100 + d;

  if from <= until then
    // All is in the same year
    with ZQ_Partner.SQL do Strings[Count - 1] := 'where birthday_c >= :MINDAY and birthday_c <= :MAXDAY'
  else
    // Look in old and new year
    with ZQ_Partner.SQL do Strings[Count - 1] := 'where birthday_c >= :MINDAY or  birthday_c <= :MAXDAY';

  BirthdaysQ.Close;
  BirthdaysQ.ParamByName('minday').AsInteger := from;
  BirthdaysQ.ParamByName('maxday').AsInteger := until;
  BirthdaysQ.Open;
end;
So while it is true that - strictly speaking - I generate a new query when going through ApplyChangedBirthdayParams, it basically stays the same (more complex) query and only the last line - the way that birthdays get evaluated changes.
This also allows me to split the problem in three (logically separated) parts:
1) Generate a query that works - saved in the form
2) Assign the paramaters that have to be applied at all times right at the start. So I can't forget to set them later on.
3) Assign the parameters that the user is allowed to change.

By implementing option three we break the users option to work this way. Also we break old code that worked until now. And honestly I fail to see what we (and others) can gain by implementing this.

Side note: I do know (now) that there are ways to write the SQL so I don't have to change the SQL anymore. But I didn't know that 15 years ago, when I first implemented this. And also this is not the point. The point is: I don't want Zeos to force me to change my code and generate more work for me. I want Zeos to support me in getting my work done.
User avatar
aehimself
Zeos Dev Team
Zeos Dev Team
Posts: 765
Joined: 18.11.2018, 17:37
Location: Hungary

Re: Two TZParams bugs

Post by aehimself »

I was about to say #3 but actually Jan is completely right. #3 would break even my code at some places.

Code: Select all

Constructor Create;
Begin
 [...]
 SQLQuery.SQL.Text := 'SELECT * FROM MyTable WHERE MyField = :pParam1';
 SQLQuery.ParamByName('pParam1').AsDateTime := Now;
End;

Procedure AdditionalFilters(Const inID: UInt64);
Begin
 SQLQuery.SQL.Text := SQLQuery.SQL.Text + ' AND ID = :pID';
 SQLQuery.ParamByname('pID').AsUInt64 := inID;
End;
Having #3 will render this code useless. Having a choice is the way to go as Michael advised: don't carry parameter values or carry them by name. I still think carrying by index is just wrong.
Delphi 12.1, Zeos 8 from latest GIT snapshot
Using:
- MySQL server 8.0.18; libmariadb.dll 3.3.8
- Oracle server 11.2.0, 12.1.0, 19.0.0; oci.dll 21.13
- MSSQL 2012, 2019; sybdb.dll FreeTDS_2435
- SQLite 3.45.2
User avatar
EgonHugeist
Zeos Project Manager
Zeos Project Manager
Posts: 1936
Joined: 31.03.2011, 22:38

Re: Two TZParams bugs

Post by EgonHugeist »

I made a compare to what i said:
EgonHugeist wrote:1. assign param by index as it's implemented since ages.
That is wrong! Looking to DB.pas:

Code: Select all

procedure TParams.AssignValues(Value: TParams);
var
  I: Integer;
  P: TParam;
begin
  for I := 0 to Value.Count - 1 do
  begin
    P := FindParam(Value[I].Name);
    if P <> nil then
      P.Assign(Value[I]);
  end;
end;
It show that the param assigning did always happen by name.

Patch done r7234 see https://sourceforge.net/p/zeoslib/code-0/7234/

So we do no longer discuss about option #1. That was plain wrong from me, i'm sorry for for that inconsistency :oops: .

Anyway i think Fr0st is right too. How to proceed now? We could:
1. implement an even "OnChangeSQL(var CopyParams: Boolean)
2. an Dataset/connection option
3. do nothing and say it's users task to clear the values before/after sql changed. That's what Jan preferes and save my time.

What do you think?
Best regards, Michael

You want to help? http://zeoslib.sourceforge.net/viewtopic.php?f=4&t=3671
You found a (possible) bug? Use the new bugtracker dude! http://sourceforge.net/p/zeoslib/tickets/

Image
User avatar
aehimself
Zeos Dev Team
Zeos Dev Team
Posts: 765
Joined: 18.11.2018, 17:37
Location: Hungary

Re: Two TZParams bugs

Post by aehimself »

Definetly event. In that case users can choose WHEN to clear the parameters - if at all.
An option will always do the same action, regardless of old / new SQL.

Edit: I'll close my pull request and roll back my fix so I'll be in sync when the commit appears on Git.

Ps.: No more access violation as in #1, fix confirmed!
Delphi 12.1, Zeos 8 from latest GIT snapshot
Using:
- MySQL server 8.0.18; libmariadb.dll 3.3.8
- Oracle server 11.2.0, 12.1.0, 19.0.0; oci.dll 21.13
- MSSQL 2012, 2019; sybdb.dll FreeTDS_2435
- SQLite 3.45.2
User avatar
aehimself
Zeos Dev Team
Zeos Dev Team
Posts: 765
Joined: 18.11.2018, 17:37
Location: Hungary

Re: Two TZParams bugs

Post by aehimself »

I can confirm that

3ecbf863acdec7103dcc9b246c6d1e20f698c3a3
egonhugeist on 1/23/2021, 9:10:38 AM
copy params by name as discussed in viewtopic.php?f=50&t=132036 and reason for ticket #479 see https://sourceforge.net/p/zeoslib/tickets/479/

and

35ff109feccb07099c3447e0b32cc465092722cf
egonhugeist on 1/21/2021, 7:34:18 PM
second appoach to fix aehimself's issue, including test(resolved for UNICODE compilers)

fixes both of the bugs in this topic. Thank you, Michael! :)
Delphi 12.1, Zeos 8 from latest GIT snapshot
Using:
- MySQL server 8.0.18; libmariadb.dll 3.3.8
- Oracle server 11.2.0, 12.1.0, 19.0.0; oci.dll 21.13
- MSSQL 2012, 2019; sybdb.dll FreeTDS_2435
- SQLite 3.45.2
Fr0sT
Zeos Dev Team
Zeos Dev Team
Posts: 280
Joined: 08.05.2014, 12:08

Re: Two TZParams bugs

Post by Fr0sT »

Well, while idea of params keeping their values on SQL change still seems dangerous to me, I think compatibility is at the 1st place. IBExpress and FireDAC seem copying values so be it
Post Reply