fix: EXPOSED-547 idParam() registers composite id value with a single placeholder

[bog-walk]

Description

Summary of the change:
idParam() now registers each value stored in a composite key value.

Detailed description:

• Why:

Using idParam() with a Column<EntityID<CompositeID>> only invokes registerArgument() once for the entire composite id value, leading to only 1 placeholder '?' being appended to the statement.

• What:

QueryParameter.toQueryBuilder() now checks whether the value to parameterize is a CompositeID and, if so, calls registerArgument() for every component value.

Using idParam() with equality or null comparison operators is also now possible.

• How:

Internal property compositeValue has been added to the QueryParameter class, both as a flag (to differentiate between other QueryParameter) and to store the CompositeID instead of always casting first.

---

Type of Change

Please mark the relevant options with an "X":

• Bug fix

Affected databases:

• All

Checklist

• Unit tests are in place
• The build is green (including the Detekt check)

---

Related Issues

EXPOSED-547

[bog-walk]

Alternatively we could add a new class like CompositeQueryParameter that would be very similar but might make some branches cleaner. But I couldn't think of any other applications for it so I went this way to keep the logic together.
I also didn't want to open QueryParameter just for an override.

[obabichevjb]

Yeah, I understand that, I tried to write something like CompositeQueryParameter also (during review of PR), and it does not look significantly better. I also tried to extend it not from QueryParameter but from Expression, what it also does not make it much better.

[bog-walk]

Not sure why this logic was added, as the function signature means it's not possible to call isNullOrEmpty() on an IdTable.id, so the first branch will always be false. charLength() also cannot be invoked on IdTable.id.

[obabichevjb]

To be honest, I don't finally understand what this statement means.

It looks like we want to query all the Towns that have non null id (that can not be null, because it's primary key). But even if we want to get all the entries with non null id (composite id), why do we need actual townAId value.

As I can see, the condition where { idParam(townAId, Towns.id).isNotNull() } generates the following sql SELECT towns.zip_code, towns."name", towns.population FROM towns WHERE (towns.zip_code IS NOT NULL) AND (towns."name" IS NOT NULL)

If I replace it with where { Towns.id.isNotNull() } I get the same SQL.

I understand that it's the test for idParam, but this part may confuse in terms of how idParam is used.

[obabichevjb]

It's not the problem of this PR, but I miss the reason fot idParam() to exist.. I expect that I just don't know something, that's why this question is here.

At the first moment I had a thought that idParam(value, column) is the way to combine the id value with the particular column, to avoid usage of id from one table with another table. But looking at how it's used in the tests I'm already not sure with it.

For example in EntityTests::testEntityIdParam() there 2 usages of that.

The first one: CreditCards.select(idParam(creditCard.id, CreditCards.id)).count() where the actual id is not used, because generated sql looks like SELECT COUNT(*) FROM creditcards

The second one is more interesting: CreditCards.select(CreditCards.spendingLimit).where { CreditCards.id eq idParam(creditCard.id, CreditCards.id) }, but from my perspective if the creditCard.id does not already contain combination of value and column, wrapping this value with idParam(creditCard.id, CreditCards.id) does not add safety, because we still can take the value from the wrong table, or to specify wrong table in the right side, for example idParam(creditCard.id, CreditCards.id).

So I'd say that the main question is do we really need this idParam? I'm not agains it, just don't see the main use case of that function.

[bog-walk]

Yup, I agree that tests should be reflective of usage so I'll drop this and change the existing one to actually use idParam() without losing it via count.

You're correct in that statements are parameterized by default, so using idParam() alone doesn't make much sense. From what I'm seeing, and given that it was implemented from the very beginning, the most reasonable explanation seems to be allowing the use of an id value in a function/operator that requires an Expression:

tester.selectAll().where { tester.id eq idParam(id1, tester.id) }
// above is equivalent to below
tester.selectAll().where { tester.id eq id1 }

// the example below is a bit trivial, but this won't compile
tester.select(coalesce(tester.id, id1)
// so we'd have to do this
tester.select(coalesce(tester.id, idParam(id1, tester.id))

This is the only example instance that I can think of when I'd ever recommend using it to a user.