Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Batch remove entries #5659

Merged
merged 44 commits into from
Nov 26, 2019
Merged

Batch remove entries #5659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
80e8377
Initial changes to remove entry functionality
abepolk Oct 8, 2019
48cb7fd
Fix logic error in KeyChangeListener
abepolk Oct 8, 2019
923b40e
Fix logic error in BasePanel
abepolk Oct 8, 2019
a0ffcb5
Changes to tests
abepolk Oct 9, 2019
b08ae88
Fix typo
abepolk Oct 9, 2019
3a830a8
Move remove entry calls to batch version
abepolk Oct 11, 2019
4e57a46
Un-propagate for loop in KeyChangeListener
abepolk Oct 15, 2019
b990e0d
Finalize changes to SharedEntriesNotPresentEvent
abepolk Oct 15, 2019
3122e67
Fix bug
abepolk Oct 15, 2019
aac840c
Fix other compile errors
abepolk Oct 15, 2019
90c27f1
Fix bug and update tests
abepolk Oct 20, 2019
a0cafc4
Fix tests
abepolk Oct 20, 2019
868deed
Merge master
abepolk Oct 20, 2019
2afb868
Fix test omission
abepolk Oct 20, 2019
460f44a
Update l10n
abepolk Oct 20, 2019
f11389e
For loop to citationStyle
abepolk Oct 20, 2019
f535a0b
Add comment for method not working
abepolk Oct 23, 2019
77ff450
Clarify var name
abepolk Oct 23, 2019
b0805cc
Allow single entry for undo
abepolk Oct 23, 2019
7ac7a1c
Replace compound edit undo with normal undo in BasePanel
abepolk Oct 24, 2019
8255ad2
Typo
abepolk Oct 25, 2019
3b8dfea
Simplify loop in DBMSSynchronizer
abepolk Oct 27, 2019
5f1b811
Use if instead of stream
abepolk Oct 28, 2019
c41dc17
Pluralize Javadoc
abepolk Oct 30, 2019
bb88484
EntryEvent -> EntriesEvent in Javadoc, comments, and var names
abepolk Oct 30, 2019
4c97c64
Make imports explicit in BasePanel
abepolk Oct 30, 2019
30d8d51
Merge master
abepolk Oct 30, 2019
125d33a
Batch delete to SQL
abepolk Nov 1, 2019
5cf5965
Final EntriesRemovedEvent fixes
abepolk Nov 1, 2019
1df2869
Fix checkStyleMain
abepolk Nov 3, 2019
0f5d76f
Merge branch 'master' into refactor_group_entryevents
abepolk Nov 3, 2019
13abf0c
More checkStyle fixes
abepolk Nov 3, 2019
17f04b0
More fixes
abepolk Nov 10, 2019
214e958
Move List coercion into DuplicateSearchResult method
abepolk Nov 10, 2019
a90c44e
Fix typo
abepolk Nov 11, 2019
648c5da
Fix pesky BibDatabaseTest error with setStrings
abepolk Nov 11, 2019
b0f585a
Fixed BibDatabase Javadoc
abepolk Nov 11, 2019
c6300e5
Merge master
abepolk Nov 18, 2019
a187b42
Finish master merge, start changing DBMS tests
abepolk Nov 20, 2019
d6d15a7
Merge refactor_group_entryevents
abepolk Nov 22, 2019
f8c94cc
Add comment
abepolk Nov 22, 2019
aaf73e7
Final fixes including checkStyle
abepolk Nov 24, 2019
120b6d5
Add DBMSProcessor tests
abepolk Nov 26, 2019
b023741
Fix checkStyleTest
abepolk Nov 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions src/main/java/org/jabref/logic/shared/DBMSProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.UUID;
Expand Down Expand Up @@ -399,18 +400,26 @@ private void insertOrUpdateFields(BibEntry localBibEntry) throws SQLException {
/**
* Removes the shared bibEntry.
*
* @param bibEntry {@link BibEntry} to be deleted
* @param bibEntries {@link BibEntry} to be deleted
*/
public void removeEntry(BibEntry bibEntry) {
public void removeEntries(List<BibEntry> bibEntries) {
Objects.requireNonNull(bibEntries);
if (bibEntries.isEmpty()) {
return;
}
StringBuilder query = new StringBuilder()
.append("DELETE FROM ")
.append(escape("ENTRY"))
.append(" WHERE ")
.append(escape("SHARED_ID"))
.append(" = ?");
.append(" IN (");
query.append("?, ".repeat(bibEntries.size() - 1));
query.append("?)");

try (PreparedStatement preparedStatement = connection.prepareStatement(query.toString())) {
preparedStatement.setInt(1, bibEntry.getSharedBibEntryData().getSharedID());
for (int j = 0; j < bibEntries.size(); j++) {
preparedStatement.setInt(j + 1, bibEntries.get(j).getSharedBibEntryData().getSharedID());
}
preparedStatement.executeUpdate();
} catch (SQLException e) {
LOGGER.error("SQL Error: ", e);
Expand Down
4 changes: 1 addition & 3 deletions src/main/java/org/jabref/logic/shared/DBMSSynchronizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,9 +115,7 @@ public void listen(EntriesRemovedEvent event) {
// In this case DBSynchronizer should not try to delete the bibEntry entry again (but it would not harm).
if (isEventSourceAccepted(event) && checkCurrentConnection()) {
List<BibEntry> entries = event.getBibEntries();
for (BibEntry entry : entries) {
dbmsProcessor.removeEntry(entry);
}
dbmsProcessor.removeEntries(entries);
synchronizeLocalMetaData();
synchronizeLocalDatabase(); // Pull changes for the case that there where some
}
Expand Down
73 changes: 69 additions & 4 deletions src/test/java/org/jabref/logic/shared/DBMSProcessorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import java.sql.ResultSet;
import java.sql.SQLException;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -164,18 +165,82 @@ void testUpdateEqualEntry(DBMSType dbmsType, DBMSConnection dbmsConnection, DBMS

@ParameterizedTest
@MethodSource("org.jabref.logic.shared.TestManager#getTestingDatabaseSystems")
void testRemoveEntry(DBMSType dbmsType, DBMSConnection dbmsConnection, DBMSProcessor dbmsProcessor) throws SQLException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be keep the single test? -> Meaning the additional test for two entries is OK, but we should also keep the simple (and probably always succeeding test). - Reason: The for loop above is not entered for one entry and thus this test case is special.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could, but it would still have to be dbmsProcessor.removeEntries(Collections.singletonList(entryToRemove)) because I got rid of DBMSProcessor.removeEntry in the singular. Is it still worth it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. The tests should be independent of the implemention somehow. Tests should cover the general case, but also edge cases, null list, 0 element list, list with 1 elements, list with integer.max elements (which is impossible IMHO), but the other cases should be easy to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, in that case I will use Collections.singletonList. Should I add a test for each of those edge cases as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, please go ahead. In case tests are easy to write, they should be written. - The thing is: The implementation could change in 1 year (who knows) and the test will still cover that case.

Please also add a test for

  • dbmsProcessor.removeEntries(Collections.emptyList())
  • dbmsProcessor.removeEntries(null) (should throw a NPE)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throw on empty list? If so, throw what? I can't imagine why someone would want to call removeEntries on purpose on an empty list.

void testRemoveAllEntries(DBMSType dbmsType, DBMSConnection dbmsConnection, DBMSProcessor dbmsProcessor) throws SQLException {
dbmsProcessor.setupSharedDatabase();
BibEntry bibEntry = getBibEntryExample();
dbmsProcessor.insertEntry(bibEntry);
dbmsProcessor.removeEntry(bibEntry);
BibEntry firstEntry = getBibEntryExample();
BibEntry secondEntry = getBibEntryExample();
List<BibEntry> entriesToRemove = Arrays.asList(firstEntry, secondEntry);
dbmsProcessor.insertEntry(firstEntry);
dbmsProcessor.insertEntry(secondEntry);
dbmsProcessor.removeEntries(entriesToRemove);

try (ResultSet resultSet = selectFrom("ENTRY", dbmsConnection, dbmsProcessor)) {
assertFalse(resultSet.next());
}
clear(dbmsConnection);
}

@ParameterizedTest
@MethodSource("org.jabref.logic.shared.TestManager#getTestingDatabaseSystems")
void testRemoveSomeEntries(DBMSType dbmsType, DBMSConnection dbmsConnection, DBMSProcessor dbmsProcessor) throws SQLException {
dbmsProcessor.setupSharedDatabase();
BibEntry firstEntry = getBibEntryExample();
BibEntry secondEntry = getBibEntryExample();
BibEntry thirdEntry = getBibEntryExample();

// Remove the first and third entries - the second should remain (SHARED_ID will be 2)

List<BibEntry> entriesToRemove = Arrays.asList(firstEntry, thirdEntry);
dbmsProcessor.insertEntry(firstEntry);
dbmsProcessor.insertEntry(secondEntry);
dbmsProcessor.insertEntry(thirdEntry);
dbmsProcessor.removeEntries(entriesToRemove);

try (ResultSet entryResultSet = selectFrom("ENTRY", dbmsConnection, dbmsProcessor)) {
assertTrue(entryResultSet.next());
assertEquals(2, entryResultSet.getInt("SHARED_ID"));
assertFalse(entryResultSet.next());
}

clear(dbmsConnection);
}

@ParameterizedTest
@MethodSource("org.jabref.logic.shared.TestManager#getTestingDatabaseSystems")
void testRemoveSingleEntry(DBMSType dbmsType, DBMSConnection dbmsConnection, DBMSProcessor dbmsProcessor) throws SQLException {
dbmsProcessor.setupSharedDatabase();
BibEntry entryToRemove = getBibEntryExample();
dbmsProcessor.insertEntry(entryToRemove);
dbmsProcessor.removeEntries(Collections.singletonList(entryToRemove));

try (ResultSet entryResultSet = selectFrom("ENTRY", dbmsConnection, dbmsProcessor)) {
assertFalse(entryResultSet.next());
}

clear(dbmsConnection);
}

@ParameterizedTest
@MethodSource("org.jabref.logic.shared.TestManager#getTestingDatabaseSystems")
void testRemoveEntriesOnNullThrows(DBMSType dbmsType, DBMSConnection dbmsConnection, DBMSProcessor dbmsProcessor) throws SQLException {
dbmsProcessor.setupSharedDatabase();
assertThrows(NullPointerException.class, () -> dbmsProcessor.removeEntries(null));
clear(dbmsConnection);
}

@ParameterizedTest
@MethodSource("org.jabref.logic.shared.TestManager#getTestingDatabaseSystems")
void testRemoveEmptyEntryList(DBMSType dbmsType, DBMSConnection dbmsConnection, DBMSProcessor dbmsProcessor) throws SQLException {
dbmsProcessor.setupSharedDatabase();
dbmsProcessor.removeEntries(Collections.emptyList());

try (ResultSet entryResultSet = selectFrom("ENTRY", dbmsConnection, dbmsProcessor)) {
assertFalse(entryResultSet.next());
}

clear(dbmsConnection);
}

@ParameterizedTest
@MethodSource("org.jabref.logic.shared.TestManager#getTestingDatabaseSystems")
void testGetSharedEntries(DBMSType dbmsType, DBMSConnection dbmsConnection, DBMSProcessor dbmsProcessor) throws SQLException {
Expand Down
8 changes: 3 additions & 5 deletions src/test/java/org/jabref/logic/shared/DBMSSynchronizerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package org.jabref.logic.shared;

import java.sql.SQLException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
Expand Down Expand Up @@ -90,7 +89,7 @@ public void testFieldChangedEventListener(DBMSType dbmsType, DBMSConnection dbms

@ParameterizedTest
@MethodSource("org.jabref.logic.shared.TestManager#getTestingDatabaseSystems")
public void testEntryRemovedEventListener(DBMSType dbmsType, DBMSConnection dbmsConnection, DBMSProcessor dbmsProcessor) throws Exception {
public void testEntriesRemovedEventListener(DBMSType dbmsType, DBMSConnection dbmsConnection, DBMSProcessor dbmsProcessor) throws Exception {
setUp(dbmsConnection);

BibEntry bibEntry = getBibEntryExample(1);
Expand Down Expand Up @@ -162,10 +161,9 @@ public void testSynchronizeLocalDatabaseWithEntryRemoval(DBMSType dbmsType, DBMS

assertEquals(expectedBibEntries, bibDatabase.getEntries());

dbmsProcessor.removeEntry(expectedBibEntries.get(0));
dbmsProcessor.removeEntry(expectedBibEntries.get(1));
dbmsProcessor.removeEntries(Collections.singletonList(expectedBibEntries.get(0)));

expectedBibEntries = new ArrayList<>();
expectedBibEntries = Collections.singletonList(expectedBibEntries.get(1));

dbmsSynchronizer.synchronizeLocalDatabase();

Expand Down