Skip to content

Adding a new test for beginRequest()/endRequest() #746

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.

Sign up for GitHub

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

Merged
ulvii merged 3 commits into from
Jul 12, 2018
Merged

Adding a new test for beginRequest()/endRequest() #746

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ffd8061
Add | Adding a new test to notify the developers to consider beginReq…
ulvii Jul 12, 2018
c5e54ff
Fix | rewording some comments
ulvii Jul 12, 2018
fe9b9f8
Fix | Fix typo
ulvii Jul 12, 2018
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
76 changes: 76 additions & 0 deletions src/test/java/com/microsoft/sqlserver/jdbc/connection/RequestBoundaryMethodsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,15 @@
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;

import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import java.sql.CallableStatement;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.CountDownLatch;

import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -356,6 +360,26 @@ public void run() {
}
}

/**
* This is not really a test. The goal is to make the build fail if there are new public non-static methods in
* SQLServerConnection and notify the developer to decide whether it needs to be handled by
* beginRequest()/endRequest().
*
* To fix the failure, you first need to check if the new method can modify connection local state after connection
* has been created. (See beginRequestInternal()/endRequestInternal() in SQLServerConnection). If yes, make sure it
* is handled by beginRequest()/endRequest() and then add it to <code>verifiedMethodNames</code>. If not, just
* adding the new method's name to the same list of verified methods is enough.
*/
@Test
public void testNewMethods() {
Method[] methods = SQLServerConnection.class.getDeclaredMethods();
for (Method method : methods) {
assertTrue(isVerified(method),
"A failure is expected if you are adding a new public non-static method to SQLServerConnection."
+ " See the test for instructions on how to fix the failure. ");
}
}

private SQLServerConnection connect() throws SQLException {
SQLServerConnection connection = null;
try {
Expand Down Expand Up @@ -412,4 +436,56 @@ private void compareValuesAgainstConnection(SQLServerConnection con, boolean aut
private void generateWarning(SQLServerConnection con) throws SQLException {
con.setClientInfo("name", "value");
}

private boolean isVerified(Method method) {
return (!Modifier.isPublic(method.getModifiers()) || Modifier.isStatic(method.getModifiers())
|| method.getName().startsWith("get") || getVerifiedMethodNames().contains(method.getName()));
Copy link
Contributor

@peterbae peterbae Jul 12, 2018

Choose a reason for hiding this comment

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

why do we allow any method that starts with "get" to be verified? if I added a new connection property and added a public getter for it, isn't it something that the boundary request method needs to handle? (it would likely get caught by the setter that comes with it, though)

Copy link
Member

@rene-ye rene-ye Jul 12, 2018

Choose a reason for hiding this comment

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

The point of beginRequest() is to "memorize" the connection state and its properties. Getting a connection property doesn't affect the properties so it shouldn't matter. On the other hand, it is kind of hacky and I wouldn't be opposed to triggering this error on all newly added public methods, just to be safe.

Copy link
Contributor Author

@ulvii ulvii Jul 12, 2018

Choose a reason for hiding this comment

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

Well, a getter won't change the connection local state. And I didn't want to hard-code all of them in the test.

}

private List<String> getVerifiedMethodNames() {
List<String> verifiedMethodNames = new ArrayList<String>();

verifiedMethodNames.add("toString");
verifiedMethodNames.add("setReadOnly");
verifiedMethodNames.add("close");
verifiedMethodNames.add("unwrap");
verifiedMethodNames.add("isReadOnly");
verifiedMethodNames.add("abort");
verifiedMethodNames.add("isValid");
verifiedMethodNames.add("setServerPreparedStatementDiscardThreshold");
verifiedMethodNames.add("setEnablePrepareOnFirstPreparedStatementCall");
verifiedMethodNames.add("isClosed");
verifiedMethodNames.add("setSendTimeAsDatetime");
verifiedMethodNames.add("setStatementPoolingCacheSize");
verifiedMethodNames.add("setDisableStatementPooling");
verifiedMethodNames.add("setTransactionIsolation");
verifiedMethodNames.add("setUseBulkCopyForBatchInsert");
verifiedMethodNames.add("commit");
verifiedMethodNames.add("clearWarnings");
verifiedMethodNames.add("prepareStatement");
verifiedMethodNames.add("prepareCall");
verifiedMethodNames.add("setCatalog");
verifiedMethodNames.add("setAutoCommit");
verifiedMethodNames.add("createStatement");
verifiedMethodNames.add("setClientInfo");
verifiedMethodNames.add("setNetworkTimeout");
verifiedMethodNames.add("setHoldability");
verifiedMethodNames.add("closeUnreferencedPreparedStatementHandles");
verifiedMethodNames.add("isStatementPoolingEnabled");
verifiedMethodNames.add("rollback");
verifiedMethodNames.add("releaseSavepoint");
verifiedMethodNames.add("createStruct");
verifiedMethodNames.add("createSQLXML");
verifiedMethodNames.add("setSchema");
verifiedMethodNames.add("createNClob");
verifiedMethodNames.add("nativeSQL");
verifiedMethodNames.add("setSavepoint");
verifiedMethodNames.add("createClob");
verifiedMethodNames.add("createBlob");
verifiedMethodNames.add("isWrapperFor");
verifiedMethodNames.add("setTypeMap");
verifiedMethodNames.add("createArrayOf");

return verifiedMethodNames;
}
}