Skip to content

Commit

Permalink
Ryan's feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@nbauernfeind
nbauernfeind committed Jun 18, 2024
1 parent 3af8830 commit 7d1b9d8
Show file tree
Hide file tree
Showing 5 changed files with 45 additions and 33 deletions.
19 changes: 11 additions & 8 deletions server/src/main/java/io/deephaven/server/appmode/ApplicationTicketResolver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@
import io.deephaven.server.session.SessionState;
import io.deephaven.server.session.TicketResolverBase;
import io.deephaven.server.session.TicketRouter;
import io.grpc.StatusRuntimeException;
import org.apache.arrow.flight.impl.Flight;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

import javax.inject.Inject;
Expand Down Expand Up @@ -79,13 +81,11 @@ private <T> SessionState.ExportObject<T> resolve(final AppFieldId id, final Stri
}
final Field<Object> field = id.app.getField(id.fieldName);
if (field == null) {
throw Exceptions.statusRuntimeException(Code.NOT_FOUND,
"Could not resolve '" + logId + "': field '" + getLogNameFor(id) + "' not found");
throw newNotFoundSRE(logId, id);
}
Object value = authorization.transform(field.value());
if (value == null) {
throw Exceptions.statusRuntimeException(Code.NOT_FOUND,
"Could not resolve '" + logId + "': field '" + getLogNameFor(id) + "' not found");
throw newNotFoundSRE(logId, id);
}
// noinspection unchecked
return SessionState.wrapAsExport((T) value);
Expand All @@ -104,8 +104,7 @@ public SessionState.ExportObject<Flight.FlightInfo> flightInfoFor(
synchronized (id.app) {
Field<?> field = id.app.getField(id.fieldName);
if (field == null) {
throw Exceptions.statusRuntimeException(Code.NOT_FOUND,
"Could not resolve '" + logId + "': field '" + getLogNameFor(id) + "' not found");
throw newNotFoundSRE(logId, id);
}
Object value = field.value();
if (value instanceof Table) {
Expand All @@ -116,8 +115,7 @@ public SessionState.ExportObject<Flight.FlightInfo> flightInfoFor(
if (value instanceof Table) {
info = TicketRouter.getFlightInfo((Table) value, descriptor, flightTicketForName(id.app, id.fieldName));
} else {
throw Exceptions.statusRuntimeException(Code.NOT_FOUND,
"Could not resolve '" + logId + "': field '" + getLogNameFor(id) + "' is not a flight");
throw newNotFoundSRE(logId, id);
}
}

Expand Down Expand Up @@ -213,6 +211,11 @@ public static Flight.FlightDescriptor descriptorForName(final ApplicationState a
.build();
}

private @NotNull StatusRuntimeException newNotFoundSRE(String logId, AppFieldId id) {
return Exceptions.statusRuntimeException(Code.NOT_FOUND,
"Could not resolve '" + logId + "': field '" + getLogNameFor(id) + "' not found");
}

private AppFieldId appFieldIdFor(final ByteBuffer ticket, final String logId) {
if (ticket == null) {
throw Exceptions.statusRuntimeException(Code.FAILED_PRECONDITION,
Expand Down
28 changes: 18 additions & 10 deletions server/src/main/java/io/deephaven/server/console/ScopeTicketResolver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@
import io.deephaven.server.session.SessionState;
import io.deephaven.server.session.TicketResolverBase;
import io.deephaven.server.session.TicketRouter;
import io.grpc.StatusRuntimeException;
import org.apache.arrow.flight.impl.Flight;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

import javax.inject.Inject;
Expand Down Expand Up @@ -54,18 +56,15 @@ public SessionState.ExportObject<Flight.FlightInfo> flightInfoFor(
final QueryScope queryScope = ExecutionContext.getContext().getQueryScope();
final Object scopeVar = queryScope.unwrapObject(queryScope.readParamValue(scopeName, null));
if (scopeVar == null) {
throw Exceptions.statusRuntimeException(Code.NOT_FOUND,
"Could not resolve '" + logId + ": no table exists with name '" + scopeName + "'");
throw newNotFoundSRE(logId, scopeName);
}
if (!(scopeVar instanceof Table)) {
throw Exceptions.statusRuntimeException(Code.NOT_FOUND,
"Could not resolve '" + logId + "': no table exists with name '" + scopeName + "'");
throw newNotFoundSRE(logId, scopeName);
}

final Table transformed = authorization.transform((Table) scopeVar);
if (transformed == null) {
throw Exceptions.statusRuntimeException(Code.NOT_FOUND,
"Could not resolve '" + logId + "': no table exists with name '" + scopeName + "'");
throw newNotFoundSRE(logId, scopeName);
}
final Flight.FlightInfo flightInfo =
TicketRouter.getFlightInfo(transformed, descriptor, flightTicketForName(scopeName));
Expand All @@ -76,8 +75,13 @@ public SessionState.ExportObject<Flight.FlightInfo> flightInfoFor(
@Override
public void forAllFlightInfo(@Nullable final SessionState session, final Consumer<Flight.FlightInfo> visitor) {
final QueryScope queryScope = ExecutionContext.getContext().getQueryScope();
queryScope.toMap(queryScope::unwrapObject, (n, t) -> t instanceof Table).forEach((name, table) -> visitor
.accept(TicketRouter.getFlightInfo((Table) table, descriptorForName(name), flightTicketForName(name))));
queryScope.toMap(queryScope::unwrapObject, (n, t) -> t instanceof Table).forEach((name, table) -> {
final Table transformedTable = authorization.transform((Table) table);
if (transformedTable != null) {
visitor.accept(TicketRouter.getFlightInfo(
transformedTable, descriptorForName(name), flightTicketForName(name)));
}
});
}

@Override
Expand Down Expand Up @@ -105,8 +109,7 @@ private <T> SessionState.ExportObject<T> resolve(final String scopeName, final S
export = authorization.transform(export);

if (export == null) {
return SessionState.wrapAsFailedExport(Exceptions.statusRuntimeException(Code.FAILED_PRECONDITION,
"Could not resolve '" + logId + "': no variable exists with name '" + scopeName + "'"));
return SessionState.wrapAsFailedExport(newNotFoundSRE(logId, scopeName));
}

return SessionState.wrapAsExport(export);
Expand Down Expand Up @@ -272,4 +275,9 @@ public static Flight.FlightDescriptor ticketToDescriptor(final Flight.Ticket tic
public static Flight.Ticket descriptorToTicket(final Flight.FlightDescriptor descriptor, final String logId) {
return flightTicketForName(nameForDescriptor(descriptor, logId));
}

private static @NotNull StatusRuntimeException newNotFoundSRE(String logId, String scopeName) {
return Exceptions.statusRuntimeException(Code.NOT_FOUND,
"Could not resolve '" + logId + ": variable '" + scopeName + "' not found");
}
}
10 changes: 5 additions & 5 deletions ...src/main/java/io/deephaven/server/hierarchicaltable/HierarchicalTableServiceGrpcImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ public void rollup(
final RollupTable transformedResult = authTransformation.transform(result);
if (transformedResult == null) {
throw Exceptions.statusRuntimeException(
Code.FAILED_PRECONDITION, "Not authorized to rollup hierarchical table.");
Code.FAILED_PRECONDITION, "Not authorized to rollup hierarchical table");
}
safelyComplete(responseObserver, RollupResponse.getDefaultInstance());
return transformedResult;
Expand Down Expand Up @@ -167,7 +167,7 @@ public void tree(
final TreeTable transformedResult = authTransformation.transform(result);
if (transformedResult == null) {
throw Exceptions.statusRuntimeException(
Code.FAILED_PRECONDITION, "Not authorized to tree hierarchical table.");
Code.FAILED_PRECONDITION, "Not authorized to tree hierarchical table");
}
safelyComplete(responseObserver, TreeResponse.getDefaultInstance());
return transformedResult;
Expand Down Expand Up @@ -272,7 +272,7 @@ public void apply(
final HierarchicalTable<?> transformedResult = authTransformation.transform(result);
if (transformedResult == null) {
throw Exceptions.statusRuntimeException(
Code.FAILED_PRECONDITION, "Not authorized to apply to hierarchical table.");
Code.FAILED_PRECONDITION, "Not authorized to apply to hierarchical table");
}
safelyComplete(responseObserver, HierarchicalTableApplyResponse.getDefaultInstance());
return transformedResult;
Expand Down Expand Up @@ -437,7 +437,7 @@ public void view(
final HierarchicalTableView transformedResult = authTransformation.transform(result);
if (transformedResult == null) {
throw Exceptions.statusRuntimeException(
Code.FAILED_PRECONDITION, "Not authorized to view hierarchical table.");
Code.FAILED_PRECONDITION, "Not authorized to view hierarchical table");
}
safelyComplete(responseObserver, HierarchicalTableViewResponse.getDefaultInstance());
return transformedResult;
Expand Down Expand Up @@ -497,7 +497,7 @@ public void exportSource(
if (transformedResult == null) {
throw Exceptions.statusRuntimeException(
Code.FAILED_PRECONDITION,
"Not authorized to export source from hierarchical table.");
"Not authorized to export source from hierarchical table");
}
final ExportedTableCreationResponse response =
ExportUtil.buildTableCreationResponse(request.getResultTableId(), transformedResult);
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/io/deephaven/server/session/ExportTicketResolver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public SessionState.ExportObject<Flight.FlightInfo> flightInfoFor(
}

throw Exceptions.statusRuntimeException(Code.NOT_FOUND,
"Could not resolve '" + logId + "': flight '" + descriptor + "' is not a table");
"Could not resolve '" + logId + "': flight '" + descriptor + "' not found");
});
}

Expand Down
19 changes: 10 additions & 9 deletions server/src/main/java/io/deephaven/server/session/SharedTicketResolver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import io.deephaven.proto.util.Exceptions;
import io.deephaven.proto.util.SharedTicketHelper;
import io.deephaven.server.auth.AuthorizationProvider;
import io.grpc.StatusRuntimeException;
import org.apache.arrow.flight.impl.Flight;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
Expand Down Expand Up @@ -63,8 +64,7 @@ public SessionState.ExportObject<Flight.FlightInfo> flightInfoFor(

SessionState.ExportObject<?> export = sharedVariables.get(sharedId);
if (export == null) {
throw Exceptions.statusRuntimeException(Code.NOT_FOUND, String.format(
"Could not resolve '%s': no shared ticket exists with id 0x%s", logId, toHexString(sharedId)));
throw newNotFoundSRE(logId, descriptor.toString());
}

return session.<Flight.FlightInfo>nonExport()
Expand All @@ -79,8 +79,7 @@ public SessionState.ExportObject<Flight.FlightInfo> flightInfoFor(
FlightExportTicketHelper.descriptorToFlightTicket(descriptor, logId));
}

throw Exceptions.statusRuntimeException(Code.NOT_FOUND, String.format(
"Could not resolve '%s': flight '%s' is not a table", logId, descriptor));
throw newNotFoundSRE(logId, descriptor.toString());
});
}

Expand Down Expand Up @@ -111,8 +110,7 @@ private <T> SessionState.ExportObject<T> resolve(
// noinspection unchecked
final SessionState.ExportObject<T> sharedVar = (SessionState.ExportObject<T>) sharedVariables.get(sharedId);
if (sharedVar == null) {
return SessionState.wrapAsFailedExport(Exceptions.statusRuntimeException(Code.NOT_FOUND, String.format(
"Could not resolve '%s': no shared ticket exists with id '%s'", logId, toHexString(sharedId))));
return SessionState.wrapAsFailedExport(newNotFoundSRE(logId, toHexString(sharedId)));
}

// we need to wrap this in a new export object to hand off to the new session and defer checking permissions
Expand All @@ -122,9 +120,7 @@ private <T> SessionState.ExportObject<T> resolve(
T result = sharedVar.get();
result = authorization.transform(result);
if (result == null) {
throw Exceptions.statusRuntimeException(Code.NOT_FOUND, String.format(
"Could not resolve '%s': no shared ticket exists with id '%s'",
logId, toHexString(sharedId)));
throw newNotFoundSRE(logId, toHexString(sharedId));
}
return result;
});
Expand Down Expand Up @@ -292,4 +288,9 @@ public static Flight.FlightDescriptor ticketToDescriptor(final Flight.Ticket tic
public static Flight.Ticket descriptorToTicket(final Flight.FlightDescriptor descriptor, final String logId) {
return flightTicketForId(idForDescriptor(descriptor, logId).toByteArray());
}

private static @NotNull StatusRuntimeException newNotFoundSRE(String logId, String sharedId) {
return Exceptions.statusRuntimeException(Code.NOT_FOUND, String.format(
"Could not resolve '%s': ticket '%s' not found", logId, sharedId));
}
}

0 comments on commit 7d1b9d8

Please sign in to comment.