Dervived classes with optional fields generate invalid code

[larws]

The template code of DerivedClassWithOptionalFields.cs is invalid since in the code from Line 60 to Line 82 the EncodingMask is not correctly taken into account. In case the base class has fields that need to be serialized or deserialized the EncodingMask has to be calculated and serialized/deserialized before any fields a serialized/deserialized. With the current template setup this is not possible.

The order of the fields and encoding mask needs to be in valid order when generating code for derived classes with optional fields.

[larws]

@opcfoundation-org This issue was introduced with the PR #193 and the relating issue #192. I currently don't have a solution and not the capacity to spend any time on that issue, thus I am writing the issue so it won't be forgotten.

[opcfoundation-org]

The EncodingMask is a property that has to be set before the Encode is called.
This means the base.Encode() call correctly encodes the EncodingMask and the bug is the duplicate EncodingMask write.
Simply deleting Line 65 should fix the issue.

[larws]

Deleting Line 65 won't solve the issue, since the derived class might have optional fields as well and thus needs to contribute to the EncodingMask property. The only way I can currently think of is providing a hook to calculate the encoding mask over the whole class hierarchy and call it from the base class, but this needs to be carefully added to check any edge cases.

[opcfoundation-org]

The derived class re-declares the enumeration but does not redeclare the property.
So writing the EncodingMask in the base.Encode() call will correctly set the bits for the derived fields.
Note the datatype of the EncodingMask is uint and not the enum because this was the expected behavoiur.

[larws]

How would the case be handled where the base class has no optional fields and thus no EncodingMask property?

[opcfoundation-org]

The enumeration for the fields is always re-declared for each subtype even if no new optional fields are added.
So there is no difference for that case.

[larws]

I think we are talking about different things. Let me try to build an example.

OPC-UA data type model:

Person { string: Name, int: Age } with the subtype Student : Person { string?: School } which has an optional property School. The generated code would look something like this, if I understand you correctly:

class Person
{
...
    public virtual void Encode(IEncoder encoder)
    {
        encoder.PushNamespace("School");

        encoder.WriteString("Name", Name);
        encoder.WriteInt("Age", Age);

        encoder.PopNamespace();
    }
}

class Student: Person
{
...
    public virtual void Encode(IEncoder encoder)
    {
        base.Encode(encoder);
        encoder.PushNamespace("School");

        if ((EncodingMask & StudentFields.School) != 0) encoder.WriteString("School", School);

        encoder.PopNamespace();
    }
}

In this case the EncodingMask property will not be written into the stream which would result into an invalid encoding. Am I missing something?

[opcfoundation-org]

The first Structure with optional fields must a direct subtype of Structure.

See:
https://reference.opcfoundation.org/Core/Part3/v105/docs/8.49

Structure | 0 | A Structure without optional fields where none of the fields allow subtyping

I realize now the text is ambiguous but the requirement 'Structure without optional fields' was supposed to imply it has to be true for all fields added by subtypes because this statement would no longer be true for the base type.

[larws]

I think this is an interesting way of interpreting that sentence in the specification.

In my opinion viewing a type representing data through its basetype interface I have no way of knowing which fields are added through any types deriving from them, thus this statement would not be affected if a subtype adds any new values which might be optional.

Another difficulty with this interpretation is that it differs from the behavior of the modeling tools around since they allow modeling structures this way. So nodesets with this type of modeling are around and should be correctly handled by the model compiler in my opinion.

[opcfoundation-org]

This needs to be discussed in the WG.

While you are correct from a data modelling perspective. The absence of a clear statement one way or the other means different implementors will make different decisions. The WG needs to determine which clarification is the least likely to lead to IOP issues.

[opcfoundation-org]

Specification will be clarified: https://mantis.opcfoundation.org/view.php?id=10110

The ModelCompiler will allow this scenario.

[larws]

@opcfoundation-org Any update on this, as I can see no progress in the mantis issue? I still have code changes to address this in a fork of mine and I would like to not have to spend time and energy updating the fork. Is there an option we can have a chat about how to resolve this issue? I am aware that my proposed changes do not solve all cases and I would be fine with changing my PR to be able to address this. Looking forward to hearing from you.

[opcfoundation-org]

This issue was orphaned because it did not get picked up by the Part 3 author.

I have updated the ModelCompiler to allow subtypes with optional fields even if the supertype does not allow.

See the member only repo: https://github.com/OPCF-Members/UA-ModelCompiler

The TestModel has the types I used for testing: https://github.com/OPCF-Members/UA-ModelCompiler/blob/master/Opc.Ua.ModelCompiler/Design.v105/TestModel.xml