Skip to content

Ensure that // +optional params default to their zero value#6228

Merged
jedevc merged 3 commits into
dagger:mainfrom
jedevc:fix-optionals-default-values
Dec 7, 2023
Merged

Ensure that // +optional params default to their zero value#6228
jedevc merged 3 commits into
dagger:mainfrom
jedevc:fix-optionals-default-values

Conversation

@jedevc
Copy link
Copy Markdown
Contributor

@jedevc jedevc commented Dec 6, 2023

This means that we will always pass in an inline opts struct without the pointers, but we preserve the default value in the other cases.

So for example, given a *string parameter, if was optional and not set, we should default to nil, not a pointer to an empty string (which was the previous behaviour).

Tests for this as well, and also handles a weird and edgy case where we could maybe have an *Optional.

Original bug reported by @aweris (thanks thanks!!):

// External workflow file to run.
// +optional=true
workflowFile *File,

as parameter does not return nil. Instead returns empty object which by passes if workflowfile != nil check

The original use for findOptsAccessPattern was for handling inline opts structs more easily, but we should only apply it for those cases, and not across the board, since it removes our ability to use the proper default zero value.

Looking forward to removing the inline opts structs maybe at some point? Lots of code to simplify if we do!

@jedevc jedevc added this to the v0.9.4 milestone Dec 6, 2023
@jedevc jedevc requested a review from sipsma December 6, 2023 18:10
This means that we can still trigger if we have a `*Optional` and call
it when it is still nil. This is an edge case, but helps to cover this.

Signed-off-by: Justin Chadwell <me@jedevc.com>
This means that we will always pass in an inline opts struct without the
pointers, but we preserve the default value in the other cases.

So for example, given a `*string` parameter, if was optional and not
set, we should default to `nil`, not a pointer to an empty string (which
was the previous behaviour).

Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc jedevc force-pushed the fix-optionals-default-values branch from 7dba23b to e0152b8 Compare December 6, 2023 18:16
If a type is wrapped in Optional we should still always set the Optional
value provided as an arg even if the value itself is unset. Otherwise
if the arg type is *Optional (or more layers of pointers) then it itself
  will be nil and methods on the Optional type won't work as expected,
  instead failing with nil pointer exceptions.

Signed-off-by: Erik Sipsma <erik@dagger.io>
@sipsma
Copy link
Copy Markdown
Contributor

sipsma commented Dec 6, 2023

@jedevc saw there were legit test failures, it was a quick fix so I just pushed that now so I can approve and everything will be ready for the release tomorrow: a7f51c1

Basically, the test cases involving this function were failing:

func (m *Minimal) EchoOptionalPointer(msg **Optional[**string]) string {
	v, ok := (*msg).Get()
	if !ok {
		v = ptr(ptr("default"))
	}
	return m.Echo(**v)
}

specifically due to a nil panic on (*msg).

That was due to the fact that the change resulted in any *Optional or **Optional to itself be nil when the value wasn't provided, which didn't feel correct besides causing that test to fail.

The new generated code that had the problem looked like:

var msg **Optional[**string]
if inputArgs["msg"] != nil {
  err = json.Unmarshal([]byte(inputArgs["msg"]), &msg)
  if err != nil {
    fmt.Println(err.Error())
    os.Exit(2)
  }
}
return (*Minimal).EchoOptionalPointer(&parent, msg), nil

After the commit I pushed that particular case of an **Optional wrapped arg goes back to the way it was before:

var msg Optional[**string]
if inputArgs["msg"] != nil {
  err = json.Unmarshal([]byte(inputArgs["msg"]), &msg)
  if err != nil {
    fmt.Println(err.Error())
    os.Exit(2)
  }
}
return (*Minimal).EchoOptionalPointer(&parent, ptr(ptr(msg))), nil

Agree that it would be nice to remove support for the inline struct soon (next release?) and maybe also consider removing Optional sometime too if the new //+optional works out as expected.

@jedevc
Copy link
Copy Markdown
Contributor Author

jedevc commented Dec 7, 2023

Ah thanks @sipsma!

@jedevc jedevc merged commit cf0ff8a into dagger:main Dec 7, 2023
@jedevc jedevc deleted the fix-optionals-default-values branch December 7, 2023 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants