From 490c2365749fdf814621d5cd56317d8dff6aa277 Mon Sep 17 00:00:00 2001 From: Devin Smith Date: Wed, 20 Nov 2024 10:23:12 -0800 Subject: [PATCH] Review response Add link to https://github.com/deephaven/deephaven-core/issues/6403 --- .../flightsql/FlightSqlCommandHelper.java | 32 ++++++------------- .../server/flightsql/FlightSqlResolver.java | 30 ++++++----------- .../flightsql/FlightSqlTicketHelper.java | 8 ++--- 3 files changed, 21 insertions(+), 49 deletions(-) diff --git a/extensions/flight-sql/src/main/java/io/deephaven/server/flightsql/FlightSqlCommandHelper.java b/extensions/flight-sql/src/main/java/io/deephaven/server/flightsql/FlightSqlCommandHelper.java index 3383629f5c8..d8a79438b59 100644 --- a/extensions/flight-sql/src/main/java/io/deephaven/server/flightsql/FlightSqlCommandHelper.java +++ b/extensions/flight-sql/src/main/java/io/deephaven/server/flightsql/FlightSqlCommandHelper.java @@ -66,10 +66,8 @@ interface CommandVisitor { } 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 @@ -77,26 +75,16 @@ public static boolean handlesCommand(FlightDescriptor descriptor) { } public static T visit(FlightDescriptor descriptor, CommandVisitor 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 index 372027a9b22..b52a29932de 100644 --- 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 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 extends CommandHandle CommandStaticTable(Table table, Function 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 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 index a3aa98a87c1..77c4681cf39 100644 --- 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 visit(ByteBuffer ticket, TicketVisitor 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) {