diff --git a/api/src/org/labkey/api/data/CompareType.java b/api/src/org/labkey/api/data/CompareType.java index 6862e55a9b3..3d1a15189b3 100644 --- a/api/src/org/labkey/api/data/CompareType.java +++ b/api/src/org/labkey/api/data/CompareType.java @@ -966,7 +966,7 @@ public String getValueSeparator() public static abstract class ArrayClause extends SimpleFilter.MultiValuedFilterClause { - public static final String ARRAY_VALUE_SEPARATOR = ","; + public static final String ARRAY_VALUE_SEPARATOR = ";"; public ArrayClause(@NotNull FieldKey fieldKey, CompareType comparison, Collection params, boolean negated) { @@ -990,7 +990,7 @@ public SQLFragment[] getParamSQLFragments(SqlDialect dialect) } for (int i = 0; i < params.length; i++) - fragments[i] = new SQLFragment().append(escapeLabKeySqlValue(params[i], type)); + fragments[i] = SQLFragment.unsafe(escapeLabKeySqlValue(params[i], type)); return fragments; } diff --git a/api/src/org/labkey/api/data/DataColumn.java b/api/src/org/labkey/api/data/DataColumn.java index 1f00b198a08..11079682e40 100644 --- a/api/src/org/labkey/api/data/DataColumn.java +++ b/api/src/org/labkey/api/data/DataColumn.java @@ -750,7 +750,7 @@ private void renderSelectFormInput(HtmlWriter out, String formFieldName, Object List options = new ArrayList<>(); // add empty option - if (!isMultiple) + if (!_boundColumn.isRequired() || !isMultiple) options.add(new OptionBuilder().build()); Set selectedValues = strValues.isEmpty() ? Set.of() : diff --git a/api/src/org/labkey/api/data/ExcelCellUtils.java b/api/src/org/labkey/api/data/ExcelCellUtils.java index bfd0d8a98ab..16eebb053c5 100644 --- a/api/src/org/labkey/api/data/ExcelCellUtils.java +++ b/api/src/org/labkey/api/data/ExcelCellUtils.java @@ -17,6 +17,7 @@ import java.util.Calendar; import java.util.Date; import java.util.GregorianCalendar; +import java.util.List; /** * This is a utility class that contains the necessary methods for writing properly formatted values to Excel cells. @@ -68,6 +69,8 @@ else if (Boolean.class.isAssignableFrom(valueClass) || Boolean.TYPE.isAssignable return TYPE_BOOLEAN; else if (File.class.isAssignableFrom(valueClass)) return TYPE_FILE; + else if (List.class.isAssignableFrom(valueClass) || valueClass.isArray()) + return TYPE_MULTILINE_STRING; else { return TYPE_UNKNOWN; diff --git a/api/src/org/labkey/api/data/MultiChoice.java b/api/src/org/labkey/api/data/MultiChoice.java index b5395b64412..f591cbeedf5 100644 --- a/api/src/org/labkey/api/data/MultiChoice.java +++ b/api/src/org/labkey/api/data/MultiChoice.java @@ -44,6 +44,7 @@ import static org.labkey.api.util.DOM.DIV; import static org.labkey.api.util.DOM.SPAN; import static org.labkey.api.util.DOM.at; +import static org.labkey.api.util.SortHelpers.CASE_INSENSITIVE_UPPERCASE_FIRST; public class MultiChoice { @@ -213,7 +214,7 @@ public static class Array implements List, java.sql.Array protected Array(Stream str) { - TreeSet setCaseSensitive = new TreeSet<>(); + TreeSet setCaseSensitive = new TreeSet<>(CASE_INSENSITIVE_UPPERCASE_FIRST); str.filter(Objects::nonNull) .map(s -> StringUtils.trimToNull(s.toString())) .filter(Objects::nonNull) diff --git a/api/src/org/labkey/api/data/MultiValuedRenderContext.java b/api/src/org/labkey/api/data/MultiValuedRenderContext.java index 6cae8968265..2217b7f3af7 100644 --- a/api/src/org/labkey/api/data/MultiValuedRenderContext.java +++ b/api/src/org/labkey/api/data/MultiValuedRenderContext.java @@ -19,6 +19,7 @@ import org.apache.commons.collections4.iterators.ArrayIterator; import org.junit.Assert; import org.junit.Test; +import org.labkey.api.exp.PropertyType; import org.labkey.api.query.FieldKey; import java.util.HashMap; @@ -113,6 +114,12 @@ public Object get(Object key) if (getFieldMap() != null) { ColumnInfo columnInfo = getFieldMap().get(key); + if (columnInfo != null && columnInfo.getPropertyType() == PropertyType.MULTI_CHOICE && value instanceof String strVal) + { + // Multi-choice values array is converted to string: "{value1,value2,...}", so strip off the braces before converting + if (strVal.startsWith("{") && strVal.endsWith("}")) + return columnInfo.convert(strVal.substring(1, strVal.length() - 1)); + } // The value was concatenated with others, so it's become a string. // Do conversion to switch it back to the expected type. if (value != null && columnInfo != null && !columnInfo.getJavaClass().isInstance(value)) diff --git a/api/src/org/labkey/api/data/TableChange.java b/api/src/org/labkey/api/data/TableChange.java index 5e8c6a1245d..cf6da3a1018 100644 --- a/api/src/org/labkey/api/data/TableChange.java +++ b/api/src/org/labkey/api/data/TableChange.java @@ -20,6 +20,7 @@ import org.labkey.api.data.PropertyStorageSpec.Index; import org.labkey.api.data.TableInfo.IndexDefinition; import org.labkey.api.exp.PropertyDescriptor; +import org.labkey.api.exp.PropertyType; import org.labkey.api.exp.property.Domain; import org.labkey.api.exp.property.DomainKind; import org.labkey.api.util.logging.LogHelper; @@ -58,6 +59,7 @@ public class TableChange private Collection _constraints; private Set _indicesToBeDroppedByName; private IndexSizeMode _sizeMode = IndexSizeMode.Auto; + private Map _oldPropTypes; /** In most cases, domain knows the storage table name **/ public TableChange(Domain domain, ChangeType changeType) @@ -329,6 +331,11 @@ public void setForeignKeys(Collection foreignKey _foreignKeys = foreignKeys; } + public Map getOldPropTypes() + { + return _oldPropTypes; + } + public final List toSpecs(Collection columnNames) { final Domain domain = _domain; @@ -349,6 +356,11 @@ public final List toSpecs(Collection columnNames) .collect(Collectors.toList()); } + public void setOldPropertyTypes(Map oldPropTypes) + { + _oldPropTypes = oldPropTypes; + } + public enum ChangeType { CreateTable, diff --git a/api/src/org/labkey/api/exp/property/DomainUtil.java b/api/src/org/labkey/api/exp/property/DomainUtil.java index dbb41950632..4d76b897f75 100644 --- a/api/src/org/labkey/api/exp/property/DomainUtil.java +++ b/api/src/org/labkey/api/exp/property/DomainUtil.java @@ -436,8 +436,6 @@ public static boolean allowMultiChoice(DomainKind kind) { if (!kind.allowMultiChoiceProperties()) return false; - if (!OptionalFeatureService.get().isFeatureEnabled(AppProps.MULTI_VALUE_TEXT_CHOICE)) - return false; return CoreSchema.getInstance().getSqlDialect().isPostgreSQL(); } @@ -1446,6 +1444,7 @@ public static ValidationException validateProperties(@Nullable Domain domain, @N ValidationException exception = new ValidationException(); Map propertyIdNameMap = getOriginalFieldPropertyIdNameMap(orig);//key: orig property id, value : orig field name + boolean allowMultiChoice = domainKind != null ? allowMultiChoice(domainKind) : updates.isAllowMultiChoiceProperties(); for (GWTPropertyDescriptor field : updates.getFields(true)) { String name = field.getName(); @@ -1461,6 +1460,12 @@ public static ValidationException validateProperties(@Nullable Domain domain, @N exception.addError(new SimpleValidationError(getDomainErrorMessage(updates, "The field name '" + name + "' is not allowed."))); } + if (!allowMultiChoice && PropertyType.MULTI_CHOICE.getTypeUri().equals(field.getRangeURI())) + { + exception.addError(new SimpleValidationError(getDomainErrorMessage(updates, "The field '" + name + "' does not support multiple values."))); + continue; + } + Matcher expMatcher = SUBSTITUTION_EXP_PATTERN.matcher(name); if (expMatcher.find()) { diff --git a/api/src/org/labkey/api/query/AbstractQueryChangeListener.java b/api/src/org/labkey/api/query/AbstractQueryChangeListener.java deleted file mode 100644 index 57c76f78dd3..00000000000 --- a/api/src/org/labkey/api/query/AbstractQueryChangeListener.java +++ /dev/null @@ -1,70 +0,0 @@ -/* - * Copyright (c) 2013-2017 LabKey Corporation - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.labkey.api.query; - -import org.jetbrains.annotations.NotNull; -import org.labkey.api.data.Container; -import org.labkey.api.data.ContainerFilter; -import org.labkey.api.security.User; - -import java.util.ArrayList; -import java.util.Collection; -import java.util.List; - -/** - * User: kevink - * Date: 4/17/13 - */ -public abstract class AbstractQueryChangeListener implements QueryChangeListener -{ - @Override - public void queryCreated(User user, Container container, ContainerFilter scope, SchemaKey schema, @NotNull Collection queries) - { - for (String query : queries) - queryCreated(user, container, scope, schema, query); - } - - protected abstract void queryCreated(User user, Container container, ContainerFilter scope, SchemaKey schema, String query); - - @Override - public void queryChanged(User user, Container container, ContainerFilter scope, SchemaKey schema, @NotNull QueryProperty property, @NotNull Collection> changes) - { - for (QueryPropertyChange change : changes) - queryChanged(user, container, scope, schema, change); - } - - protected abstract void queryChanged(User user, Container container, ContainerFilter scope, SchemaKey schema, QueryPropertyChange change); - - @Override - public void queryDeleted(User user, Container container, ContainerFilter scope, SchemaKey schema, @NotNull Collection queries) - { - for (String query : queries) - queryDeleted(user, container, scope, schema, query); - } - - protected abstract void queryDeleted(User user, Container container, ContainerFilter scope, SchemaKey schema, String query); - - @Override - public Collection queryDependents(User user, Container container, ContainerFilter scope, SchemaKey schema, @NotNull Collection queries) - { - List ret = new ArrayList<>(); - for (String query : queries) - ret.addAll(queryDependents(container, scope, schema, query)); - return ret; - } - - protected abstract Collection queryDependents(Container container, ContainerFilter scope, SchemaKey schema, String query); -} diff --git a/api/src/org/labkey/api/query/DefaultQueryUpdateService.java b/api/src/org/labkey/api/query/DefaultQueryUpdateService.java index 594ed785fef..a991fb56dd9 100644 --- a/api/src/org/labkey/api/query/DefaultQueryUpdateService.java +++ b/api/src/org/labkey/api/query/DefaultQueryUpdateService.java @@ -780,7 +780,7 @@ protected boolean validMissingValue(Container c, String mv) final protected void convertTypes(User user, Container c, Map row) throws ValidationException { - convertTypes(user, c, row, getDbTable(), null); + convertTypes(user, c, row, getQueryTable(), null); } // TODO Path->FileObject @@ -826,7 +826,7 @@ else if (null != value) row.put(col.getMvColumnName().getName(), mv); } - value = null==value ? null : convertColumnValue(col, value, user, c, fileLinkDirPath); + value = convertColumnValue(col, value, user, c, fileLinkDirPath); row.put(col.getName(), value); } } diff --git a/api/src/org/labkey/api/query/QueryChangeListener.java b/api/src/org/labkey/api/query/QueryChangeListener.java index 776c57392ff..e3c21dfd411 100644 --- a/api/src/org/labkey/api/query/QueryChangeListener.java +++ b/api/src/org/labkey/api/query/QueryChangeListener.java @@ -20,10 +20,17 @@ import org.labkey.api.data.Container; import org.labkey.api.data.ContainerFilter; import org.labkey.api.event.PropertyChange; +import org.labkey.api.exp.PropertyDescriptor; +import org.labkey.api.exp.PropertyType; import org.labkey.api.security.User; +import org.labkey.api.util.PageFlowUtil; import java.util.Collection; import java.util.Collections; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +import static org.labkey.api.data.ColumnRenderPropertiesImpl.TEXT_CHOICE_CONCEPT_URI; /** * Listener for table and query events that fires when the structure/schema changes, but not when individual data @@ -94,7 +101,9 @@ enum QueryProperty Description(String.class), Inherit(Boolean.class), Hidden(Boolean.class), - SchemaName(String.class),; + SchemaName(String.class), + ColumnName(String.class), + ColumnType(PropertyDescriptor.class),; private final Class _klass; @@ -171,6 +180,22 @@ public static void handleSchemaNameChange(@NotNull String oldValue, String newVa QueryProperty.SchemaName, Collections.singleton(change)); } + public static void handleColumnTypeChange(@NotNull PropertyDescriptor oldValue, PropertyDescriptor newValue, @NotNull SchemaKey schemaPath, @NotNull String queryName, User user, Container container) + { + if (oldValue.getPropertyType() == newValue.getPropertyType()) + return; + + QueryChangeListener.QueryPropertyChange change = new QueryChangeListener.QueryPropertyChange<>( + QueryService.get().getUserSchema(user, container, schemaPath).getQueryDefForTable(queryName), + QueryChangeListener.QueryProperty.ColumnType, + oldValue, + newValue + ); + + QueryService.get().fireQueryColumnChanged(user, container, schemaPath, + QueryProperty.ColumnType, Collections.singleton(change)); + } + @Nullable public QueryDefinition getSource() { return _queryDef; } @Override @@ -185,4 +210,151 @@ public static void handleSchemaNameChange(@NotNull String oldValue, String newVa @Nullable public V getNewValue() { return _newValue; } } + + /** + * Utility to update encoded filter string when a column type changes from Multi_Choice to a non Multi_Choice. + * This method performs targeted replacements for the given column name (case-insensitive). + */ + private static String getUpdatedFilterStrFromMVTC(String filterStr, String prefix, String columnName, @NotNull PropertyDescriptor oldType, @NotNull PropertyDescriptor newType) + { + if (filterStr == null || columnName == null) + return filterStr; + + // Only act when changing away from MULTI_CHOICE + if (oldType.getPropertyType() != PropertyType.MULTI_CHOICE || newType.getPropertyType() == PropertyType.MULTI_CHOICE) + return filterStr; + + String columnNameEncoded = PageFlowUtil.encodeURIComponent(columnName); + + String colLower = columnNameEncoded.toLowerCase(); + String sLower = filterStr.toLowerCase(); + + // No action if column doesn't match + if (!sLower.startsWith(prefix + "." + colLower + "~")) + return filterStr; + + // drop arraycontainsall since there is no good match + if (sLower.startsWith(prefix + "." + colLower + "~arraycontainsall")) + return ""; + + String updated = filterStr; + + if (TEXT_CHOICE_CONCEPT_URI.equals(newType.getConceptURI())) + { + // only keep arraymatches/arraynotmatches when converting to a TEXT_CHOICE since current values are guaranteed to be single value + if (containsOp(updated, prefix, columnNameEncoded, "arraymatches")) + { + return replaceOp(updated, prefix, columnNameEncoded, "arraymatches", "eq"); + } + if (containsOp(updated, prefix, columnNameEncoded, "arraynotmatches")) + { + return replaceOp(updated, prefix, columnNameEncoded, "arraynotmatches", "neq"); + } + } + + if (containsOp(updated, prefix, columnNameEncoded, "arrayisempty")) + { + return replaceOp(updated, prefix, columnNameEncoded, "arrayisempty", "isblank"); + } + if (containsOp(updated, prefix, columnNameEncoded, "arrayisnotempty")) + { + return replaceOp(updated, prefix, columnNameEncoded, "arrayisnotempty", "isnonblank"); + } + if (containsOp(updated, prefix, columnNameEncoded, "arraycontainsany")) + { + return replaceOp(updated, prefix, columnNameEncoded, "arraycontainsany", "in"); + } + if (containsOp(updated, prefix, columnNameEncoded, "arraycontainsnone")) + { + return replaceOp(updated, prefix, columnNameEncoded, "arraycontainsnone", "notin"); + } + + // No matching operator found for this column, drop the filter + return ""; + } + + /** + * Utility to update encoded filter string when a column type is changed to Multi_Choice (migrating operators to array equivalents). + */ + private static String getUpdatedMVTCFilterStr(String filterStr, String prefix, String columnName, @NotNull PropertyDescriptor oldType, @NotNull PropertyDescriptor newType) + { + if (filterStr == null || columnName == null || oldType == null || newType == null) + return filterStr; + + // Only act when changing to MULTI_CHOICE + if (oldType.getPropertyType() == PropertyType.MULTI_CHOICE || newType.getPropertyType() != PropertyType.MULTI_CHOICE) + return filterStr; + + String columnNameEncoded = PageFlowUtil.encodeURIComponent(columnName); + + String colLower = columnNameEncoded.toLowerCase(); + String sLower = filterStr.toLowerCase(); + + // No action if column doesn't match + if (!sLower.startsWith(prefix + "." + colLower + "~")) + return filterStr; + + String updated = filterStr; + + // Return on first matching operator for this column + if (containsOp(updated, prefix, columnNameEncoded, "eq")) + { + return replaceOp(updated, prefix, columnNameEncoded, "eq", "arraymatches"); + } + if (containsOp(updated, prefix, columnNameEncoded, "neq")) + { + return replaceOp(updated, prefix, columnNameEncoded, "neq", "arraycontainsnone"); + } + if (containsOp(updated, prefix, columnNameEncoded, "isblank")) + { + return replaceOp(updated, prefix, columnNameEncoded, "isblank", "arrayisempty"); + } + if (containsOp(updated, prefix, columnNameEncoded, "isnonblank")) + { + return replaceOp(updated, prefix, columnNameEncoded, "isnonblank", "arrayisnotempty"); + } + if (containsOp(updated, prefix, columnNameEncoded, "in")) + { + return replaceOp(updated, prefix, columnNameEncoded, "in", "arraycontainsany"); + } + if (containsOp(updated, prefix, columnNameEncoded, "notin")) + { + return replaceOp(updated, prefix, columnNameEncoded, "notin", "arraycontainsnone"); + } + + // No matching operator found for this column, drop the filter + return ""; + } + + static String getUpdatedFilterStrOnColumnTypeUpdate(String filterStr, String prefix, String columnName, @NotNull PropertyDescriptor oldType, @NotNull PropertyDescriptor newType) + { + if (oldType.getPropertyType() == PropertyType.MULTI_CHOICE) + return getUpdatedFilterStrFromMVTC(filterStr, prefix, columnName, oldType, newType); + else if (newType.getPropertyType() == PropertyType.MULTI_CHOICE) + return getUpdatedMVTCFilterStr(filterStr, prefix, columnName, oldType, newType); + else + return filterStr; + } + + private static boolean containsOp(String filterStr, String prefix, String columnName, String op) + { + String regex = "(?i)" + prefix + "\\." + Pattern.quote(columnName) + "~" + Pattern.quote(op); + return Pattern.compile(regex).matcher(filterStr).find(); + } + + private static String replaceOp(String filterStr, String prefix, String columnName, String fromOp, String toOp) + { + String regex = "(?i)(" + prefix + "\\.)(" + Pattern.quote(columnName) + ")(~)" + Pattern.quote(fromOp); + Matcher m = Pattern.compile(regex).matcher(filterStr); + StringBuffer sb = new StringBuffer(); + while (m.find()) + { + // Preserve the literal 'filter.', 'columnName' and '~', but use the new operator + String replacement = m.group(1) + m.group(2) + m.group(3) + toOp; + m.appendReplacement(sb, Matcher.quoteReplacement(replacement)); + } + m.appendTail(sb); + return sb.toString(); + } + } diff --git a/api/src/org/labkey/api/query/QueryService.java b/api/src/org/labkey/api/query/QueryService.java index bb6d87b7614..f4506494969 100644 --- a/api/src/org/labkey/api/query/QueryService.java +++ b/api/src/org/labkey/api/query/QueryService.java @@ -491,7 +491,7 @@ public String getDefaultCommentSummary() void fireQueryCreated(User user, Container container, ContainerFilter scope, SchemaKey schema, Collection queries); void fireQueryChanged(User user, Container container, ContainerFilter scope, SchemaKey schema, QueryChangeListener.QueryProperty property, Collection> changes); void fireQueryDeleted(User user, Container container, ContainerFilter scope, SchemaKey schema, Collection queries); - + void fireQueryColumnChanged(User user, Container container, @NotNull SchemaKey schemaPath, QueryChangeListener.QueryProperty property, Collection> changes); /** OLAP **/ // could make this a separate service diff --git a/api/src/org/labkey/api/settings/AppProps.java b/api/src/org/labkey/api/settings/AppProps.java index ba493df5f8d..6395044aeb2 100644 --- a/api/src/org/labkey/api/settings/AppProps.java +++ b/api/src/org/labkey/api/settings/AppProps.java @@ -51,7 +51,6 @@ public interface AppProps String QUANTITY_COLUMN_SUFFIX_TESTING = "quantityColumnSuffixTesting"; String GENERATE_CONTROLLER_FIRST_URLS = "generateControllerFirstUrls"; String REJECT_CONTROLLER_FIRST_URLS = "rejectControllerFirstUrls"; - String MULTI_VALUE_TEXT_CHOICE = "multiChoiceDataType"; String UNKNOWN_VERSION = "Unknown Release Version"; diff --git a/api/src/org/labkey/api/util/SortHelpers.java b/api/src/org/labkey/api/util/SortHelpers.java index 4e178f00f3b..a47b0ba595f 100644 --- a/api/src/org/labkey/api/util/SortHelpers.java +++ b/api/src/org/labkey/api/util/SortHelpers.java @@ -19,6 +19,58 @@ public class SortHelpers { + + /** + * Comparator that orders strings case-insensitively, but when two strings are equal ignoring case + * breaks ties by preferring uppercase characters before lowercase characters. + * + * Example ordering: "Ab", "a", "B", "b" + */ + public static final Comparator CASE_INSENSITIVE_UPPERCASE_FIRST = new Comparator<>() + { + @Override + public int compare(String s1, String s2) + { + if (s1 == s2) return 0; + if (s1 == null) return -1; + if (s2 == null) return 1; + + int len1 = s1.length(); + int len2 = s2.length(); + int min = Math.min(len1, len2); + + // Compare character-by-character using lowercased characters for primary ordering, + // but if the lowercased chars are equal and the raw chars differ in case, prefer uppercase. + for (int i = 0; i < min; i++) + { + char c1 = s1.charAt(i); + char c2 = s2.charAt(i); + char lc1 = Character.toLowerCase(c1); + char lc2 = Character.toLowerCase(c2); + + // Primary: case-insensitive ordering + if (lc1 != lc2) + return lc1 - lc2; + + // If same ignoring case but raw chars differ, prefer uppercase + if (c1 != c2) + { + boolean u1 = Character.isUpperCase(c1); + boolean u2 = Character.isUpperCase(c2); + if (u1 != u2) + return u1 ? -1 : 1; + + // Fallback deterministic tie-breaker if necessary + int diff = c1 - c2; + if (diff != 0) return diff; + } + } + + // If identical up to min length, shorter string first (keeps deterministic ordering) + return len1 - len2; + } + }; + // Natural sort ordering public static int compareNatural(Object obj1, Object obj2) { diff --git a/api/src/org/labkey/api/util/SubstitutionFormat.java b/api/src/org/labkey/api/util/SubstitutionFormat.java index 2cfec0581e0..43b0a7e7640 100644 --- a/api/src/org/labkey/api/util/SubstitutionFormat.java +++ b/api/src/org/labkey/api/util/SubstitutionFormat.java @@ -82,8 +82,16 @@ public Object format(Object value) return defaultTimeFormat.format(value); else if (value instanceof java.sql.Date) return date.format(value); - else if (value instanceof Date) - return defaultDateTimeFormat.format(value).replace('T', ' '); // replace T with whitespace for human readability + else if (value instanceof Date dateVal) + { + LocalDateTime ldt = LocalDateTime.ofInstant(dateVal.toInstant(), ZoneId.systemDefault()); + boolean isDateOnly = ldt.toLocalTime().equals(java.time.LocalTime.MIDNIGHT); + + if (isDateOnly) + return date.format(value); + else + return defaultDateTimeFormat.format(value).replace('T', ' '); // replace T with whitespace for human readability + } return value; } diff --git a/core/package-lock.json b/core/package-lock.json index e7e4a95ee63..bc8116420c8 100644 --- a/core/package-lock.json +++ b/core/package-lock.json @@ -8,7 +8,7 @@ "name": "labkey-core", "version": "0.0.0", "dependencies": { - "@labkey/components": "7.16.0", + "@labkey/components": "7.16.1-fb-mvtc-convert.0", "@labkey/themes": "1.6.0" }, "devDependencies": { @@ -3814,9 +3814,9 @@ } }, "node_modules/@labkey/api": { - "version": "1.46.0", - "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/api/-/@labkey/api-1.46.0.tgz", - "integrity": "sha512-QwSm82KBc9Hhd5GUDe99J35xvXIWBtbvFbXxJh81x6le62EZFQdptDoxJyKGpDQiv+cITlNEfvYXjY3CO0y1Kw==", + "version": "1.46.1-fb-mvtc-convert.1", + "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/api/-/@labkey/api-1.46.1-fb-mvtc-convert.1.tgz", + "integrity": "sha512-i5uiP2ueHs+3PzRdD0wdGBKCGAOD0+CCKVgoIBE6bz0KI+ncbeBxIJXc4n+dAGVvnxxXnLoyUo7Xd6oQ/+2mhQ==", "license": "Apache-2.0" }, "node_modules/@labkey/build": { @@ -3857,13 +3857,13 @@ } }, "node_modules/@labkey/components": { - "version": "7.16.0", - "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-7.16.0.tgz", - "integrity": "sha512-w4wzsgmGW5ffzxlg8wdPqiwU3usgEJnNS9duFRmolzAKpplOpU1Z8YFyyKYfZq3HYPFb0yEKy1ho3N6z8VWtuA==", + "version": "7.16.1-fb-mvtc-convert.0", + "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-7.16.1-fb-mvtc-convert.0.tgz", + "integrity": "sha512-ey08b3dsYCfQ5EDdd9+SzypnsugCCtLdROKlMvumLOg873iXYqcZy+kR81jnXMhVE42RQwdI8H3xsKD5fSTlIQ==", "license": "SEE LICENSE IN LICENSE.txt", "dependencies": { "@hello-pangea/dnd": "18.0.1", - "@labkey/api": "1.46.0", + "@labkey/api": "1.46.1-fb-mvtc-convert.1", "@testing-library/dom": "~10.4.1", "@testing-library/jest-dom": "~6.9.1", "@testing-library/react": "~16.3.2", diff --git a/core/package.json b/core/package.json index e398c389fb8..924f730c33f 100644 --- a/core/package.json +++ b/core/package.json @@ -53,7 +53,7 @@ } }, "dependencies": { - "@labkey/components": "7.16.0", + "@labkey/components": "7.16.1-fb-mvtc-convert.0", "@labkey/themes": "1.6.0" }, "devDependencies": { diff --git a/core/src/org/labkey/core/dialect/PostgreSql92Dialect.java b/core/src/org/labkey/core/dialect/PostgreSql92Dialect.java index 6ffba80804c..ce919a43acd 100644 --- a/core/src/org/labkey/core/dialect/PostgreSql92Dialect.java +++ b/core/src/org/labkey/core/dialect/PostgreSql92Dialect.java @@ -631,6 +631,9 @@ private List getChangeColumnTypeStatement(TableChange change) for (PropertyStorageSpec column : change.getColumns()) { + PropertyType oldPropertyType = null; + if (change.getOldPropTypes() != null) + oldPropertyType = change.getOldPropTypes().get(column.getName()); DatabaseIdentifier columnIdent = makePropertyIdentifier(column.getName()); if (column.getJdbcType().isDateOrTime()) { @@ -662,6 +665,76 @@ private List getChangeColumnTypeStatement(TableChange change) rename.append(" RENAME COLUMN ").appendIdentifier(tempColumnIdent).append(" TO ").appendIdentifier(columnIdent); statements.add(rename); } + else if (oldPropertyType == PropertyType.MULTI_CHOICE && column.getJdbcType().isText()) + { + // Converting from text[] (array) to text requires an intermediate column and transformation + String tempColumnName = column.getName() + "~~temp~~"; + DatabaseIdentifier tempColumnIdent = makePropertyIdentifier(tempColumnName); + + // 1) ADD temp column of text type + SQLFragment addTemp = new SQLFragment("ALTER TABLE "); + addTemp.appendIdentifier(change.getSchemaName()).append(".").appendIdentifier(change.getTableName()); + addTemp.append(" ADD COLUMN ").append(getSqlColumnSpec(column, tempColumnName)); + statements.add(addTemp); + + // 2) UPDATE: convert and copy value to temp column + // - NULL array -> NULL + // - empty array -> NULL + // - non-empty array -> concatenate array elements with comma (', ') + SQLFragment update = new SQLFragment("UPDATE "); + update.appendIdentifier(change.getSchemaName()).append(".").appendIdentifier(change.getTableName()); + update.append(" SET ").appendIdentifier(tempColumnIdent).append(" = CASE "); + update.append(" WHEN ").appendIdentifier(columnIdent).append(" IS NULL THEN NULL "); + update.append(" WHEN COALESCE(array_length(").appendIdentifier(columnIdent).append(", 1), 0) = 0 THEN NULL "); + update.append(" ELSE array_to_string(").appendIdentifier(columnIdent).append(", ', ') END"); + statements.add(update); + + // 3) DROP original column + SQLFragment drop = new SQLFragment("ALTER TABLE "); + drop.appendIdentifier(change.getSchemaName()).append(".").appendIdentifier(change.getTableName()); + drop.append(" DROP COLUMN ").appendIdentifier(columnIdent); + statements.add(drop); + + // 4) RENAME temp column to original column name + SQLFragment rename = new SQLFragment("ALTER TABLE "); + rename.appendIdentifier(change.getSchemaName()).append(".").appendIdentifier(change.getTableName()); + rename.append(" RENAME COLUMN ").appendIdentifier(tempColumnIdent).append(" TO ").appendIdentifier(columnIdent); + statements.add(rename); + } + else if (column.getJdbcType() == JdbcType.ARRAY) + { + // Converting from text to text[] requires an intermediate column and transformation + String tempColumnName = column.getName() + "~~temp~~"; + DatabaseIdentifier tempColumnIdent = makePropertyIdentifier(tempColumnName); + + // 1) ADD temp column of array type (e.g., text[]) + SQLFragment addTemp = new SQLFragment("ALTER TABLE "); + addTemp.appendIdentifier(change.getSchemaName()).append(".").appendIdentifier(change.getTableName()); + addTemp.append(" ADD COLUMN ").append(getSqlColumnSpec(column, tempColumnName)); + statements.add(addTemp); + + // 2) UPDATE: copy converted value to temp column as single-element array + // - NULL or blank ('') -> empty array [] + // - otherwise -> single-element array [text] + SQLFragment update = new SQLFragment("UPDATE "); + update.appendIdentifier(change.getSchemaName()).append(".").appendIdentifier(change.getTableName()); + update.append(" SET ").appendIdentifier(tempColumnIdent); + update.append(" = CASE WHEN ").appendIdentifier(columnIdent).append(" IS NULL OR ").appendIdentifier(columnIdent).append(" = '' THEN ARRAY[]::text[] ELSE ARRAY["); + update.appendIdentifier(columnIdent).append("]::text[] END"); + statements.add(update); + + // 3) DROP original column + SQLFragment drop = new SQLFragment("ALTER TABLE "); + drop.appendIdentifier(change.getSchemaName()).append(".").appendIdentifier(change.getTableName()); + drop.append(" DROP COLUMN ").appendIdentifier(columnIdent); + statements.add(drop); + + // 4) RENAME temp column to original column name + SQLFragment rename = new SQLFragment("ALTER TABLE "); + rename.appendIdentifier(change.getSchemaName()).append(".").appendIdentifier(change.getTableName()); + rename.append(" RENAME COLUMN ").appendIdentifier(tempColumnIdent).append(" TO ").appendIdentifier(columnIdent); + statements.add(rename); + } else { String dbType; diff --git a/experiment/package-lock.json b/experiment/package-lock.json index a971f0b6448..d0a0824a510 100644 --- a/experiment/package-lock.json +++ b/experiment/package-lock.json @@ -8,7 +8,7 @@ "name": "experiment", "version": "0.0.0", "dependencies": { - "@labkey/components": "7.16.0" + "@labkey/components": "7.16.1-fb-mvtc-convert.0" }, "devDependencies": { "@labkey/build": "8.8.0", @@ -3591,9 +3591,9 @@ } }, "node_modules/@labkey/api": { - "version": "1.46.0", - "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/api/-/@labkey/api-1.46.0.tgz", - "integrity": "sha512-QwSm82KBc9Hhd5GUDe99J35xvXIWBtbvFbXxJh81x6le62EZFQdptDoxJyKGpDQiv+cITlNEfvYXjY3CO0y1Kw==", + "version": "1.46.1-fb-mvtc-convert.1", + "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/api/-/@labkey/api-1.46.1-fb-mvtc-convert.1.tgz", + "integrity": "sha512-i5uiP2ueHs+3PzRdD0wdGBKCGAOD0+CCKVgoIBE6bz0KI+ncbeBxIJXc4n+dAGVvnxxXnLoyUo7Xd6oQ/+2mhQ==", "license": "Apache-2.0" }, "node_modules/@labkey/build": { @@ -3634,13 +3634,13 @@ } }, "node_modules/@labkey/components": { - "version": "7.16.0", - "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-7.16.0.tgz", - "integrity": "sha512-w4wzsgmGW5ffzxlg8wdPqiwU3usgEJnNS9duFRmolzAKpplOpU1Z8YFyyKYfZq3HYPFb0yEKy1ho3N6z8VWtuA==", + "version": "7.16.1-fb-mvtc-convert.0", + "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-7.16.1-fb-mvtc-convert.0.tgz", + "integrity": "sha512-ey08b3dsYCfQ5EDdd9+SzypnsugCCtLdROKlMvumLOg873iXYqcZy+kR81jnXMhVE42RQwdI8H3xsKD5fSTlIQ==", "license": "SEE LICENSE IN LICENSE.txt", "dependencies": { "@hello-pangea/dnd": "18.0.1", - "@labkey/api": "1.46.0", + "@labkey/api": "1.46.1-fb-mvtc-convert.1", "@testing-library/dom": "~10.4.1", "@testing-library/jest-dom": "~6.9.1", "@testing-library/react": "~16.3.2", diff --git a/experiment/package.json b/experiment/package.json index e387ee9d9b3..73ab8c281c1 100644 --- a/experiment/package.json +++ b/experiment/package.json @@ -13,7 +13,7 @@ "test-integration": "cross-env NODE_ENV=test jest --ci --runInBand -c test/js/jest.config.integration.js" }, "dependencies": { - "@labkey/components": "7.16.0" + "@labkey/components": "7.16.1-fb-mvtc-convert.0" }, "devDependencies": { "@labkey/build": "8.8.0", diff --git a/experiment/src/client/test/integration/DataClassCrud.ispec.ts b/experiment/src/client/test/integration/DataClassCrud.ispec.ts index 84cdea746d8..746173f820c 100644 --- a/experiment/src/client/test/integration/DataClassCrud.ispec.ts +++ b/experiment/src/client/test/integration/DataClassCrud.ispec.ts @@ -1,4 +1,11 @@ -import { ExperimentCRUDUtils, hookServer, RequestOptions, successfulResponse } from '@labkey/test'; +import { + ExperimentCRUDUtils, + generateFieldName, + getEscapedNameExpression, + hookServer, + RequestOptions, + successfulResponse +} from '@labkey/test'; import mock from 'mock-fs'; import { checkDomainName, @@ -69,6 +76,41 @@ afterEach(() => { mock.restore(); }); + +// TODO: move utils to ExperimentCRUDUtils +export async function insertDataClassData( + rows: Record[], + dataType: string, + folderOptions: RequestOptions = topFolderOptions, +): Promise<{ lsid: string; name: string; rowId: number }[]> { + const resultRows = await ExperimentCRUDUtils.insertRows( + server, + rows, + 'exp.data', + dataType, + folderOptions, + editorUserOptions, + ); + + return resultRows.map(row => ({ + lsid: caseInsensitive(row, 'lsid'), + name: caseInsensitive(row, 'name'), + rowId: caseInsensitive(row, 'rowId'), + })); +} +export async function getDataClassDataByName(dataName: string, queryName: string, columns: string = 'Name, RowId', folderOptions: RequestOptions , userOptions: RequestOptions, debug?: boolean) : Promise { + const response = await server.post('query', 'selectRows', { + schemaName: 'exp.data', + queryName, + 'query.Name~eq': dataName, + 'query.columns': columns, + }, { ...folderOptions, ...userOptions }).expect(successfulResponse); + if (debug) + console.log(response); + return response.body.rows[0]; +} + + describe('Data Class Designer', () => { it('Lack designer or Reader permission', async () => { await checkLackDesignerOrReaderPerm(server, 'DataClass', topFolderOptions, readerUserOptions, editorUserOptions, designerOptions); @@ -434,3 +476,346 @@ describe('Duplicate IDs', () => { }); }); + + +describe('Multi Value Text Choice', () => { + + const mvtcFieldProp = { + "propertyId": -1, + "propertyValidators": [ + { + "type": "TextChoice", + "name": "Text Choice Validator", + "new": true, + "expression": "Abnormal|agent|cDNA|Plasma" + } + ], + "rangeURI": "http://cpas.fhcrc.org/exp/xml#multiChoice", + }; + + const tcFieldProp = { + ...mvtcFieldProp, + rangeURI: 'http://www.w3.org/2001/XMLSchema#string', + conceptURI: 'http://www.labkey.org/types#textChoice', + } + + + it("MVTC CRUD", async () => { + let supportMultiChoice = false; + const createTestPayload = { + kind: 'DataClass', + domainDesign: { name: 'Test_mvtc_support_check', fields: [{ name: 'Prop' }] }, + options: { + name: "Test_mvtc_support_check", + } + }; + + await server.post('property', 'createDomain', createTestPayload, + {...topFolderOptions, ...designerReaderOptions}).expect((result) => { + const domain = JSON.parse(result.text); + supportMultiChoice = domain.allowMultiChoiceProperties; + return true; + }); + + const dataType = 'MVTCReq Source Type'; + const fieldName = generateFieldName(); + const fieldNameInExpression = getEscapedNameExpression((fieldName)); + console.log("Selected Required MVTC dataclass name: " + dataType + ", field name: " + fieldName); + + const fields = [ + { + ...mvtcFieldProp, + name: fieldName + } + ]; + + let domainId = -1, domainURI = '', propertyId, propertyURI; + const createPayload = { + kind: 'DataClass', + domainDesign: { name: dataType, fields }, + options: { + name: dataType, + nameExpression: 'Src-${' + fieldNameInExpression + '}' + } + }; + + if (!supportMultiChoice) { + const failedCreateDomain = await server.post('property', 'createDomain', createPayload, + {...topFolderOptions, ...adminOptions}); + + expect(failedCreateDomain?.['body']?.['exception']).toContain('does not support multiple values.'); + + console.warn("Multi Value Text Choice not supported, skipping test"); + return; + } + + await server.post('property', 'createDomain', createPayload, {...topFolderOptions, ...designerReaderOptions}).expect((result) => { + const domain = JSON.parse(result.text); + domainId = domain.domainId; + domainURI = domain.domainURI; + const field = domain.fields[0]; + propertyId = field.propertyId; + propertyURI = field.propertyURI; + return true; + }); + + let dataCount = 0; + + // import invalid mvtc values, verify import should fail + let errorResp = await ExperimentCRUDUtils.importData(server, "Name\t" + fieldName + "\tDescription\nS-" + dataCount++ + "\ta,x\timport invalid mvtc", dataType, "IMPORT", topFolderOptions, editorUserOptions); + expect(errorResp.text.indexOf("Value 'a, x' for field") > -1).toBeTruthy(); + errorResp = await ExperimentCRUDUtils.importData(server, "Name\t" + fieldName + "\tDescription\nS-" + dataCount++ + "\tabc\timport invalid mvtc", dataType, "IMPORT", topFolderOptions, editorUserOptions); + expect(errorResp.text.indexOf("Value 'abc' for field") > -1).toBeTruthy(); + + // insert data using insertRows api, with invalid mvtc value + const invalidValues = ['x', 'a, x', 'x, y', ['x'], ['agent', 'x'], ['x', 'y']]; + invalidValues.forEach( async (val) => { + await server.post('query', 'insertRows', { + schemaName: 'exp.data', + queryName: dataType, + rows: [{ + name: 'invalid', + [fieldName]: val + }] + }, { ...topFolderOptions, ...editorUserOptions }).expect((result) => { + const errorResp = JSON.parse(result.text); + expect(errorResp['exception']).toContain('is invalid.'); + }); + }); + + // insert data using insertRows api, with no mvtc value + let inserted = await insertDataClassData( + [ + { 'name': 'S-' + dataCount++, 'description': 'no column' }, + { 'name': 'S-' + dataCount++, 'description': 'null column', [fieldName]: null }, + { 'name': 'S-' + dataCount++, 'description': 'blank column', [fieldName]: '' }, + { 'name': 'S-' + dataCount++, 'description': 'empty array', [fieldName]: [] }, + ], dataType, topFolderOptions + ); + inserted.forEach(async (row) => { + let res = await ExperimentCRUDUtils.getRows(server, [caseInsensitive(row, 'RowId')], 'exp.data', dataType, '*', topFolderOptions, adminOptions); + expect(caseInsensitive(res[0], fieldName)).toEqual([]); + }); + + // import data with no mvtc column + let importText = "Name\tDescription\n"; + let dataNameImported = ["S-" + dataCount++]; + importText += dataNameImported + "\timport no column\n"; + await ExperimentCRUDUtils.importData(server, importText, dataType, "IMPORT", topFolderOptions, editorUserOptions); + let result = await getDataClassDataByName(dataNameImported[0], dataType, '*', topFolderOptions, editorUserOptions); + expect(caseInsensitive(result, fieldName)).toEqual([]); + + // import data with mvtc fieldname column header, with blank, single, multi values + dataNameImported = ["S-" + dataCount++, "S-" + dataCount++, "S-" + dataCount++]; + importText = "Name\t" + fieldName + "\tDescription\n"; + importText += dataNameImported[0] + "\t\timport blank column\n"; + importText += dataNameImported[1] + "\tagent\timport single value\n"; + importText += dataNameImported[2] + "\tPlasma, agent, cDNA, Abnormal\timport multi values\n"; + await ExperimentCRUDUtils.importData(server, importText, dataType, "IMPORT", topFolderOptions, editorUserOptions); + const dataImportedRowIds = []; + // verify imported data mvtc + result = await getDataClassDataByName(dataNameImported[0], dataType, '*', topFolderOptions, editorUserOptions); + expect(caseInsensitive(result, fieldName)).toEqual([]); + dataImportedRowIds.push(caseInsensitive(result, 'rowId')); + result = await getDataClassDataByName(dataNameImported[1], dataType, '*', topFolderOptions, editorUserOptions); + expect(caseInsensitive(result, fieldName)).toEqual(['agent']); + dataImportedRowIds.push(caseInsensitive(result, 'rowId')); + result = await getDataClassDataByName(dataNameImported[2], dataType, '*', topFolderOptions, editorUserOptions); + expect(caseInsensitive(result, fieldName)).toEqual(['Abnormal', 'agent', 'cDNA', 'Plasma']); + dataImportedRowIds.push(caseInsensitive(result, 'rowId')); + + // update data using updateRows api, mvtc column absent from update + await ExperimentCRUDUtils.updateRows(server, [ + { 'rowId': dataImportedRowIds[0], 'description': 'no update' }, + { 'rowId': dataImportedRowIds[1], 'description': 'null column' }, + { 'rowId': dataImportedRowIds[2], 'description': 'blank column' }, + ], 'exp.data', dataType, topFolderOptions, editorUserOptions); + + result = await getDataClassDataByName(dataNameImported[0], dataType, '*', topFolderOptions, editorUserOptions); + expect(caseInsensitive(result, fieldName)).toEqual([]); + result = await getDataClassDataByName(dataNameImported[1], dataType, '*', topFolderOptions, editorUserOptions); + expect(caseInsensitive(result, fieldName)).toEqual(['agent']); + result = await getDataClassDataByName(dataNameImported[2], dataType, '*', topFolderOptions, editorUserOptions); + expect(caseInsensitive(result, fieldName)).toEqual(['Abnormal', 'agent', 'cDNA', 'Plasma']); + + // update data using updateRows api, mvtc column blank or empty or empty array + await ExperimentCRUDUtils.updateRows(server, [ + { 'rowId': dataImportedRowIds[0], [fieldName]: '' }, + { 'rowId': dataImportedRowIds[1], [fieldName]: null }, + { 'rowId': dataImportedRowIds[2], [fieldName]: [] }, + ], 'exp.data', dataType, topFolderOptions, editorUserOptions); + + result = await getDataClassDataByName(dataNameImported[0], dataType, '*', topFolderOptions, editorUserOptions); + expect(caseInsensitive(result, fieldName)).toEqual([]); + result = await getDataClassDataByName(dataNameImported[1], dataType, '*', topFolderOptions, editorUserOptions); + expect(caseInsensitive(result, fieldName)).toEqual([]); + result = await getDataClassDataByName(dataNameImported[2], dataType, '*', topFolderOptions, editorUserOptions); + expect(caseInsensitive(result, fieldName)).toEqual([]); + + // values are Abnormal|agent|cDNA|Plasma + const singleValues = ['Abnormal', 'cDNA', ['Abnormal'], ['cDNA']]; + const expectedSingleResults = [['Abnormal'], ['cDNA'], ['Abnormal'], ['cDNA']]; + for (let i = 0; i < singleValues.length; i++) { + const val = singleValues[i]; + const expected = expectedSingleResults[i]; + await ExperimentCRUDUtils.updateRows(server, [ + { 'rowId': dataImportedRowIds[0], [fieldName]: val }, + ], 'exp.data', dataType, topFolderOptions, editorUserOptions + ); + result = await getDataClassDataByName(dataNameImported[0], dataType, '*', topFolderOptions, editorUserOptions); + expect(caseInsensitive(result, fieldName)).toEqual(expected); + } + + const multiValues = ['agent, cDNA', 'cDNA, Plasma, Abnormal', ['agent', 'cDNA'], ['agent', 'Abnormal', 'cDNA']]; + const expectedMultiResults = [['agent', 'cDNA'], ['Abnormal', 'cDNA', 'Plasma'], ['agent', 'cDNA'], ['Abnormal', 'agent', 'cDNA']]; + for (let i = 0; i < multiValues.length; i++) { + const val = multiValues[i]; + const expected = expectedMultiResults[i]; + await ExperimentCRUDUtils.updateRows(server, [ + { 'rowId': dataImportedRowIds[1], [fieldName]: val }, + ], 'exp.data', dataType, topFolderOptions, editorUserOptions + ); + result = await getDataClassDataByName(dataNameImported[1], dataType, '*', topFolderOptions, editorUserOptions); + expect(caseInsensitive(result, fieldName)).toEqual(expected); + } + + // update with import to set mvtc from non blank to blank + importText = "Name\t" + fieldName + "\tDescription\n"; + importText += dataNameImported[0] + "\t\timport blank column\n"; + importText += dataNameImported[1] + "\t\timport single value\n"; + await ExperimentCRUDUtils.importData(server, importText, dataType, "UPDATE", topFolderOptions, editorUserOptions); + result = await getDataClassDataByName(dataNameImported[0], dataType, '*', topFolderOptions, editorUserOptions); + expect(caseInsensitive(result, fieldName)).toEqual([]); + result = await getDataClassDataByName(dataNameImported[1], dataType, '*', topFolderOptions, editorUserOptions); + expect(caseInsensitive(result, fieldName)).toEqual([]); + + // update with import to set mvtc from blank to multi values + importText = "Name\t" + fieldName + "\tDescription\n"; + importText += dataNameImported[0] + "\tAbnormal, agent\timport multi value\n"; + importText += dataNameImported[1] + "\tcDNA, Plasma, Abnormal\timport multi values\n"; + await ExperimentCRUDUtils.importData(server, importText, dataType, "UPDATE", topFolderOptions, editorUserOptions); + result = await getDataClassDataByName(dataNameImported[0], dataType, '*', topFolderOptions, editorUserOptions); + expect(caseInsensitive(result, fieldName)).toEqual(['Abnormal', 'agent']); + result = await getDataClassDataByName(dataNameImported[1], dataType, '*', topFolderOptions, editorUserOptions); + expect(caseInsensitive(result, fieldName)).toEqual(['Abnormal', 'cDNA', 'Plasma']); + + // merge with import to change mvtc and create new data + const newMerged = 'S-' + dataCount++; + importText = "Name\t" + fieldName + "\tDescription\n"; + importText += dataNameImported[0] + "\tPlasma, Abnormal\timport multi value\n"; + importText += newMerged + "\tagent, cDNA\timport multi values\n"; + await ExperimentCRUDUtils.importData(server, importText, dataType, "MERGE", topFolderOptions, editorUserOptions); + result = await getDataClassDataByName(dataNameImported[0], dataType, '*', topFolderOptions, editorUserOptions); + expect(caseInsensitive(result, fieldName)).toEqual(['Abnormal', 'Plasma']); + result = await getDataClassDataByName(newMerged, dataType, '*', topFolderOptions, editorUserOptions); + expect(caseInsensitive(result, fieldName)).toEqual(['agent', 'cDNA']); + + // insert using insertRows api, with single mvtc value + let toInsert = []; + for (let i = 0; i < singleValues.length; i++) { + const val = singleValues[i]; + toInsert.push({ 'name': 'S-' + dataCount++, [fieldName]: val , 'description': 'insert single value' }); + } + + inserted = await ExperimentCRUDUtils.insertRows(server, toInsert, 'exp.data', dataType, topFolderOptions, editorUserOptions); + + for (let i = 0; i < singleValues.length; i++) { + const expected = expectedSingleResults[i]; + let res = await ExperimentCRUDUtils.getSourcesData(server, [caseInsensitive(inserted[i], 'RowId')], dataType, '*', topFolderOptions, adminOptions); + expect(caseInsensitive(res[0], fieldName)).toEqual(expected); + } + + // insert using insertRows api, with multi mvtc values + toInsert = []; + for (let i = 0; i < multiValues.length; i++) { + const val = multiValues[i]; + toInsert.push({ 'name': 'S-' + dataCount++, [fieldName]: val , 'description': 'insert multi values' }); + } + + inserted = await ExperimentCRUDUtils.insertRows(server, toInsert, 'exp.data', dataType, topFolderOptions, editorUserOptions); + + for (let i = 0; i < multiValues.length; i++) { + const expected = expectedMultiResults[i]; + let res = await ExperimentCRUDUtils.getSourcesData(server, [caseInsensitive(inserted[i], 'RowId')], dataType, '*', topFolderOptions, adminOptions); + expect(caseInsensitive(res[0], fieldName)).toEqual(expected); + } + + // verify convert to required column fails with existing blank values + let dataClassRowId = await getDataClassRowIdByName(server, dataType, topFolderOptions) + + let updatePayload: any = { + domainId, + domainDesign: { + name: dataType, + fields: [ + { + ...mvtcFieldProp, + name: fieldName, + required: true + } + ], + domainId, + domainURI + }, + options: { + rowId: dataClassRowId, + name: dataType, + nameExpression: 'Src-${' + fieldNameInExpression + '}' + } + }; + let failedUpdate = await server.post('property', 'saveDomain', updatePayload, {...topFolderOptions, ...adminOptions}); + expect(failedUpdate?.['body']?.['exception']).toContain('cannot be required when it contains rows with blank values.'); + + // verify convert to single value text choice fails with existing multiple values + updatePayload = { + domainId, + domainDesign: { + name: dataType, + fields: [ + { + ...tcFieldProp, + name: fieldName, + propertyId, + propertyURI + } + ], + domainId, + domainURI + }, + options: { + rowId: dataClassRowId, + name: dataType, + nameExpression: 'S-${' + fieldNameInExpression + '}' + } + }; + failedUpdate = await server.post('property', 'saveDomain', updatePayload, {...topFolderOptions, ...adminOptions}); + expect(failedUpdate?.['body']?.['exception']).toContain('Unable to change property type. There are rows with multiple values stored for'); + + // verify can convert to Text field type + updatePayload = { + domainId, + domainDesign: { + name: dataType, + fields: [ + { + name: fieldName, + propertyId, + propertyURI + } + ], + domainId, + domainURI + }, + options: { + rowId: dataClassRowId, + name: dataType, + nameExpression: 'S-${' + fieldNameInExpression + '}' + } + }; + await server.post('property', 'saveDomain', updatePayload, {...topFolderOptions, ...adminOptions}).expect(successfulResponse); + result = await getDataClassDataByName(dataNameImported[0], dataType, '*', topFolderOptions, editorUserOptions); + expect(caseInsensitive(result, fieldName)).toEqual('Abnormal, Plasma'); // convert from ['Abnormal', 'Plasma'] to 'Abnormal, Plasma' + + }); + +}); \ No newline at end of file diff --git a/experiment/src/org/labkey/experiment/ExperimentModule.java b/experiment/src/org/labkey/experiment/ExperimentModule.java index 4b588980747..c90e7321cec 100644 --- a/experiment/src/org/labkey/experiment/ExperimentModule.java +++ b/experiment/src/org/labkey/experiment/ExperimentModule.java @@ -275,8 +275,6 @@ protected void init() 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 value for text choice fields", false); } OptionalFeatureService.get().addExperimentalFeatureFlag(AppProps.QUANTITY_COLUMN_SUFFIX_TESTING, "Quantity column suffix testing", "If a column name contains a \"__\" suffix, this feature allows for testing it as a Quantity display column", false); diff --git a/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java b/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java index 2e6bc7b6375..93c200f9b51 100644 --- a/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java @@ -134,6 +134,7 @@ import java.io.IOException; import java.io.UncheckedIOException; +import java.nio.file.Path; import java.sql.SQLException; import java.util.ArrayList; import java.util.Arrays; @@ -1429,6 +1430,17 @@ protected Map updateRow(User user, Container container, Map _update(User user, Container c, Map row, Map oldRow, Object[] keys) throws SQLException, ValidationException { diff --git a/experiment/src/org/labkey/experiment/api/property/DomainPropertyImpl.java b/experiment/src/org/labkey/experiment/api/property/DomainPropertyImpl.java index 2b10c8c8718..38a07a2f279 100644 --- a/experiment/src/org/labkey/experiment/api/property/DomainPropertyImpl.java +++ b/experiment/src/org/labkey/experiment/api/property/DomainPropertyImpl.java @@ -26,6 +26,7 @@ import org.labkey.api.data.ColumnRenderPropertiesImpl; import org.labkey.api.data.ConditionalFormat; import org.labkey.api.data.Container; +import org.labkey.api.data.ContainerFilter; import org.labkey.api.data.ContainerManager; import org.labkey.api.data.DatabaseIdentifier; import org.labkey.api.data.JdbcType; @@ -33,6 +34,7 @@ import org.labkey.api.data.SQLFragment; import org.labkey.api.data.SqlExecutor; import org.labkey.api.data.Table; +import org.labkey.api.data.TableInfo; import org.labkey.api.data.dialect.SqlDialect; import org.labkey.api.exp.ChangePropertyDescriptorException; import org.labkey.api.exp.DomainDescriptor; @@ -52,6 +54,8 @@ import org.labkey.api.gwt.client.DefaultScaleType; import org.labkey.api.gwt.client.DefaultValueType; import org.labkey.api.gwt.client.FacetingBehaviorType; +import org.labkey.api.query.QueryChangeListener; +import org.labkey.api.query.SchemaKey; import org.labkey.api.security.User; import org.labkey.api.util.StringExpressionFactory; import org.labkey.api.util.TestContext; @@ -840,6 +844,11 @@ else if (newType.getJdbcType().isDateOrTime() && oldType.getJdbcType().isDateOrT changedType = true; _pd.setFormat(null); } + else if (newType == PropertyType.MULTI_CHOICE || oldType == PropertyType.MULTI_CHOICE) + { + changedType = true; + _pd.setFormat(null); + } else { throw new ChangePropertyDescriptorException("Cannot convert an instance of " + oldType.getJdbcType() + " to " + newType.getJdbcType() + "."); @@ -873,13 +882,21 @@ else if (newType.getJdbcType().isDateOrTime() && oldType.getJdbcType().isDateOrT if (changedType) { + var domainKind = _domain.getDomainKind(); + if (domainKind == null) + throw new ChangePropertyDescriptorException("Cannot change property type for domain, unknown domain kind."); + StorageProvisionerImpl.get().changePropertyType(this.getDomain(), this); if (_pdOld.getJdbcType() == JdbcType.BOOLEAN && _pd.getJdbcType().isText()) { updateBooleanValue( - new SQLFragment().appendIdentifier(_domain.getDomainKind().getStorageSchemaName()).append(".").appendIdentifier(_domain.getStorageTableName()), + new SQLFragment().appendIdentifier(domainKind.getStorageSchemaName()).append(".").appendIdentifier(_domain.getStorageTableName()), _pd.getLegalSelectName(dialect), _pdOld.getFormat(), null); // GitHub Issue #647 } + + TableInfo table = domainKind.getTableInfo(user, getContainer(), _domain, ContainerFilter.getUnsafeEverythingFilter()); + if (table != null && _pdOld.getPropertyType() != null && table.getSchema().getSqlDialect().isPostgreSQL()) + QueryChangeListener.QueryPropertyChange.handleColumnTypeChange(_pdOld, _pd, SchemaKey.fromString(table.getUserSchema().getSchemaName()), table.getName(), user, getContainer()); } else if (propResized) StorageProvisionerImpl.get().resizeProperty(this.getDomain(), this, _pdOld.getScale()); diff --git a/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java b/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java index 727423a1a70..a2e110b17ae 100644 --- a/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java +++ b/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java @@ -66,6 +66,7 @@ import org.labkey.api.exp.OntologyManager; import org.labkey.api.exp.PropertyColumn; import org.labkey.api.exp.PropertyDescriptor; +import org.labkey.api.exp.PropertyType; import org.labkey.api.exp.api.ExperimentUrls; import org.labkey.api.exp.api.StorageProvisioner; import org.labkey.api.exp.property.AbstractDomainKind; @@ -112,6 +113,8 @@ import java.util.concurrent.TimeUnit; import java.util.function.Supplier; +import static org.labkey.api.data.ColumnRenderPropertiesImpl.TEXT_CHOICE_CONCEPT_URI; + /** * Creates and maintains "hard" tables in the underlying database based on dynamically configured data types. * Will do CREATE TABLE and ALTER TABLE statements to make sure the table has the right set of requested columns. @@ -573,9 +576,40 @@ public void changePropertyType(Domain domain, DomainProperty prop) throws Change Set base = Sets.newCaseInsensitiveHashSet(); kind.getBaseProperties(domain).forEach(s -> base.add(s.getName())); + Map oldPropTypes = new HashMap<>(); if (!base.contains(prop.getName())) + { + if (prop instanceof DomainPropertyImpl dpi) + { + var oldPd = dpi._pdOld; + if (oldPd != null) + { + var newPd = dpi._pd; + if (oldPd.getPropertyType() == PropertyType.MULTI_CHOICE && TEXT_CHOICE_CONCEPT_URI.equals(newPd.getConceptURI())) + { + var selectColumnName = prop.getPropertyDescriptor().getLegalSelectName(scope.getSqlDialect()); + SQLFragment sql = new SQLFragment("SELECT COUNT(*) FROM ") + .appendDottedIdentifiers(kind.getStorageSchemaName(), domain.getStorageTableName()) + .append(" WHERE ") + .appendIdentifier(selectColumnName) + .append(" IS NOT NULL AND array_length(") + .appendIdentifier(selectColumnName) + .append(", 1) > 1"); + long count = new SqlSelector(scope, sql).getObject(Long.class); + if (count > 0) + { + throw new ChangePropertyDescriptorException("Unable to change property type. There are rows with multiple values stored for '" + prop.getName() + "'."); + } + } + oldPropTypes.put(prop.getName(), oldPd.getPropertyType()); + } + + } + propChange.addColumn(prop.getPropertyDescriptor()); + } + propChange.setOldPropertyTypes(oldPropTypes); propChange.execute(); } diff --git a/query/src/org/labkey/query/CustomViewQueryChangeListener.java b/query/src/org/labkey/query/CustomViewQueryChangeListener.java index d1c16e0fee3..f881cb24290 100644 --- a/query/src/org/labkey/query/CustomViewQueryChangeListener.java +++ b/query/src/org/labkey/query/CustomViewQueryChangeListener.java @@ -20,14 +20,21 @@ import org.labkey.api.collections.CaseInsensitiveHashMap; import org.labkey.api.data.Container; import org.labkey.api.data.ContainerFilter; +import org.labkey.api.exp.PropertyDescriptor; +import org.labkey.api.exp.property.DomainProperty; import org.labkey.api.query.CustomView; import org.labkey.api.query.CustomViewChangeListener; import org.labkey.api.query.CustomViewInfo; import org.labkey.api.query.FieldKey; import org.labkey.api.query.QueryChangeListener; +import org.labkey.api.query.QueryDefinition; import org.labkey.api.query.QueryService; import org.labkey.api.query.SchemaKey; import org.labkey.api.security.User; +import org.labkey.api.exp.PropertyType; + +import java.util.regex.Pattern; +import java.util.regex.Matcher; import org.springframework.mock.web.MockHttpServletRequest; import jakarta.servlet.http.HttpServletRequest; @@ -65,6 +72,70 @@ public void queryChanged(User user, Container container, ContainerFilter scope, { _updateCustomViewSchemaNameChange(user, container, changes); } + if (property.equals(QueryProperty.ColumnType)) + { + _updateCustomViewColumnTypeChange(user, container, schema, changes); + } + } + + + private void _updateCustomViewColumnTypeChange(User user, Container container, SchemaKey schema, @NotNull Collection> changes) + { + for (QueryPropertyChange qpc : changes) + { + QueryDefinition queryDefinition = qpc.getSource(); + if (queryDefinition == null) + continue; + String queryName = queryDefinition.getName(); + + PropertyDescriptor oldDp = (PropertyDescriptor) qpc.getOldValue(); + PropertyDescriptor newDp = (PropertyDescriptor) qpc.getNewValue(); + + if (oldDp == null || newDp == null) + continue; + + String columnName = newDp.getName() == null ? oldDp.getName() : newDp.getName(); + + List databaseCustomViews = QueryService.get().getDatabaseCustomViews(user, container, null, schema.toString(), queryName, false, false); + + for (CustomView customView : databaseCustomViews) + { + try + { + // update custom view filter and sort based on column type change + String filterAndSort = customView.getFilterAndSort(); + if (filterAndSort == null || filterAndSort.isEmpty()) + continue; + + /* Example: + * "/?filter.MCF2~arrayisnotempty=&filter.Name~in=S-5%3BS-6%3BS-8%3BS-9&filter.MCF~arraycontainsall=2%3B1%3B3&filter.sort=zz" + */ + String prefix = filterAndSort.startsWith("/?") ? "/?" : (filterAndSort.startsWith("?") ? "?" : ""); + String[] filterComponents = filterAndSort.substring(prefix.length()).split("&"); + StringBuilder updatedFilterAndSort = new StringBuilder(prefix); + String sep = ""; + for (String filterPart : filterComponents) + { + String updatedPart = QueryChangeListener.getUpdatedFilterStrOnColumnTypeUpdate(filterPart, "filter", columnName, oldDp, newDp); + updatedFilterAndSort.append(sep).append(updatedPart); + sep = "&"; + } + + String updatedFilterAndSortStr = updatedFilterAndSort.toString(); + if (!updatedFilterAndSortStr.equals(filterAndSort)) + { + customView.setFilterAndSort(updatedFilterAndSortStr); + HttpServletRequest request = new MockHttpServletRequest(); + customView.save(customView.getModifiedBy(), request); + } + } + catch (Exception e) + { + LogManager.getLogger(CustomViewQueryChangeListener.class).error("An error occurred upgrading custom view properties: ", e); + } + } + } + } @Override diff --git a/query/src/org/labkey/query/QueryServiceImpl.java b/query/src/org/labkey/query/QueryServiceImpl.java index cc576f9d55d..338e41e3895 100644 --- a/query/src/org/labkey/query/QueryServiceImpl.java +++ b/query/src/org/labkey/query/QueryServiceImpl.java @@ -3273,6 +3273,12 @@ public void fireQueryChanged(User user, Container container, ContainerFilter sco QueryManager.get().fireQueryChanged(user, container, scope, schema, property, changes); } + @Override + public void fireQueryColumnChanged(User user, Container container, @NotNull SchemaKey schemaPath, QueryChangeListener.QueryProperty property, Collection> changes) + { + QueryManager.get().fireQueryChanged(user, container, null, schemaPath, property, changes); + } + @Override public void fireQueryDeleted(User user, Container container, ContainerFilter scope, SchemaKey schema, Collection queries) { diff --git a/query/src/org/labkey/query/QueryTestCase.jsp b/query/src/org/labkey/query/QueryTestCase.jsp index 5cf3e7d3446..1831ba89ada 100644 --- a/query/src/org/labkey/query/QueryTestCase.jsp +++ b/query/src/org/labkey/query/QueryTestCase.jsp @@ -1954,6 +1954,10 @@ d,seven,twelve,day,month,date,duration,guid SELECT 'f' as test, true as expected, array_contains_element( ARRAY['A','B'], 'B') as result UNION ALL SELECT 'g' as test, false as expected, array_contains_element( ARRAY['A','B'], 'X') as result + UNION ALL + SELECT 'h' as test, true as expected, array_contains_any( ARRAY['\"A','X'], ARRAY['\"A','B'] ) as result + UNION ALL + SELECT 'i' as test, true as expected, array_is_same( ARRAY['A;','X'], ARRAY['A;','X'] ) as result """; Container container = JunitUtil.getTestContainer(); diff --git a/query/src/org/labkey/query/sql/CalculatedExpressionColumn.java b/query/src/org/labkey/query/sql/CalculatedExpressionColumn.java index 0d0664a5db4..14d839177c7 100644 --- a/query/src/org/labkey/query/sql/CalculatedExpressionColumn.java +++ b/query/src/org/labkey/query/sql/CalculatedExpressionColumn.java @@ -1,5 +1,6 @@ package org.labkey.query.sql; +import org.apache.commons.beanutils.ConversionException; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.labkey.api.data.AbstractTableInfo; @@ -371,6 +372,11 @@ private QExpr getBoundExpression(Map columnMap) return _boundExpr; } + @Override + public Object convert(Object o) throws ConversionException + { + return o; + } /* * TESTS