Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Base class properties are not take into account for model generation #14

Open
mahdighorbanpour opened this issue Nov 22, 2021 · 5 comments

Comments

@mahdighorbanpour
Copy link

If we define abstract base class to hold some repeated properties for a family of related classes, these properties are not taken into account for generating the derived class model definition. Consider this example

 public abstract class Space : TwinBase
    {
        [TwinProperty]
        public string Name { get; set; }
    }

    [DigitalTwin(Version = 1, DisplayName = "Floor")]
    public class Floor : Space
    {
        [TwinProperty]
        public string Level { get; set; }
    }

    [DigitalTwin(Version = 1, DisplayName = "Room")]
    public class Room : Space
    {
        [TwinProperty]
        public string Number { get; set; }
    }

This will produce this model:

{
  "@id": "dtmi:domain:floor;1",
  "@type": "Interface",
  "@context": "dtmi:dtdl:context;2",
  "displayName": "Floor",
  "contents": [
    {
      "@type": "Property",
      "name": "level",
      "schema": "string"
    }
  ]
}

{
  "@id": "dtmi:domain:room;1",
  "@type": "Interface",
  "@context": "dtmi:dtdl:context;2",
  "displayName": "Room",
  "contents": [
    {
      "@type": "Property",
      "name": "number",
      "schema": "string"
    }
  ]
}

The Name property is missing. If I decorate the base class with DigitalTwin attribute, still I don't get the shared properties in the output, however the model has a new part which says it is extending another model (the base class model). But then there is a new problem, when I try to create the model for the base class, I get this error:

System.MissingMethodException : Cannot create an abstract class.

  Stack Trace: 
RuntimeTypeHandle.CreateInstance(RuntimeType type, Boolean publicOnly, Boolean wrapExceptions, Boolean& canBeCached, RuntimeMethodHandleInternal& ctor, Boolean& hasNoDefaultCtor)
RuntimeType.CreateInstanceDefaultCtorSlow(Boolean publicOnly, Boolean wrapExceptions, Boolean fillCache)
RuntimeType.CreateInstanceDefaultCtor(Boolean publicOnly, Boolean skipCheckThis, Boolean fillCache, Boolean wrapExceptions)
Activator.CreateInstance(Type type, Boolean nonPublic, Boolean wrapExceptions)
Activator.CreateInstance(Type type)
DigitalTwinSerializer.SerializeModel(Type twinType, Boolean htmlEncode, JsonSerializerOptions serializerOptions) line 119
TwinSerialization_Tests.ShouldSerialiseModelToDTDL(Type type) line 39
@kavink98
Copy link
Contributor

The functionality is working as intended. As per DTDL specifications the extends field indicates the parent model and the parent model's contents are not reflected in the child model. Abstract classes cannot currently be serialised, we will look into if it is necessary to do so.

@sgryphon
Copy link
Contributor

The work around (for single inheritance) is to make the base class concrete, not abstract. (In DTDL they are all the same type, Interface). I have used this in the chocolate factory example I am converting across.

DTDL doesn't have different types of entities -- they are all Interfaces, i.e. nothing stops you from having an object that is only a Space and nothing else (the same in code, as you can implement a concrete child of an abstract class with no additional properties, although usually you don't intend those semantics).

However it would make sense to be able to generate models from both dotnet abstract classes, and also dotnet interfaces -- particularly as DTDL does have multiple inheritance (which you need .NET interfaces to implement).

There is also a possible case for base class properties to be used when the base does not have the Twin attribute.

i.e. If the intention is to generate two DTDL models, each with a name property; by not putting the Twin attribute on the child, you are saying not to have DTML inheritance. I think this is a more complicated scenario.

So, there are possibly two issues:

(1) Allow abstract classes and interfaces with the DigitalTwinAttribute to generate DTML models. This is for generating models that use DTML extends.

(2) [the original issue] Where a base class does not have the DigitalTwinAttribute, then include it's properties in the model for the target classes. This has inheritance in the code but do not use the DTML extends (the properties are copied to each serialised class).
inherits

@kavink98
Copy link
Contributor

Makes sense. I will look into these 2 issues

@mahdighorbanpour
Copy link
Author

@sgryphon Thanks
To include the parent class properties, I did the following changes:
in class ReflectionHelper I added a new method:

public static List<PropertyInfo> GetModelPropertiesFromAbstractParent(this Type t) =>
            t.BaseType.IsAbstract && !Attribute.IsDefined(t.BaseType.GetModelPropertyType(), typeof(DigitalTwinAttribute))
                ? t.BaseType.GetProperties()
                    .Where(p => p.DeclaringType == t.BaseType)
                    .Where(p => Attribute.IsDefined(p, typeof(TwinPropertyAttribute)))
                    .ToList()
                : new();

and in class ClassToTwinModelConverter I updated the following method:

protected static void AddPropertiesContent(List<Content> contents)
        {
            var typeToAnalyze = typeof(T);
            var modelProperties = typeToAnalyze.GetModelProperties()
                .ToList()
                .Select(ModelProperty.Create);
           *** var parentProperties = typeToAnalyze.GetModelPropertiesFromAbstractParent()
                .ToList()
                .Select(ModelProperty.Create);

            contents.AddRange(parentProperties); ***
            contents.AddRange(modelProperties);
        }

@sgryphon
Copy link
Contributor

Looks like you are on the right track -- if the parent has DigitalTwinAttribute then do not get the properties, but use the inheritance that is built into DTDL (i.e. extends).

If it is not marked with the attribute, then do traverse upwards and include them in the child definition, copying the attribute to each child.

@mahdighorbanpour you might want to Fork the repository, implement the code, and then raise a pull request.

When implementing, I suggest you first add some unit tests that contain the specific situation (and only that situation) which you are trying to achieve. Maybe first add a test for the existing arrangement (extends) which should work. Then add a test for the inheritance you want (which should fail). Then you can start implementing the fix until both tests work (the first test is to protect your changes don't break the existing code).

The third step of programming (red-green-refactor) is to refactor/clean up the code, while keeping the tests working.

Note that the examples you have only support one level of abstract class, but what if there are 2 levels, and then you have one with the DigitalTwinAttribute attribute. Well, you want to combine all 3 levels into the main definition, but the stop at the attribute (and have extends).

So, maybe you need to recursively check parents, and stop (and use extends) when you get to an attribute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants