test: Test fetching indexer config from registry #976
Conversation
| @@ -196,32 +233,105 @@ 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) | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🤔
Added tests for fetching individual indexer config from registry contract. This was mostly an exercise to understand the behaviour of parsing registry contract responses, as well as understanding the changes that were made recently. A thin wrapper (
JsonRpcClientWrapper) has been created to allow mocking of the JSON RPC Client responses.