3
\$\begingroup\$

So I have an user which can create his own Locations. data model

I implemented a webservice in order to add and update locations. If the user is not the owner of the location i want to throw an appropriate exception so that the user knows whats wrong.

The current method looks like this:

private void updateLocation(String loginToken, MapLocation ml) throws Exception{
    String username = this.getUsernameBySingInToken(loginToken); //throws exception is wrong token, otherwise returns username

    PreparedStatement stmt = conn.prepareStatement("SELECT username FROM Location WHERE idLoc=?");
    stmt.setInt(1, ml.getLocationId());
    ResultSet rs = stmt.executeQuery();
    String actuallUsername = null;
    while (rs.next()) {
       actuallUsername=rs.getString("username");
    }
    if(actuallUsername==null)
        throw new Exception("no location found!!"); //appropriate exception


    if(!username.equals(actuallUsername) || !username.equals(ml.getUsername()))
        throw new Exception("you are not the creator of this location!"); //appropriate exception 

    PreparedStatement stmtVerification = conn.prepareStatement("UPDATE Location SET positionX=?,positionY=?"
            + " WHERE idLoc=?");
    stmtVerification.setDouble(1, ml.getPosX());
    stmtVerification.setDouble(2, ml.getPosY());
    stmtVerification.setInt(3, ml.getLocationId());
    stmtVerification.executeUpdate();

    disconnectFromMySQL();
}

The second method I implemented is shorter but the errors are not clear:

private void updateLocationV2(String loginToken, MapLocation ml) throws Exception{
    String username = this.getUsernameBySingInToken(loginToken);
    if(!username.equals(ml.getUsername()))
        throw new Exception("username is not the same as username of maplocation object"); //not sure if neccesary 

    PreparedStatement stmtVerification = conn.prepareStatement("UPDATE Location SET positionX=?,positionY=?"
            + " WHERE idLoc=? and username=?");
    stmtVerification.setDouble(1, ml.getPosX());
    stmtVerification.setDouble(2, ml.getPosY());
    stmtVerification.setInt(3, ml.getLocationId());
    stmtVerification.setString(4, username);

    int executeUpdate = stmtVerification.executeUpdate();
    if(executeUpdate==0)
        throw new Exception("update failed. either wrong username or idLocation is wrong"); //not clear what is the error BUT shorter
    disconnectFromMySQL();
}    

So yeah, now I have two methods which update my location but I am not sure which of them is the best. The first method has the advantage that the errors are clear but its slower since I have two queries (select and update). the second method is faster but the errors are not clear. WHich one of them is better, or is there even a better solution?

EDIT: This is my webservice method which calls the update function:

@PUT
@Path("/update")
@Consumes({MediaType.APPLICATION_JSON})
public Response update(String token, MapLocation ml) {
    Response r = Response.status(Response.Status.OK).entity("location updated").build();
    try {
        isUpdated = db.updateLocation(token,ml);
    } catch (Exception ex) {
        r = Response.status(Response.Status.BAD_REQUEST).entity(ex.getMessage()).build();
    }
    return r;
}
\$\endgroup\$
  • \$\begingroup\$ Without seeing the rest of the class it is very hard to tell which is the better of the two functions. It also depends on what the requirements for the implementation are. The question is also unclear because there could be a third way that is best. \$\endgroup\$ – pacmaninbw Jun 15 at 12:40
  • \$\begingroup\$ @pacmaninbw there are no requirements. I am making my own project. So right know I dont know whats the best way to make an update. Yes there could be better solutions but i dont know which is the best \$\endgroup\$ – Savan Luffy Jun 15 at 13:29
  • 2
    \$\begingroup\$ @SavanLuffy even if it's your own project you have requirements. If you don't know them you should think them out :) \$\endgroup\$ – IEatBagels Jun 18 at 17:36
2
\$\begingroup\$

Soo ... I'm not quite sure how to formulate this without it sounding brash, but... Both of these methods are less than ideal...

Let's follow the behavioural flow to see what issues this code has:

  • The webservice portion you provide only understands PUT requests.
    Conventionally, PUT requests are used to create objects, not to update them. Instead one would expect a POST endpoint for that.
  • You first create an OK response and then overwrite it in case something fails. That's somewhat... annoying in comparison to the simpler direct return.

    try {
        db.updateLocation(token, ml);
        return Response.status(Status.OK).entity("location updated").build();
    } catch (Exception e) {
        return Response.status(Status.BAD_REQUEST).entity(ex.getMessage()).build();
    }
    
  • All this code assumes that the user gets to see the Exception message.
    This is a bad assumption. Your webservice method should be responsible for creating useful messages, not your business logic.

  • Considering that the webservice code does not compile (because the declaration for isUpdated is missing) I hope that you don't actually pretend that every Exception ever is the user's fault by throwing a HTTP 400 to them...

The root cause of your conundrum is that you're not using the power of widely used java sql abstractions. Instead of manually writing SQL statements and performing updates, you should really make use of JPA and Entities.

If you had these, all of this would boil down to the following simplified code:

private void updateLocation(String username, MapLocation update) {
    MapLocation stored = em.find(MapLocation.class, update.getLocationId());
    if (stored == null) {
        throw new LocationDoesNotExistException();
    }
    if (!stored.getUsername().equals(username)) {
        throw new IllegalLocationUpdateException();
    }
    em.persist(update);
}

Note that the codesample above makes some simplifying assumptions about transaction management and the class hierarchy of the Exceptions it throws.

Note that I'm suggesting to rewrite your webservice method to something like:

public Response update(String loginToken, MapLocation request) {
    final String username = getUsernameFromToken(loginToken);
    try {
        db.updateLocation(username, request);
        return Response.ok().build();
    } catch (LocationDoesNotExistException e) {
        return Response.status(Response.Status.NOT_FOUND).build();
    } catch (IllegalLocationUpdateException e) {
        return Response.status(Response.Status.FORBIDDEN).build();
    } catch (Exception e) {
        LOGGER.warn("Unexpected exception in updateLocation", e);
        return Response.serverError().build();
    }
}
\$\endgroup\$
  • \$\begingroup\$ @Vogel623 thank you for the detailed answer. I never heard of JPA, I will defenitly look it up. As far as I know its bad practise to have more than 1 return value, thats why I only use one response object which gets overwritten if an error occurs. Are you sure that PUT is meant to CREATE update objects and POST to UPDATE them? I thought its the exact opposite? \$\endgroup\$ – Savan Luffy Jun 15 at 23:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.