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

jOOQ parser parses indexed parameters as named #16332

Closed
michael-simons opened this issue Feb 23, 2024 · 7 comments
Closed

jOOQ parser parses indexed parameters as named #16332

michael-simons opened this issue Feb 23, 2024 · 7 comments

Comments

@michael-simons
Copy link
Contributor

michael-simons commented Feb 23, 2024

Expected behavior

Parsing INSERT INTO test(name, vorname) VALUES (?, :foo) yields two named parameters

Actual behavior

First parameter should be indexed or ordinal, second one should be named.

Steps to reproduce the problem

var context = DSL.using(SQLDialect.DEFAULT, settings);
var qom = context.parser().parse("INSERT INTO test(name, vorname) VALUES (?, :foo)");
qom.$queries().$first().getParams().forEach((k, v) -> {
	System.out.println(v.getParamType());
});

jOOQ Version

3.19.3

Database product and version

n/a

Java Version

17

OS Version

macOS

JDBC driver name and version (include name if unofficial driver)

No response

@lukaseder
Copy link
Member

Thanks a lot for your report. I'll hhave a look at this soon.

What would be a reason for mixing the parameter styles, though?

@michael-simons
Copy link
Contributor Author

My pleasure and thank you!

I mixed the parameters in the example just to make the point clear, but users might do weird things.

In our use case I'd rather have back the proper ordinal value, so that I don't have to try to parse the name back into a number and add one (and also hoping people didn't mix ordinals and named parameters).

@lukaseder lukaseder added this to To do in 3.20 Other improvements via automation Feb 26, 2024
@lukaseder
Copy link
Member

I see. Indeed, all indexed parameters seem to be parsed as named parameters, with their zero based index as name. I don't recall this being the intention here. I believe there's already an issue for this (specific to the translator website: https://www.jooq.org/translate), but I can't seem to find it. But this translation is probably not what people expect:

image

Since we already have rendering settings to transform indexed parameters into named parameters and vice versa, I don't think we'll need a flag to maintain the current behaviour in the parser.

Nevertheless, it seems to be an incompatible change so I won't backport the fix.

@michael-simons
Copy link
Contributor Author

I agree!

@lukaseder
Copy link
Member

Fixed in jOOQ 3.20.0. Thanks again for the report.

@lukaseder
Copy link
Member

Looks like this needs to be backported after all, after a few fixes around

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

No branches or pull requests

2 participants