Skip to content

Conversation

@XingY
Copy link
Contributor

@XingY XingY commented Jan 12, 2026

Copy link
Contributor

Copilot AI left a 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-value text choice properties in LabKey, enabling users to select multiple values for text choice fields. This functionality is controlled by an experimental feature flag and is currently only supported on PostgreSQL databases.

Changes:

  • Added allowMultiChoiceProperties() method to various DomainKind implementations (DatasetDomainKind, ListDomainKind, DataClassDomainKind, SampleTypeDomainKind)
  • Implemented new array comparison operators (ARRAY_CONTAINS_ALL, ARRAY_CONTAINS_ANY, ARRAY_CONTAINS_NONE, ARRAY_MATCHES, ARRAY_NOT_MATCHES) for filtering multi-choice fields
  • Added support for PostgreSQL array types in SQL dialects and JDBC type handling
  • Updated frontend dependencies to version 7.13.0-fb-mvtc.4 for @labkey/components and 1.45.0-fb-mvtc.2 for @labkey/api

Reviewed changes

Copilot reviewed 29 out of 33 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
study/src/org/labkey/study/model/DatasetDomainKind.java Enables multi-choice properties for datasets
list/src/org/labkey/list/model/ListDomainKind.java Enables multi-choice properties for lists
experiment/src/org/labkey/experiment/api/DataClassDomainKind.java Enables multi-choice properties for data classes
api/src/org/labkey/api/exp/api/SampleTypeDomainKind.java Enables multi-choice properties for sample types
query/src/org/labkey/query/QueryServiceImpl.java Registers new array comparison operators
api/src/org/labkey/api/data/CompareType.java Implements array filtering operators and clause classes
api/src/org/labkey/api/exp/PropertyType.java Adds MULTI_CHOICE property type implementation
api/src/org/labkey/api/data/dialect/SqlDialect.java Adds overloaded getJDBCArrayType method for array handling
api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java Maps ARRAY JDBC type to PostgreSQL text[] type
api/src/org/labkey/api/data/JdbcType.java Implements conversion logic for ARRAY type
api/src/org/labkey/api/data/MultiChoice.java Adds export/import formatting methods for multi-choice display
api/src/org/labkey/api/exp/property/DomainKind.java Renames method from allowMultiTextChoiceProperties to allowMultiChoiceProperties
api/src/org/labkey/api/exp/property/DomainUtil.java Adds allowMultiChoice helper method with feature flag check
api/src/org/labkey/api/reader/TabLoader.java Adds special quote parsing for multi-choice array columns
api/src/org/labkey/api/settings/AppProps.java Adds MULTI_VALUE_TEXT_CHOICE feature flag constant
api/webapp/clientapi/ext3/FilterDialog.js Fixes validation to handle null input values
api/schemas/queryCustomView.xsd Adds array operator enumerations to schema
experiment/src/org/labkey/experiment/ExperimentModule.java Registers experimental feature flag and adds usage metric
pipeline/package.json, experiment/package.json, core/package.json, assay/package.json Updates @labkey/components dependency to feature branch version
**/package-lock.json Updates lockfiles with new dependency versions
Files not reviewed (4)
  • assay/package-lock.json: Language not supported
  • core/package-lock.json: Language not supported
  • experiment/package-lock.json: Language not supported
  • pipeline/package-lock.json: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

if ((value instanceof java.sql.Array array))
property.arrayValue = MultiChoice.Array.from(array);
else if (value != null)
property.arrayValue = MultiChoice.Array.from(new Object[]{value});
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setValue method in PropertyType.MULTI_CHOICE doesn't handle the case where value is null. When value is null, property.arrayValue will remain unchanged (or undefined), which might not be the expected behavior. Consider explicitly setting property.arrayValue to null when value is null.

Suggested change
property.arrayValue = MultiChoice.Array.from(new Object[]{value});
property.arrayValue = MultiChoice.Array.from(new Object[]{value});
else
property.arrayValue = null;

Copilot uses AI. Check for mistakes.
public static final String STORAGE_UNIQUE_ID_CONCEPT_URI = "http://www.labkey.org/types#storageUniqueId";
public static final String STORAGE_UNIQUE_ID_SEQUENCE_PREFIX = "org.labkey.api.StorageUniqueId";
public static final String TEXT_CHOICE_CONCEPT_URI = "http://www.labkey.org/types#textChoice";
//public static final String MULTI_VALUE_TEXT_CHOICE_CONCEPT_URI = "http://www.labkey.org/types#mvTextChoice";
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a commented-out constant MULTI_VALUE_TEXT_CHOICE_CONCEPT_URI. If this constant is not needed, the commented line should be removed to keep the code clean. If it might be needed in the future, consider adding a comment explaining why it's commented out.

Suggested change
//public static final String MULTI_VALUE_TEXT_CHOICE_CONCEPT_URI = "http://www.labkey.org/types#mvTextChoice";

Copilot uses AI. Check for mistakes.
Comment on lines +834 to +837
/** TODO:
* <xsd:enumeration value="arrayisempty"/>
* <xsd:enumeration value="arrayisnotempty"/>
*/
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a TODO comment indicating that arrayisempty and arrayisnotempty operators are not yet implemented, but they are already added to the XSD schema (lines 101-102 in queryCustomView.xsd). This creates an inconsistency where the schema allows these operators but the backend doesn't support them. Either implement these operators or remove them from the XSD until they are ready.

Copilot uses AI. Check for mistakes.
OptionalFeatureService.get().addExperimentalFeatureFlag(NameGenerator.EXPERIMENTAL_ALLOW_GAP_COUNTER, "Allow gap with withCounter and rootSampleCount expression",
"Check this option if gaps in the count generated by withCounter or rootSampleCount name expression are allowed.", true);
OptionalFeatureService.get().addExperimentalFeatureFlag(AppProps.MULTI_VALUE_TEXT_CHOICE, "Allow multi-value Text Choice properties",
"Support selecting more than one values for text choice fields", false);
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The experimental feature flag description has a grammatical error: "more than one values" should be "more than one value" (singular).

Suggested change
"Support selecting more than one values for text choice fields", false);
"Support selecting more than one value for text choice fields", false);

Copilot uses AI. Check for mistakes.
String QUANTITY_COLUMN_SUFFIX_TESTING = "quantityColumnSuffixTesting";
String GENERATE_CONTROLLER_FIRST_URLS = "generateControllerFirstUrls";
String REJECT_CONTROLLER_FIRST_URLS = "rejectControllerFirstUrls";
String MULTI_VALUE_TEXT_CHOICE = "multiChoiceDataType";
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a naming inconsistency: the constant is named MULTI_VALUE_TEXT_CHOICE but the actual value is "multiChoiceDataType". The value should be more consistent with the constant name, such as "multiValueTextChoice" or the constant should be renamed to match the value. This makes the code harder to understand and maintain.

Suggested change
String MULTI_VALUE_TEXT_CHOICE = "multiChoiceDataType";
String MULTI_VALUE_TEXT_CHOICE = "multiValueTextChoice";

Copilot uses AI. Check for mistakes.
Comment on lines +811 to +833
public Collection getCollectionParam(Object value)
{
if (value instanceof Collection)
{
return (Collection)value;
}
else
{
List<String> values = new ArrayList<>();
if (value != null)
{
if (value.toString().trim().isEmpty())
{
values.add(null);
}
else
{
values.addAll(parseParams(value, getValueSeparator(), isNewLineSeparatorAllowed()));
}
}
return values;
}
}
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The getCollectionParam method has package-private visibility (no access modifier), but it's being called from non-static inner classes that extend CompareType. This method should be either protected to allow subclass access or private if it's only meant for internal use. Given that it's being called from the new ARRAY_* CompareType instances, it should be protected.

Copilot uses AI. Check for mistakes.
}
else
{
typeName = getJDBCArrayType(array[0]);
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the new getJDBCArrayType(Object[] array) method, when the array is non-empty (line 1346), there's no null check on array[0]. If the array contains null as the first element, this will cause a NullPointerException when calling getJDBCArrayType(array[0]). Consider adding a null check or iterating to find the first non-null element.

Suggested change
typeName = getJDBCArrayType(array[0]);
// Find the first non-null element to infer the JDBC type. If all elements are null,
// fall back to the same component-type-based inference used for empty arrays.
Object firstNonNull = null;
for (Object element : array)
{
if (element != null)
{
firstNonNull = element;
break;
}
}
if (firstNonNull != null)
{
typeName = getJDBCArrayType(firstNonNull);
}
else
{
// All elements are null; infer type from component type as for empty arrays.
Class<?> componentType = array.getClass().getComponentType();
if (String.class.equals(componentType))
{
typeName = getJDBCArrayType("");
}
else if (Integer.class.equals(componentType))
{
typeName = getJDBCArrayType(Integer.valueOf(0));
}
else if (Long.class.equals(componentType))
{
typeName = getJDBCArrayType(Long.valueOf(0L));
}
else if (Double.class.equals(componentType))
{
typeName = getJDBCArrayType(Double.valueOf(0.0d));
}
else if (Float.class.equals(componentType))
{
typeName = getJDBCArrayType(Float.valueOf(0.0f));
}
else if (Boolean.class.equals(componentType))
{
typeName = getJDBCArrayType(Boolean.FALSE);
}
else
{
// Fallback to VARCHAR which is the safest for most text use-cases
typeName = getSqlTypeName(JdbcType.VARCHAR);
if (typeName != null)
typeName = typeName.toLowerCase();
}
}

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants