FindChild() has a few problems

[Hevgen]

Type of issue

• Bug
• Enhancement
• Compliance
• Question
• Help wanted

Current Behavior

1. Overrides of NodeState.FindChild(ISystemContext context, QualifiedName browseName, bool createOrReplace, BaseInstanceState replacement) specified in nested classes break base logic (defined at base class NodeState): arguments are a handled in a different way.
2. Generated implementations (by ModelCompiler) of overrides of the same method FindChild also break the logic different at NodeState.FindChild.
3. Comments of NodeState.FindChild(ISystemContext context, QualifiedName browseName, bool createOrReplace, BaseInstanceState replacement) do not correspond to its logic.
4. NodeState.FindChild() returns null if provided replacement is just added (but do not replace existing child).

Take you attention that logic of NodeState.FindChild() is correct taking into acount its usage by method NodeState.ReplaceChild() . The overrides in the nested types do not allow to replace the child using that method.

Logic of handling of createOrReplace== true:

1. in NodeState: existing child (instance of the corresponding node-type) is replaced by provided replacement or by created instance or just set child with replacement/created if no actual one;
2. in the nested classes of NodeState (including generated by ModelCompiler): existing child (instance of the corresponding node-type) is NOT replaced or just set the child with provided replacement or by created instance if no actual child.

Expected Behavior

1. Logic of handling of createOrReplace == true in overrides of NodeState.FindChild(ISystemContext context, QualifiedName browseName, bool createOrReplace, BaseInstanceState replacement):
   existing child (instance of the corresponding node-type) is replaced by provided replacement or by the created instance, the child is set by replacement/created instance if no actual child.
2. if createOrReplace == true then replacement or the created instance should be returned always from the FindChild().

Environment

- Nuget Version: all up to 1.05
- Component: Server

Anything else?

simplified/cropped existing implementation of NodeState.ReplaceChild:

public virtual void ReplaceChild(ISystemContext context, BaseInstanceState child)
  {
  // some validation code
       FindChild(context, child.BrowseName, true, child);
   }

Here createOrReplace=true and the code clearly shows that FindChild() SHOULD replace existing child with the new provided value or just set the child by the provided value if the child is null now.

Simplified/cropped code of NodeState.FindChild() with ITS OWN BUG:

protected virtual BaseInstanceState FindChild(ISystemContext context, QualifiedName browseName, bool reateOrReplace, BaseInstanceState replacement)
{
  // some validation
  // cropped code to simplify in the Issue

  for (int ii = 0; ii < m_children.Count; ii++)
  {
      BaseInstanceState child = m_children[ii];
      if (browseName == child.BrowseName)
      {
          if (createOrReplace && replacement != null)
              m_children[ii] = child = replacement;

          return child;
      }
   }

  if (createOrReplace && replacement != null)
  {
    AddChild(replacement);
    // BUG: return is missed
    
    // next line sould be added:
    return replacement;
  }

  return null;
}

Generally logic corresponds to ReplaceChild and common sense. Note: new instance of a child cannot be created by NodeState itself because child's type is unknown - its ok for NodeState.

Sample of the corresponding code from the standard nested class: part of DialogConditionState.FindChild():

            switch (browseName.Name)
            {
                case Opc.Ua.BrowseNames.DialogState:
                {
                    if (createOrReplace)
                    {
                        if (DialogState == null)
                        {
                            if (replacement == null)
                                DialogState = new TwoStateVariableState(this);
                            else
                                DialogState = (TwoStateVariableState)replacement;
                        }
                    }

                    instance = DialogState;
                    break;
                }

// some code

            if (instance != null)
                return instance;

            return base.FindChild(context, browseName, createOrReplace, replacement);

Here existing child DialogState is not replaced if it is not null even if argument createOrReplace == true. So DialogConditionState.ReplaceChild(context, some_new_value_for_DialogState) just returns existing DialogState and ignores provided new value.

Additional question

Is it generally correct that ReplaceChild() may set named property ( explicitly defined children) in the instance of a nested class of NodeState but AddChild() never set the named property even if both methods are called with the same argument?

[romanett]

@Hevgen I suppose these findings apply to 1.5.378?

Because in the current master (1.6) many parts are completely rewritten and Source Generator used to create the generated Code.

Thank you for reporting this.
We will look into the different Issues you pointed out. However it could be some of the fixes require breaking or major changes so we only can fix them in 1.6.

[Hevgen]

I used sources with tag 1.5.378.106 (99044fd) because the latest nuGet packages have version 1.5.378.106 now.

cropped codes are from \Stack\Opc.Ua.Types\State\NodeState.cs and \Stack\Opc.Ua.Core\Stack\Generated\Opc.Ua.Classes.cs

[romanett]

Related: #3257
Fixes were Added to Generated Code for RemoveChild, maybe it was overseen to also fix FindChild/ AddChild

[romanett]

@Hevgen I took a look at the current master code (you need to check out and build the solution as the generated types are now generated on build).

To me it seems the only Issue remaining is that if a child is created (new entry in m_children) then null is returned instead of the child.

We will fix that soon.

Please check if any other issue is remaining.