diff --git a/api/src/org/labkey/api/data/SchemaColumnMetaData.java b/api/src/org/labkey/api/data/SchemaColumnMetaData.java index 95aaa20dac0..b1a108fe17b 100644 --- a/api/src/org/labkey/api/data/SchemaColumnMetaData.java +++ b/api/src/org/labkey/api/data/SchemaColumnMetaData.java @@ -59,6 +59,7 @@ public class SchemaColumnMetaData { private final SchemaTableInfo _tinfo; private final List _columns = new ArrayList<>(); + private AliasManager _aliasManager; private Map _colMap = null; private @NotNull List _pkColumnNames = new ArrayList<>(); @@ -89,14 +90,14 @@ public SchemaColumnMetaData(SchemaTableInfo tinfo, List cols, loadColumnsFromXml(_tinfo, xmlTable); } - private AliasManager getAliasManager(AliasManager aliasManager) + private AliasManager getAliasManager() { - if (null == aliasManager) + if (null == _aliasManager) { - aliasManager = new AliasManager(_tinfo.getSchema()); - aliasManager.claimAliases(_columns); + _aliasManager = new AliasManager(_tinfo.getSchema()); + _aliasManager.claimAliases(_columns); } - return aliasManager; + return _aliasManager; } private void loadColumnsFromXml(SchemaTableInfo tinfo, TableType xmlTable) @@ -153,8 +154,7 @@ private void loadColumnsFromXml(SchemaTableInfo tinfo, TableType xmlTable) colInfo = new BaseColumnInfo(FieldKey.fromParts(xmlColumn.getColumnName()), tinfo); colInfo.setNullable(true); loadFromXml(xmlColumn, colInfo, false); - aliasManager = getAliasManager(aliasManager); - aliasManager.ensureAlias(colInfo); + getAliasManager().ensureAlias(colInfo); addColumn(colInfo); } } @@ -198,8 +198,7 @@ else if (xmlColumn.isSetWrappedColumnName() && isNotBlank(xmlColumn.getWrappedCo exprColumn.getAlias(); assert exprColumn.isAliasSet(); // now reserve that alias - aliasManager = getAliasManager(aliasManager); - aliasManager.ensureAlias(exprColumn); + getAliasManager().ensureAlias(exprColumn); addColumn(exprColumn); } catch (QueryParseException qpe) @@ -422,9 +421,14 @@ protected void addColumn(MutableColumnInfo column) if (!column.isAliasSet()) { if (null != column.getMetaDataName()) + { column.setAlias(column.getMetaDataName()); + getAliasManager().claimAlias(column); + } else - column.setAlias(column.getName()); + { + getAliasManager().ensureAlias(column); + } } _colMap = null; } diff --git a/api/src/org/labkey/api/dataiterator/StatementDataIterator.java b/api/src/org/labkey/api/dataiterator/StatementDataIterator.java index 7ace4293c30..cbc47a1e3e6 100644 --- a/api/src/org/labkey/api/dataiterator/StatementDataIterator.java +++ b/api/src/org/labkey/api/dataiterator/StatementDataIterator.java @@ -58,6 +58,7 @@ import java.sql.SQLException; import java.util.ArrayList; import java.util.Collections; +import java.util.IdentityHashMap; import java.util.Map; import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; @@ -190,17 +191,22 @@ void init() ParameterMapStatement stmt = _stmts[set]; // map from source to target ArrayList bindings = new ArrayList<>(stmt.size()); + IdentityHashMap boundParametersMap = new IdentityHashMap<>(); // by name for (int i=1 ; i<=_data.getColumnCount() ; i++) { ColumnInfo col = _data.getColumnInfo(i); - Parameter to = null; - if (null != col.getPropertyURI()) + Parameter to = stmt.getParameter(col.getName()); + if (null == to && null != col.getPropertyURI()) to = stmt.getParameter(col.getPropertyURI()); - if (to == null) - to = stmt.getParameter(col.getName()); if (null != to) { + var prev = boundParametersMap.put(to,col); + if (null != prev) + { + throw new IllegalStateException("Two columns mapped to target column '" + to.getName() + "'." + + " Found '" + prev.getName() + "' and '" + col.getName() + "'."); + } FieldKey mvName = col.getMvColumnName(); bindings.add(new Triple(_data.getSupplier(i), to, (null != mvName ? getMvParameter(stmt, mvName) : null))); diff --git a/api/src/org/labkey/api/query/AliasedColumn.java b/api/src/org/labkey/api/query/AliasedColumn.java index 4159232c5c1..a673bf0fc25 100644 --- a/api/src/org/labkey/api/query/AliasedColumn.java +++ b/api/src/org/labkey/api/query/AliasedColumn.java @@ -37,6 +37,8 @@ public AliasedColumn(TableInfo parent, FieldKey key, ColumnInfo column, boolean { super(key, parent); copyAttributesFrom(column); + // property URI should be unique, and certainly within one table + setPropertyURI(null); Map remap = new HashMap<>(); remap.put(column.getFieldKey(), key); diff --git a/study/src/org/labkey/study/StudyModule.java b/study/src/org/labkey/study/StudyModule.java index b5d7bb9d2f9..a300b18baad 100644 --- a/study/src/org/labkey/study/StudyModule.java +++ b/study/src/org/labkey/study/StudyModule.java @@ -167,6 +167,7 @@ import org.labkey.study.qc.StudyQCImportExportHelper; import org.labkey.study.qc.StudyQCStateHandler; import org.labkey.study.query.DatasetQueryView; +import org.labkey.study.query.DatasetUpdateService; import org.labkey.study.query.QueryDatasetQueryChangeListener; import org.labkey.study.query.StudyPersonnelDomainKind; import org.labkey.study.query.StudyQuerySchema; @@ -777,7 +778,8 @@ public Set getIntegrationTests() StudyManager.VisitCreationTestCase.class, StudyModule.TestCase.class, TreatmentManager.TreatmentDataTestCase.class, - VisitImpl.TestCase.class + VisitImpl.TestCase.class, + DatasetUpdateService.TestCase.class ); } diff --git a/study/src/org/labkey/study/model/DatasetDefinition.java b/study/src/org/labkey/study/model/DatasetDefinition.java index eb613c11025..bb9fea83abd 100644 --- a/study/src/org/labkey/study/model/DatasetDefinition.java +++ b/study/src/org/labkey/study/model/DatasetDefinition.java @@ -1687,6 +1687,9 @@ public TableInfo getSchemaTableInfo() public CaseInsensitiveHashMap remapSchemaColumns() { CaseInsensitiveHashMap m = new CaseInsensitiveHashMap<>(); + + if (!DatasetDomainKind.PARTICIPANTID.equalsIgnoreCase(_study.getSubjectColumnName())) + m.put(DatasetDomainKind.PARTICIPANTID, _study.getSubjectColumnName()); // why did I add an underscore to the stored mv indicators??? for (ColumnInfo col : getColumns()) @@ -2754,38 +2757,40 @@ public List> getDatasetRows(User u, Collection lsids ((ArrayListMap)datas.get(0)).getFindMap().remove("_key"); } - List> canonicalDatas = new ArrayList<>(datas.size()); + // results should not be sensitive to the column aliases, convert aliases to column names + CaseInsensitiveHashMap aliasToColumnNames = new CaseInsensitiveHashMap<>(); + List columns = tInfo.getColumns(); + for (ColumnInfo col : columns) + { + var name = col.getName(); + // NOTE: case of columns in tInfo and queryTableInfo seem to match except for QCstate, leaving for now + var queryCol = queryTableInfo.getColumn(name); + if (null != queryCol) + name = queryCol.getName(); + aliasToColumnNames.put(col.getAlias(), name); + } + List> canonicalDatas = new ArrayList<>(datas.size()); for (Map data : datas) { - canonicalDatas.add(canonicalizeDatasetRow(data, queryTableInfo.getColumns())); + canonicalDatas.add(canonicalizeDatasetRow(data, aliasToColumnNames)); } return canonicalDatas; } // change a map's keys to have proper casing just like the list of columns - private Map canonicalizeDatasetRow(Map source, List columns) + private Map canonicalizeDatasetRow(Map source, Map aliasToColumnNames) { - CaseInsensitiveHashMap keyNames = new CaseInsensitiveHashMap<>(); - for (ColumnInfo col : columns) - { - keyNames.put(col.getName(), col.getName()); - } - Map result = new CaseInsensitiveHashMap<>(); - for (Map.Entry entry : source.entrySet()) { String key = entry.getKey(); - String newKey = keyNames.get(key); - if (newKey != null) - key = newKey; - else if ("_row".equals(key)) + if ("_row".equals(key)) continue; + key = aliasToColumnNames.getOrDefault(key, key); result.put(key, entry.getValue()); } - return result; } diff --git a/study/src/org/labkey/study/query/DatasetUpdateService.java b/study/src/org/labkey/study/query/DatasetUpdateService.java index abe6279dc97..afe358682ad 100644 --- a/study/src/org/labkey/study/query/DatasetUpdateService.java +++ b/study/src/org/labkey/study/query/DatasetUpdateService.java @@ -19,13 +19,20 @@ import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; import org.labkey.api.collections.CaseInsensitiveHashMap; import org.labkey.api.collections.CaseInsensitiveHashSet; import org.labkey.api.collections.ResultSetRowMapFactory; import org.labkey.api.data.BaseColumnInfo; import org.labkey.api.data.ColumnInfo; import org.labkey.api.data.Container; +import org.labkey.api.data.ContainerManager; import org.labkey.api.data.DbScope; +import org.labkey.api.data.JdbcType; +import org.labkey.api.data.PropertyStorageSpec; import org.labkey.api.data.RuntimeSQLException; import org.labkey.api.data.SQLFragment; import org.labkey.api.data.SimpleFilter; @@ -45,6 +52,7 @@ import org.labkey.api.qc.DataStateManager; import org.labkey.api.query.AbstractQueryUpdateService; import org.labkey.api.query.BatchValidationException; +import org.labkey.api.query.DefaultSchema; import org.labkey.api.query.FieldKey; import org.labkey.api.query.InvalidKeyException; import org.labkey.api.query.QueryService; @@ -55,13 +63,19 @@ import org.labkey.api.security.User; import org.labkey.api.security.UserPrincipal; import org.labkey.api.security.permissions.Permission; -import org.labkey.api.security.roles.Role; import org.labkey.api.study.Dataset; import org.labkey.api.study.Study; import org.labkey.api.study.StudyService; +import org.labkey.api.study.TimepointType; import org.labkey.api.study.security.StudySecurityEscalator; +import org.labkey.api.test.TestWhen; +import org.labkey.api.util.DateUtil; +import org.labkey.api.util.GUID; +import org.labkey.api.util.JunitUtil; +import org.labkey.api.util.TestContext; import org.labkey.study.model.DatasetDefinition; import org.labkey.study.model.DatasetDomainKind; +import org.labkey.study.model.SecurityType; import org.labkey.study.model.StudyImpl; import org.labkey.study.model.StudyManager; import org.labkey.study.visitmanager.PurgeParticipantsJob.ParticipantPurger; @@ -121,7 +135,6 @@ public enum Config private final Set _potentiallyNewParticipants = new HashSet<>(); private final Set _potentiallyDeletedParticipants = new HashSet<>(); private boolean _participantVisitResyncRequired = false; - private Set _contextualRoles = Set.of(); /** Mapping for MV column names */ private Map _columnMapping = Collections.emptyMap(); @@ -312,6 +325,7 @@ else if (!isBulkLoad()) return result; } + @Override protected DataIteratorBuilder preTriggerDataIterator(DataIteratorBuilder in, DataIteratorContext context) { // If we're using a managed GUID as a key, wire it up here so that it's available to trigger scripts @@ -488,7 +502,7 @@ public boolean equals(Object o) PurgeParticipantCommitTask that = (PurgeParticipantCommitTask) o; - if (_container != null ? !_container.equals(that._container) : that._container != null) return false; + if (!Objects.equals(_container, that._container)) return false; return true; } @@ -660,11 +674,10 @@ protected Map updateRow(User user, Container container, Map 1) } else return lsids[0]; } + + + + @TestWhen(TestWhen.When.BVT) + public static class TestCase extends Assert + { + TestContext _context = null; + User _user = null; + Container _container = null; + StudyImpl _junitStudy = null; + StudyManager _manager = StudyManager.getInstance(); + String longName = "this is a very long name (with punctuation) that raises many questions \"?\" about your database design choices"; + + @Test + public void updateRow() throws Exception + { + var dsd = new DatasetDefinition(_junitStudy, 1001, "DS1", "DS1", null, null, null); + _manager.createDatasetDefinition(_user, dsd); + dsd = _manager.getDatasetDefinition(_junitStudy, 1001); + assertNotNull(dsd); + dsd.getStorageTableInfo(); + var domain = dsd.getDomain(); + assertNotNull(domain); + domain.addProperty(new PropertyStorageSpec("Field1", JdbcType.VARCHAR)); + domain.addProperty(new PropertyStorageSpec("SELECT", JdbcType.VARCHAR)); // keyword + domain.addProperty(new PropertyStorageSpec(longName, JdbcType.VARCHAR)); // keyword + domain.save(_user); + + TableInfo t = DefaultSchema.get(_user, _container).getSchema("study").getTable("DS1"); + assertNotNull(t); + assertTrue("Field1".equalsIgnoreCase(t.getColumn("Field1").getAlias())); + assertFalse("SELECT".equalsIgnoreCase(t.getColumn("SELECT").getAlias())); + assertFalse(longName.equalsIgnoreCase(t.getColumn(longName).getAlias())); + var up = t.getUpdateService(); + assertNotNull(up); + var errors = new BatchValidationException(); + + var result = up.insertRows(_user, _container, + List.of(Map.of("subjectid", "S1", "SequenceNum", "1.2345", "Field1", "f", "SELECT", "s", longName, "l")), + errors, null, null); + if (errors.hasErrors()) + fail(errors.getMessage()); + assertFalse(errors.hasErrors()); + assertNotNull(result); + assertEquals(1, result.size()); + var map = result.get(0); + assertEquals("S1", map.get("SubjectId")); + assertEquals("f", map.get("Field1")); + assertEquals("s", map.get("SELECT")); + assertEquals("l", map.get(longName)); + assertNotNull(map.get("lsid")); + assertTrue(((String)map.get("lsid")).endsWith(":1001.S1.1.2345")); + String lsid = (String)map.get("lsid"); + + result = up.updateRows(_user, _container, + List.of(Map.of("subjectid", "S2")), + List.of(Map.of("lsid", lsid)), + errors, null, null); + if (errors.hasErrors()) + fail(errors.getMessage()); + assertNotNull(result); + assertEquals(1, result.size()); + map = result.get(0); + assertEquals("S2", map.get("SubjectId")); + assertEquals("f", map.get("Field1")); + assertEquals("s", map.get("SELECT")); + assertEquals("l", map.get(longName)); + assertTrue(((String)map.get("lsid")).contains(":1001.S2.1.2345")); + } + + @Before + public void createStudy() + { + _context = TestContext.get(); + Container junit = JunitUtil.getTestContainer(); + String name = GUID.makeHash(); + Container c = ContainerManager.createContainer(junit, name, _context.getUser()); + StudyImpl s = new StudyImpl(c, "Junit Study"); + s.setTimepointType(TimepointType.VISIT); + s.setStartDate(new Date(DateUtil.parseDateTime(c, "2014-01-01"))); + s.setSubjectColumnName("SubjectID"); + s.setSubjectNounPlural("Subjects"); + s.setSubjectNounSingular("Subject"); + s.setSecurityType(SecurityType.BASIC_WRITE); + _junitStudy = StudyManager.getInstance().createStudy(_context.getUser(), s); + _user = _context.getUser(); + _container = _junitStudy.getContainer(); + } + + @After + public void tearDown() + { + if (null != _junitStudy) + { + assertTrue(ContainerManager.delete(_junitStudy.getContainer(), _context.getUser())); + } + } + } + }