SO-2902: Add multi-language RF2 import support. #194

zoliMolnar · 2018-01-25T11:59:01Z

This pull request enables multi-language RF2 importing.

match what they represent, members ids instead of concept ids

nagyo · 2018-02-08T08:48:31Z

...d.api.rest.tests/src/com/b2international/snowowl/snomed/api/rest/io/SnomedImportApiTest.java

+ public void importMoreThanOneLanguageCodeDescription() {
+ final String enDescriptionId = "41320138114";
+ final String svDescriptionId = "24688171113";
+ final String conceptIdODescription = "301795004";


nagyo · 2018-02-08T10:37:01Z

...d.api.rest.tests/src/com/b2international/snowowl/snomed/api/rest/io/SnomedImportApiTest.java

-
+
+ @Test
+ public void importMoreThanOneLanguageCodeDescription() {


Please add a number to the test method's name like the others above. As you can see there is an annotation at the top: @FixMethodOrder(MethodSorters.NAME_ASCENDING) which enforces method execution order.

nagyo · 2018-02-08T10:39:01Z

...d.api.rest.tests/src/com/b2international/snowowl/snomed/api/rest/io/SnomedImportApiTest.java

+
+ @Test
+ public void importFromMoreThanOneLanguageRefset() {
+ final String enLanguageRefsetConceptId = "34d07985-48a0-41e7-b6ec-b28e6b00adfc";


You should decide to either merge the two test cases together (since the RF2 files are the same and you always import both descriptions and language refset members as well) or split the RF2 files and test language refset and description import separately.

nagyo · 2018-02-08T10:41:46Z

...d.api.rest.tests/src/com/b2international/snowowl/snomed/api/rest/io/SnomedImportApiTest.java

-
+
+ @Test
+ public void importMoreThanOneLanguageCodeDescription() {


importDescriptionsWithMultipleLanguageCodes or importDescriptionsWithDifferentLanguageCodes

nagyo · 2018-02-08T10:42:57Z

...d.api.rest.tests/src/com/b2international/snowowl/snomed/api/rest/io/SnomedImportApiTest.java

+ }
+
+ @Test
+ public void importFromMoreThanOneLanguageRefset() {


You are not importing more than one language refset! It should be rather something like:
importLanguageRefsetMembersWithMultipleLanguageCodes or importLanguageRefsetMembersWithDifferentLanguageCodes

nagyo · 2018-02-08T17:08:52Z

...omed.importer/src/com/b2international/snowowl/snomed/importer/net4j/ImportConfiguration.java

@@ -18,13 +18,16 @@
 import java.io.File;


I get the following exception when I open the import wizard in the client (server-client mode):
java.lang.IllegalArgumentException: Could not find property with name descriptionsFile in class class com.b2international.snowowl.snomed.importer.net4j.ImportConfiguration at org.eclipse.core.internal.databinding.beans.BeanPropertyHelper.getPropertyDescriptor(BeanPropertyHelper.java:168) at org.eclipse.core.databinding.beans.PojoProperties.value(PojoProperties.java:119) at org.eclipse.core.databinding.beans.PojoProperties.value(PojoProperties.java:89) at org.eclipse.core.databinding.beans.PojoObservables.observeValue(PojoObservables.java:76) at org.eclipse.core.databinding.beans.PojoObservables.observeValue(PojoObservables.java:56) at com.b2international.snowowl.snomed.importer.ui.wizard.page.ImportDetailsWizardPage.bindTextField(ImportDetailsWizardPage.java:319) at com.b2international.snowowl.snomed.importer.ui.wizard.page.ImportDetailsWizardPage.initDataBinding(ImportDetailsWizardPage.java:236) at com.b2international.snowowl.snomed.importer.ui.wizard.page.AbstractImportWizardPage.createControl(AbstractImportWizardPage.java:67)

nagyo · 2018-02-08T17:13:39Z

...omed.importer/src/com/b2international/snowowl/snomed/importer/net4j/SnomedImportRequest.java

 monitor.worked(); // 1

+ for (File descfile : descriptionsFiles) {
+ writeComponent(out, descfile, monitor);


This is not going to work like this, see how refset URLs or excluded refset ids are sent over the wire.

nagyo · 2018-02-08T17:14:04Z

...er.rf2/src/com/b2international/snowowl/snomed/importer/rf2/net4j/SnomedImportIndication.java

@@ -92,12 +93,12 @@ protected void indicating(final ExtendedDataInputStream in, final OMMonitor moni
 receivedFilesDirectory.deleteOnExit();

 readComponent(in, importConfiguration, receivedFilesDirectory, new FileCallback() { @Override public void setFile(final File f) { importConfiguration.setConceptsFile(f); }}, monitor.fork());
- readComponent(in, importConfiguration, receivedFilesDirectory, new FileCallback() { @Override public void setFile(final File f) { importConfiguration.setDescriptionsFile(f); }}, monitor.fork());
+ readComponent(in, importConfiguration, receivedFilesDirectory, new FileCallback() { @Override public void setFile(final File f) { importConfiguration.setDescriptionsFiles(ImmutableList.of(f)); }}, monitor.fork());
 readComponent(in, importConfiguration, receivedFilesDirectory, new FileCallback() { @Override public void setFile(final File f) { importConfiguration.setTextDefinitionFile(f); }}, monitor.fork());


This is not going to work like this, see how refset URLs or excluded refset ids are sent over the wire.

nagyo · 2018-02-08T17:16:26Z

....rf2/src/com/b2international/snowowl/snomed/importer/rf2/util/SnomedRefSetNameCollector.java

- CsvParser parser = new CsvParser(descriptionReader, getFileName(url), CSV_SETTINGS, descriptionParserCallback, DESCRIPTION_FIELD_COUNT);
- parser.parse();
+ if (!configuration.getDescriptionsFiles().isEmpty()) {
+ for(File descFile : configuration.getDescriptionsFiles()) {


missing space

nagyo · 2018-02-08T17:18:10Z

...d.api.rest.tests/src/com/b2international/snowowl/snomed/api/rest/io/SnomedImportApiTest.java

+ importArchive("SnomedCT_Release_INT_20150131_new_concept_for_languageCode.zip");
+ getComponent(branchPath, SnomedComponentType.CONCEPT, conceptIdODescription).statusCode(200);
+
+ importArchive("SnomedCT_Release_INT_20150201_new_description_with2_language_code.zip", importConfiguration);


The description in sct2_Description_Delta-sv_INT_20150201 has a language code 'en', please fix that!

refset files logic outside of loop

language refset files for net4j indications.

nagyo

I just realized that multiple Text Definition files could also be present in RF2 archives based on the language code... Please add that feature as well. They work just as the same as description files do.

nagyo · 2018-02-14T09:43:25Z

...d.api.rest.tests/src/com/b2international/snowowl/snomed/api/rest/io/SnomedImportApiTest.java

+ final String svLanguageRefsetMemberId = "34d07985-48a0-41e7-b6ec-b28e6b00adfb";
+ final String conceptIdOfDescription = "301795004";
+
+


Unnecessary extra line

nagyo · 2018-02-14T09:56:04Z

.../src/com/b2international/snowowl/snomed/importer/rf2/command/ListLanguageRefSetsCommand.java


 final SnomedRefSetNameCollector provider = new SnomedRefSetNameCollector(config, new NullProgressMonitor(), "");

 try {
- provider.parse(config.toURL(config.getLanguageRefSetFile()));
+ for (File langFile : config.getLanguageRefSetFiles()) {


You are already iterating through the available language refset files in the outer for loop, I don't think this is necessary here.

nagyo · 2018-02-14T10:00:27Z

.../src/com/b2international/snowowl/snomed/importer/rf2/command/ListLanguageRefSetsCommand.java

+ .getAllFileName(zipFiles, ReleaseComponentType.DESCRIPTION, contentSubType)
+ .stream()
+ .map(fileName -> new File(fileName))
+ .collect(Collectors.toSet());


nagyo · 2018-02-14T12:21:06Z

....rf2/src/com/b2international/snowowl/snomed/importer/rf2/util/SnomedRefSetNameCollector.java

+ descriptionReader = new InputStreamReader(url.openStream());
+ final CsvParser parser = new CsvParser(descriptionReader, getFileName(url), CSV_SETTINGS, descriptionParserCallback, DESCRIPTION_FIELD_COUNT);
+ parser.parse();
+


nagyo · 2018-02-14T12:21:32Z

.../src/com/b2international/snowowl/snomed/importer/rf2/util/RF2ReleaseRefSetFileCollector.java

}
+

nagyo · 2018-02-14T12:26:07Z

.../src/com/b2international/snowowl/snomed/importer/rf2/validation/SnomedValidationContext.java

+ if (isValidReleaseFile(langFile)) {
+ releaseFileValidators.add(new SnomedLanguageRefSetValidator(configuration, configuration.toURL(langFile), this));
+ }
+


nagyo · 2018-02-14T12:32:46Z

...omed.importer/src/com/b2international/snowowl/snomed/importer/net4j/ImportConfiguration.java

@@ -58,10 +61,10 @@
 private File archiveFile;
 private File rootFile;
 private File conceptsFile;
- private File descriptionsFile;
+ private Collection<File> descriptionsFiles = Collections.emptySet();


Thanks to this the collection will be initialized still as immutable.

nagyo · 2018-02-14T12:38:01Z

...omed.importer/src/com/b2international/snowowl/snomed/importer/net4j/SnomedImportRequest.java

@@ -70,15 +71,27 @@ protected void requesting(final ExtendedDataOutputStream out, final OMMonitor mo

 out.writeUTF(importConfiguration.getCodeSystemShortName());

+ final Collection<File> descriptionsFiles = importConfiguration.getDescriptionsFiles();
+ final Collection<File> languageRefSetFiles = importConfiguration.getLanguageRefSetFiles();
 monitor.worked(); // 1


Why is this here? Have you checked how the monitor's work is calculated here?

nagyo · 2018-02-14T12:38:10Z

...omed.importer/src/com/b2international/snowowl/snomed/importer/net4j/SnomedImportRequest.java

 writeComponent(out, importConfiguration.getTextDefinitionFile(), monitor.fork());
 writeComponent(out, importConfiguration.getRelationshipsFile(), monitor.fork());
 writeComponent(out, importConfiguration.getStatedRelationshipsFile(), monitor.fork());
 writeComponent(out, importConfiguration.getDescriptionType(), monitor.fork());
- writeComponent(out, importConfiguration.getLanguageRefSetFile(), monitor.fork()); // + 7


That comment is a clue!

nagyo · 2018-02-14T12:41:08Z

...er.rf2/src/com/b2international/snowowl/snomed/importer/rf2/net4j/SnomedImportIndication.java

 readComponent(in, importConfiguration, receivedFilesDirectory, new FileCallback() { @Override public void setFile(final File f) { importConfiguration.setTextDefinitionFile(f); }}, monitor.fork());
 readComponent(in, importConfiguration, receivedFilesDirectory, new FileCallback() { @Override public void setFile(final File f) { importConfiguration.setRelationshipsFile(f); }}, monitor.fork());
 readComponent(in, importConfiguration, receivedFilesDirectory, new FileCallback() { @Override public void setFile(final File f) { importConfiguration.setStatedRelationshipsFile(f); }}, monitor.fork());
 readComponent(in, importConfiguration, receivedFilesDirectory, new FileCallback() { @Override public void setFile(final File f) { importConfiguration.setDescriptionType(f); }}, monitor.fork());
- readComponent(in, importConfiguration, receivedFilesDirectory, new FileCallback() { @Override public void setFile(final File f) { importConfiguration.setLanguageRefSetFile(f); }}, monitor.fork());
+


textDefinitionFiles

text definition files.

issue/rf2-multi-language-import-fix Conflicts: snomed/com.b2international.snowowl.snomed.api.rest.tests/src/com/b2international/snowowl/snomed/api/rest/io/SnomedImportApiTest.java

https://snowowl.atlassian.net/browse/SO-2902

- remove unnecessary fields from import config - refactored SnomedRefSetNameCollector: - do not parse description files for each refset URL, parse them only once - "prepare" resolvable refset labels for the client - do not send language refset files twice over the wire

…ge-import-fix

apeteri

More copyright dates to move forward:

SnomedImportApiTest
ImportRefSetCommand
ListLanguageRefSetsCommand
ImportUtil
RF2ReleaseRefSetFileCollector
SnomedRefSetNameCollector
AbstractSnomedValidator
SnomedConceptValidator
SnomedTaxonomyValidator
SnomedValidationContext
ImportConfiguration
ReleaseFileSet
ReleaseFileSetSelectors

apeteri · 2018-03-07T14:55:42Z

.../src/com/b2international/snowowl/snomed/importer/rf2/command/ListLanguageRefSetsCommand.java

+ config.setSourceKind(ImportSourceKind.ARCHIVE);
+ config.setArchiveFile(archiveFile);
+
+ for (final Entry<String, String> label : provider.getRefsetIdToLabelMap().entrySet()) {


This label map could also be extended with information from the store (from MAIN, most likely), similar to how it's done in the import wizard.

Sorry, I see that the name collector does this already. ✔️

apeteri · 2018-03-07T15:22:50Z

....rf2/src/com/b2international/snowowl/snomed/importer/rf2/util/SnomedRefSetNameCollector.java

- put(ErroneousAustralianReleaseFileNames.ERRONEOUS_AU_20110531_SNOMED_LANGUAGE_REFSET_NAME, Concepts.REFSET_LANGUAGE_TYPE).
- put(ErroneousAustralianReleaseFileNames.ERRONEOUS_AU_20120229_SNOMED_LANGUAGE_REFSET_NAME, Concepts.REFSET_LANGUAGE_TYPE).build();
+ private static final String UNLABELED_REFSET_SUFFIX = " (unresolved)";
+ private static final String ESCAPED_UNLABELED_REFSET_SUFFIX = CharMatcher.is(')')


Pattern#quote(String) will return a String that is treated as a "literal pattern" in regular expressions; suggesting to use that instead of targeted replacements.

apeteri · 2018-03-07T15:26:50Z

....rf2/src/com/b2international/snowowl/snomed/importer/rf2/util/SnomedRefSetNameCollector.java

- refSetTypeIconId = getRefSetTypeIconIdFromHeader(url, record);
- } else { 
- unlabeledRefSetIds.add(record.get(CONCEPT_ID_COLUMN));
+ InputStreamReader descriptionReader = null;


descriptionReader should get the try-with-resources treatment:

final URL url = configuration.toURL(descFile); try (final InputStreamReader descriptionReader = new InputStreamReader(url.openStream())) { ... }

https://snowowl.atlassian.net/browse/SO-2902

cmark

Zoltan Molnar added 4 commits January 25, 2018 12:26

[rf2] Added tests for multi-language rf2 importer.

36ca49a

[rf2] Changed description, languagerefset file property to collection.

45be2bf

[rf2] Changed logic so it can work with collections.

03faa6a

[rf2] Added test import rf2 archives

fdbfab1

zoliMolnar self-assigned this Jan 25, 2018

zoliMolnar requested review from cmark and nagyo January 25, 2018 11:59

zoliMolnar changed the title ~~Issue/rf2 multi language import fix~~ Add multi-language RF2 import support. Jan 25, 2018

[rf2] Added statusCode check for the concept id, renamed variables to

6724fcf

match what they represent, members ids instead of concept ids

nagyo requested changes Feb 8, 2018

View reviewed changes

Zoltan Molnar added 9 commits February 9, 2018 15:50

[rf2] Changed data binding properties.

5d570af

[rf2] Changed loops to streams moved adding description and language...

8f6aa50

refset files logic outside of loop

[rf2] Merged the two test cases into one.

360660f

[rf2] Changed logic of reading and writing of description files and...

0d002c4

language refset files for net4j indications.

[rf2] Added final modifiers.

bbdfc2a

[rf2] Fixed test import file changed language code and refset id.

5f24b78

[rf2] Fixed typo.

d6d7eec

[rf2] Fixed conceptId of refset in the import file.

ce4198f

[rf2] Added placeholder properties for databinding.

cc853fb

nagyo requested changes Feb 14, 2018

View reviewed changes

Zoltan Molnar added 8 commits February 15, 2018 11:41

[rf2] Added validation for definition text files, removed extra lines.

e733067

[rf2] Added proper monitor progress. Added logic to write...

f750f87

textDefinitionFiles

[rf2] Added method to read collection of files and to monitor progress.

53eb20d

[rf2] Removed extra line.

4cc9f5f

[rf2] Removed extra line. Added logic to handle multiple...

2ed7940

text definition files.

[rf2] Removed unneeded iteration.

924ef02

[rf2] Added logic to handle collection of text definition files.

a71ce1d

[rf2] Removed extra line.

797d4d5

nagyo changed the title ~~Add multi-language RF2 import support.~~ SO-2902: Add multi-language RF2 import support. Feb 26, 2018

Merge remote-tracking branch 'origin/6.x' into

c1b1cd1

issue/rf2-multi-language-import-fix Conflicts: snomed/com.b2international.snowowl.snomed.api.rest.tests/src/com/b2international/snowowl/snomed/api/rest/io/SnomedImportApiTest.java

nagyo added 4 commits February 28, 2018 11:35

SO-2902: Simplify test case

0534a6b

https://snowowl.atlassian.net/browse/SO-2902

[snomed.import] Fix cloning of stated relationship file

4888bbc

Merge remote-tracking branch 'origin/6.x' into issue/rf2-multi-langua…

bfbbde1

…ge-import-fix

apeteri requested changes Mar 7, 2018

View reviewed changes

nagyo added 4 commits March 7, 2018 17:24

SO-2902: Update license header

222e30a

https://snowowl.atlassian.net/browse/SO-2902

SO-2902: Use Pattern.quote instead of Charmatcher.replace

9f25745

https://snowowl.atlassian.net/browse/SO-2902

SO-2902: Use try-with-resource for description readers

e409c61

https://snowowl.atlassian.net/browse/SO-2902

SO-2902: Fix NPE

a24bb80

https://snowowl.atlassian.net/browse/SO-2902

apeteri approved these changes Mar 7, 2018

View reviewed changes

nagyo approved these changes Mar 7, 2018

View reviewed changes

cmark approved these changes Mar 8, 2018

View reviewed changes

cmark merged commit b7e0973 into 6.x Mar 8, 2018

cmark deleted the issue/rf2-multi-language-import-fix branch March 8, 2018 11:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SO-2902: Add multi-language RF2 import support. #194

SO-2902: Add multi-language RF2 import support. #194

zoliMolnar commented Jan 25, 2018

nagyo Feb 8, 2018

nagyo Feb 8, 2018

nagyo Feb 8, 2018

nagyo Feb 8, 2018

nagyo Feb 8, 2018

nagyo Feb 8, 2018

nagyo Feb 8, 2018

nagyo Feb 8, 2018

nagyo Feb 8, 2018

nagyo Feb 8, 2018

nagyo left a comment

nagyo Feb 14, 2018

nagyo Feb 14, 2018

nagyo Feb 14, 2018

nagyo Feb 14, 2018

nagyo Feb 14, 2018

nagyo Feb 14, 2018

nagyo Feb 14, 2018

nagyo Feb 14, 2018

nagyo Feb 14, 2018

nagyo Feb 14, 2018

apeteri left a comment

apeteri Mar 7, 2018

apeteri Mar 7, 2018

apeteri Mar 7, 2018

apeteri Mar 7, 2018

cmark left a comment



		@Test
		public void importMoreThanOneLanguageCodeDescription() {

		final String svLanguageRefsetMemberId = "34d07985-48a0-41e7-b6ec-b28e6b00adfb";
		final String conceptIdOfDescription = "301795004";

SO-2902: Add multi-language RF2 import support. #194

SO-2902: Add multi-language RF2 import support. #194

Conversation

zoliMolnar commented Jan 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagyo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apeteri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmark left a comment

Choose a reason for hiding this comment