Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: Disable no-arg callable statement tests in simple query mode #2419

Merged
merged 1 commit into from Jan 25, 2022

Conversation

sehrope
Copy link
Member

@sehrope sehrope commented Jan 25, 2022

I took a look into fixing #2399 but it's more complicated than I thought. This PR doesn't actually fix anything yet. It just disables those three new tests in simple query mode so as not to break the CI. Nothing has changed recently in that part of the driver, those are just new tests that exposed existing broken behavior.

Here's the block in Parser where the extra parameter is being added:

int opening = s.indexOf('(') + 1;
if (opening == 0) {
// here the function call has no parameters declaration eg : "{ ? = call pack_getValue}"
sb.append(outParamBeforeFunc ? "(?)" : "()");
} else if (outParamBeforeFunc) {
// move the single out parameter into the function call
// so that it can be treated like all other parameters
boolean needComma = false;
// the following loop will check if the function call has parameters
// eg "{ ? = call pack_getValue(?) }" vs "{ ? = call pack_getValue() }
for (int j = opening + prefixLength; j < sb.length(); j++) {
char c = sb.charAt(j);
if (c == ')') {
break;
}
if (!Character.isWhitespace(c)) {
needComma = true;
break;
}
}
// insert the return parameter as the first parameter of the function call
if (needComma) {
sb.insert(opening + prefixLength, "?,");
} else {
sb.insert(opening + prefixLength, "?");
}
}
if (!suffix.isEmpty()) {
sql = sb.append(suffix).toString();
} else {
sql = sb.toString();
}
return new JdbcCallParseInfo(sql, isFunction);
}

That "parameter" is actually an out parameter to reflect the functions return value. However removing it ends up breaking subsequent invocations to CallableStatement.registerOutParameter(...) as then the driver thinks there are no out parameters.

The reason this all works fine in extended mode is that the server "helpfully" ignores any void parameters and pretends they're not part of the command. Honestly it's very odd and I think it's a server bug and fixing it there would likely break working (but errant) things like this driver's callable statement interface.

I think the fix for this would have to touch both the parsing and parameter parsing. Not sure when I'll be able to follow up on this so if anyone else wants to take a look please go ahead. That code is pretty old and we haven't heard any complaints about it so I doubt there's anyone actually running into this issue as you'd have to be both using simple mode and JDBC call syntax.

I ran the omni suite on my fork and it completed successfully (https://github.com/sehrope/pgjdbc/runs/4939369066?check_suite_focus=true) so once this cycles through CI here I'll merge it in to make CI green again.

Disables the JDBC function call syntax callable statement tests with no arguments
when running the test suite in simple query mode. The new tests fail with simple
query mode as internal JDBC-to-native query parser generates SQL that contains
the out parameters in the SQL body which is only ignored by the server when
processed in an extended parse, bind, execute sequence. When executed as a simple
query command the server does not ignore the extra parameters and the query
fails.

This commit does not fix anything yet. It simply skips the new failing test in simple
query mode so that the rest of the quite can execute. The behavior of the driver has
not changed in the recent additions either, the new tests simply brought to light an
existing issue with simple query mode.

See pgjdbc#2399 for more details.
@sehrope sehrope merged commit 90e53cb into pgjdbc:master Jan 25, 2022
@davecramer
Copy link
Member

Thx!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants