Add BindList equivalent

[kmclarnon]

This was a fun little side project. Also I added support for selecting a subfield from a list of objects since that's something I've wanted several times in the past

I'm curious if people think this will cause confusion since @BindWithRosetta Collection<Thing> is already a valid usage?

@jhaber @mjball @stevie400 @Xcelled

[mjball]

I love it. Thank you for adding this!

I'm curious if people think this will cause confusion since @BindWithRosetta Collection is already a valid usage?

This is an interesting point... do you think it would be feasible to hook up this new behavior into the existing @BindWithRosetta binding machinery iff the value refers to a list-valued parameter? I think it might be doable with Binding#findForName() (and maybe SqlArrayTypes?) but not 100% sure.

I'm also not entirely if that's a thing we'd want to do, in case existing code is relying on the current behavior. Though... what is the current behavior of @BindWithRosetta("foos") Collection<Foo> + WHERE foos IN (<foos>)?

[mjball]

i think there's an override of bindList that takes EmptyHandling. Could you use that instead of the above switch?

[kmclarnon]

🤔 I see it in the docs but I don't see it in IntelliJ, wonder if it was added in a later version of jdbi3?

[oktay-sen]

Would this change mean the recommended way to bind collections will now be BindListWithRosetta even though BindWithRosetta still works in common cases?

[kmclarnon]

what is the current behavior of @BindWithRosetta("foos") Collection + WHERE foos IN ()?

Using the tests I wrote as an example, a function like this

  @SqlQuery("SELECT * FROM test_list_table WHERE value IN (<values>)")
  List<TestListObject> getWithValueWithRosetta(@BindWithRosetta List<TestEnum> values);

Will transform the list into [1] (because of the numeric @RosettaValue on the enum), and then (because it's not an object node), embed it in an object with the default prefix ("it"), leaving us with

{ "it": [1] }

which results in a named binding ("it") to a List<Integer> in the statement. This effectively breaks the query because the <values> doesn't get replaced, and wouldn't get replaced even if we named it <it> in this case, because the <x> syntax is specific to the bindList calls (at least it seems that way from my testing).

do you think it would be feasible to hook up this new behavior into the existing @BindWithRosetta binding machinery iff the value refers to a list-valued parameter?

I've tried a few different ways of doing this but haven't come up with something workable yet. The Binding#findForName() machinery seems to be for finding bindings added via the statements various bind(map|list|etc) methods, rather than figuring out what's actually in the sql statement unfortunately.

Would this change mean the recommended way to bind collections will now be BindListWithRosetta even though BindWithRosetta still works in common cases?

My understanding is that the only common case where @BindWithRosetta is used with a collection is when using @SqlBatch, which under the hood is iteratively generating statements for each element in the array, meaning rosetta is not being expected to bind the entire collection all at once. This, on the other hand, is binding an entire list of elements into a single sql statement, which is also reflected in the way in which you set up your SQL statement to use <bindName> rather than :bindName. The <bindName> part of the statement is replaced by a comma-separated list generated from the input collection.

This is, however, exactly the type of confusion that I'm worried about. The jdbi team seemed to think that having separate annotations was worth it, but rosetta has different history/expected usage (@BindWithRosetta is more equivalent to @BindBean than @Bind, for example).

Another unrelated thought: there is @BindBeanList, which generates tuples from the selected bean fields. Do we want to add support for that in this annotation?

[kmclarnon]

Anyone else have thoughts on this?

[stevie400]

Should we disallow arrays as well? We could also disallow null nodes, though I'm not sure if it's possible for that to make it here.

[kmclarnon]

Hmm, I think disallowing array nodes makes sense here, but I'm not sure we'd want to disallow null nodes, since that's what an optional field would be mapped to. We could do something like !value.isValueNode() which would disallow object, array and missing nodes.

[stevie400]

Should we have more tests for the error cases?

[kmclarnon]

Any other thoughts here?

[stevie400]

LGTM