Validate components are not granted access to undefined k/v stores

[itowlson]

ivan@hecate:~/testing/kvmadness$ spin up
Error: host component error: Component kvmadness is granted access to key-value store 'bibblybobbly', which is not defined

Caused by:
    Component kvmadness is granted access to key-value store 'bibblybobbly', which is not defined
Error: exit status: 1
ivan@hecate:~/testing/kvmadness$ spin up --runtime-config-file runtime-config.toml
Logging component stdio to ".spin/logs/"
Storing default key-value data to ".spin/my_super_duper_store.db"

Serving http://127.0.0.1:3000
Available Routes:
  kvmadness: http://127.0.0.1:3000 (wildcard)

(I'm not mad keen on the "host component error" string or the duplicated text, but this is a "should rarely happen" error so I'm wary of changing things that might break existing error paths.)

[rylev]

I think it might be somewhat confusing to users what it means to "define" a key value store. Perhaps we should more directly point to that fact that they are not creating a runtime config that configures a non-default store?

[lann]

I'm not sure that we need to frame key_value_stores in terms of access control. Maybe something like:

Suggested change

	let err = format!("Component {} is granted access to key-value store '{allowed}', which is not defined", component.id());
	let err = format!("Component {} uses the key-value store '{allowed}', which is not defined", component.id());

[itowlson]

@rylev I tried adding more pointers and I like it but it really magnifies the effect of the doubling printing:

ivan@hecate:~/testing/kvmadness$ spin up
Error: host component error: One or more components use key-value stores which are not defined.
Check the spelling, or pass a runtime configuration file that defines these stores.
See https://developer.fermyon.com/spin/dynamic-configuration#key-value-store-runtime-configuration
Details:
- Component kvmadness uses store 'bibblybobbly'

Caused by:
    One or more components use key-value stores which are not defined.
    Check the spelling, or pass a runtime configuration file that defines these stores.
    See https://developer.fermyon.com/spin/dynamic-configuration#key-value-store-runtime-configuration
    Details:
    - Component kvmadness uses store 'bibblybobbly'
Error: exit status: 1

I'll see if I can find what's doing this; if it's not an easy and obviously-safe fix, then maybe we just accept it and record a separate issue - again, not something people should run into too often!

[itowlson]

Okay I am not sure how to fix the double printing. validate_app returns an anyhow::Error which the caller wraps in a Error::HostComponentError to conform to the return type of load_app. HostComponentError is declared as:

    #[error("host component error: {0:#}")]
    HostComponentError(#[source] anyhow::Error),

If I remove the #[source] from the attribute then I get "Error: host component error: One or more components use..." which is fine but I suspect it loses context for other, nastier, things that get turned into HCEs... If I remove the {0:#} from the attribute, it probably preserves context better (?), but the printout is now "Error: host component error [blank line] Caused by: One or more components use..." @lann is either of these two changes acceptable and if so do you have a recommendation? Or should I wrap this error differently so as to avoid the problem altogether?

[itowlson]

Or we could create a new ValidationError case and declare it as #[error(transparent)] so it gets passed through without the "host component error" bit.

[itowlson]

Now looks like:

ivan@hecate:~/testing/kvmadness$ spin up
Error: One or more components use key-value stores which are not defined.
Check the spelling, or pass a runtime configuration file that defines these stores.
See https://developer.fermyon.com/spin/dynamic-configuration#key-value-store-runtime-configuration
Details:
- Component kvmadness uses store 'bibblybobbly'
- Component moarmadness uses store 'bibblybobbly'
- Component moarmadness uses store 'hoitytoity'
Error: exit status: 1

[lann]

I personally find it easier to read long tests like this if assertions are grouped with the side-effect-inducing call they are testing, e.g.:

Suggested change

	store.delete("bar")?;
	ensure!(!store.exists("bar")?);
	ensure!(matches!(store.get("bar"), Err(Error::NoSuchKey)));
	store.delete("bar")?;
	ensure!(!store.exists("bar")?);
	ensure!(matches!(store.get("bar"), Err(Error::NoSuchKey)));

[itowlson]

actually none of this test body is needed because spin up will barf during validation and never actually run it so I just deleted it all. Wake the heck up, Ivan

[lann]

assert_eq! gives more useful errors, e.g.

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `4`,
 right: `3`', src/main.rs:5:1

I think it would also remove the need to cast as &[_] in this case, but my confidence in understanding Rust's coercion rules is NOT HIGH.

Aaaand I was wrong, but you can simplify it thusly (with or without the change to assert_eq:

assert_eq!(b"bas", &*store.get("bar")?);

[kate-goldenring]

I wonder if we need to explicitly call out that only the "default" store is implicitly defined. However, this is explained in the docs that are linked so i think this works and if there is confusion we can always iterate on user feedback

[itowlson]

Yeah, users should never see an error about the default store so hopefully they won't think they need to define it!.