diff --git a/cpp/perspective/src/cpp/server.cpp b/cpp/perspective/src/cpp/server.cpp index 29208c9188..98f221e9fc 100644 --- a/cpp/perspective/src/cpp/server.cpp +++ b/cpp/perspective/src/cpp/server.cpp @@ -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); } @@ -440,6 +444,10 @@ ServerResources::get_view_ids(const t_id& table_id) { std::shared_ptr 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); } @@ -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(); diff --git a/cpp/perspective/src/include/perspective/exception.h b/cpp/perspective/src/include/perspective/exception.h index afae761c41..598aced98c 100644 --- a/cpp/perspective/src/include/perspective/exception.h +++ b/cpp/perspective/src/include/perspective/exception.h @@ -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 \ No newline at end of file diff --git a/cpp/protos/perspective.proto b/cpp/protos/perspective.proto index 421686325e..34230e7809 100644 --- a/cpp/protos/perspective.proto +++ b/cpp/protos/perspective.proto @@ -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 { diff --git a/rust/perspective-client/src/rust/utils/mod.rs b/rust/perspective-client/src/rust/utils/mod.rs index 118348ad4f..ad6d91f57a 100644 --- a/rust/perspective-client/src/rust/utils/mod.rs +++ b/rust/perspective-client/src/rust/utils/mod.rs @@ -22,6 +22,9 @@ use crate::proto; #[derive(Error, Debug)] pub enum ClientError { + #[error("View not found")] + ViewNotFound, + #[error("Abort(): {0}")] Internal(String), @@ -64,7 +67,10 @@ pub type ClientResult = Result; impl From 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)), } } diff --git a/rust/perspective-js/src/rust/table.rs b/rust/perspective-js/src/rust/table.rs index 8f4050b174..2c9d28b9d2 100644 --- a/rust/perspective-js/src/rust/table.rs +++ b/rust/perspective-js/src/rust/table.rs @@ -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(()) } diff --git a/rust/perspective-js/src/rust/utils/errors.rs b/rust/perspective-js/src/rust/utils/errors.rs index c14b26ad05..38ae74bbf2 100644 --- a/rust/perspective-js/src/rust/utils/errors.rs +++ b/rust/perspective-js/src/rust/utils/errors.rs @@ -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 @@ -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, @@ -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`, for situations where /// error-casting through the `?` operator is insufficient. pub trait ToApiError { @@ -105,3 +115,12 @@ impl ToApiError for Result<(), ApiResult> { self.map_or_else(|x| x, |()| Ok(JsValue::UNDEFINED)) } } + +impl From for ApiError { + fn from(value: ClientError) -> Self { + match value { + ClientError::ViewNotFound => ApiError(PerspectiveViewNotFoundError::new().into()), + err => ApiError(JsError::new(format!("{}", err).as_str()).into()), + } + } +} diff --git a/rust/perspective-js/src/rust/utils/futures.rs b/rust/perspective-js/src/rust/utils/futures.rs index 880cb70717..5ee6fbd62a 100644 --- a/rust/perspective-js/src/rust/utils/futures.rs +++ b/rust/perspective-js/src/rust/utils/futures.rs @@ -84,9 +84,11 @@ where { fn from(fut: ApiFuture) -> 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() }) } } @@ -146,7 +148,7 @@ where static CANCELLED_MSG: &str = "View method cancelled"; #[extend::ext] -pub impl Result { +pub impl Result { /// 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 @@ -160,35 +162,22 @@ pub impl Result { /// 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 { + self.or_else(|x| match x.dyn_ref::() { + Some(_) => Ok(js_intern::js_intern!(CANCELLED_MSG).clone()), + None => Err(x), + }) + } +} + +#[extend::ext] +pub impl Result { fn ignore_view_delete(self) -> Result { self.or_else(|x| { let f: JsValue = x.clone().into(); - match f.dyn_ref::() { - 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::() { + Some(_) => Ok(js_intern::js_intern!(CANCELLED_MSG).clone()), + None => Err(x), } }) } diff --git a/rust/perspective-js/src/rust/view.rs b/rust/perspective-js/src/rust/view.rs index 25193db8b3..f8610d4959 100644 --- a/rust/perspective-js/src/rust/view.rs +++ b/rust/perspective-js/src/rust/view.rs @@ -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(()) } diff --git a/rust/perspective-viewer/src/rust/custom_elements/debug_plugin.rs b/rust/perspective-viewer/src/rust/custom_elements/debug_plugin.rs index ee07b08e45..97ba744636 100644 --- a/rust/perspective-viewer/src/rust/custom_elements/debug_plugin.rs +++ b/rust/perspective-viewer/src/rust/custom_elements/debug_plugin.rs @@ -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")?; diff --git a/rust/perspective-viewer/src/ts/plugin.ts b/rust/perspective-viewer/src/ts/plugin.ts index 5024adc265..dfbda30408 100644 --- a/rust/perspective-viewer/src/ts/plugin.ts +++ b/rust/perspective-viewer/src/ts/plugin.ts @@ -122,7 +122,7 @@ export interface IPerspectiveViewerPlugin { * } * ``` */ - // draw(view: perspective.View): Promise; + draw(view: View): Promise; /** * Draw under the assumption that the `ViewConfig` has not changed since @@ -136,7 +136,7 @@ export interface IPerspectiveViewerPlugin { * } * ``` */ - // update(view: perspective.View): Promise; + update(view: View): Promise; /** * Clear this plugin, though it is up to the discretion of the plugin diff --git a/rust/perspective-viewer/test/html/plugin-resize.html b/rust/perspective-viewer/test/html/plugin-resize.html new file mode 100644 index 0000000000..1f8833a283 --- /dev/null +++ b/rust/perspective-viewer/test/html/plugin-resize.html @@ -0,0 +1,45 @@ + + + + + + + + + + + + + + diff --git a/rust/perspective-viewer/test/js/cancellable.spec.js b/rust/perspective-viewer/test/js/cancellable.spec.js new file mode 100644 index 0000000000..ad31f6a644 --- /dev/null +++ b/rust/perspective-viewer/test/js/cancellable.spec.js @@ -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", + ]); + }); +}); diff --git a/tools/perspective-test/results.tar.gz b/tools/perspective-test/results.tar.gz index 92a80e3608..3ea71c1185 100644 Binary files a/tools/perspective-test/results.tar.gz and b/tools/perspective-test/results.tar.gz differ