feat!: switch form filler value options to items-based resolution#9564
feat!: switch form filler value options to items-based resolution#9564ugur-vaadin wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
The JavaDoc says:
Each item is also the label the LLM sees — the chosen label is itself the value written to the field, with no converter needed
But it runs the same applyValueOptions method that applies a potential item labeler. So when you provide a label function through the value options (and presumably also when through the field), the labels and values differ.
From my understanding this method should not use an item labeler, and potentially log a warning if one is configured in the options.
There was a problem hiding this comment.
Dropped the Function.identity() along with the two-arg overload entirely. For duplicate labels: first registered item wins, and the registration logs a warning including the duplicates and pointing at ItemLabelGenerator.
| * converter does not recognize — the write is rejected back to the LLM with | ||
| * a reason and the model can correct on the next turn. | ||
| * <p> | ||
| * A typical converter delegates to a service or repository that looks the |
There was a problem hiding this comment.
It might be worth pointing out explicitly that toValue must produce the reverse of whatever an item label generator produced. Currently this is connection is only implicit.
I feel this surfaces a gap in the design where you configure model -> label and label -> model converters in separate places, which has a higher chance for a misconfiguration. It's also problematic that this uses the label generator from the field as a fallback: That one can be easily changed in the UI code for whatever reason, but then you also need to remember to update the converter in the code that configures the form controller to resolve the model from the changed label.
I wonder if toValue is even necessary? We already keep references to the items in the value options now, so could we also add a lookup for the reverse mapping? Or was there some concern about stale data?
There was a problem hiding this comment.
Dropped toValue. We iterated on this API quite a bit and I agree that it doesn't provide much value anymore, and it creates more confusion.
| hints.itemLabelGenerator = labeler; | ||
| // Items the converter resolves chosen labels against. Pre-populated | ||
| // with the fixed list, or appended on each query-callback invocation. | ||
| var observedItems = new ArrayList<>(); |
There was a problem hiding this comment.
Would it make sense to use a map to automatically dedupe, instead of keeping lots of object references that will never be used because we only pick the first one? Also the lookups would become easier / cheaper.
There was a problem hiding this comment.
Done. We not use a LinkedHashMap that uses insertion order.
| hints.valueOptionsQuery = (filter, limit) -> { | ||
| var batch = rawQuery.apply(filter, limit); | ||
| observedItems.addAll(batch); | ||
| return batch.stream().map(labeler).toList(); | ||
| }; |
There was a problem hiding this comment.
This doesn't look correct, as it keeps adding potentially the same instances over and over if multiple queries match them. A map could fix this part as well.
Other than that it's questionable if this should keep previous entries, in case the application has removed data or updated it. Maybe it should clear on every query, or there might be some other point in the workflow where that is sensible.
There was a problem hiding this comment.
Updated the code to use the map. It should fix the potential inflation issue. For the second part, the controller now refreshes every value options binding at the start of each turn. It re-reads the field's labeler, repopulates the fixed options map from its item list, and clears the query options map.
|



Description
In form filler DX test sessions, the return type of value options APIs was a pain point. Even after updating examples and clarifying it in the documentation, it still was not intuitive: the developer had to think in labels (strings) when the rest of their codebase thought in domain objects, and had to supply a
toValueconverter that re-encoded the label-to-value mapping the field'sItemLabelGeneratoralready expressed.This PR replaces that shape with an items-based resolution path:
ItemLabelGenerator. By default, the controller reflects the field's ownsetItemLabelGenerator(...)so common cases need no extra wiring; an explicit one can be supplied onValueOptionsfor custom components or cases where the LLM should see a different label than the UI.toValueconverter is no longer needed and has been removed.Changed in
ValueOptions:options(Collection<String>)→options(Collection<V>)options(BiFunction<String, Integer, List<String>>)→options(BiFunction<String, Integer, List<V>>)New in
ValueOptions:itemLabelGenerator(ItemLabelGenerator<V>)— optional; falls back to the field's own labeler, then toString.valueOf(item).Changed in
FormAIController:fieldValueOptions(ValueOptions<String>)→fieldValueOptions(ValueOptions<V>)fieldValueOptions(ValueOptions<V>, Function<String, V> toValue)removed.Other behavior:
fill_formthat arrives before anyquery_field_optionscall is rejected with a reason namingquery_field_optionsso the LLM can recover on the next turn.Warning
The two-argument
fieldValueOptions(ValueOptions, Function<String, V>)overload and theString-only generics on the single-argument variant are gone.Based on DX test findings.
No related issue.
Type of change
Checklist