Implemented deletion of comments via Datastore #10

ihsan314 · 2020-07-14T01:09:54Z

Two deletion features exist: one simply limits how many comments appear at a time, and the other clears all comments made.

…p of page

…-steps-week3

…ment limiting input box

ccondit

Looks like this PR is adding all of the functionality from previous steps (not just delete). Is the parent branch correct?

ccondit · 2020-07-14T01:48:08Z

portfolio/pom.xml

+    <dependency>
+      <groupId>com.google.appengine</groupId>
+      <artifactId>appengine-api-1.0-sdk</artifactId>
+      <version>1.9.59</version>
+    </dependency>


nit - keep these dependencies alphabetical by groupId...

Ok I'll adjust that.

ccondit · 2020-07-14T01:56:58Z

portfolio/src/main/java/com/google/sps/servlets/DataServlet.java

-  HashMap<String, String> messages = new HashMap<String, String>();
-  // ArrayList<String> messages = new ArrayList<String>();
+  static DatastoreService datastore = DatastoreServiceFactory.getDatastoreService();
+  static List<Key> keys = new ArrayList<>();


Before going further here - what will happen if the server restarts but the data store stays persisted? We'll lose this list of keys. Can you look into another way to get the keys? Possibly through the persistence layer?
https://cloud.google.com/appengine/docs/standard/java/datastore/retrieving-query-results

Ok I'll do that.

ihsan314 · 2020-07-14T17:27:14Z

The first three commits in this pull request were on the week3 branch I believe, and they were approved for merging in pull request #4. I didn't merge them into master, instead I had the mega branch last-steps-week3 built on top of those commits. The last six commits are the only new content, and they are all datastore/deletion based.

I believe it most correctly extends post-comments since the last six commits are built on the commits in post-comments.

ccondit · 2020-07-14T18:21:41Z

portfolio/src/main/java/com/google/sps/servlets/DataServlet.java

 public class DataServlet extends HttpServlet {
-  static DatastoreService datastore = DatastoreServiceFactory.getDatastoreService();
-  static List<Key> keys = new ArrayList<>();
+  DatastoreService datastore = DatastoreServiceFactory.getDatastoreService();


Suggested change

DatastoreService datastore = DatastoreServiceFactory.getDatastoreService();

private final DatastoreService datastore = DatastoreServiceFactory.getDatastoreService();

ccondit · 2020-07-14T18:29:08Z

portfolio/src/main/java/com/google/sps/servlets/DataServlet.java

 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;

 /** Servlet that returns some example content. TODO: modify this file to handle comments data */


nit - update this comment to remove the TODO (since it's done!)

ccondit · 2020-07-14T18:29:31Z

portfolio/src/main/java/com/google/sps/servlets/DeleteServlet.java

-    List<Key> keys = DataServlet.keys;
-    datastore.delete(keys);
+    DatastoreService datastore = DatastoreServiceFactory.getDatastoreService();
+    Query query = new Query("Comment").setKeysOnly();


Are you missing an import here for Query?

Oops I was.

Note that there are more missing imports here. (DatastoreServiceFactory, for instance).

Try invoking mvn compile to ensure that your PR is building as expected.

ccondit · 2020-07-14T18:30:19Z

portfolio/src/main/java/com/google/sps/servlets/DeleteServlet.java

-    List<Key> keys = DataServlet.keys;
-    datastore.delete(keys);
+    DatastoreService datastore = DatastoreServiceFactory.getDatastoreService();
+    Query query = new Query("Comment").setKeysOnly();


Just a note that we'll probably want to find a common place to keep these constants (like "Comment") so that if they need to change, they will only need to be changed in one spot. No need to do anything now - just something to think about...

Yea that could be a static class member later.

ccondit · 2020-07-14T20:31:36Z

portfolio/src/main/java/com/google/sps/servlets/DataServlet.java

+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Query;


I don't think this class exists. And I think you meant to add
import com.google.appengine.api.datastore.Query; to the DeleteServlet.

Ah I've been gettting lazy whoops, I really need to compile before committing.

ccondit · 2020-07-14T20:33:14Z

portfolio/src/main/java/com/google/sps/servlets/DeleteServlet.java

-    List<Key> keys = DataServlet.keys;
-    datastore.delete(keys);
+    DatastoreService datastore = DatastoreServiceFactory.getDatastoreService();
+    Query query = new Query("Comment").setKeysOnly();


Note that there are more missing imports here. (DatastoreServiceFactory, for instance).

Try invoking mvn compile to ensure that your PR is building as expected.

ccondit · 2020-07-14T20:53:27Z

portfolio/src/main/java/com/google/sps/servlets/DeleteServlet.java

+    DatastoreService datastore = DatastoreServiceFactory.getDatastoreService();
+    Query query = new Query("Comment").setKeysOnly();
+    PreparedQuery keys = datastore.prepare(query);
+    datastore.delete(keys.asIterable());


DatastoreService.delete takes a Key, not an Entity. Try something like:

PreparedQuery preparedQuery = datastore.prepare(query); Iterator<Entity> entities = preparedQuery.asIterator(); while (entities.hasNext()) { Key key = entities.next().getKey(); datastore.delete(key); }

Oh ok, I will do that then.

fabielzuniga · 2020-07-14T22:58:36Z

portfolio/src/main/java/com/google/sps/servlets/DataServlet.java

-  HashMap<String, String> messages = new HashMap<String, String>();
-  // ArrayList<String> messages = new ArrayList<String>();
+  private final DatastoreService datastore = DatastoreServiceFactory.getDatastoreService();
+  private int maxNumComments;


Should we initialize this with a default value in case doGet is executed before doPost?

Sure, that's a good idea.

fabielzuniga · 2020-07-14T23:00:39Z

portfolio/src/main/java/com/google/sps/servlets/DataServlet.java

+    } catch (NumberFormatException e) {
+      System.err.println("Could not convert to int: " + maxNumCommentsString);
+      System.err.println("Using default value of 7.");
+      maxNumComments = 7;


Use a constant for the default value? It would be it easier to read if it ever needs to be changed.

Sure, I will do that.

fabielzuniga · 2020-07-14T23:06:05Z

portfolio/src/main/java/com/google/sps/servlets/DataServlet.java

  }

  @Override
  public void doPost(HttpServletRequest request, HttpServletResponse response) throws IOException {


This post is doing two things now. It is used to post a comment and also to set the configuration property maxNumComments. Have you consider using a different Servlet to post configuration? Maybe it is overkilled, thoughts?

I think each of the two things it's doing are relatively small though, especially setting the configuration property. For this example it's probably overkill, but in the future I'll be careful to scope the post method carefully.

fabielzuniga · 2020-07-14T23:08:37Z

portfolio/src/main/java/com/google/sps/servlets/DataServlet.java

+    }
+
+    Entity commentEntity = new Entity("Comment");
+    commentEntity.setProperty("username", username);


Constants like "Comment", "username", "message" and "timestamp" are used multiple times. Consider defining a constant for those to make it less error prone.

That's a good point, I'll consider it

fabielzuniga · 2020-07-14T23:14:18Z

portfolio/src/main/java/com/google/sps/servlets/DeleteServlet.java

+import javax.servlet.http.HttpServletResponse;
+
+/** Servlet that deletes data from the database. */
+@WebServlet("/delete-data")


I personally am not a fan of this, but Servlets do have a doDelete method: https://docs.oracle.com/javaee/7/api/javax/servlet/http/HttpServlet.html#doDelete-javax.servlet.http.HttpServletRequest-javax.servlet.http.HttpServletResponse-

In REST API for example methods are used like this:
doPost - to create a new resource
doPut - to update a resource
doGet - to get a resource
doDelete - to delete a resource

If you are interested instead of defining a new servlet to delete you could simply override doDelete() method.

That's interesting, I didn't know that, I'll keep that in mind for later.

fabielzuniga · 2020-07-14T23:18:29Z

portfolio/src/main/java/com/google/sps/servlets/DeleteServlet.java

+import javax.servlet.http.HttpServletResponse;
+
+/** Servlet that deletes data from the database. */
+@WebServlet("/delete-data")


Have you consider instead of using 'data' in your Servlet path using a more specific term? For example message.

I didn't consider that, I shall change it.

ccondit · 2020-07-15T01:42:40Z

portfolio/src/main/java/com/google/sps/servlets/DataServlet.java

 public class DataServlet extends HttpServlet {
  private final DatastoreService datastore = DatastoreServiceFactory.getDatastoreService();
-  private int maxNumComments;
+  private final int defaultMaxNumComments = 7;


nit - per style guide, constants should be UPPER_CASE.

Ah I didn't know that, I'll fix that.

ccondit · 2020-07-15T01:44:00Z

portfolio/src/main/java/com/google/sps/servlets/DataServlet.java

  private final DatastoreService datastore = DatastoreServiceFactory.getDatastoreService();
-  private int maxNumComments;
+  private final int defaultMaxNumComments = 7;
+  private int maxNumComments = defaultMaxNumComments;


i think you'll want to leave this variable declaration in the doGet method, otherwise you'll risk a race condition where user 1 gets user 2's max number of comments.

Ok, I'll move that.

ccondit · 2020-07-15T01:45:52Z

portfolio/src/main/java/com/google/sps/servlets/DataServlet.java

      System.err.println("Could not convert to int: " + maxNumCommentsString);
-      System.err.println("Using default value of 7.");
-      maxNumComments = 7;
+      System.err.println("Using default value of " + defaultMaxNumComments + ".");


No need to change anything now, but when the time comes for logging, we'll want to use Java.util.logging, per the app engine guide - just a heads up for the capstone.

Ok good to know.

kaveh096 · 2020-07-15T01:39:44Z

portfolio/src/main/java/com/google/sps/servlets/DataServlet.java

-import com.google.gson.Gson;
-
-/** Servlet that returns some example content. TODO: modify this file to handle comments data */
+/** Servlet that returns some example content.*/


nit: this javadoc now needs to be updated to reflect the new features.

Oh yea I forgot about that, I'll get to it.

kaveh096 · 2020-07-15T01:40:43Z

portfolio/src/main/java/com/google/sps/servlets/DataServlet.java

-  HashMap<String, String> messages = new HashMap<String, String>();
-  // ArrayList<String> messages = new ArrayList<String>();
+  private final DatastoreService datastore = DatastoreServiceFactory.getDatastoreService();
+  private final int defaultMaxNumComments = 7;


this could be a static constant:

Suggested change

private final int defaultMaxNumComments = 7;

private static final int DEFAULT_MAX_NUM_COMMENTS = 7;

kaveh096 · 2020-07-15T01:48:48Z

portfolio/src/main/java/com/google/sps/servlets/DataServlet.java

-  // ArrayList<String> messages = new ArrayList<String>();
+  private final DatastoreService datastore = DatastoreServiceFactory.getDatastoreService();
+  private final int defaultMaxNumComments = 7;
+  private int maxNumComments = defaultMaxNumComments;


Note that since you're defining this field at the class level, when a user changes the number, it'll affect all users using your website. If you only want these comment num changes to affect individual users you should use session-scoped attributes.

For example here you would call request.getSession().setAttribute("maxNumComments", maxNumComments);. And to read it back you call (int)request.getSession().getAttribute("maxNumComments");.

Oh ok, I'll do that then.

kaveh096 · 2020-07-15T01:51:14Z

portfolio/src/main/java/com/google/sps/servlets/DataServlet.java

+      System.err.println("Could not convert to int: " + maxNumCommentsString);
+      System.err.println("Using default value of " + defaultMaxNumComments + ".");


An alternative approach to format strings is to use the String.format java method. Here are some examples. It'll give you more control and in some cases it's less error prone.

Ok, I'll adjust the code to use String.format.

…ng.format

ccondit · 2020-07-15T18:47:46Z

portfolio/src/main/java/com/google/sps/servlets/DataServlet.java

-import com.google.gson.Gson;
-
-/** Servlet that returns some example content. TODO: modify this file to handle comments data */
+/** Servlet that adds/retrieves comments stored in a Datastore*/


nit - missing a space before the closing comment marker.

Suggested change

/** Servlet that adds/retrieves comments stored in a Datastore*/

/** Servlet that adds/retrieves comments stored in a Datastore */

Ok I wasn't aware, I'll fix that.

ccondit · 2020-07-15T23:27:29Z

portfolio/src/main/java/com/google/sps/servlets/DataServlet.java

-      System.err.println("Could not convert to int: " + maxNumCommentsString);
-      System.err.println("Using default value of " + defaultMaxNumComments + ".");
-      maxNumComments = defaultMaxNumComments;
+      System.err.format("Could not convert to int: %d%n", maxNumCommentsString);


there's a mismatch here since maxNumCommentsString is a string. s/%d/%s/.

Ok, I'll do that replacement.

ccondit · 2020-07-15T23:33:03Z

portfolio/src/main/java/com/google/sps/servlets/DataServlet.java

+    Entity commentEntity = new Entity("Comment");
+    commentEntity.setProperty("username", username);
+    commentEntity.setProperty("message", message);
+    commentEntity.setProperty("timestamp", System.currentTimeMillis());


One notable omission here is that the comment is never added to the datastore. Do you want to add that here?

That would be useful for the function of the website, so I'll add it.

ccondit · 2020-07-16T16:32:35Z

portfolio/src/main/java/com/google/sps/servlets/DataServlet.java

+    Query query = new Query("Comment").addSort("timestamp", SortDirection.DESCENDING);
+    PreparedQuery results = datastore.prepare(query);
+
+    int maxNumComments = (int)request.getSession().getAttribute("maxNumComments");


Just another note here - that the session won't have this attribute on your first page load.

no need to update now, but something to ponder for future work.

ihsan314 added 11 commits June 30, 2020 17:46

Changed Hello World statement to greet myself instead

dc201bc

Implemented feature to fetch greeting from /data and display it at to…

cb8faf0

…p of page

Removed unnecessary assignment and appending

eb4fb56

Merge branch 'last-steps-week3' of github.com:ihsan314/step into last…

cd2282e

…-steps-week3

Resolved merge conflicts

61d7a8e

Used datastore to display at most 7 comments on webpage

6a0d454

Used LinkedHashMap to keep messages ordered temporarily and added com…

1219285

…ment limiting input box

Implemented comments deletion feature

f056155

Resolved compilation errors

ec9fa7b

Reformatted java files with fix_java.sh

e8bd138

Removed extraneous /'s and comments

767409e

ihsan314 requested review from ccondit, fabielzuniga and kaveh096 July 14, 2020 01:21

ccondit suggested changes Jul 14, 2020

View reviewed changes

ihsan314 added 2 commits July 14, 2020 10:44

Alphabetized dependencies by groupId

bee7148

Used persistence layer to get keys

4569f51

ihsan314 requested a review from ccondit July 14, 2020 18:14

ccondit suggested changes Jul 14, 2020

View reviewed changes

Removed TODO, added missing import, privatized class members

dbc1fa3

ihsan314 requested a review from ccondit July 14, 2020 18:57

ccondit suggested changes Jul 14, 2020

View reviewed changes

fabielzuniga reviewed Jul 14, 2020

View reviewed changes

Fixed imports, added constant to hold default value

5fce08a

Renamed servlet path to delete-messages

5a5a47c

ihsan314 requested a review from ccondit July 15, 2020 01:26

ccondit suggested changes Jul 15, 2020

View reviewed changes

kaveh096 requested changes Jul 15, 2020

View reviewed changes

ihsan314 added 2 commits July 15, 2020 10:00

Moved maxNumComments declaration out of class scope, switched to Stri…

947663d

…ng.format

Updated javadoc

88ba334

ihsan314 requested review from ccondit and kaveh096 July 15, 2020 17:05

ccondit suggested changes Jul 15, 2020

View reviewed changes

Added missing space, replaced %d with %s, and added comment to Datastore

c0dfad2

ihsan314 requested a review from ccondit July 16, 2020 00:10

ccondit approved these changes Jul 16, 2020

View reviewed changes

kaveh096 approved these changes Jul 16, 2020

View reviewed changes

	DatastoreService datastore = DatastoreServiceFactory.getDatastoreService();
	private final DatastoreService datastore = DatastoreServiceFactory.getDatastoreService();

	private final int defaultMaxNumComments = 7;
	private static final int DEFAULT_MAX_NUM_COMMENTS = 7;

		System.err.println("Could not convert to int: " + maxNumCommentsString);
		System.err.println("Using default value of " + defaultMaxNumComments + ".");

	/** Servlet that adds/retrieves comments stored in a Datastore*/
	/** Servlet that adds/retrieves comments stored in a Datastore */

Implemented deletion of comments via Datastore #10

Are you sure you want to change the base?

Implemented deletion of comments via Datastore #10

Uh oh!

Conversation

ihsan314 commented Jul 14, 2020

Uh oh!

ccondit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ihsan314 commented Jul 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaveh096 Jul 15, 2020 •

edited

Loading