Conversation
Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>
Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>
Prior code was attempting to retrieve 0-1 rows in a per-driver manner, which is hard. Switched to using the standard `WHERE 1 = 0` approach. Left the now-pointless parameter in place. Added tests. Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>
Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>
|
Nothing exercises the servlet with a non-null Can you add a regression test? #452 and #453 each carry one, this is the only one of the three without. The fix works. I verified it against the smoke harness on Derby: a caller-supplied |
The selectLimit parameter is not referenced - so there is nothing to exercise. The
It's challenging to write any meaningful test for this - the unit tests do not run with a database, and since the parameter is not referenced it is a bit pointless to assert that it is not present in the generated SQL. The correct fix is the absence of the code. We don't have to test the code since it does not exist. We test the feature, though, but we can't meaningfully test for the absence of user input that might end up in the query. (To extreme, we can remove the parameter, which would guarantee that it is unused, but cause minimal compat issues). |
| @Override | ||
| public SortedSet<Table> getTables(String channelId, String channelName, String driver, String url, String username, String password, Set<String> tableNamePatterns, String selectLimit, Set<String> resourceIds) { | ||
| // selectLimit is deprecated and ignored for security reasons. Kept for backcompat. | ||
| selectLimit = null; |
There was a problem hiding this comment.
Following up on @pacmano1's question, can we deprecate this entire method and have it call an overloaded method that drops the selectLimit parameter?
There was a problem hiding this comment.
I can, I have a commit hanging around somewhere that does this, but it increases things substantially. I am happy to put a separate PR that strips the functionality in the interest of getting this merged. If you think this is blocking, lmk.
There was a problem hiding this comment.
Ok. I'll open after this closes. Only one more review needed.
There was a problem hiding this comment.
@tonygermano, do you need this in this PR? We're good to merge, otherwise. I'll open the "clean up" as soon as this merges, but that's not urgent to merge.
5 participants
Prior code was attempting to retrieve 0-1 rows in a per-driver manner,
which is hard. Switched to using the standard
WHERE 1 = 0approach.Left the now-pointless parameter in place for compatibility with existing callers.
Review notes: