Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added Cloud SQL MySQL Servlet connectivity sample. #1231

Merged
merged 4 commits into from
Nov 6, 2018
Merged

Added Cloud SQL MySQL Servlet connectivity sample. #1231

merged 4 commits into from
Nov 6, 2018

Conversation

kurtisvg
Copy link
Contributor

This is the canonical sample for a new series of samples highlighting connection best practices for Cloud SQL users. It's a simple web app that allows voting between two different groups ("Tabs" vs "Spaces") and records the votes into a MySQL backend.

@saturnism PTAL

@kurtisvg kurtisvg added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 12, 2018
@kurtisvg kurtisvg requested a review from lesv October 12, 2018 16:55
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 12, 2018
Copy link
Contributor

@lesv lesv left a comment

Choose a reason for hiding this comment

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

Very nice -- my comments and questions aren't required, just consider them. A few places that might benefit from a bit of explanation and I pointed you to shutdown hooks. Only the password should be passed as a param, but I'm an advocate of setting things once and moving on. You can actually grab envVars and pass them to appengine-web.xml in maven.

Consider asking Thea about password's in EnvVar's. AWS is ok w/ it, so maybe we should be, but it's often frowned on around here. That said, I tried to make it so that if we set them in appengine-web.xml that we could pass them as arguments to mvn, which then used them to set the appengine-web.xml values.

1. If you haven't already, set up a Java Development Environment (including google-cloud-sdk and
maven utilities) by following the [java setup guide](https://cloud.google.com/java/docs/setup).

1. Create a 2nd Gen Cloud SQL Instance by following these
Copy link
Contributor

Choose a reason for hiding this comment

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

Also point them at the create a project doc as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

export GOOGLE_APPLICATION_CREDENTIALS=/path/to/service/account/key.json
export CLOUD_SQL_CONNECTION_NAME='<MY-PROJECT>:<INSTANCE-REGION>:<MY-DATABASE>'
export DB_USER='my-db-user'
export DB_PASS='my-db-pass'
Copy link
Contributor

Choose a reason for hiding this comment

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

This is often bad practice. Letting folks pass it in on the mvn / gradle command line is usually considered better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a warning to the sections with environment variables that they aren't secure with a link to KMS as a secure solution. I would prefer to avoid using mvn / gradle to configure properties because it's permanent and forces the user to rebuild rather than restart the application. We are planning on adding instructions for GKE/GCE as well, which could mean a lot of repeated steps if they have to rebuild to change instance name.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1; Definitely do not use Maven/Gradle configuration properties for runtime environment values. Those are not idiomatic.

Copy link
Contributor

Choose a reason for hiding this comment

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

I might argue that, but I'm ok with it.

cloud-sql/mysql/servlet/README.md Outdated Show resolved Hide resolved
cloud-sql/mysql/servlet/README.md Show resolved Hide resolved
cloud-sql/mysql/servlet/pom.xml Outdated Show resolved Hide resolved
import javax.servlet.annotation.WebListener;
import javax.sql.DataSource;

@WebListener
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to explain why we do this here.

DataSource pool = new HikariDataSource(config);
// [END cloud_sql_mysql_connection_pool]
return pool;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Might you want to create a Shutdown Hook to close any currently inactive instances and mark active ones to be closed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly in contextInitialized()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to skip this for now as well - we want to expand the sample to additional run-times soon and I want to avoid any run-time specific options.

lesv
lesv previously approved these changes Oct 12, 2018

Navigate towards `http://127.0.0.1:8080` to verify your application is running correctly.

## Google AppEngine-Standard
Copy link
Contributor

Choose a reason for hiding this comment

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

"Google AppEngine-Standard" or "Google App Engine Standard"? Want to make sure we are consistent with official naming/documentation.

pool.unwrap(HikariDataSource.class).close();
} catch (SQLException e) {
// Handle exception
System.out.println("Any error occurred while the application was shutting down: " + e);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use logger, JUL if we don't already use anything. Always log exceptions in its own argument (so that the stacktrace will show).

private static final String DB_PASS = System.getenv("DB_PASS");
private static final String DB_NAME = System.getenv("DB_NAME");

private DataSource mysqlConnectionPool() {
Copy link
Contributor

Choose a reason for hiding this comment

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

method name should indicate the action, e.g. createConnectionPool


// PreparedStatements can also be executed multiple times with different arguments. This can
// improve efficiency, and project a query from being vulnerable to an SQL injection.
PreparedStatement voteCtStmt = conn.prepareStatement(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd encourage verbose variable names; Ct is unclear. I believe it means voteCount?

<div class="section">
<div class="center">
<h4>
<% if(voteDiff != 0) { %>
Copy link
Contributor

Choose a reason for hiding this comment

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

would encourage using standard taglibs, <c:if ...> In fact, when using tags, we don't need so much <%= ... %> scripts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you provide more context on this? When should I use a taglib over the <= ... %>? Is <%= ... &> unsafe or inefficient?

Is the advised taglib to use the JSTL core library?

Copy link
Contributor

Choose a reason for hiding this comment

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

JSTL core library works. The taglibs are idiomatic in the JSP world, and you don't need to use request.getAttribute() to get the variables, since taglibs will be able to get the data directly from the attribute. It'll be cleaner, less code, and safer since it'll escape tags so you don't end up w/ script injections.

Just for example (and probably don't work as is since it's been a while since I had to write JSP :):

<c:choose>
<c:when test="${tabVoteCt == spaceVoteCt}">
  TABS and SPACES are tied
</c:when>
<c:when test="${tabVoteCt > spaceVoteCt}">
  TABS won
</c:when>
<c:when test="${tabVoteCt < spaceVoteCt}">
  SPACES won
</c:when>

</c:choose>

To iterate through the list:

<c:forEach items="${recentVotes}" var="vote">
  <li class="collection-item avatar">
    <c:choose>
      ...
    </c:choose>
  </li>
</c:forEach>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the background - I've made the adjustments.

);
createTableStatement.execute();
} catch (SQLException e) {
throw new Error(
Copy link
Contributor

Choose a reason for hiding this comment

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

do not throw Error. Also, don't use e.toString() or any of its string form when propagating exceptions.

It's actually better to simply propagate the SQLException upwards and declare the throw for this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the preferred way to stop the application if this context fails to initialize correctly?

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't use Error. Instead, throw a new RuntimeException w/ the cause. throw new RuntimeException("oops", e);

try (Connection conn = pool.getConnection()) {
PreparedStatement createTableStatement = conn.prepareStatement(
"CREATE TABLE IF NOT EXISTS votes ( "
+ "vote_id SERIAL NOT NULL, time_cast timestamp NOT NULL, canidate CHAR(6) NOT NULL, "
Copy link
Contributor

Choose a reason for hiding this comment

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

what does canidate mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

canidate is the subject of the vote (e.g. tabs or spaces).

Copy link
Contributor

Choose a reason for hiding this comment

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

is it candidate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry yes - my bad. Fixed.

// If something goes wrong, the application needs to react appropriately. This might mean
// getting a new connection and executing the query again, or it might mean redirecting the
// user to a different page to let them know something went wrong.
throw new ServletException("Unable to successfully connect to the database. Please check the "
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

IRL you'd probably log the event, maybe add to some monitor value as well before propagating.

team = team.toLowerCase();
}
Timestamp now = new Timestamp(new Date().getTime());
if (team == null || !team.equals("tabs") && !team.equals("spaces")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

to be clear on the operation precedence, if (team == null || ("tabs".equals(team) && !"spaces".equals(team))

// If something goes wrong, handle the error in this section. This might involve retrying or
// adjusting parameters depending on the situation.
// [START_EXCLUDE]
System.out.println("An SQL error occurred during executions: \n" + e.toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

same comments as above regarding exception handling.

DataSource pool = (DataSource) event.getServletContext().getAttribute("my-pool");
if (pool != null) {
try {
pool.unwrap(HikariDataSource.class).close();
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we know this is already an HikariDataSource, it should simply cast to it.

`HikariDataSource pool = (HikariDataSource) ...

voteCtStmt.setString(1, "spaces");
ResultSet spacesResult = voteCtStmt.executeQuery();
spacesResult.next(); // Move to the first result
voteCt = spacesResult.getInt(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be spaceCt?

</ul>
</div>
</body>
<footer>
Copy link
Contributor

Choose a reason for hiding this comment

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

footer tag should be part of the body tag. And if that's done, we should remove footer tag altogether and simply use script in the end.

<html>
  <head></head>
  <body>
    ...
    <script>...</script>
  </body>
</html>


public class Vote {

private String candiate;
Copy link
Contributor

Choose a reason for hiding this comment

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

candidate?

@kurtisvg kurtisvg removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Nov 5, 2018
@lesv lesv merged commit ffdb971 into GoogleCloudPlatform:master Nov 6, 2018
minherz pushed a commit that referenced this pull request Nov 16, 2022
…4.0 (#1231)

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [com.google.cloud:libraries-bom](https://cloud.google.com/java/docs/bom) ([source](https://togithub.com/GoogleCloudPlatform/cloud-opensource-java)) | `25.3.0` -> `25.4.0` | [![age](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/25.4.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/25.4.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/25.4.0/compatibility-slim/25.3.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/25.4.0/confidence-slim/25.3.0)](https://docs.renovatebot.com/merge-confidence/) |

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Renovate will not automatically rebase this PR, because other commits have been found.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. ⚠ **Warning**: custom changes will be lost.

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/java-asset).
minherz pushed a commit that referenced this pull request Nov 17, 2022
…4.0 (#1231)

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [com.google.cloud:libraries-bom](https://cloud.google.com/java/docs/bom) ([source](https://togithub.com/GoogleCloudPlatform/cloud-opensource-java)) | `25.3.0` -> `25.4.0` | [![age](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/25.4.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/25.4.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/25.4.0/compatibility-slim/25.3.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/25.4.0/confidence-slim/25.3.0)](https://docs.renovatebot.com/merge-confidence/) |

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Renovate will not automatically rebase this PR, because other commits have been found.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. ⚠ **Warning**: custom changes will be lost.

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/java-asset).
anguillanneuf pushed a commit that referenced this pull request Dec 5, 2022
…4.0 (#1231)

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [com.google.cloud:libraries-bom](https://cloud.google.com/java/docs/bom) ([source](https://togithub.com/GoogleCloudPlatform/cloud-opensource-java)) | `25.3.0` -> `25.4.0` | [![age](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/25.4.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/25.4.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/25.4.0/compatibility-slim/25.3.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/25.4.0/confidence-slim/25.3.0)](https://docs.renovatebot.com/merge-confidence/) |

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Renovate will not automatically rebase this PR, because other commits have been found.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. ⚠ **Warning**: custom changes will be lost.

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/java-asset).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants