WIP: Jdbc session map #8378

raju249 · 2020-06-01T09:39:46Z

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Fixes #8172

Motivation and Context

Types of changes

[-] Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
[-] Breaking change (fix or feature that would cause existing functionality to change)

Checklist

[-] I have read the contributing document.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

shs96c

Welcome aboard :) This is a great start. I've added some comments, but we're also chatting on Slack, so you can ask things there too.

shs96c · 2020-06-01T09:42:58Z

java/server/src/org/openqa/selenium/grid/sessionmap/config/JdbcSessionMapFlags.java

+// specific language governing permissions and limitations
+// under the License.
+
+package org.openqa.selenium.grid.sessionmap.config;


Please move this to the jdbc package you've just created.

shs96c · 2020-06-01T09:43:49Z

java/server/src/org/openqa/selenium/grid/sessionmap/config/SessionMapOptions.java

@@ -40,6 +43,18 @@ public SessionMapOptions(Config config) {
    this.config = config;
  }

+  public Connection getJdbcConnection() throws SQLException {


Given that this is only used in the JdbcSessionMap, it would be better to either move this into a helper method in the JdbcSessionMap, or create a jdbc.JdbcOptions class.

java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/BUILD.bazel

shs96c · 2020-06-01T09:45:54Z

java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcBackedSessionMap.java

+public class JdbcBackedSessionMap extends SessionMap implements Closeable {
+
+  private static final Json JSON = new Json();
+  private String tableName;


java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcBackedSessionMap.java

shs96c · 2020-06-01T09:46:47Z

java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcBackedSessionMap.java

+    try {
+      connection = sessionMapOptions.getJdbcConnection();
+    } catch (SQLException e) {
+      // TODO: Handle SQLException


You can throw a ConfigException here.

shs96c · 2020-06-01T09:47:33Z

java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcBackedSessionMap.java

+      Statement insertStatement = connection.createStatement();
+      // TODO: Insert Call
+    } catch (SQLException e) {
+      // TODO: Handle SQLException


You can create a custom JdbcException which extends RuntimeException or WebDriverException if you want to.

shs96c · 2020-06-01T09:48:26Z

java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcBackedSessionMap.java

+    try {
+      connection.close();
+    } catch (SQLException e) {
+      e.printStackTrace();


Log using a java.util.logging.Logger at the warning level and swallow. There's nothing sensible to be done on the way out.

shs96c

This is a great start. It's shaping up well.

shs96c · 2020-06-03T14:16:58Z

java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcBackedSessionMap.java

+    Capabilities caps = null;
+
+    try {
+      ResultSet sessions = readSessionStatement(id).executeQuery();


Use try-with-resources so that the ResultSet will be closed properly.

shs96c · 2020-06-03T14:20:58Z

java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcBackedSessionMap.java

+    try {
+      ResultSet sessions = readSessionStatement(id).executeQuery();
+
+      while(sessions.next()) {


How many results are you expecting? Perhaps use a LIMIT 1 to ensure only one row at most is sent back?

With the limit in place, you either have 0 or 1 columns. You should be able to replace the while with an if, which may simplify this a little, as you can fast-path out by throwing the NoSuchSessionException if there's no session found.

shs96c · 2020-06-03T14:22:01Z

java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcBackedSessionMap.java

+        try {
+          uri = new URI(rawUri);
+        }
+        catch (URISyntaxException e) {


Java style is to put the closing brace and the catch statement on the same line } catch (URISyntaxException e) {

shs96c · 2020-06-03T14:27:34Z

java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcBackedSessionMap.java

+
+      sessions.close();
+
+      if (uri == null) {


Those can't happen. If rawUri was null, then new URI would have thrown.

shs96c · 2020-06-03T14:29:02Z

java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcBackedSessionMap.java

+  }
+
+  private PreparedStatement insertSessionStatement(Session session) throws SQLException {
+    PreparedStatement insertStatement = connection.prepareStatement("insert into " + tableName + " values(?,?)");


How do you know that tableName is not, itself, an SQL injection attack?

shs96c · 2020-06-03T14:32:48Z

java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcBackedSessionMap.java

+  private String sessionUri(SessionId sessionId) {
+    Require.nonNull("Session ID", sessionId);
+
+    return "session:" + sessionId.toString() + ":uri";


The session id should be a UUID (it will be unique, even if it's not formatted correctly)

shs96c · 2020-06-03T14:33:15Z

java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcBackedSessionMap.java

+
+  private PreparedStatement getDeleteSqlForSession(SessionId sessionId) throws SQLException{
+    PreparedStatement deleteSessionStatement = connection.prepareStatement("delete from " + tableName + " where " + sessionIdCol + " like ?");
+    deleteSessionStatement.setString(1, sessionUri(sessionId).concat("%"));


A session id is unique and complete. You don't need to wildcard it.

shs96c · 2020-06-03T14:34:01Z

java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcSessionMapFlags.java

+  @Parameter(
+      names = "--jdbc-sessionid-column",
+      description = "Column name where session id will be stored")
+  @ConfigValue(section = "sessions", name = "jdbc-sessionid-column", example = "session_id")


Make life easy for yourself. Don't let this be configurable.

shs96c · 2020-06-03T14:34:59Z

java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcSessionMapFlags.java

+  @Parameter(
+      names = "--jdbc-table",
+      description = "Name of the table in database to store sessions in.")
+  @ConfigValue(section = "sessions", name = "jdbc-table", example = "myP@ssw%d")


I'd be very tempted to not make this configurable. If someone wants to use this, they'll just need to create the table needed.

shs96c · 2020-06-03T14:35:12Z

java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcSessionMapFlags.java

+      names = "--jdbc-capabilities-column",
+      description = "Column name where session id will be stored")
+  @ConfigValue(section = "sessions", name = "jdbc-capabilities-column", example = "capabilities")
+  private String sessionCapsColumn;


Again, don't let this be configurable.

shs96c

Just a couple of nits, and we're there. This looks great :) Thank you!

shs96c · 2020-06-04T10:50:26Z

java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcBackedSessionMap.java

+    try {
+      ResultSet sessions = readSessionStatement(id).executeQuery();
+
+      while(sessions.next()) {


With the limit in place, you either have 0 or 1 columns. You should be able to replace the while with an if, which may simplify this a little, as you can fast-path out by throwing the NoSuchSessionException if there's no session found.

shs96c · 2020-06-04T10:52:45Z

java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcBackedSessionMap.java

+    Tracer tracer = new LoggingOptions(config).getTracer();
+    JdbcSessionMapOptions sessionMapOptions = new JdbcSessionMapOptions(config);
+    String tableName = sessionMapOptions.getJdbcTableName();
+    String sessionIdColName = sessionMapOptions.getJdbcSessionIdColName();


All of these should be hard-coded now, so there's no need to look them up in the options :)

shs96c · 2020-06-04T10:53:19Z

java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcBackedSessionMap.java

+  private static final Logger LOG = Logger.getLogger(JdbcBackedSessionMap.class.getName());
+  private final Connection connection;
+  private final String  tableName;
+  private final String sessionIdCol;


Since we're hard-coding the table names, make these constants or just use magic strings....

shs96c · 2020-06-04T10:55:07Z

java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcSessionMapOptions.java

+
+  private static final String SESSIONS_SECTION = "sessions";
+  private static final Logger LOG = Logger.getLogger(JdbcSessionMapOptions.class.getName());
+  private static final String  tableName = "sessions_map";


It's safe to move these constants into the JdbcSessionMap itself.

shs96c

One more round, then let's land this and iterate :)

shs96c · 2020-06-05T09:15:13Z

java/server/src/org/openqa/selenium/grid/BUILD.bazel

@@ -73,6 +73,7 @@ java_export(
        ":base-command",
        "//java/server/src/org/openqa/selenium/cli",
        "//java/server/src/org/openqa/selenium/grid/config",
+        "//java/server/src/org/openqa/selenium/grid/sessionmap/jdbc",


Is this deliberate? Or a debugging aid?

After speaking on Slack, it seems that this is here to help bootstrap things so that new flags appear on the classpath.

shs96c · 2020-06-05T09:16:01Z

java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcBackedSessionMap.java

+
+  private static final Json JSON = new Json();
+  private static final Logger LOG = Logger.getLogger(JdbcBackedSessionMap.class.getName());
+  private final EventBus bus;


Nit: place the instance fields below the constants.

shs96c · 2020-06-05T09:46:09Z

java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcBackedSessionMap.java

+    try {
+      connection = sessionMapOptions.getJdbcConnection();
+    } catch (SQLException e) {
+      throw new ConfigException(e.toString());


Maybe add the underlying exception too? new ConfigException(e)

shs96c · 2020-06-05T09:47:50Z

java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcBackedSessionMap.java

+    String rawUri = null;
+
+    try (ResultSet sessions = readSessionStatement(id).executeQuery()){
+      if (sessions.next()) {


Prefer multiple return points and less indenting:

if (!session.next()) { throw new NoSuchSessionException("Unable to find..."); }

shs96c · 2020-06-05T09:49:33Z

java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcBackedSessionMap.java

+  }
+
+  private PreparedStatement insertSessionStatement(Session session) throws SQLException {
+    PreparedStatement insertStatement = connection.prepareStatement("insert into " + TABLE_NAME


Nit: Newline before the arguments begin. That avoids excessive line length and makes the code a bit easier to read.

shs96c · 2020-06-05T09:50:16Z

java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcBackedSessionMap.java

+  }
+
+  private PreparedStatement insertSessionStatement(Session session) throws SQLException {
+    PreparedStatement insertStatement = connection.prepareStatement("insert into " + TABLE_NAME


Consider using String.format instead of concatenating strings. When there's just one, it's fine, but it starts to get hard to read after a while.

shs96c · 2020-06-05T09:50:48Z

java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcBackedSessionMap.java

+  }
+
+  private PreparedStatement readSessionStatement(SessionId sessionId) throws SQLException {
+    PreparedStatement getSessionsStatement = connection.prepareStatement("select * from " + TABLE_NAME


Nit: prefer String.format for readability

shs96c · 2020-06-05T09:53:14Z

java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcSessionMapFlags.java

+  @Parameter(
+      names = "--jdbc-password",
+      description = "Password for the user to make a JDBC connection")
+  @ConfigValue(section = "sessions", name = "jdbc-password", example = "myP@ssw%d")


Humorous aside: We tend to use hunter2 as the password of choice: https://knowyourmeme.com/memes/hunter2 https://w3c.github.io/webdriver/#example-7

shs96c · 2020-06-05T09:53:29Z

java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcSessionMapOptions.java

+  private final Config config;
+
+  public JdbcSessionMapOptions(Config config) {
+    this.config = config;


Reuire.nonNull

shs96c

LGTM. Congratulations on landing this. It's a lovely feature!

shs96c · 2020-06-05T10:44:19Z

java/server/src/org/openqa/selenium/grid/BUILD.bazel

@@ -73,6 +73,7 @@ java_export(
        ":base-command",
        "//java/server/src/org/openqa/selenium/cli",
        "//java/server/src/org/openqa/selenium/grid/config",
+        "//java/server/src/org/openqa/selenium/grid/sessionmap/jdbc",


After speaking on Slack, it seems that this is here to help bootstrap things so that new flags appear on the classpath.

Provides an implementation of the `SessionMap` that's backed by JDBC.

shs96c requested changes Jun 1, 2020

View reviewed changes

raju249 force-pushed the jdbc-session-map branch 2 times, most recently from 30f1300 to 00854be Compare June 3, 2020 10:31

shs96c requested changes Jun 3, 2020

View reviewed changes

shs96c reviewed Jun 4, 2020

View reviewed changes

raju249 force-pushed the jdbc-session-map branch from 1b90c77 to 516d60d Compare June 4, 2020 12:02

raju249 added 16 commits June 5, 2020 10:21

Add boiler plate code for JDBC backed session map feature

1c410bd

Add JdbcSessionMapFlags class

7c19cf4

Add jdbc class for session storage

a740245

Apply suggestions from Simon

b71ab6c

Update jdbc and MAIN grid BUILD.bzael

4970226

Add jdc exception and SessionMapOptions

dd7fb5f

Add SESSION_MAP_ROLE for sessions command

12d9508

Add get, remove and add logic for jdbc backed session map implementation

0df3d3c

Change separator for sessionid and URI

5575732

Add 2 tests for jdbcbackedsessionmap

9986336

Use preparedstatement for SQL statements and add delete session test

58f74fe

Add test for table does not exist

f15dec5

Review changes for non configurable items

1036c28

Handle NP for rawUri

d1d7d02

Add session uri column and remove wildcard

25900a3

Remove database meta from options to Maps class

01d6210

raju249 force-pushed the jdbc-session-map branch from 516d60d to 01d6210 Compare June 5, 2020 04:51

Add event bus to test and JdbcBackedSessionMap implmentation

b109ebc

shs96c reviewed Jun 5, 2020

View reviewed changes

raju249 added 2 commits June 5, 2020 15:59

Use string.format for prepare statements

ab5274d

Use hunter2 as example password

0a0e746

shs96c approved these changes Jun 5, 2020

View reviewed changes

shs96c merged commit f5fc6cd into SeleniumHQ:master Jun 5, 2020

raju249 deleted the jdbc-session-map branch June 5, 2020 10:49

titusfortner pushed a commit to titusfortner/selenium that referenced this pull request Aug 13, 2020

JDBC session map (SeleniumHQ#8378)

61d31fb

Provides an implementation of the `SessionMap` that's backed by JDBC.

WIP: Jdbc session map #8378

WIP: Jdbc session map #8378

Conversation

raju249 commented Jun 1, 2020 • edited Loading

Description

Motivation and Context

Types of changes

Checklist

shs96c left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shs96c left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shs96c left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shs96c left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shs96c left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raju249 commented Jun 1, 2020 •

edited

Loading