Skip to content

Commit

Permalink
Fix cancellable methods in perspective-viewer
Browse files Browse the repository at this point in the history
Signed-off-by: Andrew Stein <steinlink@gmail.com>
  • Loading branch information
texodus committed Sep 18, 2024
1 parent 47f6cbe commit c28d83e
Show file tree
Hide file tree
Showing 13 changed files with 193 additions and 39 deletions.
15 changes: 15 additions & 0 deletions cpp/perspective/src/cpp/server.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,10 @@ ServerResources::get_table_for_view(const t_id& view_id) {
ServerResources::t_id
ServerResources::get_table_id_for_view(const t_id& view_id) {
PSP_READ_LOCK(m_write_lock);
if (!m_view_to_table.contains(view_id)) {
throw PerspectiveViewNotFoundException();
}

return m_view_to_table.at(view_id);
}

Expand All @@ -440,6 +444,10 @@ ServerResources::get_view_ids(const t_id& table_id) {
std::shared_ptr<ErasedView>
ServerResources::get_view(const t_id& id) {
PSP_READ_LOCK(m_write_lock);
if (!m_views.contains(id)) {
throw PerspectiveViewNotFoundException();
}

return m_views.at(id);
}

Expand Down Expand Up @@ -680,6 +688,13 @@ ProtoServer::handle_request(
auto* err = resp.mutable_server_error()->mutable_message();
*err = std::string(e.what());
responses.emplace_back(std::move(resp));
} catch (const PerspectiveViewNotFoundException& e) {
proto::Response resp;
auto* err = resp.mutable_server_error();
err->set_status_code(proto::StatusCode::VIEW_NOT_FOUND);
auto* msg = err->mutable_message();
*msg = std::string(e.what());
responses.emplace_back(std::move(resp));
} catch (const std::exception& e) {
proto::Response resp;
auto* err = resp.mutable_server_error()->mutable_message();
Expand Down
12 changes: 12 additions & 0 deletions cpp/perspective/src/include/perspective/exception.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,16 @@ class PERSPECTIVE_EXPORT PerspectiveException : public std::exception {
std::string message;
};

class PERSPECTIVE_EXPORT PerspectiveViewNotFoundException
: public std::exception {
public:
explicit PerspectiveViewNotFoundException() {}

[[nodiscard]]
const char*
what() const noexcept override {
return "View not found";
}
};

} // namespace perspective
6 changes: 6 additions & 0 deletions cpp/protos/perspective.proto
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,15 @@ option optimize_for = LITE_RUNTIME;
//
// Common

enum StatusCode {
SERVER_ERROR = 0;
VIEW_NOT_FOUND = 1;
}

// Recoverable, user-readable error reporting from the engine.
message ServerError {
string message = 1;
StatusCode status_code = 2;
}

message Schema {
Expand Down
8 changes: 7 additions & 1 deletion rust/perspective-client/src/rust/utils/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ use crate::proto;

#[derive(Error, Debug)]
pub enum ClientError {
#[error("View not found")]
ViewNotFound,

#[error("Abort(): {0}")]
Internal(String),

Expand Down Expand Up @@ -64,7 +67,10 @@ pub type ClientResult<T> = Result<T, ClientError>;
impl From<proto::response::ClientResp> for ClientError {
fn from(value: proto::response::ClientResp) -> Self {
match value {
proto::response::ClientResp::ServerError(x) => ClientError::Internal(x.message),
proto::response::ClientResp::ServerError(x) => match x.status_code() {
proto::StatusCode::ServerError => ClientError::Internal(x.message),
proto::StatusCode::ViewNotFound => ClientError::ViewNotFound,
},
x => ClientError::ResponseFailed(Box::new(x)),
}
}
Expand Down
2 changes: 1 addition & 1 deletion rust/perspective-js/src/rust/table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ impl Table {

#[doc = inherit_docs!("table/delete.md")]
#[wasm_bindgen]
pub async fn delete(self) -> ApiResult<()> {
pub async fn delete(&self) -> ApiResult<()> {
self.0.delete().await?;
Ok(())
}
Expand Down
21 changes: 20 additions & 1 deletion rust/perspective-js/src/rust/utils/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

use std::fmt::Display;

use perspective_client::ClientError;
use wasm_bindgen::prelude::*;

/// A bespoke error class for chaining a litany of various error types with the
Expand Down Expand Up @@ -78,7 +79,6 @@ define_api_error!(
serde_json::Error,
rmp_serde::encode::Error,
rmp_serde::decode::Error,
perspective_client::ClientError,
&str,
String,
futures::channel::oneshot::Canceled,
Expand All @@ -87,6 +87,16 @@ define_api_error!(
prost::DecodeError
);

#[wasm_bindgen(inline_js = r#"
export class PerspectiveViewNotFoundError extends Error {}
"#)]
extern "C" {
pub type PerspectiveViewNotFoundError;

#[wasm_bindgen(constructor)]
fn new() -> PerspectiveViewNotFoundError;
}

/// Explicit conversion methods for `ApiResult<T>`, for situations where
/// error-casting through the `?` operator is insufficient.
pub trait ToApiError<T> {
Expand All @@ -105,3 +115,12 @@ impl ToApiError<JsValue> for Result<(), ApiResult<JsValue>> {
self.map_or_else(|x| x, |()| Ok(JsValue::UNDEFINED))
}
}

impl From<perspective_client::ClientError> for ApiError {
fn from(value: ClientError) -> Self {
match value {
ClientError::ViewNotFound => ApiError(PerspectiveViewNotFoundError::new().into()),
err => ApiError(JsError::new(format!("{}", err).as_str()).into()),
}
}
}
49 changes: 19 additions & 30 deletions rust/perspective-js/src/rust/utils/futures.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,11 @@ where
{
fn from(fut: ApiFuture<T>) -> Self {
future_to_promise(async move {
let x = Ok(fut.0.await?);
let y = Ok(x.into_js_result()?);
Ok(y.ignore_view_delete()?)
fut.0
.await
.map_err(|x| x.into())
.into_js_result()
.ignore_view_delete()
})
}
}
Expand Down Expand Up @@ -146,7 +148,7 @@ where
static CANCELLED_MSG: &str = "View method cancelled";

#[extend::ext]
pub impl Result<JsValue, ApiError> {
pub impl Result<JsValue, JsValue> {
/// Wraps an error `JsValue` return from a caught JavaScript exception,
/// checking for the explicit error type indicating that a
/// `JsPerspectiveView` call has been cancelled due to it already being
Expand All @@ -160,35 +162,22 @@ pub impl Result<JsValue, ApiError> {
/// silently be replaced with `Ok`. The message itself is returned in this
/// case (instead of whatever the `async` returns), which is helpful for
/// detecting this condition when debugging.
fn ignore_view_delete(self) -> Result<JsValue, JsValue> {
self.or_else(|x| match x.dyn_ref::<PerspectiveViewNotFoundError>() {
Some(_) => Ok(js_intern::js_intern!(CANCELLED_MSG).clone()),
None => Err(x),
})
}
}

#[extend::ext]
pub impl Result<JsValue, ApiError> {
fn ignore_view_delete(self) -> Result<JsValue, ApiError> {
self.or_else(|x| {
let f: JsValue = x.clone().into();
match f.dyn_ref::<js_sys::Error>() {
Some(err) => {
if err.message() != CANCELLED_MSG {
Err(x)
} else {
Ok(js_intern::js_intern!(CANCELLED_MSG).clone())
}
},
_ => match f.as_string() {
Some(x) if x == CANCELLED_MSG => {
Ok(js_intern::js_intern!(CANCELLED_MSG).clone())
},
Some(_) => Err(x),
_ => {
if js_sys::Reflect::get(&f, js_intern::js_intern!("message"))
.unwrap()
.as_string()
.unwrap_or_else(|| "".to_owned())
== CANCELLED_MSG
{
Ok(js_intern::js_intern!(CANCELLED_MSG).clone())
} else {
Err(x)
}
},
},
match f.dyn_ref::<PerspectiveViewNotFoundError>() {
Some(_) => Ok(js_intern::js_intern!(CANCELLED_MSG).clone()),
None => Err(x),
}
})
}
Expand Down
2 changes: 1 addition & 1 deletion rust/perspective-js/src/rust/view.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ impl View {

#[doc = inherit_docs!("view/delete.md")]
#[wasm_bindgen]
pub async fn delete(self) -> ApiResult<()> {
pub async fn delete(&self) -> ApiResult<()> {
self.0.delete().await?;
Ok(())
}
Expand Down
11 changes: 8 additions & 3 deletions rust/perspective-viewer/src/rust/custom_elements/debug_plugin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,13 +65,18 @@ impl PerspectiveDebugPluginElement {
JsValue::UNDEFINED
}

pub fn update(&self, view: perspective_js::View) -> ApiFuture<()> {
pub fn update(&self, view: &perspective_js::View) -> ApiFuture<()> {
self.draw(view)
}

pub fn draw(&self, view: perspective_js::View) -> ApiFuture<()> {
/// # Notes
///
/// When you pass a `wasm_bindgen` wrapped type _into_ Rust, it acts like a
/// move. Ergo, if you replace the `&` in the `view` argument, the JS copy
/// of the `View` will be invalid
pub fn draw(&self, view: &perspective_js::View) -> ApiFuture<()> {
let css = "margin:0;overflow:scroll;position:absolute;width:100%;height:100%";
clone!(self.elem);
clone!(self.elem, view);
ApiFuture::new(async move {
let csv = view.to_csv(None).await?;
elem.style().set_property("background-color", "#fff")?;
Expand Down
4 changes: 2 additions & 2 deletions rust/perspective-viewer/src/ts/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ export interface IPerspectiveViewerPlugin {
* }
* ```
*/
// draw(view: perspective.View): Promise<void>;
draw(view: View): Promise<void>;

/**
* Draw under the assumption that the `ViewConfig` has not changed since
Expand All @@ -136,7 +136,7 @@ export interface IPerspectiveViewerPlugin {
* }
* ```
*/
// update(view: perspective.View): Promise<void>;
update(view: View): Promise<void>;

/**
* Clear this plugin, though it is up to the discretion of the plugin
Expand Down
45 changes: 45 additions & 0 deletions rust/perspective-viewer/test/html/plugin-resize.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
<!DOCTYPE html>
<html>
<head>
<script type="module" src="/node_modules/@finos/perspective-viewer/dist/cdn/perspective-viewer.js"></script>
<script type="module">
await customElements.whenDefined("perspective-viewer-plugin");
const BASE = customElements.get("perspective-viewer-plugin");

class TestPluginImplementsResize extends BASE {
get name() {
return "Resizing Plugin";
}

async draw(view) {
this._view = view;
console.log(await this._view.num_rows());
return await super.draw(view);
}

async update(view) {
this._view = view;
console.log(await this._view.num_rows());
return await super.update(view);
}

async resize(view) {
console.log(await this._view.num_rows());
await this._view.to_csv();
}
}

customElements.define("resize-plugin", TestPluginImplementsResize);
await customElements.whenDefined("perspective-viewer").then((viewer) => {
viewer.registerPlugin("resize-plugin");
});
</script>

<script type="module" src="/node_modules/@finos/perspective-test/load-viewer-csv.js"></script>
<link rel="stylesheet" href="../css/demo.css" />
<link rel="stylesheet" href="/node_modules/@finos/perspective-viewer/dist/css/pro.css" />
</head>
<body>
<perspective-viewer></perspective-viewer>
</body>
</html>
57 changes: 57 additions & 0 deletions rust/perspective-viewer/test/js/cancellable.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
// ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
// ┃ ██████ ██████ ██████ █ █ █ █ █ █▄ ▀███ █ ┃
// ┃ ▄▄▄▄▄█ █▄▄▄▄▄ ▄▄▄▄▄█ ▀▀▀▀▀█▀▀▀▀▀ █ ▀▀▀▀▀█ ████████▌▐███ ███▄ ▀█ █ ▀▀▀▀▀ ┃
// ┃ █▀▀▀▀▀ █▀▀▀▀▀ █▀██▀▀ ▄▄▄▄▄ █ ▄▄▄▄▄█ ▄▄▄▄▄█ ████████▌▐███ █████▄ █ ▄▄▄▄▄ ┃
// ┃ █ ██████ █ ▀█▄ █ ██████ █ ███▌▐███ ███████▄ █ ┃
// ┣━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┫
// ┃ Copyright (c) 2017, the Perspective Authors. ┃
// ┃ ╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌ ┃
// ┃ This file is part of the Perspective library, distributed under the terms ┃
// ┃ of the [Apache License 2.0](https://www.apache.org/licenses/LICENSE-2.0). ┃
// ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛

import { test } from "@finos/perspective-test";
import { compareContentsToSnapshot } from "@finos/perspective-test";

async function get_contents(page) {
return await page.evaluate(async () => {
const viewer = document
.querySelector("perspective-viewer")
.shadowRoot.querySelector("#app_panel");
return viewer ? viewer.innerHTML : "MISSING";
});
}

test.beforeEach(async ({ page }) => {
await page.goto("/rust/perspective-viewer/test/html/plugin-resize.html");
await page.evaluate(async () => {
while (!window["__TEST_PERSPECTIVE_READY__"]) {
await new Promise((x) => setTimeout(x, 10));
}
});
});

test.describe("Cancellable methods", () => {
test("Cancellable view methods do not error", async ({ page }) => {
await page.evaluate(async () => {
const viewer = document.querySelector("perspective-viewer");
await viewer.restore({
group_by: ["State"],
columns: ["Sales"],
settings: true,
filter: [
["State", "not in", ["California", "Texas", "New York"]],
],
});

const view = await viewer.getView();
await view.delete();
await viewer.resize(true);
});

const contents = await get_contents(page);
await compareContentsToSnapshot(contents, [
"regressions-not_in-filter-works-correctly.txt",
]);
});
});
Binary file modified tools/perspective-test/results.tar.gz
Binary file not shown.

0 comments on commit c28d83e

Please sign in to comment.