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

Lack of setNullParameter() functionality on io.ebean.SqlQuery interface #2619

Closed
raphaelNguyen opened this issue Mar 29, 2022 · 11 comments
Closed
Assignees
Labels
Milestone

Comments

@raphaelNguyen
Copy link
Contributor

Expected behavior

io.ebean.SqlQuery interface has a functionality to set null parameter. Prior to ebean 12.15.0, SqlQuery.setParameter accepts null for the value parameter.

Actual behavior

Since ebean 12.15.0, passing null into SqlQuery.setParameter triggers a java assertion to use setNullParameter instead. However, the SqlQuery interface does not seem to offer this functionality.

Is this a functionality that SqlQuery should have or is there an alternative functionality somewhere that I've missed? I'm making use of the SqlQuery instance returned from io.ebean.Database.sqlQuery().

Steps to reproduce

List<SqlRow> list =
  Db.sqlQuery(sql)
       .setParameter("param1", null)
       .findList();
java.lang.AssertionError: use setNullParameter

Thank you very much for your time and assistance.

@rbygrave
Copy link
Member

Just checking, what's the actual sql in this case? Is it a formula?

@rbygrave
Copy link
Member

FYI: This change was brought in with ba21eea ..

@raphaelNguyen
Copy link
Contributor Author

Hi @rbygrave, these sqls are SELECT queries and I use the parameter to compare to fields along the line of

SELECT [...] FROM table1 [...] WHERE field1 = :param1 [...]

One use case of setting parameter to null is when I need to get records with a field have NULL as the value.

Regarding ba21eea. I did find this when looking into the code trying to look for an alternative. However, I was not able to see any obvious way to set a null parameter from SqlQuery.

@rbygrave
Copy link
Member

WHERE field1 = :param1

Yeah interesting, in theory we should be using where field1 is null in this case and not be binding null ... but it used to work and doesn't now right. In this sense this is a regression bug - we should remove those asserts that were added.

set a null parameter from SqlQuery.

I see this as a second [related] issue. This is likely an incorrect omission from the API of SqlQuery with the only mitigation is that in theory we should be using is null in where clauses and not binding null in a where clause ... but that said, we can still in theory bind parameters (that happen to be null) into the select clause (e.g. some database nvl function) so to me it looks like an omission and we should add this to the API on SqlQuery.

@rbygrave rbygrave self-assigned this Mar 29, 2022
@rbygrave rbygrave added the bug label Mar 29, 2022
@rPraml
Copy link
Contributor

rPraml commented Mar 29, 2022

@rbygrave Not all JDBC drivers will allow to bind null parameters. That was one reason, why we introduced this check.
It would prevent you from mistakes, if you have to port your application to an other platform

We should have a multi-platform test for this

@rbygrave
Copy link
Member

Not all JDBC drivers will allow to bind null parameters.

Well yes, I get that - hmmm (on one hand it's a regression, on the other hand mutli-platform code would probably like this check in place ... )

@raphaelNguyen What driver / db is being used here that is allowing binding null for this case? What do you think?

Hmmm ...

@raphaelNguyen
Copy link
Contributor Author

@rbygrave, I'm using this driver: https://github.com/pgjdbc/pgjdbc, for a postgres database.

Not all JDBC drivers will allow to bind null parameters.

If the change has been made for this reason and we have the functionality setNullParameter, I think consumer code such as mine should be adjusted to make use of it. Of course, this would require exposure of this functionality somewhere we are able to get to. This problem will hopefully be addressed by this second issue?

This is likely an incorrect omission from the API of SqlQuery

@rbygrave rbygrave added this to the 12.16.1 milestone Mar 29, 2022
@rbygrave
Copy link
Member

Ok, that PR adds setNullParameter() methods to SqlQuery + it removed those 2 asserts with the thought that we should do a little more thinking about if they should be there (more strictly follow the standard) or not.

So roll this out with the view that we should discuss and maybe put those asserts back in.

@rbygrave
Copy link
Member

Created the ticket there to followup. Closing this one as released 12.16.1 with this change.

@raphaelNguyen
Copy link
Contributor Author

raphaelNguyen commented Mar 29, 2022

Thank you so much for your time and assistance in this matter, @rbygrave.

@rbygrave
Copy link
Member

No worries :) 12.16.1 is on it's way to maven central now so you should be able to use that shortly.

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

No branches or pull requests

3 participants