Defaults and Conversions have deep entanglement

[thockin]

Given

type Foo struct {
   Spec FooSpec
}

If you have a custom conversion for Foo and that func does not explicitly call Convert() on FooSpec, then no conversion function will be called on Spec - not too surprising. HOWEVER - it will also bypass the defaulting function of FooSpec. This is a bit more surprising.

Worse - the tests for defaults run with no custom conversion functions loaded, it seems, so even though things pass the test, they don't actually work.

This needs some thought or AT LEAST some good docs.

[yujuhong]

Yes, defaulting is tangled with conversions, and there are some caveats unfortunately. One example is that if you convert an object multiple times between different versions, you may end up having an object populated with default values from multiple versions. I am not sure that this is the intended behavior. We could use more clarity on this, and the example you mentioned.

As for the tests, as far as I know, most custom conversion functions convert versioned objects (e.g. pkg/api/v1beta1) to api objects (pkg/api), or vice versa, which is likely the case you were referring to. The defaults_tests.go in each version tests encoding a versioned object and decoding it into an object of the same version again. Nowhere in that path invokes api <-> versioned conversions. What I can do to improve the tests is to Decode the object into an api object, and then convert it back to the versioned object before verifying the default values. What do you think, @thockin?

[thockin]

I'm not sure this is worth a lot of attention right now. I wanted to file
it so we can think about it for a larger revamp of
conversion/validation/defaulting.

On Tue, Mar 31, 2015 at 11:27 AM, Yu-Ju Hong notifications@github.com
wrote:

Yes, defaulting is tangled with conversions, and there are some caveats
unfortunately. One example is that if you convert an object multiple times
between different versions, you may end up having an object populated with
default values from multiple versions. I am not sure that this is the
intended behavior. We could use more clarity on this, and the example you
mentioned.

As for the tests, as far as I know, most custom conversion functions
convert versioned objects (e.g. pkg/api/v1beta1) to api objects (pkg/api),
or vice versa, which is likely the case you were referring to. The
defaults_tests.go in each version tests encoding a versioned object and
decoding it into an object of the same version again. Nowhere in that path
invokes api <-> versioned conversions. What I can do to improve the tests
is to Decode the object into an api object, and then convert it back to
the versioned object before verifying the default values. What do you
think, @thockin https://github.com/thockin?

—
Reply to this email directly or view it on GitHub
#6210 (comment)
.

[yujuhong]

Agreed that the entanglement should be addressed in a larger revamp, but the tests should still catch most of the unexpected errors until we have time for the revamp. I submitted a PR #6235.

[yujuhong]

Default tests should be able to catch most problems now that the PR has been merged.

[dchen1107]

• me on latest update on this.

[lavalamp]

Unfortunately, this is probably a post-1.0 item.

[pmorie]

Related: #7337, might be able to be closed as a dupe.

[lavalamp]

Kicking this can down the road.

[smarterclayton]

Fixed!