microsoft / RulesEngine

A Json based Rules Engine with extensive Dynamic expression support
https://microsoft.github.io/RulesEngine/
MIT License
3.47k stars 528 forks source link

Handling of List type child property #567

Closed akshukla009 closed 6 months ago

akshukla009 commented 6 months ago

While initializing the RuleParameter input of type ExpandoObject, it is converted to a typed object. However, if input has a list type child property, then the list internal type is created based on the first item of the list.

var internalType = CreateAbstractClassType(((IList)expando.Value)[0]);
  value = new List<object>().Cast(internalType).ToList(internalType).GetType();

https://github.com/microsoft/RulesEngine/blob/main/src/RulesEngine/HelperFunctions/Utils.cs#L51

This has below issues with input having child property of type list

var input = {
  "InvoiceLines": [
    {
      "Id": 1,
      "AppliedVatRate": 10.0
    },
    {
      "Id": 2
    }
  ]
}

ActualTypedObject:= {
  "InvoiceLines": [
    {
      "Id": 1,
      "AppliedVatRate": 10.0
    },
    {
      "Id": 2,
      "AppliedVatRate": 0
    }
  ]
}

ExpectedTypedObject:= {
  "InvoiceLines": [
    {
      "Id": 1,
      "AppliedVatRate": 10.0
    },
    {
      "Id": 2,
      "AppliedVatRate": null
    }
  ]
}
var input = {
  "InvoiceLines": [
    {
      "Id": 1     
    },
    {
      "Id": 2,
      "AppliedVatRate": 10.0
    }
  ]
}

ActualTypedObject:= {
  "InvoiceLines": [
    {
      "Id": 1      
    },
    {
      "Id": 2     
    }
  ]
}

ExpectedTypedObject:= {
  "InvoiceLines": [
    {
      "Id": 1,
      "AppliedVatRate": null
    },
    {
      "Id": 2,
      "AppliedVatRate": 10.0
    }
  ]
}
akshukla009 commented 6 months ago

Pull Request for fix: https://github.com/microsoft/RulesEngine/pull/568

abbasc52 commented 6 months ago

Hi @akshukla009 , this is known limitation with ExpandoObject to type conversion. There are lot of permutation and combination possible when a list has different number of properties and type. Also there is a hit to performance which is not required in many cases.

If you know beforehand what type of input is going to be passed, it is better to use Typed object instead to ensure code runs the correct way.

If you need to deal with interchanging list or array, it is better to pass JObject instead of ExpandoObject

akshukla009 commented 6 months ago

Hi @abbasc52, have you looked into MR?

In our case, we don't know the type of object it can be anything. Fix that I have added only cause a little overhead while checking if property exists in all list item. Do you think adding that will impact?

abbasc52 commented 6 months ago

@akshukla009 all list item can be 1 to even 100K, it would degrade performance for people who have consistent types. Also there can be lot of weird scenarios.

E.g. first item has a value in int, second item has value in string with same key. What to do in this case?

It ends up being too much guessing, which would make it very hard to fit everyone's requirement. There is an older discussion on the same issue - please refer this - https://github.com/microsoft/RulesEngine/issues/432

Also you can try this approach with JObject - https://github.com/microsoft/RulesEngine/issues/447#issuecomment-1382682765

akshukla009 commented 6 months ago

Agree, so better to create own variant of CreateAbstractType and use it before passing to RulesEngine. Thanks for your time.