-
Notifications
You must be signed in to change notification settings - Fork 7
PropertyType.MULTI_CHOICE #7168
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
Conversation
uses Map of String or String[] internally insteadof Map<String,String> checkpoint...
NOTE: BeanViewForm also probably doesn't need to extend DynaBean and use StringBeanDynaClass
use setValueToBind() in more places
Collections.unmodifiableMap()
StrintUtils.trimToNull()
Support [] as a form field suffix
|
ERROR: A pull request from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for multi-choice (array-valued) text properties to the LabKey platform. The implementation extends the existing text choice validator to handle collections and introduces a new MULTI_CHOICE property type with associated display and input handling capabilities.
- Extends
TextChoiceValidatorto validate both single string values and collections - Adds new
MULTI_CHOICEPropertyType with corresponding JdbcType.ARRAY support - Introduces
MultiChoice.Arrayclass for handling array values with conversions - Updates display and input rendering logic to handle multi-select controls
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| TextChoiceValidator.java | Extended validation logic to handle Collection values in addition to String values |
| PropertyServiceImpl.java | Enhanced validator registration to support multiple lookup keys (URI, short name, full name) |
| HtmlString.java | Added SP constant for space character |
| DomainKind.java | Added allowMultiTextChoiceProperties() method (defaults to false) |
| PropertyType.java | Added new MULTI_CHOICE enum value with array handling logic |
| TableViewForm.java | Updated to use constants for field and array markers |
| Table.java | Added typed parameter support in SQL WHERE clause construction |
| SQLFragment.java | Added add(Object, JdbcType) method for typed parameters |
| MultiChoice.java | New file introducing array handling infrastructure |
| JdbcType.java | Added ARRAY type and minor formatting fix |
| DisplayColumn.java | Enhanced JSON type handling for arrays/collections and fixed input value retrieval |
| DataColumn.java | Updated form input rendering to support multiple selected values |
| ColumnRenderPropertiesImpl.java | Added special handling for MULTI_CHOICE with ARRAY type |
| ColumnRenderProperties.java | Fixed conversion logic and added array type recognition |
| ColumnInfo.java | Added MultiChoice.DisplayColumn factory logic and removed unused import |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
experiment/src/org/labkey/experiment/api/property/PropertyServiceImpl.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
experiment/src/org/labkey/experiment/api/property/PropertyServiceImpl.java
Outdated
Show resolved
Hide resolved
| } | ||
| }, | ||
|
|
||
| ARRAY(Types.ARRAY, Array.class), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still unclear to me how are we going to support comma character in choice. TabLoader would have stripped the enclosing quotes from "a,b" and then ARRAY needs to decide if this is a single choice a,b or [a, b]. But sure that can be dealt with in a later story.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See PageFlowUtil$TestCase.testGoogleSheetMultiValue(), the string value will be
a, b, "A,B"
This will then get escaped according to the rules of the particular file file format. This will look much better in a TAB separated file thans COMMA separated file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a, b, "A,B" will parse to array.
but what about just "A,B"? TabLoader would have stripped the enclosing quotes so we still have to decide if it's A,B or [A, B]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RowId, MultiChoice
5, "a, b, ""A,B"""
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's kinda like writing a regular expression in a .java file. Regex has it's own escape rules and Java strings have their own escape rules. And you have to nest them.
|
|
||
| public class MultiChoice | ||
| { | ||
| public static final String ARRAY_MARKER = "[]"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should start disallowing domain fields to end with [].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe. I convinced myself this was OK, because this looks like Spring's form binding syntax (e.g. Spring will interpret mycolumn[0]=1). I could change it to be a prefix instead of a suffix like @.
basic Converter junit test
Rationale
First story of MultiChoice epic.
Support PropertyType.MULTI_CHOICE and JdbcType.ARRAY
DataRegion Form for support <input> and <select multiple>
Related Pull Requests
Changes