Skip to content

fix(cloud-sql): postgres servlet reported a failed vote as success - #10382

Draft
rootkiller6788 wants to merge 3 commits into
GoogleCloudPlatform:mainfrom
rootkiller6788:fix-cloud-sql-vote-error-response
Draft

rootkiller6788 wants to merge 3 commits into
GoogleCloudPlatform:mainfrom
rootkiller6788:fix-cloud-sql-vote-error-response

Conversation

@rootkiller6788

Copy link
Copy Markdown

The three Cloud SQL servlet samples (mysql, postgres, sqlserver) have a bit of a trap in doPost: when the vote insert throws SQLException, the catch block sets a 500 and writes the "Unable to successfully cast vote" message, but it never returns. Execution keeps going and hits the setStatus(200) plus the "Vote successfully cast" line at the end of the method. So the status gets overwritten to 200 and the body ends up saying both that it failed and that it succeeded - the vote never made it into the database but the page says it did.

I added the missing return in all three. It's inside the START_EXCLUDE block, so the published code snippets on cloud.google.com don't change.

For tests I didn't want anything that needs a real database, so each module gets a test that mocks a DataSource whose getConnection() throws SQLException, then checks doPost answers 500 and that the body does not contain "Vote successfully cast". I verified they actually catch the bug by removing the return again - they fail with expected: 500 but was: 200. With the fix in place all three pass (Tests run: 1, Failures: 0, Errors: 0).

The catch block writes a 500 and the error text, but without a return
the code falls through to setStatus(200) and the "vote successfully
cast" message, so a failed insert looked like a success to the user.
Added the missing return. The line sits inside the START_EXCLUDE
region, so the published snippet is unchanged.
Both have the identical fall-through after the error response.
…vlets

Mocks a DataSource that throws SQLException so doPost takes the catch
branch, and asserts the status is 500 and the body does not claim the
vote was cast. Reverting the return makes these fail with 200.
@product-auto-label product-auto-label Bot added api: cloudsql Issues related to the Cloud SQL for MySQL API. samples Issues that are directly related to samples. labels Nov 12, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request fixes a bug in the IndexServlet implementations for MySQL, PostgreSQL, and SQL Server where a failed vote could be reported as a success due to a missing return statement in the error-handling block. It also introduces corresponding unit tests to verify this behavior. The review feedback suggests simplifying the newly added tests by removing unused imports and using Mockito's 'verify' method instead of manually tracking the HTTP status code with 'doAnswer' and 'AtomicInteger'.

Comment on lines +20 to +28
import static org.mockito.ArgumentMatchers.anyInt;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import java.io.PrintWriter;
import java.io.StringWriter;
import java.sql.SQLException;
import java.util.concurrent.atomic.AtomicInteger;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Remove unused imports (anyInt, doAnswer, AtomicInteger) and statically import verify to simplify the Mockito verification in the test.

Suggested change
import static org.mockito.ArgumentMatchers.anyInt;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
import java.io.PrintWriter;
import java.io.StringWriter;
import java.sql.SQLException;
import java.util.concurrent.atomic.AtomicInteger;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import java.io.PrintWriter;
import java.io.StringWriter;
import java.sql.SQLException;

Comment on lines +52 to +68
AtomicInteger status = new AtomicInteger();
HttpServletResponse response = mock(HttpServletResponse.class);
when(response.getWriter()).thenReturn(writer);
doAnswer(
invocation -> {
status.set(invocation.getArgument(0));
return null;
})
.when(response)
.setStatus(anyInt());

new IndexServlet().doPost(request, response);
writer.flush();

assertThat(status.get()).isEqualTo(500);
assertThat(body.toString()).contains("Unable to successfully cast vote");
assertThat(body.toString()).doesNotContain("Vote successfully cast");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Simplify the verification of the HTTP status code by using Mockito's verify instead of stubbing setStatus with doAnswer and tracking it with an AtomicInteger. This makes the test cleaner and more idiomatic.

    HttpServletResponse response = mock(HttpServletResponse.class);
    when(response.getWriter()).thenReturn(writer);

    new IndexServlet().doPost(request, response);
    writer.flush();

    verify(response).setStatus(500);
    assertThat(body.toString()).contains("Unable to successfully cast vote");
    assertThat(body.toString()).doesNotContain("Vote successfully cast");

Comment on lines +20 to +28
import static org.mockito.ArgumentMatchers.anyInt;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import java.io.PrintWriter;
import java.io.StringWriter;
import java.sql.SQLException;
import java.util.concurrent.atomic.AtomicInteger;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Remove unused imports (anyInt, doAnswer, AtomicInteger) and statically import verify to simplify the Mockito verification in the test.

Suggested change
import static org.mockito.ArgumentMatchers.anyInt;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
import java.io.PrintWriter;
import java.io.StringWriter;
import java.sql.SQLException;
import java.util.concurrent.atomic.AtomicInteger;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import java.io.PrintWriter;
import java.io.StringWriter;
import java.sql.SQLException;

Comment on lines +52 to +68
AtomicInteger status = new AtomicInteger();
HttpServletResponse response = mock(HttpServletResponse.class);
when(response.getWriter()).thenReturn(writer);
doAnswer(
invocation -> {
status.set(invocation.getArgument(0));
return null;
})
.when(response)
.setStatus(anyInt());

new IndexServlet().doPost(request, response);
writer.flush();

assertThat(status.get()).isEqualTo(500);
assertThat(body.toString()).contains("Unable to successfully cast vote");
assertThat(body.toString()).doesNotContain("Vote successfully cast");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Simplify the verification of the HTTP status code by using Mockito's verify instead of stubbing setStatus with doAnswer and tracking it with an AtomicInteger. This makes the test cleaner and more idiomatic.

    HttpServletResponse response = mock(HttpServletResponse.class);
    when(response.getWriter()).thenReturn(writer);

    new IndexServlet().doPost(request, response);
    writer.flush();

    verify(response).setStatus(500);
    assertThat(body.toString()).contains("Unable to successfully cast vote");
    assertThat(body.toString()).doesNotContain("Vote successfully cast");

Comment on lines +20 to +28
import static org.mockito.ArgumentMatchers.anyInt;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import java.io.PrintWriter;
import java.io.StringWriter;
import java.sql.SQLException;
import java.util.concurrent.atomic.AtomicInteger;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Remove unused imports (anyInt, doAnswer, AtomicInteger) and statically import verify to simplify the Mockito verification in the test.

Suggested change
import static org.mockito.ArgumentMatchers.anyInt;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
import java.io.PrintWriter;
import java.io.StringWriter;
import java.sql.SQLException;
import java.util.concurrent.atomic.AtomicInteger;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import java.io.PrintWriter;
import java.io.StringWriter;
import java.sql.SQLException;

Comment on lines +52 to +68
AtomicInteger status = new AtomicInteger();
HttpServletResponse response = mock(HttpServletResponse.class);
when(response.getWriter()).thenReturn(writer);
doAnswer(
invocation -> {
status.set(invocation.getArgument(0));
return null;
})
.when(response)
.setStatus(anyInt());

new IndexServlet().doPost(request, response);
writer.flush();

assertThat(status.get()).isEqualTo(500);
assertThat(body.toString()).contains("Unable to successfully cast vote");
assertThat(body.toString()).doesNotContain("Vote successfully cast");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Simplify the verification of the HTTP status code by using Mockito's verify instead of stubbing setStatus with doAnswer and tracking it with an AtomicInteger. This makes the test cleaner and more idiomatic.

    HttpServletResponse response = mock(HttpServletResponse.class);
    when(response.getWriter()).thenReturn(writer);

    new IndexServlet().doPost(request, response);
    writer.flush();

    verify(response).setStatus(500);
    assertThat(body.toString()).contains("Unable to successfully cast vote");
    assertThat(body.toString()).doesNotContain("Vote successfully cast");

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: cloudsql Issues related to the Cloud SQL for MySQL API. samples Issues that are directly related to samples.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant