Fb sample type export fixes#1946
Conversation
# Conflicts: # experiment/src/org/labkey/experiment/samples/SampleTypeAndDataClassFolderImporter.java # experiment/src/org/labkey/experiment/xar/FolderXarImporterFactory.java
|
WARNING: This PR appears to have the default title generated by GitHub. Please use something more descriptive. |
| if (expData == null && expDataClass != null) | ||
| { | ||
| // Try resolving it by name within the data class in case we have it under a different LSID | ||
| ExpData data = expDataClass.getData(getContainer(), xbData.getName()); |
There was a problem hiding this comment.
Should be able to avoid this instanceof by using ExperimentServiceImpl to fetch a ExpDataClassImpl. Its getData() returns a ExpDataImpl
| ExpData data = (ExpData)o; | ||
| if (data.getDataFileUrl() == null) | ||
| // Most DataClass data don't have a dataFileUrl, but some do -- like NucSequence imported from a genbank file | ||
| // UNDONE: Don't have user and fetching DataClass is expensive-ish. Maybe add data.hasDataClass() or something |
There was a problem hiding this comment.
Are these UNDONEs intended to stay here?
labkey-kevink
left a comment
labkey-kevink
left a comment
There was a problem hiding this comment.
This looks good but I think we need another pass to clear up some of the UNDONE comments.
| { | ||
| // UNDONE: Now that "Data" prefix is used for DataClass, the AutoFileLSID is not a good default. | ||
| // UNDONE: Can we be more restrictive about which LSIDs this is applied to? Maybe only if the objectId part of the LSID includes a "/" (%2F) or something? | ||
| // UNDONE: Maybe there is a better way to detect when we should use ${AutoFileLSID}? |
There was a problem hiding this comment.
@labkey-jeckels Any thoughts about these comments? We don't want to use ${AutoFileLSID} for DataClass data and I don't think we have enough context from the LSID alone to know if it is an exp.data for a file or a DataClass.
There was a problem hiding this comment.
I don't know what the DataClass LSIDs look like, but maybe we should build out RelativizedLSIDs.relativize(ExpObject) so that we can use more of the ExpData to help sniff the right approach (checking for a DataFileURL, for example).
| public XarExporter(LSIDRelativizer.RelativizedLSIDs relativizedLSIDs, URLRewriter urlRewriter, User user) | ||
| { | ||
| _relativizedLSIDs = new LSIDRelativizer.RelativizedLSIDs(lsidRelativizer); | ||
| // UNDONE: Is it ok to share the relativizedLSIDs across XarExporters and tsv writers? |
|
|
||
| private String relativizeLSIDPropertyValue(String value, SimpleTypeNames.Enum type) | ||
| { | ||
| {// NOTE: this is interesting - we fixup LSID values in the xar xml |
There was a problem hiding this comment.
comment was a note to self. remove comment
|
|
||
| if (exportXar) | ||
| // UNDONE: The other exporters use FOLDER_RELATIVE, but it wants to use ${AutoFileLSID} replacements for DataClass LSIDs when exporting the TSV data.. see comment in ExportLsidDataColumn | ||
| LSIDRelativizer.RelativizedLSIDs relativizedLSIDs = new LSIDRelativizer.RelativizedLSIDs(LSIDRelativizer.FOLDER_RELATIVE); |
There was a problem hiding this comment.
I think the relativizedLSIDs instance should be shared across all of the exporters by stashing it in the ImportContext<FolderDocument.Folder>
#### Rationale LabKey/kanban#1946 Encoding updates for data class search template. Encoding updates for issue resolution in summary.
3 participants
Rationale
In order to address some recent issues with sample type and data class export/import we have been reconsidering the roles that the XAR and query update service play. In the current design, rows that represent derivation or lineage are captured in the XAR format and all other rows are captured as TSV data. In this PR we pivot so that all rows are exported/imported as TSV, this will happen first and allows any special handling that the QUS may provide on all rows. The XAR handlers will then just be responsible for wiring up and creating derivation runs but will not be tasked with creating materials or datas.
Related issues:
Changes
XarReadera bit smarter about resolving existing sample types or data classes