From c883e8da80caf46560a34254eb753bbb9bc03310 Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Fri, 11 Dec 2020 19:38:56 +0100 Subject: [PATCH 1/6] Fix newly added entry not synced to db Newly added entries have empty fields; don't update the field table to prevent SQL Exception Fix shared entry not found by id use left outer join for this --- CHANGELOG.md | 1 + .../jabref/logic/shared/DBMSProcessor.java | 60 ++++++++++--------- .../logic/shared/DBMSSynchronizerTest.java | 3 +- 3 files changed, 33 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cfbb06fed9a..8eee7e9cc1e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -78,6 +78,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve - We fixed an issue where the "Document Viewer" did not show the first page of the opened pdf document and did not show the correct total number of pages [#7108](https://github.com/JabRef/jabref/issues/7108) - We fixed an issue where the context menu was not updated after a file link was changed. [#5777](https://github.com/JabRef/jabref/issues/5777) - We fixed an issue where the password for a shared SQL database was not remembered [#6869](https://github.com/JabRef/jabref/issues/6869) +- We fixed an issue where newly added entires where not synced to a shared SQL database [#7176](https://github.com/JabRef/jabref/issues/7176) ### Removed diff --git a/src/main/java/org/jabref/logic/shared/DBMSProcessor.java b/src/main/java/org/jabref/logic/shared/DBMSProcessor.java index 539ac974e61..4a674405a01 100644 --- a/src/main/java/org/jabref/logic/shared/DBMSProcessor.java +++ b/src/main/java/org/jabref/logic/shared/DBMSProcessor.java @@ -165,7 +165,7 @@ protected void insertIntoEntryTable(List bibEntries) { .append(escape("TYPE")) .append(") VALUES(?)"); // Number of commas is bibEntries.size() - 1 - for (int i = 0; i < bibEntries.size() - 1; i++) { + for (int i = 0; i < (bibEntries.size() - 1); i++) { insertIntoEntryQuery.append(", (?)"); } @@ -239,6 +239,7 @@ protected void insertIntoFieldTable(List bibEntries) { // Coerce to ArrayList in order to use List.get() List> fields = bibEntries.stream().map(bibEntry -> new ArrayList<>(bibEntry.getFields())) .collect(Collectors.toList()); + StringBuilder insertFieldQuery = new StringBuilder() .append("INSERT INTO ") .append(escape("FIELD")) @@ -253,8 +254,13 @@ protected void insertIntoFieldTable(List bibEntries) { for (List entryFields : fields) { numFields += entryFields.size(); } + + if (numFields == 0) { + return; //Prevent SQL Exception + } + // Number of commas is fields.size() - 1 - for (int i = 0; i < numFields - 1; i++) { + for (int i = 0; i < (numFields - 1); i++) { insertFieldQuery.append(", (?, ?, ?)"); } try (PreparedStatement preparedFieldStatement = connection.prepareStatement(insertFieldQuery.toString())) { @@ -493,7 +499,7 @@ public List getSharedEntries(List sharedIDs) { .append("F.").append(escape("VALUE")) .append(" FROM ") .append(escape("ENTRY")) - .append(" inner join ") + .append(" left outer join ") .append(escape("FIELD")) .append(" F on ") .append(escape("ENTRY")).append(".").append(escape("SHARED_ID")) @@ -508,41 +514,37 @@ public List getSharedEntries(List sharedIDs) { query.append(" order by ") .append(escape("SHARED_ID")); - PreparedStatement preparedStatement; - try { - preparedStatement = connection.prepareStatement(query.toString()); + try (PreparedStatement preparedStatement = connection.prepareStatement(query.toString())) { for (int i = 0; i < sharedIDs.size(); i++) { preparedStatement.setInt(i + 1, sharedIDs.get(i)); } - } catch (SQLException e) { - LOGGER.debug("Executed >{}<", query.toString()); - LOGGER.error("SQL Error", e); - return Collections.emptyList(); - } - try (ResultSet selectEntryResultSet = preparedStatement.executeQuery()) { - BibEntry bibEntry = null; - int lastId = -1; - while (selectEntryResultSet.next()) { - // We get a list of field values of bib entries "grouped" by bib entries - // Thus, the first change in the shared id leads to a new BibEntry - if (selectEntryResultSet.getInt("SHARED_ID") > lastId) { - bibEntry = new BibEntry(); - bibEntry.getSharedBibEntryData().setSharedID(selectEntryResultSet.getInt("SHARED_ID")); - bibEntry.setType(EntryTypeFactory.parse(selectEntryResultSet.getString("TYPE"))); - bibEntry.getSharedBibEntryData().setVersion(selectEntryResultSet.getInt("VERSION")); - sharedEntries.add(bibEntry); - lastId = selectEntryResultSet.getInt("SHARED_ID"); - } - // In all cases, we set the field value of the newly created BibEntry object - String value = selectEntryResultSet.getString("VALUE"); - if (value != null) { - bibEntry.setField(FieldFactory.parseField(selectEntryResultSet.getString("NAME")), value, EntriesEventSource.SHARED); + try (ResultSet selectEntryResultSet = preparedStatement.executeQuery()) { + BibEntry bibEntry = null; + int lastId = -1; + while (selectEntryResultSet.next()) { + // We get a list of field values of bib entries "grouped" by bib entries + // Thus, the first change in the shared id leads to a new BibEntry + if (selectEntryResultSet.getInt("SHARED_ID") > lastId) { + bibEntry = new BibEntry(); + bibEntry.getSharedBibEntryData().setSharedID(selectEntryResultSet.getInt("SHARED_ID")); + bibEntry.setType(EntryTypeFactory.parse(selectEntryResultSet.getString("TYPE"))); + bibEntry.getSharedBibEntryData().setVersion(selectEntryResultSet.getInt("VERSION")); + sharedEntries.add(bibEntry); + lastId = selectEntryResultSet.getInt("SHARED_ID"); + } + + // In all cases, we set the field value of the newly created BibEntry object + String value = selectEntryResultSet.getString("VALUE"); + if (value != null) { + bibEntry.setField(FieldFactory.parseField(selectEntryResultSet.getString("NAME")), value, EntriesEventSource.SHARED); + } } } } catch (SQLException e) { LOGGER.error("Executed >{}<", query.toString()); LOGGER.error("SQL Error", e); + return Collections.emptyList(); } return sharedEntries; diff --git a/src/test/java/org/jabref/logic/shared/DBMSSynchronizerTest.java b/src/test/java/org/jabref/logic/shared/DBMSSynchronizerTest.java index 36534eb7da2..11fa69ad521 100644 --- a/src/test/java/org/jabref/logic/shared/DBMSSynchronizerTest.java +++ b/src/test/java/org/jabref/logic/shared/DBMSSynchronizerTest.java @@ -1,6 +1,5 @@ package org.jabref.logic.shared; -import java.sql.SQLException; import java.util.Arrays; import java.util.Collections; import java.util.List; @@ -69,7 +68,7 @@ public void setup() throws Exception { } @AfterEach - public void clear() throws SQLException { + public void clear() { dbmsSynchronizer.closeSharedDatabase(); } From 7317104c3f7a94890b33bdc720fff8ef722b66e3 Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Fri, 11 Dec 2020 19:45:50 +0100 Subject: [PATCH 2/6] fix checkstyle --- src/main/java/org/jabref/logic/shared/DBMSProcessor.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/jabref/logic/shared/DBMSProcessor.java b/src/main/java/org/jabref/logic/shared/DBMSProcessor.java index 4a674405a01..9b73f384557 100644 --- a/src/main/java/org/jabref/logic/shared/DBMSProcessor.java +++ b/src/main/java/org/jabref/logic/shared/DBMSProcessor.java @@ -256,7 +256,7 @@ protected void insertIntoFieldTable(List bibEntries) { } if (numFields == 0) { - return; //Prevent SQL Exception + return; // Prevent SQL Exception } // Number of commas is fields.size() - 1 From ae37d4bf391295f07319b5dd84ca8071717b9062 Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Fri, 11 Dec 2020 19:46:41 +0100 Subject: [PATCH 3/6] fix wording --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8eee7e9cc1e..1ba8762b633 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -78,7 +78,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve - We fixed an issue where the "Document Viewer" did not show the first page of the opened pdf document and did not show the correct total number of pages [#7108](https://github.com/JabRef/jabref/issues/7108) - We fixed an issue where the context menu was not updated after a file link was changed. [#5777](https://github.com/JabRef/jabref/issues/5777) - We fixed an issue where the password for a shared SQL database was not remembered [#6869](https://github.com/JabRef/jabref/issues/6869) -- We fixed an issue where newly added entires where not synced to a shared SQL database [#7176](https://github.com/JabRef/jabref/issues/7176) +- We fixed an issue where newly added entires were not synced to a shared SQL database [#7176](https://github.com/JabRef/jabref/issues/7176) ### Removed From 503670a7193e0835ed9318fff53cbc252aa7661f Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Sun, 13 Dec 2020 14:37:12 +0100 Subject: [PATCH 4/6] add tests for fix --- .../logic/shared/DBMSProcessorTest.java | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/src/test/java/org/jabref/logic/shared/DBMSProcessorTest.java b/src/test/java/org/jabref/logic/shared/DBMSProcessorTest.java index 97732a6ca5d..dc12cb4de89 100644 --- a/src/test/java/org/jabref/logic/shared/DBMSProcessorTest.java +++ b/src/test/java/org/jabref/logic/shared/DBMSProcessorTest.java @@ -95,6 +95,37 @@ void testInsertEntry() throws SQLException { assertEquals(expectedFieldMap, actualFieldMap); } + @Test + void testInsertEntryWithEmptyFields() throws SQLException { + BibEntry expectedEntry = new BibEntry(StandardEntryType.Article); + + dbmsProcessor.insertEntry(expectedEntry); + + BibEntry emptyEntry = getBibEntryExample(); + emptyEntry.getSharedBibEntryData().setSharedID(1); + dbmsProcessor.insertEntry(emptyEntry); // does not insert, due to same sharedID. + + Map actualFieldMap = new HashMap<>(); + + try (ResultSet entryResultSet = selectFrom("ENTRY", dbmsConnection, dbmsProcessor)) { + assertTrue(entryResultSet.next()); + assertEquals(1, entryResultSet.getInt("SHARED_ID")); + assertEquals("article", entryResultSet.getString("TYPE")); + assertEquals(1, entryResultSet.getInt("VERSION")); + assertFalse(entryResultSet.next()); + + try (ResultSet fieldResultSet = selectFrom("FIELD", dbmsConnection, dbmsProcessor)) { + while (fieldResultSet.next()) { + actualFieldMap.put(fieldResultSet.getString("NAME"), fieldResultSet.getString("VALUE")); + } + } + } + + Map expectedFieldMap = expectedEntry.getFieldMap().entrySet().stream().collect(Collectors.toMap((entry) -> entry.getKey().getName(), Map.Entry::getValue)); + + assertEquals(expectedFieldMap, actualFieldMap); + } + private static BibEntry getBibEntryExample() { return new BibEntry(StandardEntryType.InProceedings) .withField(StandardField.AUTHOR, "Wirthlin, Michael J and Hutchings, Brad L and Gilson, Kent L") @@ -119,6 +150,19 @@ void testUpdateEntry() throws Exception { assertEquals(Optional.of(expectedEntry), actualEntry); } + @Test + void testUpdateEmptyEntry() throws Exception { + BibEntry expectedEntry = new BibEntry(StandardEntryType.Article); + dbmsProcessor.insertEntry(expectedEntry); + + expectedEntry.setField(StandardField.AUTHOR, "Michael J and Hutchings"); + expectedEntry.setField(new UnknownField("customField"), "custom value"); + dbmsProcessor.updateEntry(expectedEntry); + + Optional actualEntry = dbmsProcessor.getSharedEntry(expectedEntry.getSharedBibEntryData().getSharedID()); + assertEquals(Optional.of(expectedEntry), actualEntry); + } + @Test void testGetEntriesByIdList() throws Exception { BibEntry firstEntry = getBibEntryExample(); From 33f11a64c8aaf5248a334c9a61d4df49a92cf4e0 Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Mon, 14 Dec 2020 19:23:21 +0100 Subject: [PATCH 5/6] adjust test --- .../jabref/logic/shared/DBMSProcessorTest.java | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/src/test/java/org/jabref/logic/shared/DBMSProcessorTest.java b/src/test/java/org/jabref/logic/shared/DBMSProcessorTest.java index dc12cb4de89..abc597cb186 100644 --- a/src/test/java/org/jabref/logic/shared/DBMSProcessorTest.java +++ b/src/test/java/org/jabref/logic/shared/DBMSProcessorTest.java @@ -101,12 +101,6 @@ void testInsertEntryWithEmptyFields() throws SQLException { dbmsProcessor.insertEntry(expectedEntry); - BibEntry emptyEntry = getBibEntryExample(); - emptyEntry.getSharedBibEntryData().setSharedID(1); - dbmsProcessor.insertEntry(emptyEntry); // does not insert, due to same sharedID. - - Map actualFieldMap = new HashMap<>(); - try (ResultSet entryResultSet = selectFrom("ENTRY", dbmsConnection, dbmsProcessor)) { assertTrue(entryResultSet.next()); assertEquals(1, entryResultSet.getInt("SHARED_ID")); @@ -114,16 +108,11 @@ void testInsertEntryWithEmptyFields() throws SQLException { assertEquals(1, entryResultSet.getInt("VERSION")); assertFalse(entryResultSet.next()); + // Adding an empty entry should not create an entry in field table, only in entry table try (ResultSet fieldResultSet = selectFrom("FIELD", dbmsConnection, dbmsProcessor)) { - while (fieldResultSet.next()) { - actualFieldMap.put(fieldResultSet.getString("NAME"), fieldResultSet.getString("VALUE")); - } + assertFalse(fieldResultSet.next()); } } - - Map expectedFieldMap = expectedEntry.getFieldMap().entrySet().stream().collect(Collectors.toMap((entry) -> entry.getKey().getName(), Map.Entry::getValue)); - - assertEquals(expectedFieldMap, actualFieldMap); } private static BibEntry getBibEntryExample() { @@ -157,6 +146,7 @@ void testUpdateEmptyEntry() throws Exception { expectedEntry.setField(StandardField.AUTHOR, "Michael J and Hutchings"); expectedEntry.setField(new UnknownField("customField"), "custom value"); + // Update field should now find the entry dbmsProcessor.updateEntry(expectedEntry); Optional actualEntry = dbmsProcessor.getSharedEntry(expectedEntry.getSharedBibEntryData().getSharedID()); From 3c3358406b14fefcacb1d67bcdd48ae264ceb541 Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Mon, 14 Dec 2020 20:16:29 +0100 Subject: [PATCH 6/6] Update src/main/java/org/jabref/logic/shared/DBMSProcessor.java --- src/main/java/org/jabref/logic/shared/DBMSProcessor.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/org/jabref/logic/shared/DBMSProcessor.java b/src/main/java/org/jabref/logic/shared/DBMSProcessor.java index 9b73f384557..ec364110a91 100644 --- a/src/main/java/org/jabref/logic/shared/DBMSProcessor.java +++ b/src/main/java/org/jabref/logic/shared/DBMSProcessor.java @@ -499,6 +499,7 @@ public List getSharedEntries(List sharedIDs) { .append("F.").append(escape("VALUE")) .append(" FROM ") .append(escape("ENTRY")) + // Handle special case if entry does not have any fields (yet) .append(" left outer join ") .append(escape("FIELD")) .append(" F on ")