Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: Test fetching indexer config from registry #976

Merged
merged 5 commits into from
Aug 5, 2024
Merged

test: Test fetching indexer config from registry #976

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a82b24f
feat: Wrap json rpc client to allow mocking
morgsmccauley Aug 4, 2024
b177fba
test: Test indexer fetching from registry
morgsmccauley Aug 4, 2024
40960f3
feat: Remove unnecessary null check
morgsmccauley Aug 4, 2024
153dde2
test: Test invalid config response
morgsmccauley Aug 4, 2024
f3b5337
ci: Lock rust version in coordinator ci
morgsmccauley Aug 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions .github/workflows/coordinator-ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,13 @@ jobs:
uses: arduino/setup-protoc@v2
with:
repo-token: ${{ secrets.GITHUB_TOKEN }}
- name: Install Rust
uses: actions-rs/toolchain@v1
with:
toolchain: 1.75.0
override: true
profile: minimal
components: rustfmt
- name: Check
working-directory: ./coordinator
run: cargo check
Expand All @@ -33,6 +40,13 @@ jobs:
uses: arduino/setup-protoc@v2
with:
repo-token: ${{ secrets.GITHUB_TOKEN }}
- name: Install Rust
uses: actions-rs/toolchain@v1
with:
toolchain: 1.75.0
override: true
profile: minimal
components: rustfmt
- name: Test
working-directory: ./coordinator
run: cargo test
Expand Down
198 changes: 174 additions & 24 deletions coordinator/src/registry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,13 +85,39 @@ impl std::ops::Deref for IndexerRegistry {
}
}

#[cfg(not(test))]
use JsonRpcClientWrapperImpl as JsonRpcClientWrapper;
#[cfg(test)]
use MockJsonRpcClientWrapperImpl as JsonRpcClientWrapper;

struct JsonRpcClientWrapperImpl {
inner: JsonRpcClient,
}

#[cfg_attr(test, mockall::automock)]
impl JsonRpcClientWrapperImpl {
pub fn new(inner: JsonRpcClient) -> Self {
Self { inner }
}

pub async fn call<M>(
&self,
method: M,
) -> near_jsonrpc_client::MethodCallResult<M::Response, M::Error>
where
M: near_jsonrpc_client::methods::RpcMethod + 'static,
{
self.inner.call(method).await
}
}

#[cfg(test)]
pub use MockRegistryImpl as Registry;
#[cfg(not(test))]
pub use RegistryImpl as Registry;

pub struct RegistryImpl {
json_rpc_client: JsonRpcClient,
json_rpc_client: JsonRpcClientWrapper,
registry_contract_id: AccountId,
}

Expand All @@ -100,12 +126,23 @@ impl RegistryImpl {
const LIST_METHOD: &'static str = "list_all";
const GET_METHOD: &'static str = "read_indexer_function";

#[cfg(test)]
pub fn new(
registry_contract_id: AccountId,
json_rpc_client_wrapper: JsonRpcClientWrapper,
) -> Self {
Self {
registry_contract_id,
json_rpc_client: json_rpc_client_wrapper,
}
}

pub fn connect(registry_contract_id: AccountId, rpc_url: &str) -> Self {
let json_rpc_client = JsonRpcClient::connect(rpc_url);

Self {
registry_contract_id,
json_rpc_client,
json_rpc_client: JsonRpcClientWrapper::new(json_rpc_client),
}
}

Expand Down Expand Up @@ -196,32 +233,145 @@ impl RegistryImpl {
.context("Failed to fetch indexer")?;

if let QueryResponseKind::CallResult(call_result) = response.kind {
// Handle case where call returns successfully but returns null due to not matching
let raw_json: Value = serde_json::from_slice(&call_result.result)
Copy link
Collaborator Author

@morgsmccauley morgsmccauley Aug 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why this should be needed. If the returned response is set to null, either intentionally from the registry contract, or through some error in the RPC, parsing to an Option will handle it correctly. Added relevant tests to prove this.

@darunrs can you please provide some context here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the idea here was that the call has two successful responses, which are null and some indexer data. There's also edge cases where the call is successful but either incorrectly returns null, or incorrectly returns some invalid data. So, given that, I wanted to handle null and actual data separately. The reason is, if I do from_slice::Option<T> on something, the case where the returned data is null, and the case where the returned data is incorrect JSON, get combined. In both cases, we return Option. And as a result, we never have a change to log the invalid JSON or handle it in anyway. Which I think we should do, as returning incorrect JSON from a successful call to RPC is anomalous, and shouldn't happen in a perfect world. If this kind of thing causes more problems, we can at least know we have logs of the invalid JSON. We can't do much about correct/incorrect null responses though, so I still have it return None. But I changed some logic elsewhere to expect a non-Null where it makes sense.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except based on the tests you added, it seems the parse failure does in fact still trigger the parse error. Which is odd, as my testing showed it didn't. Maybe I did something wrong in it. Anyway, you added a unit test which covers the case of successfully returning invalid JSON anyway. And it correctly threw an error. So, we should be good.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that serde should only ever succeed if the JSON is valid. So for an Option, that would either be null or a JSON object which matches the target type. I even tried parsing "" which still resulted in an error.

It seems the error we faced recently might be somewhere else 🤔

.context("Failed to deserialize config from JSON provided by RPC call")?;
if raw_json.is_null() {
return Ok(None);
}

// Handle case where we now expect returned JSON to actually parse into config
let config: registry_types::IndexerConfig =
serde_json::from_slice::<registry_types::IndexerConfig>(&call_result.result)
let config: Option<registry_types::IndexerConfig> =
serde_json::from_slice(&call_result.result)
.context("Failed to deserialize config from JSON provided by RPC call")?;
let indexer = IndexerConfig {
account_id: account_id.clone(),
function_name: function_name.to_string(),
code: config.code,
schema: config.schema,
rule: config.rule,
start_block: config.start_block,
updated_at_block_height: config.updated_at_block_height,
created_at_block_height: config.created_at_block_height,
deleted_at_block_height: config.deleted_at_block_height,
};

return Ok(Some(indexer));
return if let Some(config) = config {
Ok(Some(IndexerConfig {
account_id: account_id.clone(),
function_name: function_name.to_string(),
code: config.code,
schema: config.schema,
rule: config.rule,
start_block: config.start_block,
updated_at_block_height: config.updated_at_block_height,
created_at_block_height: config.created_at_block_height,
deleted_at_block_height: config.deleted_at_block_height,
}))
} else {
Ok(None)
};
}

anyhow::bail!("Invalid registry response")
}
}

#[cfg(test)]
mod tests {
use super::*;

use mockall::predicate::*;

#[tokio::test]
async fn parses_non_existant_config() {
let mut mock_json_rpc_client = JsonRpcClientWrapper::default();
mock_json_rpc_client
.expect_call::<RpcQueryRequest>()
.with(always())
.returning(|_| {
Ok(near_jsonrpc_client::methods::query::RpcQueryResponse {
kind: QueryResponseKind::CallResult(near_primitives::views::CallResult {
result: "null".as_bytes().to_vec(),
logs: vec![],
}),
block_height: Default::default(),
block_hash: Default::default(),
})
});

let registry = RegistryImpl::new("registry".parse().unwrap(), mock_json_rpc_client);

let config = registry
.fetch_indexer(&"account".parse().unwrap(), "function")
.await
.unwrap();

assert!(config.is_none());
}

#[tokio::test]
async fn parses_existing_config() {
let mut mock_json_rpc_client = JsonRpcClientWrapper::default();
mock_json_rpc_client
.expect_call::<RpcQueryRequest>()
.with(always())
.returning(|_| {
Ok(near_jsonrpc_client::methods::query::RpcQueryResponse {
kind: QueryResponseKind::CallResult(near_primitives::views::CallResult {
result: serde_json::json!({
"code": "code",
"schema": "schema",
"rule": {
"affected_account_id": "queryapi.dataplatform.near",
"kind": "ACTION_ANY",
"status": "SUCCESS"
},
"start_block": {
"HEIGHT": 0,
},
"updated_at_block_height": 0,
"created_at_block_height": 0,
"deleted_at_block_height": 0
})
.to_string()
.as_bytes()
.to_vec(),
logs: vec![],
}),
block_height: Default::default(),
block_hash: Default::default(),
})
});

let registry = RegistryImpl::new("registry".parse().unwrap(), mock_json_rpc_client);

let config = registry
.fetch_indexer(&"account".parse().unwrap(), "function")
.await
.unwrap();

assert!(config.is_some());
}

#[tokio::test]
async fn propagates_parse_failures() {
let mut mock_json_rpc_client = JsonRpcClientWrapper::default();
mock_json_rpc_client
.expect_call::<RpcQueryRequest>()
.with(always())
.returning(|_| {
Ok(near_jsonrpc_client::methods::query::RpcQueryResponse {
kind: QueryResponseKind::CallResult(near_primitives::views::CallResult {
result: serde_json::json!({
"rule": "invalid",

"code": "code",
"schema": "schema",
"start_block": {
"HEIGHT": 0,
},
"updated_at_block_height": 0,
"created_at_block_height": 0,
"deleted_at_block_height": 0
})
.to_string()
.as_bytes()
.to_vec(),
logs: vec![],
}),
block_height: Default::default(),
block_hash: Default::default(),
})
});

let registry = RegistryImpl::new("registry".parse().unwrap(), mock_json_rpc_client);

let parse_result = registry
.fetch_indexer(&"account".parse().unwrap(), "function")
.await;

assert!(parse_result.is_err());
}
}
Loading