Skip to content

Commit

Permalink
fix: return better error message (#2077)
Browse files Browse the repository at this point in the history
## 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

<!-- start pr-codex -->

---

## 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}`

<!-- end pr-codex -->
  • Loading branch information
Wazzymandias committed Jun 21, 2024
1 parent 2d26d30 commit eacf29c
Show file tree
Hide file tree
Showing 5 changed files with 39 additions and 17 deletions.
6 changes: 6 additions & 0 deletions .changeset/yellow-chefs-dance.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@farcaster/core": patch
"@farcaster/hubble": patch
---

fix: http endoint return not found instead of internal database error
7 changes: 2 additions & 5 deletions apps/hubble/src/addon/src/store/cast_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
14 changes: 7 additions & 7 deletions apps/hubble/src/addon/src/store/message.rs
Original file line number Diff line number Diff line change
@@ -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;

Expand Down Expand Up @@ -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(),
));
}
}

Expand Down
11 changes: 9 additions & 2 deletions apps/hubble/src/addon/src/store/store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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())));
}
}
Expand Down Expand Up @@ -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());
Expand Down
18 changes: 15 additions & 3 deletions apps/hubble/src/rpc/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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,
});
};
Expand Down Expand Up @@ -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,
Expand Down

0 comments on commit eacf29c

Please sign in to comment.