h-REA / hREA

A ValueFlows / REA economic network coordination system implemented on Holochain and with supplied Javascript GraphQL libraries
https://docs.hrea.io
Other
142 stars 15 forks source link

retrieving nonexistent agreement record breaks resolver #399

Open LeosPrograms opened 5 months ago

LeosPrograms commented 5 months ago

using clauseOf { id } for records that don't have caluseOf causes the error

ERROR wasm_trace: hc_zome_rea_agreement::__get_agreement_extern:zomes/rea_agreement/zome/src/lib.rs:31 output_type = "core::result::Result<hc_zome_rea_agreement_rpc::ResponseData, holochain_wasmer_common::result::WasmError>"; bytes = [129, 167, 97, 100, 100, 114, 101, 115, 115, 192]; Deserialize("invalid type: unit value, expected tuple struct AgreementAddress")

this appears to be due to an attempt to return a record for a blank agreement address

pospi commented 5 months ago

Is there a failing test for this anywhere at present, or do we need to add coverage? No failure in test_agreement_links.ts- good catch! I think this might be an issue that affects many record types.

Looks like the underlying symptom is a deserialization error being thrown for the input- i.e. the call to get_agreement is being sent a null value rather than an AgreementAddress. So yea what you said (; I am guessing in an app you are using Commitment.clauseOf against a list of Commitments rather than a single one that's known to have an associated Agreement?

See if altering this line for commitment to return record.clauseOf ? ... : null fixes the error. That should give the expected behaviour to avoid calling the zome API if there is no associated value to send.

If that solves the bug, can you please take a look through all resolvers/ and add the same check for any resolver that calls an API with a Maybe<?> value? Your IDE should make it easy to tell which ones are optional, eg. VSCode shows a tooltip with Maybe<Agreement> | undefined when I hover record.clauseOf for Commitment.

It would probably also be best practise to add some failing tests for these but shrug this is actually something that should be caught at the typechecker level (resolvers/commitment pulls the queries/agreement and the latter does not have a type signature for args in agreement: async (root, args)). I am happy to pop a couple of tests in for Agreement and leave it at that?

pospi commented 5 months ago

I just popped a test in, but haven't been able to successfully run it 'cos my conductor is timing out and I don't have time to debug that atm. Hopefully it fails when you run it and making the above changes fixes it?