Getting one value from a Choice invalidates other values as a side effect

[betterworld]

Hi,

I stumbled upon this bug:

from pyasn1.type import tag, namedtype, univ

class Ctype(univ.Choice):
    componentType = namedtype.NamedTypes(
            namedtype.NamedType('foo', univ.OctetString().subtype(implicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatSimple, 0))),
            namedtype.NamedType('bar', univ.OctetString().subtype(implicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatSimple, 1)))
    )

obj = Ctype()
obj['bar'] = 'xyz'

print(repr(obj['bar']))  # Output: <OctetString value object ...
print(repr(obj['foo']))  # Output: <OctetString schema object ...

# Now the wrong behavior:
print(repr(obj['bar']))  # Output: <OctetString schema object ...

You see that the first time bar is accessed, it returns the correct value. The second time it returns a schema object instead, which is triggered by accessing foo.

I traced the cause of this problem to the implementation of Choice.setComponentByPosition:

https://github.com/etingof/pyasn1/blob/d0b7f2ec8677eec8f9aa31103a66f5cab18e9308/pyasn1/type/univ.py#L3121-L3122

This method remembers oldIdx, which is the last index that has been set, and when you set another index, it resets the previous index to noValue (which will later be turned into a schema object). The method setComponentByPosition is called from getComponentByPosition here:

https://github.com/etingof/pyasn1/blob/40d5a7f27b8f56e103cdc83d3a294f02c5eb1496/pyasn1/type/univ.py#L2500-L2501

I believe the implementation of the oldIdx handling should be changed or it should not be called like this from getComponentByPosition.

The reason I was accessing all of the component values in a Choice is that I was using obj['foo'].hasValue() to find the one value that is defined.

[etingof]

That's a fair point! The behavior you observe is such to account for possible inner structures such as:

obj['bar']['foo']

If obj['bar'] would not instantiate the inner structure, then its foo component could not be accessed at all. In other words, if we disable this automatic instantiation, we'd probably have to do this dance:

foo_struct = obj['bar']
foo_struct['foo'] = 'baz'
obj['bar'] = foo_struct

I totally agree that what's happening now is weird. Any better ideas?

[markbourne]

We've just been affected by this. It seems to me that setComponentByPosition should probably just not reset the previous component (and not change self._currentIdx) if the new value is noValue? Something like:

        if value is not noValue:  # Add this condition
            self._currentIdx = idx
            if oldIdx is not None and oldIdx != idx:
                self._componentValues[oldIdx] = noValue

I don't know if that might have other side-effects though, but it seems logical to me that setting a component of Choice to noValue shouldn't replace an actual value set on another component.

[markbourne]

We've just been affected by this. It seems to me that setComponentByPosition should probably just not reset the previous component (and not change self._currentIdx) if the new value is noValue?

...although that would probably mean obj['bar'] doesn't get set to the current component if trying to set up an initial value in @etingof's obj['bar']['foo'] example. The automatic creation of component values on access for sequence and set types does generally seem to be quite useful. e.g. when components have constraints or tags, I've found it avoids the hassle of creating values with compatible constraints/tags, since they get created automatically.

For Choice, perhaps it would be reasonable to change the current component if the new value is not noValue, or if no current component has yet been set (even if the new value is noValue, although that might be getting a bit complex and end up seeming to be inconsistent or at least difficult to explain.

If just trying to find the one currently set component (as in the original report here), I notice that its name and value can be obtained via getName and getComponent methods of Choice objects.

I'm not sure if there might be cases where it's useful to get the other components even if they are schemas rather than values, but it looks like getComponentByPosition(idx, instantiate=False) might work for that case.