-
Notifications
You must be signed in to change notification settings - Fork 290
Spin up checks required variables for some Providers
#3265
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.
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
Spin up checks required variables for some Providers
#3265
Conversation
I noticed something unusual about the It is meant to target With the new setup, Is it by design that it was treated as cc: @rylev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noted some problems in the comments, but one other thing is that this would really benefit from tests - those would have provided assurance around what I currently suspect are logic errors. At minimum we should have these test cases:
- The application uses no variables (expected: valid)
- The application uses one variable, is equipped with a single static provider, and:
- the provider has the variable (expected: valid)
- the provider does not have the variable (expected: invalid)
- The application uses one variable, and is equipped with a single dynamic provider (expected: valid)
- The application uses one variable, is equipped with both a dynamic and a static provider, and:
- the static provider has the variable (expected: valid)
- the static provider does not have the variable (expected: valid)
- The application uses one variable, is equipped with two static providers, and:
- the first provider we check has the variable (expected: valid)
- the first provider we check does not have the variable, but the second does (expected: valid)
- neither provider has the variable (expected: invalid)
I am not sure if we write tests for cases where there are multiple variables - in theory we do but in practice the logic there is fairly simple.
crates/expressions/src/provider.rs
Outdated
pub trait Provider: Debug + Send + Sync { | ||
/// Returns the value at the given config path, if it exists. | ||
async fn get(&self, key: &Key) -> anyhow::Result<Option<String>>; | ||
fn kind(&self) -> &ProviderVariableKind; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically this doesn't need &self
for functionality, but I guess maybe it needs it so it can be called polymorphically via a boxed reference?
crates/trigger/src/cli/variable.rs
Outdated
let variables_factor = configured_app.app_state::<VariablesFactor>()?; | ||
|
||
let expression_resolver = variables_factor.expression_resolver(); | ||
expression_resolver.pre_runtime_prepare().await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what pre_runtime_prepare
means in this context. My understanding is that you intend to check if variables references are doomed to fail: can we find a name that expresses that?
crates/expressions/src/lib.rs
Outdated
|
||
match provider.get(&Key(key)).await { | ||
Ok(Some(_)) => return Ok(()), | ||
Err(_) | Ok(None) => return self.internal.resolve_variable(key).map(|_| ()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am pretty sure the logic here is wrong. Suppose I have a variable myvar
and there are two static providers in play. myvar
exists in Provider B but not in Provider A. If Provider A is hit first, it will say myvar
doesn't exist, even though Provider B would have actually satisfied it.
crates/expressions/src/lib.rs
Outdated
async fn check_variable_existence(&self, key: &str) -> Result<()> { | ||
for provider in &self.providers { | ||
if provider.kind() == &ProviderVariableKind::Dynamic { | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am pretty sure the logic here is wrong. If there is any dynamic provider in play, then we cannot assert that the variable is doomed. We do not go on and try to find a static provider (and we definitely do not fail if no static provider has the variable).
crates/expressions/src/lib.rs
Outdated
|
||
match provider.get(&Key(key)).await { | ||
Ok(Some(_)) => return Ok(()), | ||
Err(_) | Ok(None) => return self.internal.resolve_variable(key).map(|_| ()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good for the PR description to show the output on failure - I am not sure what message this would produce and whether it would be suitable for startup time.
Providers
I'm looking at Line 552 in d1eb9ac
If it's only this:
then that I think could be an error in your validation code (where you check the static providers even if dynamic providers are in play). You did also mention that in debugging you saw it going to the environment provider - that's not unexpected in itself because the providers are checked in priority order - you should be concerned only if you don't see it going to the Vault provider. |
I should maybe expand that penultimate comment. When a developer makes a change, and an existing test fails (especially one in the area they're changing), they should treat that as "I broke existing behaviour" until proven otherwise. They should not assume that their changes are correct and have shown up a flaw in the test. That's not to say that existing tests never have flaws. They do! They have loads of flaws! And sometimes it turns out that the code change is correct and the test had a hidden flaw. But it's wise to start from a position of "the problem is likely in the new, untested code" before resorting to "fixing" the test. |
As you rightly pointed out, the validation logic was flawed. So the reason for seeing a Static was an expected occurrence, as it is part of the available Providers. The logic sees Static first and just returns an error, as it couldn't find a variable. The new logic seems to have resolved that issue and should do what is expected. However, there needs to be some updates to the logic, as some |
Before we go further could you write some unit tests please? For example, I'm guessing the change to explicitly check for default values was motivated by discovering a case where a variable should have been allowed but wasn't. Which is a good catch... but... there's no test to identify the case and prevent a regression. Test cases allow the reviewer to look at the covered cases and go either "okay I can see this all works and am reviewing only for maintainability" or "huh but what happens in this weird case, I need to review for correctness." Thanks. |
@itowlson - I just realised there's no way you could have known I included the unit tests you suggested I add, I'm sorry. Here's informing you. Thanks for the review! |
Thanks for that - it definitely moves things in the right direction. Unfortunately, the current tests are hard to read and understand because:
In addition, from what I can tell, the tests don't cover many scenarios. I appreciate that the current format is quite labour-intensive and so I understand why you focused on a few cases. But for example there seems to be nothing covering "there's more than one provider in play" which is core and which was one of the bugs I think we had in a previous iteration. My suggestion for moving this forward would be:
What I'd like the tests to look like is something like:
(you'll probably need an extra knob to set up which vars have defaults, although that could be hardwired? something to consider in the design) I'd find that way easier to understand, and so way easier to check if 1. the test is doing what it says it's doing and 2. whether the test suite covers the cases I'm concerned about. Hope this helps, happy to chat and iterate further if you want to bounce ideas before diving in! |
Adding to the above: it may well be worth keeping a factor test or hook test or whatever to verify that the factor or hook or whatever calls the validator. And that test will likely be quite ceremonious, and that's okay. But that test doesn't need to cover a variety of cases: it's testing only that the call happens, and can trust the |
Thanks for the feedback, @itowlson. Made a new commit that updates the test to
I'd love to receive more feedback on ways to improve this PR. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this update. I'm definitely appreciating the new unit tests - lots more coverage, more clearly named, easier to see what's being tested.
But there's still a lot of confusion about other naming, and the tests are still a bit cluttery to read. Why are there two ways of constructing a resolver? Why are they called Provider? Why don't they return the resolver directly? Why do they take values that are inconvenient to construct?
What I'd suggest you aim for is something like:
let resolver = make_resolver(...);
resolver.validate().await
and look at how you can keep the make_resolver
stuff as simple as possible at the call site. E.g.
let resolver = make_resolver(
&[("a", Some("hello")), ("b", None)], // the variables with their defaults
&[&StaticProvider::with("b", "something"), &DynamicProvider] // the providers in play
);
// or
let resolver = make_resolver(&[("a", Some("hello")])
.with_provider(StaticProvider::...)
.with_provider(DynamicProvider);
And the providers should be actual providers:
struct StaticProvider {
known: HashMap<Key, String>
}
impl Provider for StaticProvider {
fn get(&self, key) {
self.known.get(key) // yep you'll need to massage this
}
}
impl Provider for DynamicProvider {
fn get(&self, _) {
panic!("this should never be called during validation");
}
}
Hope this helps - happy to chat if you want more detail or to explore possible approaches.
fn with_variables( | ||
variable_key: Option<String>, | ||
default: Option<String>, | ||
other_providers: Option<Box<dyn Provider>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does a provider have other providers? The ProviderResolver can have multiple providers: there's no need for a Provider to have its own sub-providers.
resolver: ProviderResolver, | ||
} | ||
|
||
impl DynamicProvider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused. Neither of these providers appears to implement Provider?
|
||
static_provider | ||
.resolver | ||
.validate_variable_existence() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh this validates all variables known to the resolver! The name implies one variable.
Some("baz".to_string()), | ||
Some("foo".to_string()), | ||
Some(Box::new(StaticMockProvider)), | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like you are putting a lot of ceremony in the test (which is where you want to avoid ceremony and focus on the scenario and outcome). You control ::with_variables
: make it take &str
instead of Option<String>
.
struct StaticMockProvider; | ||
|
||
#[spin_world::async_trait] | ||
impl Provider for StaticMockProvider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh this is the provider? I am not sure why we have StaticProvider and StaticMockProvider
} | ||
|
||
#[derive(Debug)] | ||
struct ExtraStaticMockProvider; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...and ExtraStaticMockProvider!
I'd ask you to think about someone (possibly yourself) trying to maintain this in a few months. They're not in the moment like you are - they're going to grapple with "okay what is the difference between these three "static provider" things and how do they relate and when do I use one rather than the other." Naming is a huge part of guiding the reader towards understanding.
(And none of us are perfect at it, because it's hard!)
} | ||
|
||
#[tokio::test(flavor = "multi_thread")] | ||
async fn static_provider_with_two_static_providers_neither_has_data() -> anyhow::Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm appreciating the test names - these are much more helpful.
But "static provider with two static providers" mystifies me. I think what this is talking about is the case where there are two static providers? So just "two static providers" would do, you don't need another provider to "with" them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's already a tests
module in expressions/lib.rs
- maybe use that instead of creating a new file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually now I look at this I can't figure out how it's getting incorporated. Or where the conditional compilation directive is. Is this some Cargo convention I've not learned about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also just realised it's possible. There are a couple of tests in Spin that are similar. Apparently, if the folder is named "tests", Cargo automatically incorporates it.
The reason I created a new file for it was that I thought separating the tests would be better, as the tests in lib, while similar, test for different purposes. They also have a different boilerplate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this update. I like the pattern you're establising with the builder object. I've left a few nitpicky notes where I think some of the ceremony can be removed to tighten, but I'm really happy with the way this is going now. Thank you for your patience!
|
||
fn make_resolver( | ||
self, | ||
key: Option<&str>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't need to be an Option. How about if you did .with_variable(name, default)
(in the same way as with_provider
), then make_resolver()
with no args: that would allow you to specify zero, one or many variables.
Self::default() | ||
} | ||
|
||
fn with_provider(mut self, provider: Box<dyn Provider>) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can have this take an impl Provider
instead of a Box<dyn>
- that would save the caller having to box it at the call site (you would box it inside with_provider
).
#[tokio::test(flavor = "multi_thread")] | ||
async fn single_static_provider_with_no_variable_provided_is_valid() -> anyhow::Result<()> { | ||
let resolver = ResolverTester::new() | ||
.with_provider(Box::new(StaticMockProvider::with_variables("foo", "bar"))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just call it StaticProvider
. The Mock
isn't really adding anything in the test context (and "mock" is a testing term of art with a specific meaning that doesn't apply here):
.with_provider(Box::new(StaticMockProvider::with_variables("foo", "bar"))) | |
.with_provider(StaticProvider::with_variable("foo", "bar"))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good - the checking logic is clear and the tests are readable and give good coverage. Just a few naming, style and coverage nits. Thanks!
crates/expressions/Cargo.toml
Outdated
thiserror = { workspace = true } | ||
|
||
[dev-dependencies] | ||
spin-world = { path = "../world" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this dependency still needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the Provider
trait that you're implementing uses async_trait
directly rather than the spin-world
re-export: https://github.com/spinframework/spin/blob/main/crates/expressions/src/provider.rs. Are you sure you need the spin-world
re-export?
(There is also a re-export from spin-locked-app
which is already a dependency.)
Self::default() | ||
} | ||
|
||
fn with_dynamic_provider(mut self) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change to bake provider kinds into the resolver seems unlovely - I appreciate the goal of making the tests easier to read but for me this begins to move into then territory of "hiding what we are actually testing." (It also seems to rule out a category of tests along the lines of "a single provider has one variable but not another".)
} | ||
|
||
#[tokio::test(flavor = "multi_thread")] | ||
async fn single_static_provider_with_no_variable_provided_is_valid() -> anyhow::Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name here is a bit confusing. The provider provides a variable. It's more that the resolver has no variables.
self | ||
} | ||
|
||
fn with_variable(mut self, (key, default): (&str, Option<&str>)) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No point tupling these when they are method args - this was needed if we were going to put them in a slice (to group key-default pairs) but now it's implicit by them being args to the same method call.
} | ||
|
||
#[tokio::test(flavor = "multi_thread")] | ||
async fn if_there_is_a_dynamic_provider_and_a_static_provider_then_validation_succeeds_even_if_a_static_provider_with_the_variable_is_in_play( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But in this case the static provider does not have the variable? The resolver needs baz
and the provider has foo
.
(There's a case that names that guide the writer/reader might help avoid errors like this - it's hard to have intuition about things called foo
and baz
.)
} | ||
|
||
#[tokio::test(flavor = "multi_thread")] | ||
async fn if_there_is_two_static_providers_where_first_provider_does_not_have_data_while_second_provider_does( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a comment to distinguish this from the previous test. (I know the case here is "we don't barf just because the first one did, we check all providers" and agree it's worth testing - but it may be less obvious to a future reader.)
assert!(resolver.validate_variables().await.is_err()); | ||
|
||
Ok(()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you previously had a test for "providers don't have the data but there is a default value"? That test is needed.
crates/expressions/src/lib.rs
Outdated
} | ||
|
||
/// Validates `Provider`(s) that are `ProviderVariableKind::Static` provide their variables at startup. | ||
pub async fn validate_variables(&self) -> Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to change the name away from validate
because that word is strongly overloaded. What are we checking here? That no variable is doomed?
crates/expressions/src/lib.rs
Outdated
Ok(PreparedResolver { variables }) | ||
} | ||
|
||
/// Validates `Provider`(s) that are `ProviderVariableKind::Static` provide their variables at startup. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is misleading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All right, this looks good - thank you! I did have a couple of naming nits but they are not blockers and shouldn't affect correctness.
However, could you squash the commits please - the history is quite long and I don't think we need to keep it.
But otherwise good to go!
crates/expressions/src/lib.rs
Outdated
} | ||
|
||
/// Ensures that all required variables are resolvable at startup | ||
pub async fn ensure_required_variables_resolved(&self) -> Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd go with the form in the comment:
pub async fn ensure_required_variables_resolved(&self) -> Result<()> { | |
pub async fn ensure_required_variables_resolvable(&self) -> Result<()> { |
|
||
Ok(()) | ||
} | ||
// This is a bit of an edge case, but we want to ensure that if there are two or more static providers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not really an "edge case": there's no reason to expect that the first provider in line is the one with the answer! I'd just drop this phrasing:
// This is a bit of an edge case, but we want to ensure that if there are two or more static providers | |
// Ensure that if there are two or more static providers |
a736c20
to
5acc79b
Compare
Sorry to be fussy but the commit message doesn't really reflect the purpose or impact of the commit, only a minor part of the implementation. Could you change it to e.g. "Check required variables are potentially resolvable" or something similar please? Thanks. |
5acc79b
to
b0447ea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One test name looks iffy. It's not a blocker but if you have a moment to check then that would be awesome. Thanks!
} | ||
|
||
#[tokio::test(flavor = "multi_thread")] | ||
async fn if_there_is_a_dynamic_provider_and_a_static_provider_then_validation_succeeds_even_if_a_static_provider_with_the_variable_is_in_play( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
async fn if_there_is_a_dynamic_provider_and_a_static_provider_then_validation_succeeds_even_if_a_static_provider_with_the_variable_is_in_play( | |
async fn if_there_is_a_dynamic_provider_and_a_static_provider_then_validation_succeeds_even_if_a_static_provider_without_the_variable_is_in_play( |
or am I misreading?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wording is confusing, apologies. Although it has a variable, the naming doesn't need to say "static_provider_with_the_variable
"
I think if_there_is_a_dynamic_provider_and_a_static_provider_then_validation_succeeds_even_if_there_is_a_variable_in_play
might be a better name
Adds new dynamism metadata for Providers. Allow for no default value before runtime if Provider is a Dynamic type. PR also includes unit tests Signed-off-by: Aminu Oluwaseun Joshua <[email protected]>
b0447ea
to
0e4c4c1
Compare
New logic only allows no default variable before runtime if the Provider is of a Dynamic type.
Closes #3115, including tests
The PR checks if the variable default isn't provided when required. A new hook handles the check: VariablesValidatorHook.
Inspired by @itowlson's #3115 (comment), new metadata has been added to Providers.
It runs a validation where there's no Dynamic variable provider. One should get an error like this.
NB. It's variables now, as there's a possibility of having multiple variables.