Add KDbConnection::drv_getTableNames for low level list of table names, make tableNames() skip names with non-existing physical tables
ClosedPublic

Authored by staniek on Mar 21 2018, 11:33 AM.

Details

Summary
  • KDbTestUtils: add convenience APIs for connecting and using db, support connection options, use it in parser test
  • Add KDbConnection::drv_getTableNames for low level list of table names, make tableNames() skip names with non-existing physical tables
  • This change is backward compatible since metadata without physical table is not usable anyway.

CCBUG:392112
FIXED-IN:3.2.0

+ Add autotest for handling missing physical tables

Test Plan

Run ctest

Diff Detail

Repository
R15 KDb
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
staniek created this revision.Mar 21 2018, 11:33 AM
Restricted Application added a project: KDb. · View Herald TranscriptMar 21 2018, 11:33 AM
staniek requested review of this revision.Mar 21 2018, 11:33 AM
piggz added inline comments.Mar 23 2018, 7:59 PM
src/KDbConnection.cpp
1025 ↗(On Diff #30112)

should we also set ok to false to indicate a failure? or, how else to indicate to the caller an error occurred, and not just that there are no tables?

1031 ↗(On Diff #30112)

what about using QSet::intersect() ?
something like
QSet<QString> physical = QSet::fromList(std::transform(physicalTableNames.begin(), physicalTableNames.end(), physicalTableNames.begin(), [](const QString &s) { return s.toLower(); });_
<same for list>
QStringList results = (QSet<QString> intersect = physical.intersect(list)).toList()

staniek added inline comments.Mar 23 2018, 9:40 PM
src/KDbConnection.cpp
1025 ↗(On Diff #30112)

This is good question for other reason. So far I assume (even documented it) that drv_getTableNames returns false only if this approach to enumerating tables is not supported by the driver. But you're right in that it would be future proof to add bool*ok to drv_getTableNames... so we can see difference between 'unsupported' and 'failure'.

It that makes sense I will rework.

1031 ↗(On Diff #30112)

Initially I tried to use QSet::intersect but we have to call toLower() on each name. Until Qt 6 has more algorithms I am not fan of converting to stl and back.
I see you proposed to convert both lists to set and only because we wanted to use intersect(). This solution has no less lines (if clang-formatted) and looks more complex. I looked at even more clever approach similar to https://stackoverflow.com/a/3396154 and it is really not worth the risk that people will not get what's happening here...

piggz added inline comments.Mar 23 2018, 10:01 PM
src/KDbConnection.cpp
1025 ↗(On Diff #30112)

that is another case I didnt think of, but yes, could be done. I was thinking about setting the bool* ok to false that is passed into this function to tell the caller of an error.

1031 ↗(On Diff #30112)

ok

staniek updated this revision to Diff 30359.Mar 23 2018, 10:13 PM
  • MissingTableTest: call again with ok == nullptr and verify
  • Rework KDbConnection::drv_getTableNames API so error information is passed
staniek updated this revision to Diff 30667.Mar 26 2018, 10:23 PM
staniek marked 3 inline comments as done.
  • MissingTableTest: call again with ok == nullptr and verify
  • Rework KDbConnection::drv_getTableNames API so error information is passed
staniek added inline comments.Mar 26 2018, 10:25 PM
src/KDbConnection.cpp
1025 ↗(On Diff #30112)
piggz accepted this revision.Apr 3 2018, 6:18 AM
This revision is now accepted and ready to land.Apr 3 2018, 6:18 AM
This revision was automatically updated to reflect the committed changes.