asulwer / RulesEngine

Rules Engine with extensive Dynamic expression support
MIT License
6 stars 1 forks source link

Add JSON Element Handling for System.Text.Json Compatibility #37

Closed RenanCarlosPereira closed 4 days ago

RenanCarlosPereira commented 4 days ago

Reason for Change: The primary reason for this change is the difference in how System.Text.Json handles ExpandoObject compared to Newtonsoft.Json. When using System.Text.Json, properties within an ExpandoObject are deserialized as JsonElement rather than their respective .NET types (e.g., string, int). This behavior can lead to issues where the compiler cannot work with the dynamically deserialized properties directly.

Examples:

Newtonsoft.Json Example: When you deserialize JSON to a dynamic object with Newtonsoft.Json, it converts the JSON properties to their corresponding .NET types directly:

dynamic config = JsonConvert.DeserializeObject<ExpandoObject>(json);
Console.WriteLine(config.name);  // Outputs: John

System.Text.Json Example: In contrast, System.Text.Json does not convert properties to their .NET types but keeps them as JsonElement:

dynamic config = JsonSerializer.Deserialize<ExpandoObject>(json);
Console.WriteLine(config.name.GetType());  // Outputs: System.Text.Json.JsonElement

This difference means you can't directly access or manipulate the values without additional conversion steps. As a result, handling JsonElement requires converting them into more manageable types, such as ExpandoObject.

Changes Made:

  1. In CreateAbstractClassType Method:

    • Added code to convert JsonElement to ExpandoObject:
    if (input is JsonElement jsonElement)
    {
        input = jsonElement.ToExpandoObject();
    }
  2. In CreateObject Method:

    • Added code to handle JsonElement within expando properties:
    else if (expando.Value is JsonElement expandoElement)
    {
        val = expandoElement.ToExpandoObject();
    }

Tests Added:

  1. CreateAbstractClassType_WithJsonElement_ShouldConvertToExpandoObject

    • Ensures that JsonElement input is correctly converted and that the dynamic type has the expected properties.
  2. CreateObject_WithJsonElement_ShouldConvertToExpandoObject

    • Ensures that CreateObject correctly handles JsonElement and converts it to an ExpandoObject.
  3. CreateObject_WithJsonElementNested_ShouldConvertToExpandoObject

    • Verifies correct handling of nested JSON objects.
  4. CreateObject_WithJsonElementArray_ShouldConvertToExpandoObject

    • Ensures JSON arrays within objects are properly converted.

This pull request resolves issues caused by the serialization behavior of System.Text.Json, allowing the dynamic type creation and object creation methods to work seamlessly with JSON data serialized by System.Text.Json.

References:

RenanCarlosPereira commented 4 days ago

hey @abbasc52 I might found the issue.

check out the PR, that's probably why you didn't migrate fully to System.Text.Json. I found a way to sort it out if you could take a look at this implementation:

@asulwer please could you check it carefully too.

the explanation is in the PR description thanks, guys

asulwer commented 4 days ago

when i attempted the change a week or so ago, i ran into something. i cannot remember what that was exactly, something to do with the object array. vague. maybe same thing or related?

asulwer commented 4 days ago

yes that is exactly what i experienced. this only effects object array parameters? if a RuleParameter is used this is not applicable?

RenanCarlosPereira commented 4 days ago

when i attempted the change a week or so ago, i ran into something. i cannot remember what that was exactly, something to do with the object array. vague. maybe same thing or related?

not sure, but it could be related, I covered the arrays in the unit test

asulwer commented 4 days ago

let me know if you want this merged. looks good on my end

RenanCarlosPereira commented 4 days ago

That doesn't affect only arrays That affects all parameters that are of type JsonElement.

We can merge it, but let's wait some time I want to get some insight from @abbasc52

RenanCarlosPereira commented 4 days ago

32 implement testing for this implementation