[PUP-4483] Add NotUndef type to the set of Puppet types Created: 2015/04/29  Updated: 2015/05/20  Resolved: 2015/05/20

Status: Closed
Project: Puppet
Component/s: None
Affects Version/s: None
Fix Version/s: PUP 3.8.1, PUP 4.1.0

Type: Improvement Priority: Normal
Reporter: Thomas Hallgren Assignee: Nicholas Fagerlund
Resolution: Done Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Blocks
is blocked by PUP-4511 Types Array[?], Hash[?,?], and Option... Closed
Relates
Template:
Story Points: 2
Sprint: Language 2015-05-13, Language 2015-05-27

 Description   

The Puppet type system has currently no way of defining a type with the meaning "any value except undefined" and hence no way of differentiating between a missing value and a given value of any type (the existing Any type accepts undef values).

The problem becomes apparent when declaring a hash where certain entries must be present but the value is unrestricted.

Two improvements are needed in order to fully resolve this issue:

Adding NotUndef[T]

A new parameterized type NotUndef[T] should be added to reflect a type that is assignable from all types that T is assignable from except all types that are assignable from Undef. The parameter T is optional and defaults to Any. The default type will be especially useful when declaring untyped parameters that require a value (such as current Resource parameters that have no default).

Optional[string]

It is not possible to parameterize Optional with a non empty string value. This is a shorthand notation for Optional[Enum[value]]. It is added for the purpose of shortening what has to be typed when an Optional type is used for keys in a Struct.

Struct member key as Type

The key of a Struct member can currently only be a string. This is too limited since it's impossible to declare that an entry is required even when it's OK to pass undef as that entry's value. It's also not possible to declare that the entry is optional but if it's provided, it cannot be undef. The key should therefore accept a Type. The Type must be limited such that it must be possible to extract a non-empty string, i.e.an Enum with exactly one entry or a String that is inferred from a literal non-empty string. This type can then be wrapped in an NotUndef or Optional to explicitly declare if the entry is required or optional.

Keys declared as string literals will be automatically converted to their corresponding String type. If the value type of the entry is assignable from Undef, then that String type will be wrapped in a Optional. This special rule will only apply when keys are given as string literals.

New Struct Features

    # New feature: key may not be missing, value may be undef or an Integer
    Struct[{ NotUndef['mykey'] => Optional[Integer] }] 
 
    # New feature: key may be missing, if set it must be an Integer
    Struct[{ Optional['mykey'] => Integer}]
 
   # Same as before: mykey may be missing, value may be undef, or an Integer
   Struct[{ 'mykey' => Optional[Integer] }]
 
   # Same as before: mykey may not be missing, value must be an Integer
   Struct[{ 'mykey' => Integer }]

QA


risk: medium
probability: low
severity: medium
test level: unit



 Comments   
Comment by Henrik Lindberg [ 2015/04/29 ]

We have one more option, and that is to allow a richer specification on the key side. That feels more natural as we are basically adding a constraint on the key. Currently, the Struct type only accepts string keys, but it could accept something else.

A: Currently the use of a string key can be seen as a shorthand for Optional[Enum['key']], if we reverse the decision that a key is optional if value type is optional, we would instead treat key as shorthand for Enum['key'], and to make the key optional, user can write Optional[Enum['key']]. We could possible allow Optional[<astring>] be shorthand for Optional[Enum[<astring>]] to save some typing.

B: If we keep the behavior that the key may be missing of the value type is optional, then we need a way to change that. We could then use the NotUndef type to do that - i.e. NotUndef[Enum['key']], possibly with shortened notation NotUndef['key'].

I prefer option A) as I have always thought that the optionality of the value leaking into a constraint on the key is smelly. It is however difficult to change in puppet 4 since it is an API change. We can however add both options. A key can be entered as Optional['key'] or NotUndef['key'], where the NotUndef variant overrules the interpretation that an Optional value means key can be missing, and the use of an Optional key means that the key may be missing even if the value is not declared as being Optional.

Comment by Thomas Hallgren [ 2015/04/29 ]

I agree that option A) is better but as you say, it's an API change and a rather significant one. I also like that keys can be explicitly typed although that too is an API change. Today we only infer a Hash with String[1] keys to a Struct. All other hashes are inferred to Hash with key type being the common type of all keys. This would then need to change.

Comment by Henrik Lindberg [ 2015/04/29 ]

Adding the ability that a key can be a typed is a new feature, it should be kept backwards compatible, and thus it can be introduced before puppet 5.

I though we inferred a Struct from a Hash instance? But yes, naturally, the handling of the Struct type would need to change a bit.

Comment by Thomas Hallgren [ 2015/04/29 ]

We do infer at Struct from a Hash (infer_set) but only when keys are strings. See PUP-3947.

Comment by Henrik Lindberg [ 2015/04/29 ]

We can continue doing that, if it is not a string in a key it cannot represent a Struct.

Comment by Henrik Lindberg [ 2015/04/29 ]

We had a discussion and agreed that NotUndef was the best name we could think of as it is explicit. We also agreed that it is of value to be able to have a type parameter - e.g NotUndef[T] and that this removes the validity of Undef if accepted by T. Also agreed is that an NotUndef without a type parameter means the same as NotUndef[Any] (which makes it very easy to implement.

When dealing with construction of an NotUndef, it should also accept a type parameter being a String, to construct a String['the_string_value'] type, which cannot otherwise be directly constructed in the language - e.g. NotUndef[some_key] becomes NotUndef[String[some_key]] (although that type cannot be constructed directly in puppet). This is done to avoid having to construct an Enum with a single string value.

Comment by Eric Thompson [ 2015/05/01 ]

at validation: add test link in existing testcase, and test results

Comment by Henrik Lindberg [ 2015/05/05 ]

Thomas Hallgren Please also make a PR against 'puppet-specifications' regarding the changes to the type system. Please also update the description in this ticket to reflect the resulting fix (to aid those that write release notes, update documentation, etc.).

Comment by Henrik Lindberg [ 2015/05/06 ]

specification merged at: 32acd5b (puppet-specifications master)

Comment by Henrik Lindberg [ 2015/05/07 ]

Merged to 3.x at: 1678a9d

Waiting on merge to stable and master because of issues on windows a.t.m.

Comment by Eric Thompson [ 2015/05/13 ]

validating on ubuntu14.04 at master SHA: c4d461a (after 3.x merge to stable and stable merge to master)
(clipped some notices for brevity)

[root@lvp4j5pvncnlg9k ~]# puppet apply -e 'notice undef =~ Undef'
Notice: Scope(Class[main]): true
[root@lvp4j5pvncnlg9k ~]# puppet apply -e 'notice undef =~ NotUndef'
Notice: Scope(Class[main]): false
[root@lvp4j5pvncnlg9k ~]# puppet apply -e 'notice 0 =~ NotUndef'
Notice: Scope(Class[main]): true
[root@lvp4j5pvncnlg9k ~]# puppet apply -e 'notice "a" =~ NotUndef'
Notice: Scope(Class[main]): true
[root@lvp4j5pvncnlg9k ~]# puppet apply -e 'notice [undef] =~ NotUndef'
Notice: Scope(Class[main]): true
[root@lvp4j5pvncnlg9k ~]# puppet apply -e 'notice [] =~ NotUndef'
Notice: Scope(Class[main]): true
[root@lvp4j5pvncnlg9k puppet]# puppet apply -e 'notice NotUndef in [undef]'
Notice: Scope(Class[main]): false
[root@lvp4j5pvncnlg9k puppet]# puppet apply -e 'notice NotUndef in [1,undef]'
Notice: Scope(Class[main]): true
[root@lvp4j5pvncnlg9k puppet]# puppet apply -e 'notice NotUndef in undef'
Notice: Scope(Class[main]): false
# expected (in only works against arrays/hashes/etc):
[root@lvp4j5pvncnlg9k puppet]# puppet apply -e 'notice NotUndef in 1'
Notice: Scope(Class[main]): false
[root@lvp4j5pvncnlg9k puppet]# puppet apply -e 'notice NotUndef in []'
Notice: Scope(Class[main]): false

Thomas Hallgren some of those "in" cases at the end surprise me. does "in" not work properly on atomic values? it seems to work as i'd guess on arrays, except in that first case where the array only contains undef.

Comment by Nicholas Fagerlund [ 2015/05/14 ]

Did this ticket incorporate the changes to the Struct type? I can't understand what those changes were from the description given. Can someone involved in writing these changes please add some examples showing the changes to the Struct type?

Comment by Nicholas Fagerlund [ 2015/05/14 ]

It also appears that the behavior of the Optional type may have changed, since it can take something other than a Type as its parameter now? Or has it always been able to do that? Is there a ticket for this change to Optional (if it was a change)?

Comment by Nicholas Fagerlund [ 2015/05/14 ]

Weird. So the spec states, in the section on Optional,

The parameter can be a literal string in which case it is converted into its corresponding String type.

This is untrue on the current stable (commit 9a73150). I get Error: Evaluation Error: Optional-Type[] arguments must be types. Got String at /Users/nick/Documents/manifests/future_41_optional_notundef_struct_pup-4483.pp:5:24 on node magpie.lan

Comment by Henrik Lindberg [ 2015/05/14 ]

Ping Thomas Hallgren See Nick's comment above.

Comment by Thomas Hallgren [ 2015/05/14 ]

I just added a maintenance commit for that. The change was made in the TypeFactory but I forgot the parser. Merge to stable is imminent.

Comment by Henrik Lindberg [ 2015/05/14 ]

Nicholas Fagerlund The changes to Optional (to behave as NotUndef wrt being given just a string value) and to Struct were all done in this ticket as the purpose of adding NotUndef was driven by two use cases: the ability to have a type that is Any (but not Undef), and to be able to make struct keys required (irrespective of accepting Undef or not for the value).

I have updated the description with two examples for Struct that you could not specify earlier.

Comment by Nicholas Fagerlund [ 2015/05/15 ]

Can you please update the description to include the fact that Optional's behavior has changed?

The examples for struct look good, thanks. The part about extracting a non-empty string from a type looks difficult to explain to users, though. (I understand it, it's just squirrelly.)

Comment by Henrik Lindberg [ 2015/05/15 ]

Nicholas Fagerlund Can you explain it as Optional[somestring] is a shorthand for Optional[Enum[somestring]] except that empty string is not allowed (and leave it at that, since the internals are not that interesting).

(Description updated with the change to Optional type)

Comment by Nicholas Fagerlund [ 2015/05/15 ]

Yeah, that's probably a good approach. Thanks.

Comment by Kylo Ginsberg [ 2015/05/18 ]

Henrik Lindberg who should review this?

Comment by Kylo Ginsberg [ 2015/05/18 ]

Assigning to Nicholas Fagerlund. Please resolve this ticket if the conversation above is sorted out.

Generated at Wed Aug 21 01:34:35 PDT 2019 using JIRA 7.7.1#77002-sha1:e75ca93d5574d9409c0630b81c894d9065296414.