1

Here is my code. I am trying to use JUnit to test the deleteUsers() method, but everytime I write my test, it deletes all the users that I have in the database. How can i delete a single user? Below is the code for the method and for the test.

@Override
public boolean deleteUsers(List<String> ids) throws Exception {
    StringBuilder sql = new StringBuilder();
    sql.append("delete from user where ");
    for (String id : ids) {
        sql.append(" id = ? or");
    }
    String strSql = sql.toString().substring(0, sql.length() - 2) + ";";

    PreparedStatement preparedStatement = this.connection.prepareStatement(strSql);

    for (int i = 0; i < ids.size(); i++) {
        preparedStatement.setInt(1 + i, Integer.parseInt(ids.get(i)));
    }

    int lines = preparedStatement.executeUpdate();

    preparedStatement.close();
    return lines > 0;
}
Sycan
  • 37
  • 8
  • Are you running these tests against a live database or have you implemented an in-memory db? – Toby Cook Apr 29 '22 at 08:52
  • But your ddl statement is specifically designed to delete several users... So pass one id and get rid of `OR` – g00se Apr 29 '22 at 08:54
  • @TobyCook I am running these tests against a live database. – Sycan Apr 29 '22 at 08:57
  • @g00se I am aware that my ddl statement is designed to delete several users, however i am having a problem passing a single id to the deleteUsers() method. If you don't mind showing me how to implement that according to your suggestion, I will highly appreciate it. – Sycan Apr 29 '22 at 08:59
  • 1
    at no point in the test is the id of an existing user being added to the ids list – Steve Bosman Apr 29 '22 at 09:01

3 Answers3

1

The problem is caused by how the SQL is built. When deleteUsers is passed an empty list then the generated SQL will be:

 delete from user wher

which will result in all data being deleted (the table user is given the alias "wher"). I highly recommend checking at the start of the method if the collection is empty and either raising an exception or returning.

Add the following check

if (ids == null || ids.isEmpty()) {
  throw new IllegalArgumentException("ids must not be empty");
}
Steve Bosman
  • 2,599
  • 1
  • 25
  • 41
1

You're missing a check for empty input. In your test you pass an empty list to deleteUsers which results in this SQL statement:

delete from user wher;

I'd expect that the DBMS would reject this as invalid SQL but perhaps there are some where this is interpreted as delete from user which simply deletes all users. (As @SteveBosman pointed out the wher is interpreted as table alias as it is - due to the missing last e - no reserved word anymoere)

Basically you have 2 options. Either deleting all users by passing an empty list is a valid use case - in which case you should handle it properly by producing proper SQL. Or this is not expected and you should adapt your code to throw an Exception if ids is empty.

@Override
public boolean deleteUsers(List<String> ids) throws Exception {
  if (ids == null || ids.size() == 0) {
    throw new IllegalArgumentException("List of IDs must not be empty");
  }
  
  ...
}

You could of course return false in case of an empty input as well to indicate no users were deleted.

To pass values to the deleteUsers method in your test you need to add values to the used list:

userDAOImpl.addUser("admin3", "111222");
final List<String> idsToDelete = new ArrayList<>();
idsToDelete.add("111222");
userDAOImpl.deleteUsers(idsToDelete);
dpr
  • 10,591
  • 3
  • 41
  • 71
  • 1
    Ah, okay. I'm blind and totally didn't see the substring that cuts of the last 2 characters of the string. – OH GOD SPIDERS Apr 29 '22 at 09:10
  • @dpr Thanks for your answer. This is helpful. However, when i try doing what you have suggested, it gives me the following error: java.lang.AssertionError: expected null, but was:<[]> – Sycan Apr 29 '22 at 09:27
  • Looks like your `findAllUsers` returns an empty list in case there are no users. I'd personally think this is correct. You should probably adapt the assertion and check for an empty list instead of `null` – dpr Apr 29 '22 at 09:29
  • 1
    The "wher" is treated as a table alias which is why it isn't being rejected. – Steve Bosman Apr 29 '22 at 09:33
  • 1
    @SteveBosman you're right of course. Good catch! – dpr Apr 29 '22 at 09:34
  • @dpr I appreciate your help last time. I have another problem with the same project, but this is on adding the users. How can i make sure that users with empty username and /or password should not be added to my database? here is my implementation ```public boolean addUser(String userName, String password) throws Exception { //This is not working if(userName == null || password == null) { throw new IllegalArgumentException("username or password can't be empty"); } String sql = "insert into user(username,password) values(?,?) ";``` – Sycan May 06 '22 at 02:57
  • @Sycan See https://stackoverflow.com/a/3598792 – dpr May 06 '22 at 05:09
0
StringBuilder sql = new StringBuilder();
sql.append("delete from user where");
String orClause = "";
for (String id : ids) {
    sql.append(orClause);
    sql.append(" id = ?");
    orClause = " or";
}
g00se
  • 3,207
  • 2
  • 5
  • 9