azist / azos

A to Z Sky Operating System / Microservice Chassis Framework
MIT License
213 stars 29 forks source link

ApiProtoclController.GetLogicResult AS IEnumerable<object> cast is wrong should we not use cast to IEnumerable? Or does this rely on covarience? #224

Open itadapter opened 4 years ago

itadapter commented 4 years ago
/// <summary>
    /// Analyzes the result of a logic call and returns either JSON {OK=true|false} with HTP status 404 when nothing is returned
    /// </summary>
    public async Task<object> GetLogicResult<T>(Task<T> result)
    {
      var data = await result.NonNull(nameof(result));
      var is404 = data == null;
      if (!is404)
      {
        var en = data as IEnumerable<object>;

        is404 = en != null && !en.Any();
      }

      if (is404)
      {
        WorkContext.Response.StatusCode = WebConsts.STATUS_404;
        WorkContext.Response.StatusDescription = WebConsts.STATUS_404_DESCRIPTION;
      }
      return new { OK = !is404, data };
    }
jkosherx commented 4 years ago
using System;
using System.Collections.Generic;
using System.Collections;

public class Program
{
    public struct car{}

    public static void Main()
    {
        var lst = new List<car>();
        object o = lst;
        var en = o is IEnumerable<object>;
        var en2 = o is IEnumerable;
        Console.WriteLine("of IEnumerable<object>: "+ (en));
        Console.WriteLine("of IEnumerable: " + en2);
    }
}

Produces for struct value types, however for classes it produces true true

of IEnumerable<object>: False
of IEnumerable: True
itadapter commented 4 years ago

yeah, it is a very dangerous bug as it only affects structs as they are invariant waiting for @zhabis to fix later tonight

itadapter commented 4 years ago

IEnumerable has a Cast<T> extension so proper casting of IEnumerable with ANY T (ref or struct) is this:

    (obj as IEnumerable)?.Cast<Object>()  -> IEnumerable<object>

also for the test above:

Console.WriteLine((o as IEnumerable).Cast<object>().GetType().ToString());

yields

System.Linq.Enumerable+<CastIterator>d__97`1[System.Object]
itadapter commented 4 years ago

Keep open until all testing is done

itadapter commented 4 years ago

This was released in new pkg today. @JohnPKosh how are the unit tests? can this be closed? @jkosherx

itadapter commented 4 years ago

@JohnPKosh @jkosherx Can this issue be closed now? Do we have unit tests for GetLogicResult()? Do unit tests pass?

jkosherx commented 4 years ago

After reviewing test implementation this requires additional integration tests for ApiProtocolController.