Skip to content

Commit

Permalink
[kbss-cvut/termit-ui#449] Prevent import of term with existing label …
Browse files Browse the repository at this point in the history 
…and different identifier.

Existing identifier would be deleted and re-inserted again, but a different identifier would lead to two terms with the same label (and different identifiers).
  • Loading branch information
@ledsoft
ledsoft committed Aug 29, 2024
1 parent e65e256 commit dd376ba
Show file tree
Hide file tree
Showing 5 changed files with 127 additions and 23 deletions.
74 changes: 57 additions & 17 deletions src/main/java/cz/cvut/kbss/termit/persistence/dao/TermDao.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
*/
package cz.cvut.kbss.termit.persistence.dao;

import cz.cvut.kbss.jopa.exceptions.NoResultException;
import cz.cvut.kbss.jopa.model.EntityManager;
import cz.cvut.kbss.jopa.model.query.TypedQuery;
import cz.cvut.kbss.jopa.vocabulary.SKOS;
Expand Down Expand Up @@ -235,15 +236,15 @@ public void setState(Term term, URI state) {
eventPublisher.publishEvent(new AssetUpdateEvent(this, term));
evictPossiblyCachedReferences(term);
em.createNativeQuery("DELETE {" +
"?t ?hasState ?oldState ." +
"} INSERT {" +
"GRAPH ?g {" +
"?t ?hasState ?newState ." +
"}} WHERE {" +
"OPTIONAL {?t ?hasState ?oldState .}" +
"GRAPH ?g {" +
"?t ?inScheme ?glossary ." +
"}}").setParameter("t", term)
"?t ?hasState ?oldState ." +
"} INSERT {" +
"GRAPH ?g {" +
"?t ?hasState ?newState ." +
"}} WHERE {" +
"OPTIONAL {?t ?hasState ?oldState .}" +
"GRAPH ?g {" +
"?t ?inScheme ?glossary ." +
"}}").setParameter("t", term)
.setParameter("hasState", URI.create(cz.cvut.kbss.termit.util.Vocabulary.s_p_ma_stav_pojmu))
.setParameter("inScheme", URI.create(SKOS.IN_SCHEME))
.setParameter("newState", state).executeUpdate();
Expand Down Expand Up @@ -403,14 +404,15 @@ public List<TermDto> findAllRoots(Vocabulary vocabulary, Pageable pageSpec, Coll
Objects.requireNonNull(vocabulary);
Objects.requireNonNull(pageSpec);
TypedQuery<TermDto> query = em.createNativeQuery("SELECT DISTINCT ?term WHERE {" +
"SELECT DISTINCT ?term ?hasLocaleLabel WHERE {" +
"SELECT DISTINCT ?term ?hasLocaleLabel WHERE {" +
"GRAPH ?context { " +
"?term a ?type ;" +
"?hasLabel ?label ." +
"?vocabulary ?hasGlossary/?hasTerm ?term ." +
"BIND((lang(?label) = ?labelLang) as ?hasLocaleLabel) ." +
"FILTER (?term NOT IN (?included))" +
"}} ORDER BY DESC(?hasLocaleLabel) lang(?label) " + orderSentence("?label") + "}",
"}} ORDER BY DESC(?hasLocaleLabel) lang(?label) " + orderSentence(
"?label") + "}",
TermDto.class);
query = setCommonFindAllRootsQueryParams(query, false);
try {
Expand Down Expand Up @@ -467,14 +469,15 @@ private static String r(String string, String from, String to) {
public List<TermDto> findAllRoots(Pageable pageSpec, Collection<URI> includeTerms) {
Objects.requireNonNull(pageSpec);
TypedQuery<TermDto> query = em.createNativeQuery("SELECT DISTINCT ?term WHERE {" +
"SELECT DISTINCT ?term ?hasLocaleLabel WHERE {" +
"SELECT DISTINCT ?term ?hasLocaleLabel WHERE {" +
"?term a ?type ; " +
"?hasLabel ?label . " +
"?vocabulary ?hasGlossary/?hasTerm ?term . " +
"BIND((lang(?label) = ?labelLang) as ?hasLocaleLabel) ." +
"FILTER (?term NOT IN (?included)) . " +
"FILTER NOT EXISTS {?term a ?snapshot .} " +
"} ORDER BY DESC(?hasLocaleLabel) lang(?label) " + orderSentence("?label") + "}",
"} ORDER BY DESC(?hasLocaleLabel) lang(?label) " + orderSentence(
"?label") + "}",
TermDto.class);
query = setCommonFindAllRootsQueryParams(query, false);
try {
Expand Down Expand Up @@ -552,14 +555,15 @@ public List<TermDto> findAllRootsIncludingImports(Vocabulary vocabulary, Pageabl
Objects.requireNonNull(vocabulary);
Objects.requireNonNull(pageSpec);
TypedQuery<TermDto> query = em.createNativeQuery("SELECT DISTINCT ?term WHERE {" +
"SELECT DISTINCT ?term ?hasLocaleLabel WHERE {" +
"SELECT DISTINCT ?term ?hasLocaleLabel WHERE {" +
"?term a ?type ;" +
"?hasLabel ?label ." +
"?vocabulary ?imports* ?parent ." +
"?parent ?hasGlossary/?hasTerm ?term ." +
"BIND((lang(?label) = ?labelLang) as ?hasLocaleLabel) ." +
"FILTER (?term NOT IN (?included))" +
"} ORDER BY DESC(?hasLocaleLabel) lang(?label) " + orderSentence("?label") + "}",
"} ORDER BY DESC(?hasLocaleLabel) lang(?label) " + orderSentence(
"?label") + "}",
TermDto.class);
query = setCommonFindAllRootsQueryParams(query, true);
try {
Expand Down Expand Up @@ -697,8 +701,9 @@ public List<TermDto> findAllIncludingImported(String searchString, Vocabulary vo
* Note that this method uses comparison ignoring case, so that two labels differing just in character case are
* considered same here.
*
* @param label Label to check
* @param vocabulary Vocabulary in which terms will be searched
* @param label Label to check
* @param vocabulary Vocabulary in which terms will be searched
* @param languageTag Language tag of the label, optional. If {@code null}, any language is accepted
* @return Whether term with {@code label} already exists in vocabulary
*/
public boolean existsInVocabulary(String label, Vocabulary vocabulary, String languageTag) {
Expand All @@ -722,6 +727,41 @@ public boolean existsInVocabulary(String label, Vocabulary vocabulary, String la
}
}

/**
* Gets the identifier of a term with the specified label in a vocabulary with the specified URI.
* <p>
* Note that this method uses comparison ignoring case, so that two labels differing just in character case are
* considered same here.
*
* @param label Label to search by
* @param vocabulary Vocabulary in which terms will be searched
* @param languageTag Language tag of the label
* @return Identifier of matching term wrapped in an {@code Optional}, empty {@code Optional} if there is no such
* term
*/
public Optional<URI> findIdentifierByLabel(String label, Vocabulary vocabulary, String languageTag) {
Objects.requireNonNull(label);
Objects.requireNonNull(vocabulary);
try {
return Optional.of(em.createNativeQuery("SELECT ?term { ?term a ?type ; " +
"?hasLabel ?label ;" +
"?inVocabulary ?vocabulary ." +
"FILTER (LCASE(?label) = LCASE(?searchString)) . "
+ "}", URI.class)
.setParameter("type", typeUri)
.setParameter("hasLabel", LABEL_PROP)
.setParameter("inVocabulary",
URI.create(cz.cvut.kbss.termit.util.Vocabulary.s_p_je_pojmem_ze_slovniku))
.setParameter("vocabulary", vocabulary.getUri())
.setParameter("searchString", label,
languageTag != null ? languageTag : config.getLanguage()).getSingleResult());
} catch (NoResultException e) {
return Optional.empty();
} catch (RuntimeException e) {
throw new PersistenceException(e);
}
}

/**
* Gets identifiers of all terms in the specified vocabulary that have no occurrences (file or definitional).
*
Expand Down
10 changes: 10 additions & 0 deletions src/main/java/cz/cvut/kbss/termit/service/importer/excel/ExcelImporter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import java.util.HashSet;
import java.util.List;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;

/**
Expand Down Expand Up @@ -119,6 +120,15 @@ public Vocabulary importVocabulary(ImportConfiguration config, ImportInput data)
rawDataToInsert.addAll(sheetImporter.getRawDataToInsert());
}
terms.stream().peek(t -> t.setUri(resolveTermIdentifier(targetVocabulary, t)))
.peek(t -> t.getLabel().getValue().forEach((lang, value) -> {
final Optional<URI> existingUri = termService.findIdentifierByLabel(value,
targetVocabulary,
lang);
if (existingUri.isPresent() && !existingUri.get().equals(t.getUri())) {
throw new VocabularyImportException(
"Vocabulary already contains a term with label '" + value + "' with a different identifier than the imported one.");
}
}))
.filter(t -> termService.exists(t.getUri())).forEach(t -> {
LOG.trace("Term {} already exists. Removing old version.", t);
termService.forceRemove(termService.findRequired(t.getUri()));
Expand Down
19 changes: 18 additions & 1 deletion src/main/java/cz/cvut/kbss/termit/service/repository/TermRepositoryService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -333,14 +333,31 @@ public List<TermDto> findAllIncludingImported(String searchString, Vocabulary vo
*
* @param label Label to check
* @param vocabulary Vocabulary in which terms will be searched
* @param language Language to check the existence in
* @param language Language to check the existence in, optional. If not specified, any language is accepted
* @return Whether term with {@code label} already exists in vocabulary
*/
@Transactional(readOnly = true)
public boolean existsInVocabulary(String label, Vocabulary vocabulary, String language) {
return termDao.existsInVocabulary(label, vocabulary, language);
}

/**
* Gets the identifier of a term with the specified label in a vocabulary with the specified URI.
* <p>
* Note that this method uses comparison ignoring case, so that two labels differing just in character case are
* considered same here.
*
* @param label Label to search by
* @param vocabulary Vocabulary in which terms will be searched
* @param language Language tag of the label, optional. If not specified, any language is accepted
* @return Identifier of matching term wrapped in an {@code Optional}, empty {@code Optional} if there is no such
* term
*/
@Transactional(readOnly = true)
public Optional<URI> findIdentifierByLabel(String label, Vocabulary vocabulary, String language) {
return termDao.findIdentifierByLabel(label, vocabulary, language);
}

/**
* Retrieves aggregated information about the specified Term's occurrences in Resources and other Terms
* definitions.
Expand Down
29 changes: 24 additions & 5 deletions src/test/java/cz/cvut/kbss/termit/persistence/dao/TermDaoTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -430,8 +430,9 @@ void updatePublishesVocabularyContentModifiedEvent() {
transactional(() -> sut.update(term));
final ArgumentCaptor<ApplicationEvent> captor = ArgumentCaptor.forClass(ApplicationEvent.class);
verify(eventPublisher, atLeastOnce()).publishEvent(captor.capture());
final Optional<VocabularyContentModified> evt = captor.getAllValues().stream().filter(VocabularyContentModified.class::isInstance)
.map(VocabularyContentModified.class::cast).findFirst();
final Optional<VocabularyContentModified> evt = captor.getAllValues().stream()
.filter(VocabularyContentModified.class::isInstance)
.map(VocabularyContentModified.class::cast).findFirst();
assertTrue(evt.isPresent());
assertEquals(vocabulary.getUri(), evt.get().getVocabularyIri());
}
Expand Down Expand Up @@ -751,9 +752,9 @@ void findAllRootsReturnsTermsThatAreMissingDefaultLanguageLabel() {
final List<TermDto> result = sut.findAllRoots(vocabulary, Constants.DEFAULT_PAGE_SPEC, Collections.emptyList());
assertEquals(4, result.size());
assertEquals(Arrays
.asList("China", "Germany", "Spain", "Syria"),
result.stream().map(r -> r.getLabel().get("en"))
.toList());
.asList("China", "Germany", "Spain", "Syria"),
result.stream().map(r -> r.getLabel().get("en"))
.toList());
}

/**
Expand Down Expand Up @@ -1353,4 +1354,22 @@ void setStatePublishesAssetUpdateEvent() {
assertTrue(evt.isPresent());
assertEquals(term, evt.get().getAsset());
}

@Test
void findIdentifierByLabelReturnsIdentifierOfMatchingTerm() {
final List<Term> terms = generateTerms(2);
addTermsAndSave(new HashSet<>(terms), vocabulary);

final Term term = terms.get(0);
final String label = term.getLabel().get(Environment.LANGUAGE);
final Optional<URI> result = sut.findIdentifierByLabel(label, vocabulary, Environment.LANGUAGE);
assertTrue(result.isPresent());
assertEquals(term.getUri(), result.get());
}

@Test
void findIdentifierByLabelReturnsEmptyOptionalIfNoTermIsFound() {
final Optional<URI> result = sut.findIdentifierByLabel("foo", vocabulary, Environment.LANGUAGE);
assertFalse(result.isPresent());
}
}
18 changes: 18 additions & 0 deletions src/test/java/cz/cvut/kbss/termit/service/importer/excel/ExcelImporterTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
Expand Down Expand Up @@ -629,4 +630,21 @@ void importSupportsSpecifyingStateAndTypeOnlyInOneSheet() throws Exception {
assertEquals(state.getUri(), captor.getValue().getState());
verify(languageService, never()).getInitialTermState();
}

@Test
void importThrowsVocabularyImportExceptionWhenVocabularyAlreadyContainsTermWithSameLabelAndDifferentIdentifier() {
when(vocabularyDao.exists(vocabulary.getUri())).thenReturn(true);
when(vocabularyDao.find(vocabulary.getUri())).thenReturn(Optional.of(vocabulary));
when(termService.findIdentifierByLabel(any(), any(), any())).thenReturn(Optional.empty());
doReturn(Optional.of(URI.create(
vocabulary.getUri() + config.getNamespace().getTerm().getSeparator() + "/Construction"))).when(
termService).findIdentifierByLabel("Construction", vocabulary, Constants.DEFAULT_LANGUAGE);


assertThrows(VocabularyImportException.class, () -> sut.importVocabulary(
new VocabularyImporter.ImportConfiguration(false, vocabulary.getUri(), prePersist),
new VocabularyImporter.ImportInput(Constants.MediaType.EXCEL,
Environment.loadFile(
"data/import-simple-en.xlsx"))));
}
}

0 comments on commit dd376ba

Please sign in to comment.