Review response

Add link to deephaven#6403
devinrsmith · Nov 20, 2024 · 490c236 · 490c236
1 parent 2c6c1e8
commit 490c236
Show file tree

Hide file tree

Showing 3 changed files with 21 additions and 49 deletions.
diff --git a/...nsions/flight-sql/src/main/java/io/deephaven/server/flightsql/FlightSqlCommandHelper.java b/...nsions/flight-sql/src/main/java/io/deephaven/server/flightsql/FlightSqlCommandHelper.java
@@ -66,37 +66,25 @@ interface CommandVisitor<T> {
     }
 
     public static boolean handlesCommand(FlightDescriptor descriptor) {
-        if (descriptor.getType() != FlightDescriptor.DescriptorType.CMD) {
-            // noinspection DataFlowIssue
-            throw Assert.statementNeverExecuted();
-        }
+        // If not CMD, there is an error with io.deephaven.server.session.TicketRouter.getPathResolver / handlesPath
+        Assert.equals(descriptor.getType(), "descriptor.getType()", FlightDescriptor.DescriptorType.CMD);
         // No good way to check if this is a valid command without parsing to Any first.
         final Any command = parseOrNull(descriptor.getCmd());
         return command != null
                 && command.getTypeUrl().startsWith(FlightSqlSharedConstants.FLIGHT_SQL_COMMAND_TYPE_PREFIX);
     }
 
     public static <T> T visit(FlightDescriptor descriptor, CommandVisitor<T> visitor, String logId) {
-        if (descriptor.getType() != FlightDescriptor.DescriptorType.CMD) {
-            // If we get here, there is an error with io.deephaven.server.session.TicketRouter.getPathResolver /
-            // handlesPath
-            // noinspection DataFlowIssue
-            throw Assert.statementNeverExecuted();
-        }
+        // If not CMD, there is an error with io.deephaven.server.session.TicketRouter.getPathResolver / handlesPath
+        Assert.equals(descriptor.getType(), "descriptor.getType()", FlightDescriptor.DescriptorType.CMD);
         final Any command = parseOrNull(descriptor.getCmd());
-        if (command == null) {
-            // If we get here, there is an error with io.deephaven.server.session.TicketRouter.getCommandResolver /
-            // handlesCommand
-            // noinspection DataFlowIssue
-            throw Assert.statementNeverExecuted();
-        }
+        // If null, there is an error with io.deephaven.server.session.TicketRouter.getCommandResolver / handlesCommand
+        Assert.neqNull(command, "command");
         final String typeUrl = command.getTypeUrl();
-        if (!typeUrl.startsWith(FlightSqlSharedConstants.FLIGHT_SQL_COMMAND_TYPE_PREFIX)) {
-            // If we get here, there is an error with io.deephaven.server.session.TicketRouter.getCommandResolver /
-            // handlesCommand
-            // noinspection DataFlowIssue
-            throw Assert.statementNeverExecuted();
-        }
+        // If not true, there is an error with io.deephaven.server.session.TicketRouter.getCommandResolver /
+        // handlesCommand
+        Assert.eqTrue(typeUrl.startsWith(FlightSqlSharedConstants.FLIGHT_SQL_COMMAND_TYPE_PREFIX),
+                "typeUrl.startsWith");
         switch (typeUrl) {
             case FlightSqlSharedConstants.COMMAND_STATEMENT_QUERY_TYPE_URL:
                 return visitor.visit(unpack(command, CommandStatementQuery.class, logId));

diff --git a/extensions/flight-sql/src/main/java/io/deephaven/server/flightsql/FlightSqlResolver.java b/extensions/flight-sql/src/main/java/io/deephaven/server/flightsql/FlightSqlResolver.java
@@ -551,12 +551,8 @@ public void doAction(
             @Nullable final SessionState session,
             final Action action,
             final StreamObserver<Result> observer) {
-        if (!handlesActionType(action.getType())) {
-            // If we get here, there is an error with io.deephaven.server.session.ActionRouter.doAction /
-            // handlesActionType
-            // noinspection DataFlowIssue
-            throw Assert.statementNeverExecuted();
-        }
+        // If false, there is an error with io.deephaven.server.session.ActionRouter.doAction / handlesActionType
+        Assert.eqTrue(handlesActionType(action.getType()), "handlesActionType(action.getType())");
         if (session == null) {
             throw unauthenticatedError();
         }
@@ -758,17 +754,11 @@ public Table resolve() {
                 final Table table = CommandHandlerFixedBase.this.table(command);
                 final long totalRecords = totalRecords();
                 if (totalRecords != -1) {
-                    if (table.isRefreshing()) {
-                        // TicketHandler implementation error; should only override totalRecords for non-refreshing
-                        // tables
-                        // noinspection DataFlowIssue
-                        throw Assert.statementNeverExecuted();
-                    }
-                    if (table.size() != totalRecords) {
-                        // Ticket handler implementation error; totalRecords does not match the table size
-                        // noinspection DataFlowIssue
-                        throw Assert.statementNeverExecuted();
-                    }
+                    // If false, TicketHandler implementation error; should only override totalRecords for
+                    // non-refreshing tables
+                    Assert.eqTrue(table.isRefreshing(), "table.isRefreshing()");
+                    // If false, Ticket handler implementation error; totalRecords does not match the table size
+                    Assert.eq(table.size(), "table.size()", totalRecords, "totalRecords");
                 }
                 return table;
             }
@@ -984,10 +974,7 @@ private static class CommandStaticTable<T extends Message> extends CommandHandle
 
         CommandStaticTable(Table table, Function<T, Ticket> f) {
             super();
-            if (table.isRefreshing()) {
-                // noinspection DataFlowIssue
-                throw Assert.statementNeverExecuted();
-            }
+            Assert.eqFalse(table.isRefreshing(), "table.isRefreshing()");
             this.table = Objects.requireNonNull(table);
             this.f = Objects.requireNonNull(f);
             this.schemaBytes = BarrageUtil.schemaBytesFromTable(table);
@@ -1686,6 +1673,7 @@ private synchronized void closeImpl() {
      */
     @VisibleForTesting
     static Predicate<String> flightSqlFilterPredicate(String flightSqlPattern) {
+        // TODO(deephaven-core#6403): Flight SQL filter pattern improvements
         // This is the technically correct, although likely represents a Flight SQL client mis-use, as the results will
         // be empty (unless an empty db_schema_name is allowed).
         //

diff --git a/extensions/flight-sql/src/main/java/io/deephaven/server/flightsql/FlightSqlTicketHelper.java b/extensions/flight-sql/src/main/java/io/deephaven/server/flightsql/FlightSqlTicketHelper.java
@@ -82,12 +82,8 @@ public static <T> T visit(ByteBuffer ticket, TicketVisitor<T> visitor, String lo
 
     private static Any partialUnpackTicket(ByteBuffer ticket, final String logId) {
         ticket = ticket.slice();
-        if (ticket.get() != TICKET_PREFIX) {
-            // If we get here, it means there is an error with FlightSqlResolver.ticketRoute /
-            // io.deephaven.server.session.TicketRouter.getResolver
-            // noinspection DataFlowIssue
-            throw Assert.statementNeverExecuted();
-        }
+        // If false, it means there is an error with FlightSqlResolver.ticketRoute / TicketRouter.getResolver
+        Assert.eq(ticket.get(), "ticket.get()", TICKET_PREFIX, "TICKET_PREFIX");
         try {
             return Any.parseFrom(ticket);
         } catch (InvalidProtocolBufferException e) {