Properly deal with nil values in `convert.FromTyped` #1511

pietern · 2024-06-20T15:34:56Z

Changes

When a configuration defines:

run_as:

It first showed up as run_as -> nil in the dynamic configuration only to later be converted to run_as -> {} while going through typed conversion. We were using the presence of a key to initialize an empty value. This is incorrect and it should have remained a nil value.

This conversion was happening in convert.FromTyped where any struct always returned a map value. Instead, it should only return a map value in any one of these cases: 1) the struct has elements, 2) the struct was originally a map in the dynamic configuration, or 3) the struct was initialized to a non-empty pointer value.

Stacked on top of #1516 and #1518.

Tests

Unit tests pass.
Integration tests pass.
Manually ran through bundle CRUD with a bundle without resources.

libs/dyn/convert/from_typed.go

bundle/config/mutator/process_target_mode.go

pietern · 2024-06-20T15:36:04Z

bundle/config/mutator/run_as_test.go

@@ -18,7 +18,7 @@ import (

 func allResourceTypes(t *testing.T) []string {
 	// Compute supported resource types based on the `Resources{}` struct.
-	r := config.Resources{}
+	r := &config.Resources{}


Needed because an empty struct with a nil reference returns a nil value.

pietern · 2024-06-20T15:36:29Z

bundle/config/mutator/run_as_test.go

+	// This is not part of the typed configuration, so we need to add it manually.
+	v, err = dyn.Set(v, "resources", dyn.V(map[string]dyn.Value{}))
+	require.NoError(t, err)
+


Needed because the resources key is no longer populated if empty in the typed value.

bundle/deploy/terraform/convert_test.go

pietern · 2024-06-20T15:37:00Z

bundle/permissions/filter.go

+			if dyn.IsNoSuchKeyError(err) {
+				return v, nil
+			}
+


Needed because "resources" may not exist (this is the case in some tests).

This cherry-picks from #1490 to address an issue that came up in #1511. The function `dyn.SetByPath` requires intermediate values to be present. If they are not, it returns an error that it cannot index a map. This is not an issue on main where the intermediate maps are always created, even if they are not present in the dynamic configuration tree. As of #1511 we'll no longer populate empty maps for empty structs if they are not explictly set (i.e. a non-nil pointer). We can write a bool pointer to avoid this issue altogether.

## Changes This cherry-picks from #1490 to address an issue that came up in #1511. The function `dyn.SetByPath` requires intermediate values to be present. If they are not, it returns an error that it cannot index a map. This is not an issue on main, where the intermediate maps are always created, even if they are not present in the dynamic configuration tree. As of #1511, we'll no longer populate empty maps for empty structs if they are not explicitly set (i.e., a non-nil pointer). This change writes a bool pointer to avoid this issue altogether. ## Tests Unit tests pass.

## Changes This came up in integration testing for #1511. One of the tests converted a `map[string]any` to a dynamic value and encountered a `nil` and errored out. We can safely return a nil in this case. ## Tests Unit test passes.

shreyas-goenka

thanks! just some minor comments.

libs/dyn/convert/from_typed.go

bundle/deploy/terraform/convert_test.go

shreyas-goenka

Could you also update the top level doc string above the FromTyped function and document the significance of ref? It's no longer about just the location but also now about the presence or absence of a value in the configuration tree.

#1514) ## Changes With #1507 and #1511 we are clarifying the semantics associated with `dyn.InvalidValue` and `dyn.NilValue`. An invalid value is the default zero value and is used to signals the complete absence of the value. A nil value, on the other hand, is a valid value for a piece of configuration and signals explicitly setting a key to nil in the configuration tree. In keeping with that theme, this PR returns `dyn.InvalidValue` instead of `dyn.NilValue` at error sites. This change is not expected to have a material change in behaviour and is being done to set the right convention since we have well-defined semantics associated with both `NilValue` and `InvalidValue`. ## Tests Unit tests and integration tests pass. Also manually scanned the changes and the associated call sites to verify the `NilValue` value itself was not being relied upon.

CLI: * Add link to documentation for Homebrew installation to README ([#1505](#1505)). * Fix `databricks configure` to use `DATABRICKS_CONFIG_FILE` environment variable if exists as config file ([#1325](#1325)). Bundles: The Terraform upgrade to v1.48.0 includes a fix for library order not being respected. * Fix conditional in query in `default-sql` template ([#1479](#1479)). * Remove user credentials specified in the Git origin URL ([#1494](#1494)). * Serialize dynamic value for `bundle validate` output ([#1499](#1499)). * Override variables with lookup value even if values has default value set ([#1504](#1504)). * Pause quality monitors when "mode: development" is used ([#1481](#1481)). * Return `fs.ModeDir` for Git folders in the workspace ([#1521](#1521)). * Upgrade TF provider to 1.48.0 ([#1527](#1527)). * Added support for complex variables ([#1467](#1467)). Internal: * Add randIntn function ([#1475](#1475)). * Avoid multiple file tree traversals on bundle deploy ([#1493](#1493)). * Clean up unused code ([#1502](#1502)). * Use `dyn.InvalidValue` to indicate absence ([#1507](#1507)). * Add ApplyPythonMutator ([#1430](#1430)). * Set bool pointer to disable lock ([#1516](#1516)). * Allow the any type to be set to nil in `convert.FromTyped` ([#1518](#1518)). * Properly deal with nil values in `convert.FromTyped` ([#1511](#1511)). * Return `dyn.InvalidValue` instead of `dyn.NilValue` when errors happen ([#1514](#1514)). * PythonMutator: replace stdin/stdout with files ([#1512](#1512)). * Add context type and value to path rewriting ([#1525](#1525)). API Changes: * Added schedule CRUD commands to `databricks lakeview`. * Added subscription CRUD commands to `databricks lakeview`. * Added `databricks apps start` command. OpenAPI commit 7437dabb9dadee402c1fc060df4c1ce8cc5369f0 (2024-06-24) Dependency updates: * Bump golang.org/x/text from 0.15.0 to 0.16.0 ([#1482](#1482)). * Bump golang.org/x/term from 0.20.0 to 0.21.0 ([#1483](#1483)). * Bump golang.org/x/mod from 0.17.0 to 0.18.0 ([#1484](#1484)). * Bump golang.org/x/oauth2 from 0.20.0 to 0.21.0 ([#1485](#1485)). * Bump github.com/briandowns/spinner from 1.23.0 to 1.23.1 ([#1495](#1495)). * Bump github.com/spf13/cobra from 1.8.0 to 1.8.1 ([#1496](#1496)). * Bump github.com/databricks/databricks-sdk-go from 0.42.0 to 0.43.0 ([#1522](#1522)).

## Changes The map function ignores cases where either a key in a map is not present or an index in a sequence is out of bounds. As of recently, we retain nil values as valid values in a configuration tree. As such, it makes sense to also ignore cases where a map or sequence is expected but nil is found. This is semantically no different from an empty map where a key is not found. Without this fix, all calls to `dyn.Map` would need to be updated with nil-checks at every path component. Related PRs: * #1507 * #1511 ## Tests Unit tests pass.

Properly deal with nil values in convert.FromTyped

bf6e1b5

pietern requested a review from shreyas-goenka June 20, 2024 15:34

pietern commented Jun 20, 2024

View reviewed changes

pietern mentioned this pull request Jun 21, 2024

Set bool pointer to disable lock #1516

Merged

pietern added 2 commits June 21, 2024 10:46

Merge branch 'disable-lock-typed' into dyn-from-typed-nil

f3c6ca0

Rename flag

d185e96

pietern marked this pull request as ready for review June 21, 2024 09:40

Allow the any type to be set to nil in convert.FromTyped

9836da9

pietern mentioned this pull request Jun 21, 2024

Allow the any type to be set to nil in convert.FromTyped #1518

Merged

Merge branch 'dyn-from-typed-allow-nil' into dyn-from-typed-nil

a13196e

shreyas-goenka approved these changes Jun 21, 2024

View reviewed changes

libs/dyn/convert/from_typed.go Outdated Show resolved Hide resolved

bundle/deploy/terraform/convert_test.go Show resolved Hide resolved

shreyas-goenka reviewed Jun 21, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into dyn-from-typed-nil

34728ad

shreyas-goenka mentioned this pull request Jun 21, 2024

Return dyn.InvalidValue instead of dyn.NilValue when errors happen #1514

Merged

Comments

61c85b0

pietern enabled auto-merge June 21, 2024 13:42

pietern added this pull request to the merge queue Jun 21, 2024

Merged via the queue into main with commit 446a9d0 Jun 21, 2024
5 checks passed

pietern deleted the dyn-from-typed-nil branch June 21, 2024 13:50

pietern mentioned this pull request Jun 26, 2024

Release v0.222.0 #1529

Merged

pietern mentioned this pull request Jul 1, 2024

Ignore dyn.NilValue when traversing value from dyn.Map #1547

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Properly deal with nil values in `convert.FromTyped` #1511

Properly deal with nil values in `convert.FromTyped` #1511

pietern commented Jun 20, 2024 •

edited

Loading

pietern Jun 20, 2024

pietern Jun 20, 2024

pietern Jun 20, 2024

shreyas-goenka left a comment

shreyas-goenka left a comment •

edited

Loading

Properly deal with nil values in convert.FromTyped #1511

Properly deal with nil values in convert.FromTyped #1511

Conversation

pietern commented Jun 20, 2024 • edited Loading

Changes

Tests

pietern Jun 20, 2024

Choose a reason for hiding this comment

pietern Jun 20, 2024

Choose a reason for hiding this comment

pietern Jun 20, 2024

Choose a reason for hiding this comment

shreyas-goenka left a comment

Choose a reason for hiding this comment

shreyas-goenka left a comment • edited Loading

Choose a reason for hiding this comment

Properly deal with nil values in `convert.FromTyped` #1511

Properly deal with nil values in `convert.FromTyped` #1511

pietern commented Jun 20, 2024 •

edited

Loading

shreyas-goenka left a comment •

edited

Loading