Possible bug in v5 ConvertFromOldConvention

[p-m-j]

Changes to PropertyMapperNameConvention.FindMember (the introduction of the more robust IsPocoMemberEqual) results in level being updated where previously it was not. For our usage this later prevents another member from being found due to level filtering.

var pocoMemberLevels = members.Where(x => !used.ContainsKey(x.PocoMember) && x.Level >= level);

I have a sample repo setup to demonstrate the issue which can be found at https://github.com/p-m-j/npoco5-umbraco-issues

Some further details and description of the problem can be found at https://github.com/umbraco/Umbraco-CMS/pull/9800

[schotime]

Ok, I think I have an idea about this. Will try and get a beta release out tomorrow to test against the repo. Sorry for the delay.

[p-m-j]

Awesome cheers!

[schotime]

https://www.nuget.org/packages/NPoco.SqlServer/5.3.2-preview1 has been pushed. I don't know if you can try it out, but it works in your repo repo.

[p-m-j]

Works like a charm, thanks very much.

[tonlap]

Please note that I am also having this issue with the 5+ release. I also tried to use the 5.3.2-preview1 and it did not work.

When I modified the code (ConvertFromOldConvention) as follows (just a temporary hack) to see if it would resolve the issue, all pocos were mapped properly. I believe that the issue is with pocos that have nested class hierarchies where it is possible for mapped member levels to decrease and then increase. Anyway - just some thoughts.

internal static IEnumerable<PosName> ConvertFromOldConvention(this IEnumerable<PosName> posNames, List<PocoMember> pocoMembers)
{
    var used = new Dictionary<PocoMember, int>();
    var members = FlattenPocoMembers(pocoMembers).ToList();
    var level = 0;

    foreach (var posName in posNames)
    {

        // *** ***
        // Temporary hack to resolve issue with level.
        // Seems that if the previous member is at a lower level and the new member
        // being found is at a higher level, the member will not be found.
        // Modified to ensure that if level does go back up, then the new level is used
        // as the basis for finding members.
        // *** ***
        if ((posName.Pos < members.Count) && (members[posName.Pos].Level < level))
        {
            level = members[posName.Pos].Level;
        }
        // *** ***

        var pocoMemberLevels = members.Where(x => !used.ContainsKey(x.PocoMember) && x.Level >= level);
        var member = FindMember(pocoMemberLevels, posName.Name);

        if (member?.PocoMember?.PocoColumn != null)
        {
            level = member.Level;                    
            used.Add(member.PocoMember, level);
            posName.Name = member.PocoMember.PocoColumn.MemberInfoKey;
        }

        yield return posName;
    }
}

[tonlap]

Just adding a test case so that the issue could be reproduced (hopefully this explains the issue better). The following tables were added to SQLLocalDatabase so that a test case could be created for the issue.

cmd.CommandText = @"
    CREATE TABLE MainComplex(
        MainComplexId int PRIMARY KEY NOT NULL,
        NestedComplex4Id int NOT NULL,
        Name nvarchar(50) NULL                    
    );
";
cmd.ExecuteNonQuery();

cmd.CommandText = @"
    CREATE TABLE NestedComplex1(
        NestedComplex1Id int PRIMARY KEY NOT NULL, 
        MainComplexId int NOT NULL, 
        Name nvarchar(50) NULL,
        Description nvarchar(50) NULL,
        Notes nvarchar(50) NULL,
    );
";
cmd.ExecuteNonQuery();

cmd.CommandText = @"
    CREATE TABLE NestedComplex2(
        NestedComplex2Id int PRIMARY KEY NOT NULL, 
        NestedComplex1Id int NOT NULL, 
        Name nvarchar(50) NULL                    
    );
";
cmd.ExecuteNonQuery();

cmd.CommandText = @"
    CREATE TABLE NestedComplex3(
        NestedComplex3Id int PRIMARY KEY NOT NULL,
        NestedComplex2Id int NOT NULL, 
        Name nvarchar(50) NULL                    
    );
";
cmd.ExecuteNonQuery();

cmd.CommandText = @"
    CREATE TABLE NestedComplex4(
        NestedComplex4Id int PRIMARY KEY NOT NULL,                    
        Name nvarchar(50) NULL                    
    );
";
cmd.ExecuteNonQuery();

NewMapperDecoratedTests (within ComplexMappngTests) was then modified to include Test2 which tests for the issue. If the test is executed, it will fail due to the issue. Note that additional classes were also added to ComplexMappngTests to support the test case.

[Test]
public void Test2()
{

    var obj4 = new NestedComplex4
    {
        NestedComplex4Id = 40,
        Name = "Nested Complex 4"
    };
    var nestedComplex4Id = Database.Insert(obj4);

    var objMain = new MainComplex()
    {
        MainComplexId = 1,
        NestedComplex4Id = (int)nestedComplex4Id,
        Name = "Main Complex"                
    };
    var mainComplexId = Database.Insert(objMain);

    var obj1 = new NestedComplex1
    {
        NestedComplex1Id = 10,
        MainComplexId = (int)mainComplexId,
        Name = "Nested Complex 1",
        Description = "Description Nested Complex 1",
        Notes = "Notes Nested Complex 1",
    };
    var nestedComplex1Id = Database.Insert(obj1);

    var obj2 = new NestedComplex2
    {
        NestedComplex2Id = 20,
        NestedComplex1Id = (int)nestedComplex1Id,
        Name = "Nested Complex 2"
    };
    var nestedComplex2Id = Database.Insert(obj2);

    var obj3 = new NestedComplex3
    {
        NestedComplex3Id = 30,
        NestedComplex2Id = (int)nestedComplex2Id,
        Name = "Nested Complex 3"
    };
    var nestedComplex3Id = Database.Insert(obj3);

    var results = Database.Fetch<MainComplex>( Sql.Builder
        .Select("MainComplex.*", "NestedComplex1.*", "NestedComplex2.*", "NestedComplex3.*", "NestedComplex4.*")
        .From("MainComplex")
        .LeftJoin("NestedComplex1")
            .On("NestedComplex1.MainComplexId = MainComplex.MainComplexId")
        .LeftJoin("NestedComplex2")
            .On("NestedComplex2.NestedComplex1Id = NestedComplex1.NestedComplex1Id")
        .LeftJoin("NestedComplex3")
            .On("NestedComplex3.NestedComplex2Id = NestedComplex2.NestedComplex2Id")
        .LeftJoin("NestedComplex4")
            .On("NestedComplex4.NestedComplex4Id = MainComplex.NestedComplex4Id")
        ).Single();

    Assert.AreEqual(objMain.MainComplexId, results.MainComplexId);
    Assert.AreEqual(objMain.NestedComplex4Id, results.NestedComplex4Id);
    Assert.AreEqual(objMain.Name, results.Name);

    Assert.AreEqual(obj1.NestedComplex1Id, results.NestedComplex1.NestedComplex1Id);
    Assert.AreEqual(obj1.MainComplexId, results.NestedComplex1.MainComplexId);
    Assert.AreEqual(obj1.Name, results.NestedComplex1.Name);
    Assert.AreEqual(obj1.Description, results.NestedComplex1.Description);
    Assert.AreEqual(obj1.Notes, results.NestedComplex1.Notes);

    Assert.AreEqual(obj2.NestedComplex2Id, results.NestedComplex1.NestedComplex2.NestedComplex2Id);
    Assert.AreEqual(obj2.NestedComplex1Id, results.NestedComplex1.NestedComplex2.NestedComplex1Id);
    Assert.AreEqual(obj2.Name, results.NestedComplex1.NestedComplex2.Name);

    Assert.AreEqual(obj3.NestedComplex3Id, results.NestedComplex1.NestedComplex2.NestedComplex3.NestedComplex3Id);
    Assert.AreEqual(obj3.NestedComplex2Id, results.NestedComplex1.NestedComplex2.NestedComplex3.NestedComplex2Id);
    Assert.AreEqual(obj3.Name, results.NestedComplex1.NestedComplex2.NestedComplex3.Name);

    Assert.IsNotNull(results.NestedComplex4);
    Assert.AreEqual(obj4.NestedComplex4Id, results.NestedComplex4.NestedComplex4Id);
    Assert.AreEqual(obj4.Name, results.NestedComplex4.Name);
}

When the test is executed, results.NestedComplex4 is null as the FindMember method within ConvertFromOldConvention returns null.

The following classes were added (ComplexMappingTests) to support the test case.

[PrimaryKey("MainComplexId", AutoIncrement = false)]
public class MainComplex
{

    public int MainComplexId { get; set; }
    public int NestedComplex4Id { get; set; }
    public string Name { get; set; }

    [ComplexMapping, ResultColumn]
    public NestedComplex1 NestedComplex1 { get; set; }

    [ComplexMapping, ResultColumn]
    public NestedComplex4 NestedComplex4 { get; set; }
}

[PrimaryKey("NestedComplex1Id", AutoIncrement = false)]
public class NestedComplex1
{
    public int NestedComplex1Id { get; set; }
    public int MainComplexId { get; set; }
    public string Name { get; set; }
    public string Description { get; set; }
    public string Notes { get; set; }

    [ComplexMapping, ResultColumn]
    public NestedComplex2 NestedComplex2 { get; set; }
}

[PrimaryKey("NestedComplex2Id", AutoIncrement = false)]
public class NestedComplex2
{
    public int NestedComplex2Id { get; set; }
    public int NestedComplex1Id { get; set; }
    public string Name { get; set; }

    [ComplexMapping, ResultColumn]
    public NestedComplex3 NestedComplex3 { get; set; }
}

[PrimaryKey("NestedComplex3Id", AutoIncrement = false)]
public class NestedComplex3
{
    public int NestedComplex3Id { get; set; }
    public int NestedComplex2Id { get; set; }
    public string Name { get; set; }
}

[PrimaryKey("NestedComplex4Id", AutoIncrement = false)]
public class NestedComplex4
{
    public int NestedComplex4Id { get; set; }        
    public string Name { get; set; }
}

[tonlap]

After looking at this further, the issue may be with with FlattenPocoMembers method within the PropertyMapperNameConvention class. It may be that levelMonitor is not tracking the level properly for the poco members. For the test case (i.e., Test2 above), the poco member levels are currently being determined as follows:

MainComplex members, level = 1 NestedComplex1 members, level = 2 NestedComplex2 members, level = 3 NestedComplex3 members, level = 4 NestedComplex4 members, level = 3

If the objective is to track levels, based on the class hierarchy, then the level is incorrect for NestedComplex4 (as it should be 2 based on the class hierarchy). If the objective of the level monitor it to have a unique "level" based on the nested class, then it should be 5. It probably should be 5 based on the ConvertFromOldConvention condition:

var pocoMemberLevels = members.Where(x => !used.ContainsKey(x.PocoMember) && x.Level >= level);

as it seems that the condition assumes that the level keeps increasing when a member is found.

It seems that the following modified FlattenPocoMembers method should resolve the issue (may not be the best implementation).

private static IEnumerable<PocoMemberLevel> FlattenPocoMembers(List<PocoMember> pocoMembers, int levelMonitor = 1)
{
    foreach (var pocoMember in pocoMembers.OrderBy(x => x.PocoMemberChildren.Count != 0))
    {
        if (pocoMember.PocoColumn != null)
        {
            yield return new PocoMemberLevel { PocoMember = pocoMember, Level = levelMonitor };
        }

        if (pocoMember.PocoMemberChildren.Count == 0)
            continue;

        levelMonitor++;

        // *** ***
        // Keep track of the maximum child level so that it can be used if the
        // current level that is being monitored has been exceeded by a child level.
        // *** ***
        int maxChildLevelMonitor = 0;
        // *** ***

        foreach (var pocoMemberLevel in FlattenPocoMembers(pocoMember.PocoMemberChildren, levelMonitor))
        {                    
            maxChildLevelMonitor = Math.Max(pocoMemberLevel.Level, maxChildLevelMonitor);
            yield return pocoMemberLevel;                    
        }

        // *** ***
        // Replace the current level by the maximum child level, if the current level
        // has been exceeded. Ensures that each nested complex member is provided
        // with a unique level that is always increasing.
        // *** ***
        levelMonitor = Math.Max(levelMonitor, maxChildLevelMonitor);
        // *** ***
    }
}

[p-m-j]

No longer an issue with 5.3.2