schnauzer: add if_not_null template helper #3838

sam-berning · 2024-03-20T18:27:51Z

Issue number:

N/A

Description of changes:

There's currently not a safe way to render potentially-null falsy values in a schnauzer template. Gating with {{#if falsy}} will skip rendering, and {{default false falsy}} will always render something, even if falsy is unset.

{{#if-not-null falsy}} checks only that the value is not null, it doesn't care if it's falsy. This allows us to conditionally render template portions that may contain falsy values.

So for example, in packages/os/host-containers-toml, we currently render:

{{#each settings.host-containers}}
[host-containers."{{{@key}}}"]
...
{{#if this.enabled}}
enabled = {{{this.enabled}}}
{{/if}}
{{#if this.superpowered}}
superpowered = {{{this.superpowered}}}
{{/if}}
...
{{/each}}

where this.enabled and this.superpowered are booleans. If they are explicitly set to false, they will not be rendered. In this particular case it doesn't cause a behavior issue because those settings default to false, but it could cause problems in a case where the default is true.

With this helper, we can instead write:

{{#each settings.host-containers}}
[host-containers."{{{@key}}}"]
...
{{#if-not-null this.enabled}}
enabled = {{{this.enabled}}}
{{/if-not-null}}
{{#if-not-null this.superpowered}}
superpowered = {{{this.superpowered}}}
{{/if-not-null}}
...
{{/each}}

This will correctly render enabled = false and superpowered = false.

Related: #3724, #3777

Testing done:

updated the host-containers-toml template using the {{#if-not-null}} helper, and configured settings to reflect the three different states we care about (set to a truthy value, set to a falsy value, and unset) and confirmed that the template rendered as expected.

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

jmt-lab

LGTM

sam-berning · 2024-03-20T21:39:17Z

^ fixed doc comment

arnaldo2792

Could you please add in the description how this helper improves or changes how we render settings today? I didn't review the other PRs so I don't fully understand how this will affect templates in the future (e.g. I had a similar problem in another PR and we could probably solve both problems with one helper)

arnaldo2792 · 2024-03-20T23:03:58Z

sources/api/schnauzer/src/helpers.rs

+        renderctx: &mut RenderContext<'reg, 'rc>,
+        out: &mut dyn Output,
+    ) -> Result<(), RenderError> {
+        trace!("Starting default helper");


This is not the default helper

sources/api/schnauzer/src/helpers.rs

webern · 2024-03-20T23:12:00Z

sources/api/schnauzer/src/v2/import/helpers.rs

@@ -66,6 +66,7 @@ fn all_helpers() -> HashMap<ExtensionName, HashMap<HelperName, Box<dyn HelperDef
            "default" => helper!(handlebars_helpers::default),
            "join_array" => helper!(handlebars_helpers::join_array),
            "join_map" => helper!(handlebars_helpers::join_map),
+            "if-not-null" => Box::new(handlebars_helpers::IfNotNullHelper),


It looks like the naming convention is underscore:

Suggested change

"if-not-null" => Box::new(handlebars_helpers::IfNotNullHelper),

"if_not_null" => Box::new(handlebars_helpers::IfNotNullHelper),

the naming convention is a bit all over the place (see ecr-prefix, tuf-prefix above), but I'm good to stick with the convention within std

There used to be a comment to help us remember which way we are supposed to name them. I know because I wrote it because Tom told me to! Maybe this comment should be restored!

bottlerocket/sources/api/schnauzer/src/lib.rs

Line 121 in b530f30

// Prefer snake case for helper names (we accidentally created a few with kabob case)

I need to fix the commit title to be snake case too so I'll add this back

webern · 2024-03-20T23:17:57Z

sources/api/schnauzer/src/helpers.rs

+    #[test]
+    fn null_value() {
+        let result = setup_and_render_template(
+            "{{#if-not-null setting}}foo{{/if-not-null}}",


Is it possible to create this so that the natural/built-in conditionals work as expected, i.e. the native #if?

Suggested change

"{{#if-not-null setting}}foo{{/if-not-null}}",

"{{#if not_null setting}}foo{{/if}}",

I couldn't get this to work. for some reason handlebars treats a boolean return from a helper as a string, so the #if helper resolves to true when we return false from not_null.

it could work if I returned an empty string on false, because handlebars treats empty string as falsy, but it felt hackier to me and essentially limits us to using it in the same scenarios as #if-not-null

I couldn't get this to work. for some reason handlebars treats a boolean return from a helper as a string

That's because the value returned by the helpers is a string, not a boolean. I had the same issue.

sam-berning · 2024-03-20T23:28:07Z

Could you please add in the description how this helper improves or changes how we render settings today? I didn't review the other PRs so I don't fully understand how this will affect templates in the future (e.g. I had a similar problem in another PR and we could probably solve both problems with one helper)

updated description with an example

arnaldo2792 · 2024-03-21T15:16:10Z

For your use case, wouldn't it be simpler to just do:

{{#each settings.host-containers}}
[host-containers."{{{@key}}}"]
...
enabled = {{default false this.enabled}}
superpowered = {{default true this.superpowered}}
...
{{/each}}

We know that by default, those two values are false, it should be OK to just default to those values.

bcressey

Nice!

sam-berning · 2024-03-21T17:28:14Z

For your use case, wouldn't it be simpler to just do:
{{#each settings.host-containers}}
[host-containers."{{{@key}}}"]
...
enabled = {{default false this.enabled}}
superpowered = {{default true this.superpowered}}
...
{{/each}}
We know that by default, those two values are false, it should be OK to just default to those values.

Yeah that would work behavior-wise in this case. But I don't think we want both host-containers and the template to have to be aware and manage default values for the settings. This approach leans towards giving that responsibility fully to host-containers, but @bcressey also proposed that we could set defaults in the config file and make it an error for a setting to be unset in host-containers.

bcressey · 2024-03-21T18:22:31Z

I am sympathetic toward @sam-berning's view that if_not_null lets us directly reuse the existing logic from "read settings from API, deal with None values somehow" to "read settings from config, deal with None values somehow".

We can evolve the client programs and the templates over time but I like if_not_null because it gives us a simple rule to follow in the code reviews ("always use if_not_null, not if, for these conversions") that lets us avoid the surprises (empty strings and other falsy values).

There's currently not a safe way to render potentially-null falsy values in a schnauzer template. Gating with `{{#if falsy}}` will skip rendering, and `{{default false falsy}}` will always render something, even if `falsy` is unset. `{{#if_not_null falsy}}` checks only that the value is not null, it doesn't care if it's falsy. This allows us to conditionally render template portions that may contain falsy values. Signed-off-by: Sam Berning <[email protected]>

sam-berning requested review from bcressey, webern and jmt-lab March 20, 2024 18:27

jmt-lab approved these changes Mar 20, 2024

View reviewed changes

sam-berning force-pushed the not-null-template-helper branch from 5da2c90 to a57c8f3 Compare March 20, 2024 21:38

arnaldo2792 reviewed Mar 20, 2024

View reviewed changes

webern reviewed Mar 20, 2024

View reviewed changes

sam-berning force-pushed the not-null-template-helper branch 3 times, most recently from 9024db7 to 6d8139b Compare March 20, 2024 23:45

bcressey approved these changes Mar 21, 2024

View reviewed changes

webern approved these changes Mar 21, 2024

View reviewed changes

sam-berning force-pushed the not-null-template-helper branch from 6d8139b to d549604 Compare March 21, 2024 19:30

sam-berning changed the title ~~schnauzer: add if-not-null template helper~~ schnauzer: add if_not_null template helper Mar 21, 2024

webern approved these changes Mar 21, 2024

View reviewed changes

sam-berning merged commit 4f80bf4 into bottlerocket-os:develop Mar 21, 2024
50 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

schnauzer: add if_not_null template helper #3838

schnauzer: add if_not_null template helper #3838

sam-berning commented Mar 20, 2024 •

edited

Loading

jmt-lab left a comment

sam-berning commented Mar 20, 2024

arnaldo2792 left a comment

arnaldo2792 Mar 20, 2024

webern Mar 20, 2024

sam-berning Mar 20, 2024

webern Mar 21, 2024

sam-berning Mar 21, 2024

webern Mar 20, 2024

sam-berning Mar 20, 2024

arnaldo2792 Mar 21, 2024

sam-berning commented Mar 20, 2024

arnaldo2792 commented Mar 21, 2024

bcressey left a comment

sam-berning commented Mar 21, 2024

bcressey commented Mar 21, 2024

	"if-not-null" => Box::new(handlebars_helpers::IfNotNullHelper),
	"if_not_null" => Box::new(handlebars_helpers::IfNotNullHelper),

	"{{#if-not-null setting}}foo{{/if-not-null}}",
	"{{#if not_null setting}}foo{{/if}}",

schnauzer: add if_not_null template helper #3838

schnauzer: add if_not_null template helper #3838

Conversation

sam-berning commented Mar 20, 2024 • edited Loading

jmt-lab left a comment

Choose a reason for hiding this comment

sam-berning commented Mar 20, 2024

arnaldo2792 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-berning commented Mar 20, 2024

arnaldo2792 commented Mar 21, 2024

bcressey left a comment

Choose a reason for hiding this comment

sam-berning commented Mar 21, 2024

bcressey commented Mar 21, 2024

sam-berning commented Mar 20, 2024 •

edited

Loading