[server] Added some TODOs

2 files changed, 40 insertions, 4 deletions

diff --git a/dozentenmodulserver/src/main/java/server/ServerHandler.java b/dozentenmodulserver/src/main/java/server/ServerHandler.java
index 837cc64a..b5d7bc54 100644
--- a/dozentenmodulserver/src/main/java/server/ServerHandler.java
+++ b/dozentenmodulserver/src/main/java/server/ServerHandler.java
@@ -37,6 +37,7 @@ public class ServerHandler implements Server.Iface {
 	static SQL sql = new SQL();
 
 	// saves the current tokens and the mapped userdata, returning from the server
+	// TODO: Handle/cache tokens in own class, add timeout to tokens in case client never marks it invalid
 	private Map<String, UserInfo> tokenManager = new HashMap<>();
 
 	public boolean authenticated(String token) throws TException {
@@ -165,6 +166,8 @@ public class ServerHandler implements Server.Iface {
 				return false;
 			} else {
 				// set permissions
+				// TODO: Just no. Check if it's writable and bail out if not, but don't
+				// blindly try to set permissions when you don't even check if it worked. 
 				try {
 					Runtime.getRuntime().exec("chmod 777 " + path);
 				} catch (IOException e) {
@@ -181,7 +184,7 @@ public class ServerHandler implements Server.Iface {
 		try {
 			MessageDigest md = MessageDigest.getInstance("SHA1");
 			md.update(key.getBytes());
-			log.info("successfully returned EncodedSha1Sum");
+			log.info("successfully returned EncodedSha1Sum"); // How do you know? You didn't return anything yet
 			return new BigInteger(1, md.digest()).toString(16);
 		} catch (NoSuchAlgorithmException e) {
 			// handle error case to taste
@@ -192,7 +195,6 @@ public class ServerHandler implements Server.Iface {
 	@Override
 	public long DeleteFtpUser(String user, String token) throws TException {
 		if (authenticated(token)) {
-
 			return sql.DeleteUser(user);
 		}
 		return -1;
@@ -211,7 +213,7 @@ public class ServerHandler implements Server.Iface {
 	@Override
 	public String setInstitution(String university, String token) throws TException {
 		if (authenticated(token)) {
-
+			// TODO: wat. Institutions are defined globally on the master server, including their ID
 			return sql.setInstitution(university);
 		}
 		return null;
@@ -234,6 +236,8 @@ public class ServerHandler implements Server.Iface {
 			// OS impl Select and write
 			// ACHTUNG: Anzahl der Leerzeichen muss eingehalten werden:
 			// 'Windows 7 32 bit"
+			// TODO: Might be the biggest mess around here. We should define OS types on the
+			// master server in the future and have them synced to the satellite.
 			String pk_os = sql.getOSpk(os.substring(0, nthIndexOf(os, " ", 2)),
 					os.substring(nthIndexOf(os, " ", 2), os.lastIndexOf(" ")).replace(" ", ""));
 
@@ -307,6 +311,10 @@ public class ServerHandler implements Server.Iface {
 	// UserInfo does not return the institution, so in this case, the local
 	// method is prepared, but not yet executed, as the institution has to be
 	// added to UserInfo (or selected by institutionID)
+	// TODO: What is this even supposed to do?
+	// institutionID is a member of UserInfo, and the master server
+	// implements a method called getOrganizations, so you can map the ID to a
+	// name.... !? Also why the hell is this using a map instead of a class?
 	@Override
 	public Map<String, String> getPersonData(String Vorname, String Nachname, String token) throws TException {
 
@@ -330,6 +338,7 @@ public class ServerHandler implements Server.Iface {
 	}
 
 	public void setPerson(String userID, String token, String institution) throws TException {
+		// TODO: Again, what's going on with institution as a parameter here? It's part of the UserInfo...
 		if (authenticated(token)) {
 			UserInfo ui = getUserFromToken(token);
 			sql.setPerson(userID, ui.getLastName(), ui.getFirstName(), ui.getEMail(), new Date(), institution);
@@ -341,6 +350,7 @@ public class ServerHandler implements Server.Iface {
 			String endDate, boolean isActive, String imageID, String token, String Tel, String Fak,
 			String lectureID, String university) throws TException {
 		if (authenticated(token)) {
+			// TODO: Check if the user has the permissions to set this lecture's meta data...
 			UserInfo ui = getUserFromToken(token);
 
 			Map<String, String> map = new HashMap<String, String>();
@@ -492,6 +502,7 @@ public class ServerHandler implements Server.Iface {
 	@Override
 	public boolean deleteImageServer(String imageid, String imageversion, String token) throws TException {
 		if (authenticated(token)) {
+			// TODO: Has user permissions to delete this file?
 			String stringFile = sql.getFile(imageid, imageversion);
 			log.info("File to Delete: " + stringFile);
 
@@ -526,7 +537,8 @@ public class ServerHandler implements Server.Iface {
 		return success;
 	}
 
-	// TODO
+	// TODO: ... I can write a small java app that calls this function to delete random files anywhere on the sat
+	// If this function is not really required for some obscure reason then it should be removed
 	public boolean deleteImageByPath(String image_path) throws TException {
 
 		log.info("File to Delete: " + image_path);
@@ -551,6 +563,7 @@ public class ServerHandler implements Server.Iface {
 	@Override
 	public boolean connectedToLecture(String id, String version, String token) throws TException {
 		if (authenticated(token)) {
+			// TODO: Permissions
 			return sql.connectedToLecture(id, version);
 		}
 		return true;
@@ -560,6 +573,7 @@ public class ServerHandler implements Server.Iface {
 		boolean success = false;
 
 		if (authenticated(token)) {
+			// TODO: Permissions
 			if (sql.deleteLecture(id) == true) {
 
 				success = true;
@@ -573,6 +587,7 @@ public class ServerHandler implements Server.Iface {
 	@Override
 	public List<String> getAllUniversities(String token) throws TException {
 		if (authenticated(token)) {
+			// TODO: Sync with list from master server (.getOrganizations() - call every now and then and add to local DB)
 			return sql.getAllUniversities();
 		}
 		return null;
@@ -611,6 +626,7 @@ public class ServerHandler implements Server.Iface {
 	@Override
 	public boolean createUser(String token, String university) throws TException {
 		if (authenticated(token)) {
+			// TODO: Do not use university param...
 			UserInfo ui = getUserFromToken(token);
 			String pk_institution = sql.setInstitution(university);
 			sql.setPerson(ui.getEMail(), ui.getLastName(), ui.getFirstName(), ui.getEMail(), new Date(),
@@ -718,6 +734,7 @@ public class ServerHandler implements Server.Iface {
 
 	@Override
 	public List<Person> getAllOtherSatelliteUsers(List<String> userID, String token) throws TException {
+		// TODO: Like we couldn't filter the current user on the client side...
 		if (authenticated(token)) {
 			return sql.getAllOtherSatelliteUsers(userID);
 			// return null;
diff --git a/dozentenmodulserver/src/main/java/sql/SQL.java b/dozentenmodulserver/src/main/java/sql/SQL.java
index c953de34..890e6e39 100644
--- a/dozentenmodulserver/src/main/java/sql/SQL.java
+++ b/dozentenmodulserver/src/main/java/sql/SQL.java
@@ -27,6 +27,7 @@ public class SQL {
 	private static final Logger log = Logger.getLogger(SQL.class);

 

 	public Connection getConnection() {

+		// TODO: Connection pooling, better yet some abstraction layer for mysql, eg. like dalesbred in master-server

 		try {

 			Class.forName("com.mysql.jdbc.Driver").newInstance();

 		} catch (InstantiationException | IllegalAccessException | ClassNotFoundException e) {

@@ -140,6 +141,13 @@ public class SQL {
 	}

 

 	public String setInstitution(String university) {

+		// TODO: WTF!?

+		// 1) This method is called set* and has a return value (other than boolean)

+		// 2) This method generates a random ID for an institution name (which is called university in the

+		//    parameter), so every satellite server has a different ID for the same institution

+		// 3) If an institution is renamed, it gets a new id, and the old one remains in the db

+		// 4) In case it inserts a new institution with an ID, it queries the DB for the ID right

+		//    after doing so. So it's expected to be different from what was just inserted!??

 		try {

 			Connection con = getConnection();

 

@@ -242,6 +250,9 @@ public class SQL {
 

 				con.commit();

 

+				// TODO: why... WHY!? We had the user id from the beginning! We just inserted it!

+				// Also if there are two users with same first and last name we might get

+				// the wrong id! JUST WHY!!!!!?

 				sql = "SELECT userID FROM bwLehrpool.m_user where Nachname like " + "?"

 						+ " and Vorname like " + "?" + ";";

 

@@ -352,6 +363,7 @@ public class SQL {
 

 			log.info("Failed to setImageData.");

 			e.printStackTrace();

+			// TODO: Yeah great - it failed, we log it, but we still return true....

 		}

 		return true;

 

@@ -385,6 +397,8 @@ public class SQL {
 		} catch (SQLException e) {

 

 			e.printStackTrace();

+			// TODO: Shouldn't we return null or something that gives the client a chance

+			// to detect the failure and notify the user?

 		}

 		return list;

 	}

@@ -484,6 +498,7 @@ public class SQL {
 		return list;

 	}

 

+	// TODO: OK this method is pretty much the tenth copy&paste version of the stuff above. Maybe refactor something here?

 	public List<Image> getImageListPermissionAdmin(String userID) {

 		Connection con = getConnection();

 

@@ -783,6 +798,7 @@ public class SQL {
 

 			// if map is empty, then itemID does not belong to an image -> must

 			// be a lecture

+			// TODO: Errr what? Why? Trial and error until we find the right table?

 			if (map.isEmpty() == true) {

 				// check if itemID belongs to a lecture

 				sql = "SELECT admin_owner FROM bwLehrpool.m_VLData_lecture WHERE lectureID=?;";

@@ -928,6 +944,8 @@ public class SQL {
 			ResultSet data = prest.executeQuery();

 			con.commit();

 			while (data.next()) {

+				// TODO: This is just a horrible idea. Make a typo anywhere for the key, it still compiles but doesn't work.

+				// Somebody was too lazy to create a proper struct/class here....

 				map.put("name", data.getString("image_name"));

 				map.put("internet", data.getString("cond_hasInternetRestriction"));

 				map.put("license", data.getString("cond_hasLicenseRestriction"));

@@ -1516,6 +1534,7 @@ public class SQL {
 				link_allowed = false;

 				image_admin = false;

 

+				// TODO: Are those really non-numeric columns? If yes, keep in mind parseInt can throw an exception...

 				if (Integer.parseInt(res.getString("image_read")) != 0) {

 					image_read = true;

 				}