Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// Copyright 2019 Google LLC
//
// 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
//
// https://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 com.google.sps.servlets;

import com.google.appengine.api.blobstore.BlobstoreService;
import com.google.appengine.api.blobstore.BlobstoreServiceFactory;
import java.io.IOException;
import javax.servlet.annotation.WebServlet;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

/**
* When the fetch() function requests the /blobstore-upload-url URL, the content of the response is
* the URL that allows a user to upload a file to Blobstore. If this sounds confusing, try running a
* dev server and navigating to /blobstore-upload-url to see the Blobstore URL.
*/
@WebServlet("/blob-url")
public class BlobUrlServlet extends HttpServlet {
@Override
public void doGet(HttpServletRequest request, HttpServletResponse response) throws IOException {

BlobstoreService blobstoreService = BlobstoreServiceFactory.getBlobstoreService();
String uploadUrl = blobstoreService.createUploadUrl("/file-handler");
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please put "/file-handler" into a shared constant.


response.setContentType("text/html");
response.getWriter().println(uploadUrl);
}
}
16 changes: 8 additions & 8 deletions portfolio/src/main/java/com/google/sps/servlets/DataServlet.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@
import com.google.gson.Gson;
import com.google.sps.data.Post;
import java.io.IOException;
import java.util.*;
import java.util.ArrayList;
import java.util.List;
import javax.servlet.annotation.WebServlet;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
Expand All @@ -32,7 +33,7 @@
/** Servlet that returns some example content. TODO: modify this file to handle comments data */
@WebServlet("/Comments")
public class DataServlet extends HttpServlet {
private int max=5;
private int maxNumberOfComments=5;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since maxNumberOfComments can be set concurrently by different requests, all access to this field should be synchronized. We can discuss how to do that offline.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused as to why this isn't a constant. Either it's configurable (possibly with a constant default) in which case it should be a local variable or it isn't and should be final. Seeing this random shared mutable value is a big code smell.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: keep final values separate from non-final values to draw attention to the fact that they have different mutability. In this case, I would put maxNumberOfComments below all private static final fields and add a newline to separate it.

private static final String COMMENTS_ENTITY_NAME = "Comments";
private static final String TIMESTAMP_PROPERTY_KEY = "timestamp";
private static final String COMMENT_PROPERTY_KEY = "comment";
Expand All @@ -46,7 +47,7 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) thro
PreparedQuery results = datastore.prepare(query);

List<Post> comments = new ArrayList<>();
FetchOptions fetchOptions = FetchOptions.Builder.withLimit(max);
FetchOptions fetchOptions = FetchOptions.Builder.withLimit(maxNumberOfComments);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This read needs to be synchronized.

for (Entity entity : results.asIterable(fetchOptions)) {
long id = entity.getKey().getId();
String comment = (String) entity.getProperty(COMMENT_PROPERTY_KEY);
Expand All @@ -68,7 +69,7 @@ public void doPost(HttpServletRequest request, HttpServletResponse response) thr
String userComment = request.getParameter(COMMENT_PARAMETER_NAME);
long timestamp = System.currentTimeMillis();
String maxString = request.getParameter(MAX_COMMENTS_PARAMETER_NAME);
if (userComment != null) {
if (userComment != null ) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where's my formatter meme?

Entity taskEntity = new Entity(COMMENTS_ENTITY_NAME);
taskEntity.setProperty(COMMENT_PROPERTY_KEY, userComment);
taskEntity.setProperty(TIMESTAMP_PROPERTY_KEY, timestamp);
Expand All @@ -77,11 +78,10 @@ public void doPost(HttpServletRequest request, HttpServletResponse response) thr
datastore.put(taskEntity);
} else if (maxString != null) {
try {
max = Integer.parseInt(maxString);

maxNumberOfComments = Integer.parseInt(maxString);
} catch (NumberFormatException e) {
System.out.println("Could not convert to int: " + maxString);
max = 1;
System.out.println("Could not convert to int|" + maxString+"|");
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use String.format instead of these string concat chains.

System.out.println(String.format("Could not convert to int: \"%s\".", maxString);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moreover I'm going to cut you a bug to switch to a real logger.

maxNumberOfComments = 1;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to return an error code to the caller (probably SC_BAD_REQUEST).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This write should be synchronized.

}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
package com.google.sps.servlets;

import com.google.appengine.api.blobstore.BlobInfo;
import com.google.appengine.api.blobstore.BlobInfoFactory;
import com.google.appengine.api.blobstore.BlobKey;
import com.google.appengine.api.blobstore.BlobstoreService;
import com.google.appengine.api.blobstore.BlobstoreServiceFactory;
import com.google.appengine.api.images.ImagesService;
import com.google.appengine.api.images.ImagesServiceFactory;
import com.google.appengine.api.images.ServingUrlOptions;
import java.io.IOException;
import java.io.PrintWriter;
import java.net.MalformedURLException;
import java.net.URL;
import java.util.List;
import java.util.Map;
import javax.servlet.annotation.WebServlet;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

/**
* When the user submits the form, Blobstore processes the file upload and then forwards the request
* to this servlet. This servlet can then process the request using the file URL we get from
* Blobstore.
*/
@WebServlet("/file-handler")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if you could reference this path from a static constant, but I don't believe that's possible with annotations. This is another benefit of using a Guice filter to resolve serving paths. (No need to do this here, but consider adopting this in a future change.)

public class FileHandlerServlet extends HttpServlet {
@Override
public void doPost(HttpServletRequest request, HttpServletResponse response) throws IOException {

// Get the URL of the image that the user uploaded to Blobstore.
String imageUrl = getUploadedFileUrl(request, "images");
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that imageUrl may be null, which you should be able to handle. For example, you probably don't want to add an anchor tag with href="null". Do you want to set different HTTP response codes in this case?


//Will change when walkthrough is finished
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you tho?

System.out.println(imageUrl);
PrintWriter out = response.getWriter();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/formattermeme

out.println("<p>Here's the image you uploaded:</p>");
out.println("<a href=\"" + imageUrl + "\">");
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's risky to manually escape values within raw HTML like this. I'll try to figure out if there's a safer way to render this (e.g., using JSPs).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're using JSPs now? I'm ded.

out.println("<img src=\"" + imageUrl + "\" />");
out.println("</a>");


}
/** Returns a URL that points to the uploaded file, or null if the user didn't upload a file. */
private String getUploadedFileUrl(HttpServletRequest request, String formInputElementName) {
BlobstoreService blobstoreService = BlobstoreServiceFactory.getBlobstoreService();
Map<String, List<BlobKey>> blobs = blobstoreService.getUploads(request);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What fresh hell is? This API takes an HttpServletRequest?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also you don't need to actually do anything for this. Just commenting on how much I hate this API.

List<BlobKey> blobKeys = blobs.get("images");
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this a constant instead of inlining it if it's a keyword.


// User submitted form without selecting a file, so we can't get a URL. (dev server)
if (blobKeys == null || blobKeys.isEmpty()) {
return null;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're returning null values as part of the API, you should add the @Nullable annotation tag to the method to indicate this so callers (and code linters) can see this. Alternatively, Return a java.util.Optional<String>.

}

// Our form only contains a single file input, so get the first index.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. The form only contains a single file input, but what if the user hijacked the form or manually submitted a request to submit multiple files? We probably don't need to worry about that here, but we may want to consider that during upload.

BlobKey blobKey = blobKeys.get(0);

// User submitted form without selecting a file, so we can't get a URL. (live server)
BlobInfo blobInfo = new BlobInfoFactory().loadBlobInfo(blobKey);
if (blobInfo.getSize() == 0) {
blobstoreService.delete(blobKey);
System.out.println("Nothing here");
return null;
}


// Use ImagesService to get a URL that points to the uploaded file.
ImagesService imagesService = ImagesServiceFactory.getImagesService();
ServingUrlOptions options = ServingUrlOptions.Builder.withBlobKey(blobKey);


try {
URL url = new URL(imagesService.getServingUrl(options));
return url.getPath();
} catch (MalformedURLException e) {
return imagesService.getServingUrl(options);
}
}
}
23 changes: 14 additions & 9 deletions portfolio/src/main/webapp/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -7,31 +7,36 @@
<script src="script.js"></script>

</head>
<body onload="getComments()">
<div id="content">
<body onload="getComments()" >
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you reverse format this file?

<div id="content" >
<h1>Charles Lovett's Portfolio</h1>

<form action="/Comments" method="POST">


<p>Post a comment</p>
<input type="text" name="comment">
<input type="text" name="comment" >
<br/><br/>
<input type="submit" />

<input type="submit" />
</form>

<form action="/Comments" method="POST">
<p>Maxium comments</p>
<input type="number" name="maxnum">
<input type="number" name="maxnum" >
<br/><br/>
<input type="submit" />
<input type="submit" />
</form>

<br/><br/>

<ul id="fetch-comment"></ul>

<form id="uploaded-image" method="POST" enctype="multipart/form-data" >
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need to set an action URL here? (I don't know the answer to this, but I suspect that by default, this just sends a post request to the index page URL.)

<br/><br/>
<p>Upload Image</p>
<input type="file" name="images">
<br/><br/>
<input type="submit" />
</form>

<p>This Portfolio contains my life growing up and during college. Hope you enjoy learning a little bit about me.</p>
<p>Real Quick if you want to know some fun facts about me press the button below:</p>
<button onclick="addRandomFunFact()">Fun Facts</button>
Expand Down
29 changes: 20 additions & 9 deletions portfolio/src/main/webapp/script.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,18 @@ function addRandomFunFact() {
}

function getComments() {


fetch('/Comments').then(response => response.json()).then((com) => {
const commentList = document.getElementById('fetch-comment');
com.forEach((comment) => {
commentList.appendChild(createTaskElement(comment));
commentList.appendChild(createCommentElement(comment));
})
});
getImage();
}

function createTaskElement(comment) {
function createCommentElement(comment) {
const taskElement = document.createElement('li');

taskElement.className = 'comment';
Expand All @@ -50,7 +53,7 @@ function createTaskElement(comment) {
const deleteButtonElement = document.createElement('button');
deleteButtonElement.innerText = 'Delete';
deleteButtonElement.addEventListener('click', () => {
deleteTask(comment);
deleteComment(comment);

// Remove the task from the DOM.
taskElement.remove();
Expand All @@ -62,13 +65,21 @@ function createTaskElement(comment) {
}


function createListElement(text) {
const liElement = document.createElement('li');
liElement.innerText = text;
return liElement;
}
function deleteTask(task) {
function deleteComment(task) {
const params = new URLSearchParams();
params.append('id', task.id);
fetch('/delete-data', {method: 'POST', body: params});
}

function getImage(){

fetch('/blob-url')
.then((response) => {
return response.text();
})
.then((imageUploadUrl) => {
const messageForm = document.getElementById('uploaded-image');
messageForm.action = imageUploadUrl;
// messageForm.classList.remove('hidden');
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

//

});
}