From ac000e351855bbcf5e9389cd9d9f65e2ec05f3f1 Mon Sep 17 00:00:00 2001 From: Paul Boon Date: Tue, 10 Sep 2024 14:12:19 +0200 Subject: [PATCH] DD-1586 Added truncate-notifications with commandline input parsing (#6) * Added truncate-notifications with commandline input parsing * Added database configuration for truncate-notifications * Initial database connection for truncate-notifications * Database connection for truncate-notifications working with a query * Database query refactoring for truncate-notifications * Implemented deleting notifications for specific user * Implemented deleting notifications for all users * Refactor notifications truncation, using batch processing * Throwing exceptions instead of just printing messages in notifications truncation * Initial implementation of unit tests for notification truncation * Refactoring the Database class; extracting result now in separate function * Added test for notification truncation of all users * Added db settings to example config * Rename the dataverse setting to api * Refactoring * Removing erroneous ';' in yml configs * Added @Positive annotation to numberOfRecordsToKeep * Renamed test functions to snake-case * Remove redundant use of stdin manipulation in the NotificationTruncateTest * make sure database connection is closed, even if an exception is thrown * Using try-with-resources statements in Database query and update * Removing @Positive annotation on numberOfRecordsToKeep * Clarify the user input parameter with the notification truncation * Added the delay option for notification truncate * Getting rid of most warnings * Have AbstractCmd doCall signature throw a general Exception --- pom.xml | 4 + src/main/assembly/dist/cfg/example-config.yml | 8 +- .../nl/knaw/dans/dvcli/DdDataverseCli.java | 11 +- .../nl/knaw/dans/dvcli/action/Database.java | 127 ++++++++++++++ .../knaw/dans/dvcli/command/AbstractCmd.java | 2 +- .../dvcli/command/NotificationTruncate.java | 162 ++++++++++++++++++ .../dvcli/config/DdDataverseCliConfig.java | 6 +- .../config/DdDataverseDatabaseConfig.java | 38 ++++ .../command/NotificationTruncateTest.java | 131 ++++++++++++++ src/test/resources/debug-etc/config.yml | 10 +- 10 files changed, 492 insertions(+), 7 deletions(-) create mode 100644 src/main/java/nl/knaw/dans/dvcli/action/Database.java create mode 100644 src/main/java/nl/knaw/dans/dvcli/command/NotificationTruncate.java create mode 100644 src/main/java/nl/knaw/dans/dvcli/config/DdDataverseDatabaseConfig.java create mode 100644 src/test/java/nl/knaw/dans/dvcli/command/NotificationTruncateTest.java diff --git a/pom.xml b/pom.xml index 9b14c59..32790e7 100644 --- a/pom.xml +++ b/pom.xml @@ -85,6 +85,10 @@ org.apache.commons commons-csv + + org.postgresql + postgresql + org.junit.jupiter junit-jupiter diff --git a/src/main/assembly/dist/cfg/example-config.yml b/src/main/assembly/dist/cfg/example-config.yml index 396f3e3..d340765 100644 --- a/src/main/assembly/dist/cfg/example-config.yml +++ b/src/main/assembly/dist/cfg/example-config.yml @@ -1,7 +1,13 @@ -dataverse: +api: baseUrl: "http://localhost:8080" apiKey: "your-api-token" +db: + host: localhost + database: "dvndb" + user: "dvnuser" + password: "dvnsecret" + # # See https://www.dropwizard.io/en/latest/manual/configuration.html#logging # diff --git a/src/main/java/nl/knaw/dans/dvcli/DdDataverseCli.java b/src/main/java/nl/knaw/dans/dvcli/DdDataverseCli.java index 7ae6935..e59046a 100644 --- a/src/main/java/nl/knaw/dans/dvcli/DdDataverseCli.java +++ b/src/main/java/nl/knaw/dans/dvcli/DdDataverseCli.java @@ -17,6 +17,7 @@ package nl.knaw.dans.dvcli; import lombok.extern.slf4j.Slf4j; +import nl.knaw.dans.dvcli.action.Database; import nl.knaw.dans.dvcli.command.CollectionAssignRole; import nl.knaw.dans.dvcli.command.CollectionCmd; import nl.knaw.dans.dvcli.command.CollectionCreateDataset; @@ -34,7 +35,9 @@ import nl.knaw.dans.dvcli.command.DatasetCmd; import nl.knaw.dans.dvcli.command.DatasetValidateFiles; import nl.knaw.dans.dvcli.command.DeleteDraft; +import nl.knaw.dans.dvcli.command.NotificationTruncate; import nl.knaw.dans.dvcli.config.DdDataverseCliConfig; +import nl.knaw.dans.lib.dataverse.DataverseClient; import nl.knaw.dans.lib.util.AbstractCommandLineApp; import nl.knaw.dans.lib.util.PicocliVersionProvider; import picocli.CommandLine; @@ -57,7 +60,10 @@ public String getName() { @Override public void configureCommandLine(CommandLine commandLine, DdDataverseCliConfig config) { log.debug("Building Dataverse client"); - var dataverseClient = config.getDataverse().build(); + var dataverseClient = config.getApi().build(); + var databaseConfig = config.getDb(); + var database = new Database(databaseConfig); + commandLine.addSubcommand(new CommandLine(new CollectionCmd(dataverseClient)) .addSubcommand(new CollectionAssignRole()) .addSubcommand(new CollectionCreateDataset()) @@ -75,7 +81,8 @@ public void configureCommandLine(CommandLine commandLine, DdDataverseCliConfig c .addSubcommand(new CommandLine(new DatasetCmd(dataverseClient)) .addSubcommand(new DatasetValidateFiles()) .addSubcommand(new DeleteDraft()) - ); + ) + .addSubcommand(new CommandLine(new NotificationTruncate(database))); log.debug("Configuring command line"); } } diff --git a/src/main/java/nl/knaw/dans/dvcli/action/Database.java b/src/main/java/nl/knaw/dans/dvcli/action/Database.java new file mode 100644 index 0000000..5f5696f --- /dev/null +++ b/src/main/java/nl/knaw/dans/dvcli/action/Database.java @@ -0,0 +1,127 @@ +/* + * Copyright (C) 2024 DANS - Data Archiving and Networked Services (info@dans.knaw.nl) + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package nl.knaw.dans.dvcli.action; + +import lombok.extern.slf4j.Slf4j; +import nl.knaw.dans.dvcli.config.DdDataverseDatabaseConfig; + +import java.sql.Connection; +import java.sql.DriverManager; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.sql.Statement; +import java.util.ArrayList; +import java.util.List; + +/** + * Provides access to the Dataverse Database (Postgres). + * Some actions are not supported by the Dataverse API (yet) + * and must be done by direct access to the database. + *

+ * Note that the sql input strings are not filtered in any way, + * so don't put user input in there! + */ +@Slf4j +public class Database { + + public Database(DdDataverseDatabaseConfig config) { + this.host = config.getHost(); + this.database = config.getDatabase(); + this.user = config.getUser(); + this.password = config.getPassword(); + } + + Connection connection = null; + + String port = "5432"; // Fixed port for Postgres + + String host; + String database; + String user; + String password; + + public void connect() throws ClassNotFoundException, SQLException { + Class.forName("org.postgresql.Driver"); + if (connection == null) { + log.debug("Starting connecting to database"); + connection = DriverManager + .getConnection("jdbc:postgresql://" + host + ":" + port + "/" + database, + user, + password); + } + + } + + public void close() { + try { + if (connection != null) { + log.debug("Close connection to database"); + connection.close(); + } + } catch (SQLException e) { + System.err.println( "Database error: " + e.getClass().getName() + " " + e.getMessage() ); + } finally { + connection = null; + } + } + + public List> query(String sql) throws SQLException { + return query(sql, false); + } + + public List> query(String sql, Boolean startResultWithColumnNames) throws SQLException { + log.debug("Querying database with: {}", sql); + + try ( + Statement stmt = connection.createStatement(); + ResultSet rs = stmt.executeQuery(sql) + ) { + return extractResult(rs, startResultWithColumnNames); + } + } + + List> extractResult(ResultSet rs, Boolean startResultWithColumnNames) throws SQLException { + List> rows = new ArrayList<>(); + // get column names + int numColumns = rs.getMetaData().getColumnCount(); + if (startResultWithColumnNames) { + List columnNames = new ArrayList(); + for (int i = 1; i <= numColumns; i++) { + columnNames.add(rs.getMetaData().getColumnName(i)); + } + // make it the first row, for simplicity, a bit like with a csv file + rows.add(columnNames); + } + + // get the data rows + while (rs.next()) { + List row = new ArrayList(); + for (int i = 1; i <= numColumns; i++) { + row.add(rs.getString(i)); + } + rows.add(row); + } + return rows; + } + + public int update(String sql) throws SQLException { + log.debug("Updating database with: {}", sql); + + try (Statement stmt = connection.createStatement()) { + return stmt.executeUpdate(sql); + } + } +} diff --git a/src/main/java/nl/knaw/dans/dvcli/command/AbstractCmd.java b/src/main/java/nl/knaw/dans/dvcli/command/AbstractCmd.java index 2ddb09d..53208e0 100644 --- a/src/main/java/nl/knaw/dans/dvcli/command/AbstractCmd.java +++ b/src/main/java/nl/knaw/dans/dvcli/command/AbstractCmd.java @@ -35,5 +35,5 @@ public Integer call() throws Exception { } } - public abstract void doCall() throws IOException, DataverseException; + public abstract void doCall() throws Exception; } diff --git a/src/main/java/nl/knaw/dans/dvcli/command/NotificationTruncate.java b/src/main/java/nl/knaw/dans/dvcli/command/NotificationTruncate.java new file mode 100644 index 0000000..6cc840e --- /dev/null +++ b/src/main/java/nl/knaw/dans/dvcli/command/NotificationTruncate.java @@ -0,0 +1,162 @@ +/* + * Copyright (C) 2024 DANS - Data Archiving and Networked Services (info@dans.knaw.nl) + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package nl.knaw.dans.dvcli.command; + +import lombok.NonNull; +import lombok.extern.slf4j.Slf4j; +import nl.knaw.dans.dvcli.action.BatchProcessor; +import nl.knaw.dans.dvcli.action.ConsoleReport; +import nl.knaw.dans.dvcli.action.Database; +import nl.knaw.dans.dvcli.action.Pair; +import nl.knaw.dans.dvcli.action.ThrowingFunction; + +import picocli.CommandLine; + +import java.io.IOException; +import java.sql.SQLException; +import java.util.ArrayList; +import java.util.List; + +@CommandLine.Command(name = "truncate-notifications", + mixinStandardHelpOptions = true, + description = "Remove user notifications but keep up to a specified amount.", + sortOptions = false) +@Slf4j +public class NotificationTruncate extends AbstractCmd { + protected Database db; + public NotificationTruncate(@NonNull Database database) { + this.db = database; + } + + private static final long DEFAULT_DELAY = 10L; // 10 ms, database should be able to handle this + + @CommandLine.Option(names = { "-d", "--delay" }, description = "Delay in milliseconds between requests to the server (default: ${DEFAULT-VALUE}).", defaultValue = "" + DEFAULT_DELAY) + protected long delay = DEFAULT_DELAY; + + @CommandLine.ArgGroup(exclusive = true, multiplicity = "1") + UserOptions users; + + static class UserOptions { + @CommandLine.Option(names = { "--user" }, required = true, + description = "The user database id (a number) whose notifications to truncate.") + int user; // a number, preventing accidental SQL injection + // This id is visible for 'superusers' in the Dataverse Dashboard + + @CommandLine.Option(names = { "--all-users" }, required = true, + description = "Truncate notifications for all users.") + boolean allUsers; + } + + @CommandLine.Parameters(index = "0", paramLabel = "number-of-records-to-keep", + description = "The number of notification records to keep.") + private int numberOfRecordsToKeep; + + private record NotificationTruncateParams(Database db, int userId, int numberOfRecordsToKeep) { + } + + private BatchProcessor.BatchProcessorBuilder paramsBatchProcessorBuilder() throws IOException { + return BatchProcessor. builder(); + } + + private static class NotificationTruncateAction implements ThrowingFunction { + + @Override + public String apply(NotificationTruncateParams notificationTruncateParams) throws Exception { + // Dry-run; show what will be deleted + //List> results = notificationTruncateParams.db.query(String.format("SELECT * FROM usernotification WHERE user_id = '%d' AND id NOT IN (SELECT id FROM usernotification WHERE user_id = '%d' ORDER BY senddate DESC LIMIT %d);", + // notificationTruncateParams.userId, notificationTruncateParams.userId, + // notificationTruncateParams.numberOfRecordsToKeep), true + //); + //return "Notifications for user " + notificationTruncateParams.userId + " that will be deleted: \n" + // + getResultsAsString(results); + + // Actually delete the notifications + try { + log.info("Deleting notifications for user with id {}", notificationTruncateParams.userId); + int rowCount = notificationTruncateParams.db.update(String.format("DELETE FROM usernotification WHERE user_id = '%d' AND id NOT IN (SELECT id FROM usernotification WHERE user_id = '%d' ORDER BY senddate DESC LIMIT %d);", + notificationTruncateParams.userId, notificationTruncateParams.userId, + notificationTruncateParams.numberOfRecordsToKeep)); + return "Deleted " + rowCount + " record(s) for user with id " + notificationTruncateParams.userId; + } catch (SQLException e) { + throw new Exception("Error deleting notifications for user with id " + notificationTruncateParams.userId, e); + } + } + } + + @Override + public void doCall() throws Exception { + // validate input + if (numberOfRecordsToKeep < 0) { + throw new Exception("Number of records to keep must be a positive integer, now it was " + numberOfRecordsToKeep + "."); + } + + db.connect(); + try { + paramsBatchProcessorBuilder() + .labeledItems(getItems()) + .action(new NotificationTruncate.NotificationTruncateAction()) + .delay(delay) + .report(new ConsoleReport<>()) + .build() + .process(); + } finally { + db.close(); + } + } + + List> getItems() throws Exception { + List> items = new ArrayList<>(); + try { + if (users.allUsers) { + getUserIds(db).forEach(user_id -> items.add(new Pair<>(Integer.toString(user_id), + new NotificationTruncateParams(db, user_id, numberOfRecordsToKeep)))); + } else { + // single user + items.add(new Pair<>(Integer.toString(users.user), + new NotificationTruncateParams(db, users.user, numberOfRecordsToKeep))); + } + } catch (SQLException e) { + throw new Exception("Error getting user ids: ", e); + } + return items; + } + + // get the user_id for all users that need truncation + private List getUserIds(Database db) throws SQLException { + List users = new ArrayList(); + // Could just get all users with notifications + // String sql = "SELECT DISTINCT user_id FROM usernotification;"; + // Instead we want only users with to many notifications + String sql = String.format("SELECT user_id FROM usernotification GROUP BY user_id HAVING COUNT(user_id) > %d;", numberOfRecordsToKeep); + List> results = db.query(sql); + for (List row : results) { + users.add(Integer.parseInt(row.get(0))); + } + return users; + } + + private static String getResultsAsString(List> results) { + // Note that the first row could be the column names + StringBuilder sb = new StringBuilder(); + for (List row : results) { + for (String cell : row) { + sb.append(cell).append(" "); + } + sb.append("\n"); + } + return sb.toString(); + } +} diff --git a/src/main/java/nl/knaw/dans/dvcli/config/DdDataverseCliConfig.java b/src/main/java/nl/knaw/dans/dvcli/config/DdDataverseCliConfig.java index 3c31fee..666b29d 100644 --- a/src/main/java/nl/knaw/dans/dvcli/config/DdDataverseCliConfig.java +++ b/src/main/java/nl/knaw/dans/dvcli/config/DdDataverseCliConfig.java @@ -19,10 +19,14 @@ import io.dropwizard.core.Configuration; import lombok.Data; import lombok.EqualsAndHashCode; +import lombok.NonNull; import nl.knaw.dans.lib.util.DataverseClientFactory; @Data @EqualsAndHashCode(callSuper = true) public class DdDataverseCliConfig extends Configuration { - private DataverseClientFactory dataverse; + private DataverseClientFactory api; + + @NonNull + private DdDataverseDatabaseConfig db = new DdDataverseDatabaseConfig(); } diff --git a/src/main/java/nl/knaw/dans/dvcli/config/DdDataverseDatabaseConfig.java b/src/main/java/nl/knaw/dans/dvcli/config/DdDataverseDatabaseConfig.java new file mode 100644 index 0000000..ca62896 --- /dev/null +++ b/src/main/java/nl/knaw/dans/dvcli/config/DdDataverseDatabaseConfig.java @@ -0,0 +1,38 @@ +/* + * Copyright (C) 2024 DANS - Data Archiving and Networked Services (info@dans.knaw.nl) + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package nl.knaw.dans.dvcli.config; + +import lombok.Data; + +import javax.validation.constraints.NotEmpty; + +@Data +public class DdDataverseDatabaseConfig { + + @NotEmpty + private String host = "localhost"; + + @NotEmpty + private String database = "dvndb"; + + @NotEmpty + private String user = "dvnuser"; + + @NotEmpty + private String password = "dvnsecret"; +} + diff --git a/src/test/java/nl/knaw/dans/dvcli/command/NotificationTruncateTest.java b/src/test/java/nl/knaw/dans/dvcli/command/NotificationTruncateTest.java new file mode 100644 index 0000000..3f88232 --- /dev/null +++ b/src/test/java/nl/knaw/dans/dvcli/command/NotificationTruncateTest.java @@ -0,0 +1,131 @@ +/* + * Copyright (C) 2024 DANS - Data Archiving and Networked Services (info@dans.knaw.nl) + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package nl.knaw.dans.dvcli.command; + +import nl.knaw.dans.dvcli.AbstractCapturingTest; +import nl.knaw.dans.dvcli.action.Database; +import org.junit.jupiter.api.Test; +import org.mockito.Mockito; + +import java.sql.SQLException; +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; + + +public class NotificationTruncateTest extends AbstractCapturingTest { + + @Test + public void doCall_with_wrong_database_connection_fails() throws Exception { + + var database = Mockito.mock(Database.class); + doThrow(new SQLException("test database fails to connect")).when(database).connect(); + + var userOptions = new NotificationTruncate.UserOptions(); + userOptions.user = 13; + userOptions.allUsers = false; + + var cmd = getCmd(database, 1, userOptions); + + assertThatThrownBy(cmd::doCall) + .isInstanceOf(SQLException.class) + .hasMessage("test database fails to connect"); + } + + @Test + public void doCall_with_negative_numberOfRecordsToKeep_fails() throws Exception { + var database = Mockito.mock(Database.class); + + var userOptions = new NotificationTruncate.UserOptions(); + userOptions.user = 13; + userOptions.allUsers = false; + + var cmd = getCmd(database, -1, userOptions); + + assertThatThrownBy(cmd::doCall) + .isInstanceOf(Exception.class) + .hasMessage("Number of records to keep must be a positive integer, now it was -1."); + } + + @Test + public void doCall_with_several_users_to_truncate_notifications_works() throws Exception { + var database = Mockito.mock(Database.class); + + Mockito.doNothing().when(database).connect(); + Mockito.doNothing().when(database).close(); + + var fakeQueryOutput = List.of( + List.of("1", "user1-dontcare"), + List.of("2", "user2-dontcare"), + List.of("3", "user3-dontcare") + ); + Mockito.when(database.query(anyString())).thenReturn( fakeQueryOutput ); + Mockito.when(database.update(anyString())).thenReturn( 3,2,1); + + var userOptions = new NotificationTruncate.UserOptions(); + userOptions.user = 0; + userOptions.allUsers = true; + + var cmd = getCmd(database, 1, userOptions); + cmd.doCall(); + + assertThat(stderr.toString()).isEqualTo("1: OK. 2: OK. 3: OK. "); + assertThat(stdout.toString()).isEqualTo(""" + INFO Starting batch processing + INFO Processing item 1 of 3 + INFO Deleting notifications for user with id 1 + Deleted 3 record(s) for user with id 1 + DEBUG Sleeping for 10 ms + INFO Processing item 2 of 3 + INFO Deleting notifications for user with id 2 + Deleted 2 record(s) for user with id 2 + DEBUG Sleeping for 10 ms + INFO Processing item 3 of 3 + INFO Deleting notifications for user with id 3 + Deleted 1 record(s) for user with id 3 + INFO Finished batch processing of 3 items + """); + + verify(database, times(1)).connect(); + verify(database, times(1)).query(any()); + verify(database, times(3)).update(any()); + verify(database, times(1)).close(); + verifyNoMoreInteractions(database); + } + + private static NotificationTruncate getCmd(Database database, int numberOfRecordsToKeep, NotificationTruncate.UserOptions userOptions ) throws NoSuchFieldException, IllegalAccessException { + var cmd = new NotificationTruncate(database); + + // set private fields with reflection + + var numberOfRecordsToKeepField = NotificationTruncate.class.getDeclaredField("numberOfRecordsToKeep"); + numberOfRecordsToKeepField.setAccessible(true); + numberOfRecordsToKeepField.set(cmd, numberOfRecordsToKeep); + + var usersField = NotificationTruncate.class.getDeclaredField("users"); + usersField.setAccessible(true); + usersField.set(cmd, userOptions); + return cmd; + } + +} diff --git a/src/test/resources/debug-etc/config.yml b/src/test/resources/debug-etc/config.yml index 942bfba..98b199d 100644 --- a/src/test/resources/debug-etc/config.yml +++ b/src/test/resources/debug-etc/config.yml @@ -1,7 +1,13 @@ -dataverse: +api: baseUrl: "http://dev.archaeology.datastations.nl:8080" apiKey: # fill in your API key here - + +db: + host: localhost + database: "dvndb" + user: "dvnuser" + password: "dvnsecret" + # # See https://www.dropwizard.io/en/latest/manual/configuration.html#logging #