Objects / Write property allows wrong datatype by presumption

[ChristianTremblay]

objects.py From line 144 to 220

    def WriteProperty(self, obj, value, arrayIndex=None, priority=None, direct=False):
        if _debug:
            Property._debug("WriteProperty(%s) %s %r arrayIndex=%r priority=%r direct=%r",
                self.identifier, obj, value, arrayIndex, priority, direct
                )

        if (not direct):
            # see if it must be provided
            if not self.optional and value is None:
                raise ValueError("%s value required" % (self.identifier,))

            # see if it can be changed
            if not self.mutable:
                raise ExecutionError(errorClass='property', errorCode='writeAccessDenied')

    #=================================
    # THIS IS THE PART I'M QUESTIONNING
        #if it's atomic assume correct datatype
        if issubclass(self.datatype, Atomic):
            if _debug: Property._debug("    - property is atomic, assumed correct type")
        if isinstance(value, self.datatype):
            if _debug: Property._debug("    - correct type")
        elif arrayIndex is not None:
            if not issubclass(self.datatype, Array):
                raise ExecutionError(errorClass='property', errorCode='propertyIsNotAnArray')

            # check the array
            arry = obj._values[self.identifier]
            if arry is None:
                raise RuntimeError("%s uninitialized array" % (self.identifier,))

            # seems to be OK, let the array object take over
            if _debug: Property._debug("    - forwarding to array")
            arry[arrayIndex] = value

            return
        elif value is not None:
            # coerce the value
            value = self.datatype(value)
            if _debug: Property._debug("    - coerced the value: %r", value)

        # seems to be OK
        obj._values[self.identifier] = value

It allows value of any kind to be sent to a write request example : I can send a string to writeProperty of a BinaryValue...

Actually test that should fail are passing

I think we should restraint the value sent to a writeProperty fucntion to the datatype selected in the obejct.

Comparing the dataype of the object class.... without regard to the value sent to the function should be remove I think.

[ChristianTremblay]

I'm not making a pull request ritght now but code can be examied in issue-45 branch Please, only look for objects.py file... 3 others are there becasue I switched to the master branch of my fork... sorry...

(BTW, now that I can work directly here, I'll delete the fork so problems like that won't occur again)

[JoelBender]

I understand your question, and you are correct. I was trying to avoid the overhead of validating the value for code like this, because I want to assume that the developer knew what they wanted to do:

>>> x = AnalogValueObject()
>>> x.presentValue = 75.3

But that ends up allowing this:

>>> x.presentValue = "hi there"

I'm going to change Property.WriteProperty() so that when direct is true it avoids the data type checking (because it is being called from Object.__setattr__). If direct is false it calls some kind of class method validation routine (like Integer.is_valid(), Real.is_valid(), etc) and raises the relatively new InvalidParameterDatatype exception if the function returns false.

There's a non-zero chance that when I merge in the current stage into this branch I'm going to muck something up, you'll hear from me if I do!

[JoelBender]

I made the changes, just in py27 so far, 1bd0fb4a235c9fee4e1ef98de37b861749ad24cd, and the tests run successfully, but I'm not convinced that the current object tests are complete enough to show this change. In fact the WriteProperty() code is stricter...

>>> from bacpypes.object import AnalogValueObject
>>> avo = AnalogValueObject()
>>> avo.WriteProperty('presentValue', 12.3)
bacpypes.errors.ExecutionError: ('property', 'writeAccessDenied')
>>> 

...because the present value isn't required to be writable. But applications are expected to be able to change the value, and that works as you might expect...

>>> avo.presentValue = 12.3

More testing!