From eacf29c98cdc81fcb614a8e7ae4dbedf889f15da Mon Sep 17 00:00:00 2001 From: Wasif Iqbal Date: Fri, 21 Jun 2024 18:59:23 -0500 Subject: [PATCH] fix: return better error message (#2077) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Motivation Parameters that contained invalid cast URLs with valid cast URL prefixes were surfacing database error instead of not found error. The url itself did not contain parent casts, but because it contained a valid prefix, the database lookups were returning errors. For example, a valid URL may be: "https://warpcast.com/~/channel/japan777iine" An invalid URL may be: "https://warpcast.com/~/chanel-makeup" Since both URLs contain same prefix, _part_ of the invalid param gets processed. That "prefix" then yields an error when entries aren't found in the database. However, no entries found is expected, because the URL is garbage. Instead of surfacing the database error, we now correctly surface not found error. This should fix #2067 - Certain queries to Hub API caused db errors to surface, e.g: ``` { "code": 2, "details": "db.internal_error/could not get message with key: [1, 129, 11, 134, 0, 1, 111, 110, 101, 6, 78, 209, 172, 203, 162, 11, 104, 228, 239, 164, 151, 212, 243, 3, 81, 109, 55, 199, 100, 32]", "metadata": { "errcode": [ null ] } } ``` ## Change Summary **Empty URL** ``` { "errCode": "bad_request.invalid_param", "presentable": false, "name": "HubError", "code": 3, "details": "url < 1 byte", "metadata": { "errcode": [ "bad_request.invalid_param" ] } } ``` **Invalid URL** ``` { "errCode": "not_found", "presentable": false, "name": "HubError", "code": 2, "details": "could not get message with key: [1, 129, 11, 134, 0, 1, 111, 110, 101, 6, 78, 209, 172, 203, 162, 11, 104, 228, 239, 164, 151, 212, 243, 3, 81, 109, 55, 199, 100, 32]", "metadata": { "errcode": [ "not_found" ] } } ``` ## Merge Checklist _Choose all relevant options below by adding an `x` now or at any time before submitting for review_ - [x] PR title adheres to the [conventional commits](https://www.conventionalcommits.org/en/v1.0.0/) standard - [ ] PR has a [changeset](https://github.com/farcasterxyz/hub-monorepo/blob/main/CONTRIBUTING.md#35-adding-changesets) - [x] PR has been tagged with a change label(s) (i.e. documentation, feature, bugfix, or chore) - [ ] PR includes [documentation](https://github.com/farcasterxyz/hub-monorepo/blob/main/CONTRIBUTING.md#32-writing-docs) if necessary. - [x] All [commits have been signed](https://github.com/farcasterxyz/hub-monorepo/blob/main/CONTRIBUTING.md#22-signing-commits) ## Additional Context If this is a relatively large or complex change, provide more details here that will help reviewers --- ## PR-Codex overview The focus of this PR is to fix the issue where the HTTP endpoint returns "not found" instead of an internal database error. ### Detailed summary - Fixed the issue in the `cast_store.rs` file related to generating message primary key - Updated error handling in `message.rs` and `store.rs` files - Added a new function `not_found` in `store.rs` - Modified error conversion logic in `server.ts` to handle Rust errors > ✨ Ask PR-Codex anything about this PR by commenting with `/codex {your question}` --- .changeset/yellow-chefs-dance.md | 6 ++++++ apps/hubble/src/addon/src/store/cast_store.rs | 7 ++----- apps/hubble/src/addon/src/store/message.rs | 14 +++++++------- apps/hubble/src/addon/src/store/store.rs | 11 +++++++++-- apps/hubble/src/rpc/server.ts | 18 +++++++++++++++--- 5 files changed, 39 insertions(+), 17 deletions(-) create mode 100644 .changeset/yellow-chefs-dance.md diff --git a/.changeset/yellow-chefs-dance.md b/.changeset/yellow-chefs-dance.md new file mode 100644 index 0000000000..471ea299ec --- /dev/null +++ b/.changeset/yellow-chefs-dance.md @@ -0,0 +1,6 @@ +--- +"@farcaster/core": patch +"@farcaster/hubble": patch +--- + +fix: http endoint return not found instead of internal database error diff --git a/apps/hubble/src/addon/src/store/cast_store.rs b/apps/hubble/src/addon/src/store/cast_store.rs index 6229ee0285..a1c0e40048 100644 --- a/apps/hubble/src/addon/src/store/cast_store.rs +++ b/apps/hubble/src/addon/src/store/cast_store.rs @@ -574,11 +574,8 @@ impl CastStore { let ts_hash = key[ts_hash_offset..ts_hash_offset + TS_HASH_LENGTH] .try_into() .unwrap(); - let message_primary_key = crate::store::message::make_message_primary_key( - fid, - store.postfix(), - Some(&ts_hash), - ); + let message_primary_key = + message::make_message_primary_key(fid, store.postfix(), Some(&ts_hash)); message_keys.push(message_primary_key.to_vec()); if message_keys.len() >= page_options.page_size.unwrap_or(PAGE_SIZE_MAX) { diff --git a/apps/hubble/src/addon/src/store/message.rs b/apps/hubble/src/addon/src/store/message.rs index 2201c5ce81..1923a299cf 100644 --- a/apps/hubble/src/addon/src/store/message.rs +++ b/apps/hubble/src/addon/src/store/message.rs @@ -1,10 +1,11 @@ -use super::{store::HubError, PageOptions, PAGE_SIZE_MAX}; +use prost::Message as _; + use crate::{ db::{RocksDB, RocksDbTransactionBatch}, protos::{CastId, Message as MessageProto, MessageData, MessageType}, }; -use prost::Message as _; -use std::convert::TryFrom; + +use super::{store::HubError, PageOptions, PAGE_SIZE_MAX}; pub const FID_BYTES: usize = 4; @@ -295,10 +296,9 @@ pub fn get_many_messages_as_bytes( if let Ok(Some(value)) = db.get(&key) { messages.push(value); } else { - return Err(HubError { - code: "db.internal_error".to_string(), - message: format!("could not get message with key: {:?}", key), - }); + return Err(HubError::not_found( + format!("could not get message with key: {:?}", key).as_str(), + )); } } diff --git a/apps/hubble/src/addon/src/store/store.rs b/apps/hubble/src/addon/src/store/store.rs index 562d57edfa..051ad6ac64 100644 --- a/apps/hubble/src/addon/src/store/store.rs +++ b/apps/hubble/src/addon/src/store/store.rs @@ -51,6 +51,13 @@ impl HubError { message: error_message.to_string(), } } + + pub fn not_found(error_message: &str) -> HubError { + HubError { + code: "not_found".to_string(), + message: error_message.to_string(), + } + } } impl Display for HubError { @@ -226,7 +233,7 @@ pub trait StoreDef: Send + Sync { if maybe_existing_remove.is_some() { conflicts.push(maybe_existing_remove.unwrap()); } else { - warn!(LOGGER, "Message's ts_hash exists but message not found in store"; + warn!(LOGGER, "Message's ts_hash exists but message not found in store"; o!("remove_ts_hash" => format!("{:x?}", remove_ts_hash.unwrap()))); } } @@ -267,7 +274,7 @@ pub trait StoreDef: Send + Sync { )?; if maybe_existing_add.is_none() { - warn!(LOGGER, "Message's ts_hash exists but message not found in store"; + warn!(LOGGER, "Message's ts_hash exists but message not found in store"; o!("add_ts_hash" => format!("{:x?}", add_ts_hash.unwrap()))); } else { conflicts.push(maybe_existing_add.unwrap()); diff --git a/apps/hubble/src/rpc/server.ts b/apps/hubble/src/rpc/server.ts index 29f5f433a9..e446bccabf 100644 --- a/apps/hubble/src/rpc/server.ts +++ b/apps/hubble/src/rpc/server.ts @@ -65,6 +65,7 @@ import { AddressInfo } from "net"; import * as net from "node:net"; import axios from "axios"; import { fidFromEvent } from "../storage/stores/storeEventHandler.js"; +import { rustErrorToHubError } from "../rustfunctions.js"; const HUBEVENTS_READER_TIMEOUT = 1 * 60 * 60 * 1000; // 1 hour @@ -204,6 +205,10 @@ export const checkPortAndPublicAddress = async ( }; export const toServiceError = (err: HubError): ServiceError => { + // hack: After rust migration, requests that propagate to RocksDB may yield string errors that don't have an errCode. + // Since the rustErrorToHubError function is not called in these cases, we attempt conversion here. + const hubErr: HubError = err.errCode ? err : rustErrorToHubError(err); + let grpcCode: number; if (err.errCode === "unauthenticated") { grpcCode = status.UNAUTHENTICATED; @@ -231,10 +236,10 @@ export const toServiceError = (err: HubError): ServiceError => { grpcCode = status.UNKNOWN; } const metadata = new Metadata(); - metadata.set("errCode", err.errCode); - return Object.assign(err, { + metadata.set("errCode", hubErr.errCode); + return Object.assign(hubErr, { code: grpcCode, - details: err.message, + details: hubErr.message, metadata, }); }; @@ -752,6 +757,13 @@ export default class Server { const { parentCastId, parentUrl, pageSize, pageToken, reverse } = call.request; + if (!parentCastId && !parentUrl) { + callback( + toServiceError(new HubError("bad_request.invalid_param", "Parent cast identifier must be provided")), + ); + return; + } + const castsResult = await this.engine?.getCastsByParent(parentCastId ?? parentUrl ?? "", { pageSize, pageToken,