As far as I understood there's no FB-specific methods in the call chain at all. Everything that deals with query parsing (that's where quotes come from) and field creating is generic. So - "all or nothing". I guess this issue is neither too critical nor frequent so there's no big trouble if 7.2 will remain unchanged. Quoted identifiers are evil anyway.marsupilami wrote:This is what we propose for Firebird in Zeos 7.2. If I understood you correctly, the problem is, that a caller calls GetColumns with the tablename quoted -> "TableName" instead of TableName. So for Zeos 7.2 the Firebird driver should remove the quotes if they are supplied before the value is used for any comparisons. This should solver the problem there too?
Problems in the field editor with fields of the same name in two different tables
Re: Problems in the field editor with fields of the same name in two different tables
Hello Jan,
Re: Problems in the field editor with fields of the same name in two different tables
I think that the ability to correctly manage quoted field names is an important feature to be implemented in ZeosLib, also because other components do it without problems. I can agree with Fr0sT that it is not advisable to use them but it is also true that it is not prohibited. In fact, Firebird allows it. I hope this feature can soon be implemented also because it would solve many problems for me. I have been using ZeosLib for many years and have never had any problems.
Thank you for your great work.
Thank you for your great work.
Re: Problems in the field editor with fields of the same name in two different tables
So - what's the final decision?
As fas as I'm concerned, the problem completely lies in generic (for all drivers) methods. I found no FB-specific methods that could be modified to resolve the issue.
As fas as I'm concerned, the problem completely lies in generic (for all drivers) methods. I found no FB-specific methods that could be modified to resolve the issue.
-
- Platinum Boarder
- Posts: 1939
- Joined: 17.01.2011, 14:17
Re: Problems in the field editor with fields of the same name in two different tables
Hello Fr0st,
I had a look into the problem and have to admint the current code is a mess While all you have said is true, the real problem for me is that TZAbstractResultSetMetadata.LoadColumn overwrites the TZColumnInfo without regarding what information already is there. Even worse - it simply destroys everything because it calls ReadColumnByRef which in turn clears the TZColumnInfo on its first line.
So my thought was to try to keep the information and assuming that everything was alright if the table name and the column name already have been set. It turned out that this uncovers another problem: TZInterbase6XSQLDAResultSet.Open doesn't set Writable and Definitlywritable. So I solved that one too.
This still raises sume bugs in the test suites but I am not sure wether that are problems in the tests or problems in the library. I already fixed one test.
I created a patch file for Zeos 7.2 and attached it. Let me know what you think
Jan
I had a look into the problem and have to admint the current code is a mess While all you have said is true, the real problem for me is that TZAbstractResultSetMetadata.LoadColumn overwrites the TZColumnInfo without regarding what information already is there. Even worse - it simply destroys everything because it calls ReadColumnByRef which in turn clears the TZColumnInfo on its first line.
So my thought was to try to keep the information and assuming that everything was alright if the table name and the column name already have been set. It turned out that this uncovers another problem: TZInterbase6XSQLDAResultSet.Open doesn't set Writable and Definitlywritable. So I solved that one too.
This still raises sume bugs in the test suites but I am not sure wether that are problems in the tests or problems in the library. I already fixed one test.
I created a patch file for Zeos 7.2 and attached it. Let me know what you think
Jan
You do not have the required permissions to view the files attached to this post.
Re: Problems in the field editor with fields of the same name in two different tables
Wow, this is pretty smart!
So FB code seems to completely ignore metadata when creating ColumnInfo objects? I'd say it's not a right way. I think it needs a redesign to use metadata. It also will help to implement domain-based GUID type assignment. I'll do some experiments in this area
So FB code seems to completely ignore metadata when creating ColumnInfo objects? I'd say it's not a right way. I think it needs a redesign to use metadata. It also will help to implement domain-based GUID type assignment. I'll do some experiments in this area
-
- Platinum Boarder
- Posts: 1939
- Joined: 17.01.2011, 14:17
Re: Problems in the field editor with fields of the same name in two different tables
Well - it is more like the generic layers of the DBC layer don't trust the lower, driver specific layers to do their work correctly. I thought about having some mechanism to inform the generic parts, which info in TZColumnInfo already could be determined by the driver specific part and which couldn't. I don't expect that all driverc can fill out TZColumnInfo in the open event correctly (yet).
I assume trhat not all drivers could deliver correct information in the TZColumnINfo at some time and that this is the reason why there was the decision to try to get all the informatioon using GetColumns and the like.
I assume trhat not all drivers could deliver correct information in the TZColumnINfo at some time and that this is the reason why there was the decision to try to get all the informatioon using GetColumns and the like.
- EgonHugeist
- Zeos Project Manager
- Posts: 1936
- Joined: 31.03.2011, 22:38
Re: Problems in the field editor with fields of the same name in two different tables
Hi all,
i had a talk with Jan yesterday. I'm against the proposed patch.
This patch try's to fix the reported issue in a generic way which is wrong from my POV.
Before going into more details:
First:
There is a component TZMetaData?. This component uses the Dbc metainformation. Currently it seem's this component isn't realy tested in our suits. But from what i see (if there would be extended tests using this component), most tests would fail because trying to fix field-types/names/etc for the component layer by simply skipping updating the column in the metadata-resultsets. This is wrong. All informations should be available.
In some circumstances this makes trouble like the report of tkszeos or just think about:
The column-informations of the native resultset and the metainformations for the table "myTable" will never fit..
Second:
I had a look to the current code. The TZAbstractResultSetMetadata offers all we need. Almost all methods are virtual. Such as:
TZAbstractResultSetMetadata.ClearColumn
TZAbstractResultSetMetadata.GetTableName .. All
From my POV the missing point here are driver based descandants of TZAbstractResultSetMetadata which resolve all the ResultsetMetaData settings for the IZResultSetMetadata in it's own way.
Drivers like FireBird are pretty powerfull on decribing the bound columns whereas thinking about Access or SQLite(ColumnTypes) we need a crystal ball OR the Metainformations .
In addition current code like forces always to load columninformations via retriving the metainformations.. Is this neccessary in all cases? F.e. in a ReadOnly+ForwardOnly mode as well as scrollable/updatable resultsets? I don't think so.
I'd like to vote for:
Keep away trying to resolve such issues in a generic way. It's not possible.
ALLWAYS try to complete the cached dbc-metainformations for all drivers!
Introducing TZAbstractResultSetMetadata descendants for the Drivers. Some are done but i'ts code seems to be incomplete too. e.g. MySQL
Edit: another handy solution could be a driver based TZColumninfo. Just think about if the fields where assigned by using virtual Setters and Getter methods. A TZFBColumnInfo could dou something like:
I know it means loads more work 4 us. What you guys think?
I could start an example if you want..
Cheers, Michael
i had a talk with Jan yesterday. I'm against the proposed patch.
This patch try's to fix the reported issue in a generic way which is wrong from my POV.
Before going into more details:
First:
There is a component TZMetaData?. This component uses the Dbc metainformation. Currently it seem's this component isn't realy tested in our suits. But from what i see (if there would be extended tests using this component), most tests would fail because trying to fix field-types/names/etc for the component layer by simply skipping updating the column in the metadata-resultsets. This is wrong. All informations should be available.
In some circumstances this makes trouble like the report of tkszeos or just think about:
Code: Select all
select cast(my_int_field) as varchar) as Number from myTable
Second:
I had a look to the current code. The TZAbstractResultSetMetadata offers all we need. Almost all methods are virtual. Such as:
TZAbstractResultSetMetadata.ClearColumn
TZAbstractResultSetMetadata.GetTableName .. All
From my POV the missing point here are driver based descandants of TZAbstractResultSetMetadata which resolve all the ResultsetMetaData settings for the IZResultSetMetadata in it's own way.
Drivers like FireBird are pretty powerfull on decribing the bound columns whereas thinking about Access or SQLite(ColumnTypes) we need a crystal ball OR the Metainformations .
In addition current code like
Code: Select all
function TZAbstractResultSetMetadata.GetTableName(ColumnIndex: Integer): string;
begin
if not Loaded then
LoadColumns;
Result := TZColumnInfo(FResultSet.ColumnsInfo[ColumnIndex {$IFNDEF GENERIC_INDEX}-1{$ENDIF}]).TableName;
end;
I'd like to vote for:
Keep away trying to resolve such issues in a generic way. It's not possible.
ALLWAYS try to complete the cached dbc-metainformations for all drivers!
Introducing TZAbstractResultSetMetadata descendants for the Drivers. Some are done but i'ts code seems to be incomplete too. e.g. MySQL
Edit: another handy solution could be a driver based TZColumninfo. Just think about if the fields where assigned by using virtual Setters and Getter methods. A TZFBColumnInfo could dou something like:
Code: Select all
procedure TZFBColumnInfo.SetTableName(const Value: String);
begin
if (GetTableName = '') and (Value <> '' then
inherited SetTableName;
end;
I know it means loads more work 4 us. What you guys think?
I could start an example if you want..
Cheers, Michael
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/
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/
Re: Problems in the field editor with fields of the same name in two different tables
I'm now examining the corresponding metadata/columninfo code and tend to agree. Currently there's a weird situation: RS.Open queries column data (probably incomplete/incorrect) and then occasionally LoadColumns is called that invokes Metadata query which loads correct column info for the current statement and then... aww, some things are taken from what RS.Open has assigned and some from metadata and the brain just blows down into pieces.
My guess of what should be: RS.Open > Metadata query > Fill column info.
OTOH, this change is somewhat global while Jan's patch seem to solve the issue with minimal efforts.
ADDED
I think I got the idea...
- RS.Open - inits ColumnInfo with data it received by executing a statement
- Dataset.InternalOpen > InternalInitFieldDefs - utilizes ColumnInfo data, then checks IsReadOnly/Writable/etc which calls metadata.LoadColumns
- Metadata.LoadColumns - queries metadata and overrides ColumnInfo completely.
My guess on how it should be:
- RS.Open - inits ColumnInfo with data it received by executing a statement
- Dataset.InternalOpen > InternalInitFieldDefs - calls metadata.LoadColumns
- Metadata.LoadColumns - queries metadata and extends (optionally overrides) ColumnInfo - without ClearColumn.
- InternalInitFieldDefs - utilizes final ColumnInfo data
My guess of what should be: RS.Open > Metadata query > Fill column info.
OTOH, this change is somewhat global while Jan's patch seem to solve the issue with minimal efforts.
ADDED
I think I got the idea...
- RS.Open - inits ColumnInfo with data it received by executing a statement
- Dataset.InternalOpen > InternalInitFieldDefs - utilizes ColumnInfo data, then checks IsReadOnly/Writable/etc which calls metadata.LoadColumns
- Metadata.LoadColumns - queries metadata and overrides ColumnInfo completely.
My guess on how it should be:
- RS.Open - inits ColumnInfo with data it received by executing a statement
- Dataset.InternalOpen > InternalInitFieldDefs - calls metadata.LoadColumns
- Metadata.LoadColumns - queries metadata and extends (optionally overrides) ColumnInfo - without ClearColumn.
- InternalInitFieldDefs - utilizes final ColumnInfo data
Re: Problems in the field editor with fields of the same name in two different tables
Test change: disable ClearColumn and call metadata loading before field creation. Could you guys run test suite with this modification?
Code: Select all
src/component/ZAbstractRODataset.pas | 4 ++++
src/dbc/ZDbcResultSetMetadata.pas | 3 ++-
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/component/ZAbstractRODataset.pas b/src/component/ZAbstractRODataset.pas
index 63f825b..bc8c69a 100644
--- a/src/component/ZAbstractRODataset.pas
+++ b/src/component/ZAbstractRODataset.pas
@@ -3269,6 +3269,10 @@ begin
with ResultSet.GetMetadata do
begin
+
+ // ** TEST ** - force metadata load and fill column props over existing ones
+ IsWritable(FirstDbcIndex);
+
ConSettings := ResultSet.GetConSettings;
if GetColumnCount > 0 then
for I := FirstDbcIndex to GetColumnCount{$IFDEF GENERIC_INDEX}-1{$ENDIF} do
diff --git a/src/dbc/ZDbcResultSetMetadata.pas b/src/dbc/ZDbcResultSetMetadata.pas
index bade232..dd256b2 100644
--- a/src/dbc/ZDbcResultSetMetadata.pas
+++ b/src/dbc/ZDbcResultSetMetadata.pas
@@ -778,7 +778,8 @@ function TZAbstractResultSetMetadata.ReadColumnByRef(
FieldRef: TZFieldRef; ColumnInfo: TZColumnInfo): Boolean;
begin
Result := False;
- ClearColumn(ColumnInfo);
+ // ** TEST - do not delete already assigned props. Probably should remain for early exits
+ //ClearColumn(ColumnInfo);
{ Checks for uncompleted field reference. }
if not Assigned(FieldRef) or not Assigned(FieldRef.TableRef) then
Exit;
- EgonHugeist
- Zeos Project Manager
- Posts: 1936
- Joined: 31.03.2011, 22:38
Re: Problems in the field editor with fields of the same name in two different tables
@FrOst
could you please move an generic example to the test-suites?
Your patch didn't fix the issue, and loads of more protocols raising error and more fails..
@All
iv'e attached an proposal to get rid of that issue. Honestly i was out of time to test the example of tkszeos.
The patch minimizes metadata roundtrips for pure dbc users like me and should also handle the reported problem. It's not 100% complete.
The way i did it:
1. do not flush Columninfos if we know the driver is able to determine them correctly.
2. Add an comparision if the FieldRef of TableRef fit's to the previous determined columninfo by compare Catalog, Schema, TableName.
Note that there is a missing MySql point see commit 4115. MySQL is able to use case sensitive identifiers for all objects except tables where it depends to a global setting (mysql stores each table as files optional case (in) sensitive).
3. Add missing overrides to the protocol descendants to avoid running into the LoadColumns trap what i really don't need it.
Finally we need a generic example. Postgres as well as Oracle(may be, but how?) are not able to describe the real tablename of a fetched column in the NativeResultsets without more roundtrips. All others including SQLite are doing that very well.
As far as i remember Jan wrote an InfoCache for PG. Which could be an full replacement for the MetaInfos?
My impression is like FrOst's: current code doesn't trust the generic Column-Informations and overwrites everything.
Please test my patch and give a reply.
could you please move an generic example to the test-suites?
Your patch didn't fix the issue, and loads of more protocols raising error and more fails..
@All
iv'e attached an proposal to get rid of that issue. Honestly i was out of time to test the example of tkszeos.
The patch minimizes metadata roundtrips for pure dbc users like me and should also handle the reported problem. It's not 100% complete.
The way i did it:
1. do not flush Columninfos if we know the driver is able to determine them correctly.
2. Add an comparision if the FieldRef of TableRef fit's to the previous determined columninfo by compare Catalog, Schema, TableName.
Note that there is a missing MySql point see commit 4115. MySQL is able to use case sensitive identifiers for all objects except tables where it depends to a global setting (mysql stores each table as files optional case (in) sensitive).
3. Add missing overrides to the protocol descendants to avoid running into the LoadColumns trap what i really don't need it.
Finally we need a generic example. Postgres as well as Oracle(may be, but how?) are not able to describe the real tablename of a fetched column in the NativeResultsets without more roundtrips. All others including SQLite are doing that very well.
As far as i remember Jan wrote an InfoCache for PG. Which could be an full replacement for the MetaInfos?
My impression is like FrOst's: current code doesn't trust the generic Column-Informations and overwrites everything.
Please test my patch and give a reply.
You do not have the required permissions to view the files attached to this post.
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/
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/
Re: Problems in the field editor with fields of the same name in two different tables
Michael, yep, my quick fix was too quick. There are several comparisons that were true after column clearing and they executed some necessary actions. I tried commenting some conditions out and tests succeeded though another tests started failing
- EgonHugeist
- Zeos Project Manager
- Posts: 1936
- Joined: 31.03.2011, 22:38
Re: Problems in the field editor with fields of the same name in two different tables
Not a problem, FrOsT. OpenSource is an open discussion and no one known who wins it. Thank for your help btw!
It would be nice if someone can post his findings. If all do agree .. we'll comit the patch. Inbetween i'll add a test running against the known tables.
Something like
IIRC such a name exchange could make trouble too.
It would be nice if someone can post his findings. If all do agree .. we'll comit the patch. Inbetween i'll add a test running against the known tables.
Something like
Code: Select all
select dep_id as dep_name, dep_name as dep_id from department;
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/
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/
- EgonHugeist
- Zeos Project Manager
- Posts: 1936
- Joined: 31.03.2011, 22:38
Re: Problems in the field editor with fields of the same name in two different tables
Double post as Notification...
@tkszeos
please update from \testing-7.2, patch done R4126. This should fix your issue. Note fastes way for Column to Table determination is using a Alias or append the TableName to the fieldname.
@Jan&FrOsT,
The quoting rules are damn ugly. Propose to omit the DecomposeObjectString function in the uncached Metadata functions.
If the IZSelectSchema determines the TZFieldRefs all Identifiers schould be dequoted, TableRefs determined and all pattern fields for the Metadata should work with unquoted patterns only. On forming a statment the quoting rules should be used same as a user should do it.
Current code tests everything a bit paranoid for Quoted/Unquoted LeftSet <> Quoted/Unquoted RightSet to determine "something".
For me this is buggy and somewhat error prone! After the patch the current code is able to resolve most things but as long there are no clear docs about it i propose to raise a ticket.
What do you think?
@tkszeos
please update from \testing-7.2, patch done R4126. This should fix your issue. Note fastes way for Column to Table determination is using a Alias or append the TableName to the fieldname.
@Jan&FrOsT,
The quoting rules are damn ugly. Propose to omit the DecomposeObjectString function in the uncached Metadata functions.
If the IZSelectSchema determines the TZFieldRefs all Identifiers schould be dequoted, TableRefs determined and all pattern fields for the Metadata should work with unquoted patterns only. On forming a statment the quoting rules should be used same as a user should do it.
Current code tests everything a bit paranoid for Quoted/Unquoted LeftSet <> Quoted/Unquoted RightSet to determine "something".
For me this is buggy and somewhat error prone! After the patch the current code is able to resolve most things but as long there are no clear docs about it i propose to raise a ticket.
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/
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/
Re: Problems in the field editor with fields of the same name in two different tables
Michael, nice and big work!
1) "the generic resolver should add Catalog and Schema only of Supported" - hmm, could there really be a case when Schema or Catalog <> '' but driver doesn't support these properties?
2) Links "http://zeoslib.sourceforge.net/viewtopi ... sid=97f200..." contain sid parameter which is session ID and is useless
3) In TZSelectSchema.LinkFieldByIndexAndShortName:
- Convertor.Quote(Field) value is used pretty frequently to be cached
- Construction
could be replaced with
1) "the generic resolver should add Catalog and Schema only of Supported" - hmm, could there really be a case when Schema or Catalog <> '' but driver doesn't support these properties?
2) Links "http://zeoslib.sourceforge.net/viewtopi ... sid=97f200..." contain sid parameter which is session ID and is useless
3) In TZSelectSchema.LinkFieldByIndexAndShortName:
- Convertor.Quote(Field) value is used pretty frequently to be cached
- Construction
Code: Select all
{$IFDEF GENERIC_INDEX}
if (ColumnIndex >= 0) and (ColumnIndex < FFields.Count) then
{$ELSE}
if (ColumnIndex > 0) and (ColumnIndex <= FFields.Count) then
{$ENDIF}
Code: Select all
if (ColumnIndex >= FirstDbcIndex) and (ColumnIndex <= FFields.Count {$IFDEF GENERIC_INDEX} - 1 {$ENDIF}) then
Totally agreed! This quoting madness makes me crazy. Zeos should operate with plain names internally, using quotes only when constructing SQL queries.If the IZSelectSchema determines the TZFieldRefs all Identifiers schould be dequoted, TableRefs determined and all pattern fields for the Metadata should work with unquoted patterns only. On forming a statment the quoting rules should be used same as a user would do it.
I tend to agree (some of overrides are 1:1 as parent one) but some overrides do quote identifiers in these methods. Omitting them will break these drivers.Propose to omit the DecomposeXXX function in the uncached Metadata functions.
- EgonHugeist
- Zeos Project Manager
- Posts: 1936
- Joined: 31.03.2011, 22:38
Re: Problems in the field editor with fields of the same name in two different tables
1) "the generic resolver should add Catalog and Schema only of Supported" - hmm, could there really be a case when Schema or Catalog <> '' but driver doesn't support these properties?
Yep this is version related using MySQL f.e. There are folks like Sphinx which use the mySQL protocols but they do not support Catalog or schema but MySQL since 3.5 is able to use the "Catalog" (usually it's the schema but JDBC maps it as Catalog). ODBC, OleDB and ADO (don't know how) support same syntax..
Lorbs...2) Links "http://zeoslib.sourceforge.net/viewtopi ... sid=97f200..." contain sid parameter which is session ID and is useless
Do it! I've noticed you did a nice simplification on my ugly code- Construction
Code: Select all
{$IFDEF GENERIC_INDEX}
if (ColumnIndex >= 0) and (ColumnIndex < FFields.Count) then
{$ELSE}
if (ColumnIndex > 0) and (ColumnIndex <= FFields.Count) then
{$ENDIF} [/code]
Will it? Needs to be tested. And this job can't be made in atomic commits. Either all or nothing..I tend to agree (some of overrides are 1:1 as parent one) but some overrides do quote identifiers in these methods. Omitting them will break these drivers.
In this case not all can work on it... volunteers ?
Or by using patches as interface..
If all do agree, of course.
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/
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/