GitHub Issue 903: Remove Cross-Container Sample and Data Class Import Feature#7698
GitHub Issue 903: Remove Cross-Container Sample and Data Class Import Feature#7698XingY wants to merge 11 commits into
Conversation
| Map<Integer, Map<String, Object>> result = new LinkedHashMap<>(); | ||
| for (Map.Entry<Integer, Map<String, Object>> key : keys.entrySet()) | ||
| { | ||
| String keyDisplay = key.getValue().toString(); |
There was a problem hiding this comment.
nit: Perhaps just inline so we only need to compute once when there is an error.
| return false; | ||
| } | ||
|
|
||
| protected void configureCrossFolderImport(DataIteratorBuilder rows, DataIteratorContext context) throws IOException |
There was a problem hiding this comment.
nit: (unrelated) unused imports
| const server = hookServer(process.env); | ||
| const PROJECT_NAME = 'DataClassCrudJestProject'; | ||
| const PROJECT_NAME = 'AssayDesignCrudJestProject'; | ||
| console.log(`[AssayDesignCrud] Random seed: ${testSeed} (rerun with: TEST_SEED=${testSeed})`); |
There was a problem hiding this comment.
Perhaps we console log this during init() in integrationUtils.ts like we do the server and container path? Then each test would not need to log it themselves.
There was a problem hiding this comment.
Yep, it's already logged in init(). I decided to also log it individually for ease of locating the seed as teamcity build log could get long and/or corrupted.
| private final TSVWriter _tsvWriter; | ||
|
|
||
| private MultiDataTypeCrossProjectDataIterator(DataIterator di, DataIteratorContext context, Container container, User user, boolean isCrossType, boolean isCrossFolder, ExpObject dataType, boolean isSamples) | ||
| private MultiDataTypeCrossProjectDataIterator(DataIterator di, DataIteratorContext context, Container container, User user, boolean isCrossType, ExpObject dataType, boolean isSamples) |
There was a problem hiding this comment.
Feels good to reduce complexity.
| if (foundId.isPresent()) | ||
| { | ||
| index = map.get(foundId.get()); | ||
| dataKey = RowId.fieldKey(); |
|
|
||
| if (dataRow == null && allowCrossContainer) | ||
| { | ||
| // data not found from queryTable but exist in exp.data, which happens when users lack of permission to data's container |
There was a problem hiding this comment.
I don't quite follow. Shouldn't the row in the queryTable's container always align with the container of the exp.data? Isn't it the same thing, hence why we don't have a container column on the provisioned table?
There was a problem hiding this comment.
queryTable is a filtered ContainerFilterable table with container filter for the DataClassData table, that checks for user's permission. ExperimentService.get().getTinfoData()returns a SchemaTableInfo, that doesn't care about user, nor container.
| { | ||
| // data not found from queryTable but exist in exp.data, which happens when users lack of permission to data's container | ||
| selector = new TableSelector(ExperimentService.get().getTinfoData(), filter, null); | ||
| try (var results = selector.getResults()) |
There was a problem hiding this comment.
Could this use exists()?
if (dataRow == null && allowCrossContainer)
{
// data not found from queryTable but exist in exp.data, which happens when users lack of permission to data's container
if (new TableSelector(ExperimentService.get().getTinfoData(), Collections.singleton(ExpDataTable.Column.RowId.name()), filter, null).exists())
{
String keyDisplay = name != null ? name : (rowId != null ? "{RowId=" + rowId + "}" : lsid);
throw new InvalidKeyException("Data does not exist in " + container.getName() + ": " + keyDisplay + ".");
}
}nit: Just select the RowId column.
2 participants
Rationale
This PR removes the support for cross-container sample and data class import feature. The feature allowed import of samples and data class rows belonging to other folders in a single file, which was implemented via temp partitioned files, which is error-prone. Attempts to update/merge rows that does not exist in the current container now generate a generic error message ("Sample/Data does not exist in : "), without revealing its existence in other folders.
Related Pull Requests
Changes