henkmollema / Dommel

CRUD operations with Dapper made simple.
MIT License
611 stars 99 forks source link

Automatic multi mapping not properly mapping on 3.3.3 update #308

Open bromike opened 1 month ago

bromike commented 1 month ago

From 3.3.2 to 3.3.3, we noticed that some queries became flaky and started failing, at some times where it seemed random.

We are mostly using queries similar to:

[Table("elements")]
public class Element
{
    [Key][DatabaseGenerated(DatabaseGeneratedOption.None)]
    public Guid ElementKeyId { get; set; }

    public Guid OwnerId { get; set; }

    [ForeignKey(nameof(ElementTag.ElementKeyId))]
    public List<ElementTag> ElementTags { get; set; }
}

[Table("element_tags")]
public class ElementTag
{
    [Key][DatabaseGenerated(DatabaseGeneratedOption.None)]
    public Guid ElementKeyId { get; set; }
    [Key][DatabaseGenerated(DatabaseGeneratedOption.None)]
    public string Tag { get; set; }
}

public class ElementsService
{
    public List<Element> GetElementsAsync(Guid ownerId)
    {
        await using var connection = await _connectionFactory.CreateAsync();
        var elements = (await connection.SelectAsync<Element, ElementTag, Element>(i => i.OwnerId == ownerId)).ToList();
        // some logic here
        return elements;
    }
}

We noticed that sometimes, it would wrongly map element tags to an element. It would return this:

[
  Element
  {
    ElementKeyId = {11111111-1111-1111-1111-111111111111},
    OwnerId = {00000000-0000-0000-0000-00000000000A},
    ElementTags = ElementTag
      {
        ElementKeyId = {11111111-1111-1111-1111-111111111111}, 
        Tag = "a"
      }, 
      ElementTag
      {
        ElementKeyId = {33333333-3333-3333-3333-333333333333}, <-- this should go with element of the same ElementKeyId
        Tag = "c"
      }
    },
  }, 
  Element
  {
    ElementKeyId = {22222222-2222-2222-2222-222222222222},
    OwnerId = {00000000-0000-0000-0000-00000000000A},
    ElementTags = ElementTag
    {
      {
        ElementKeyId = {22222222-2222-2222-2222-222222222222}, 
        Tag = "b"
      }
    },
  }
]

Rather than returning tags according to their ElementKeyIds:

[
  Element
  {
    ElementKeyId = {11111111-1111-1111-1111-111111111111},
    OwnerId = {00000000-0000-0000-0000-00000000000A},
    ElementTags = ElementTag
    {
      {
        ElementKeyId = {11111111-1111-1111-1111-111111111111}, 
        Tag = "a"
      }
    },
  },
  Element
  {
    ElementKeyId = {22222222-2222-2222-2222-222222222222}, 
    OwnerId = {00000000-0000-0000-0000-00000000000A},
    ElementTags = ElementTag
    {
      {
        ElementKeyId = {22222222-2222-2222-2222-222222222222}, 
        Tag = "b"
      }
    }
  },
  Element
  {
    ElementKeyId = {33333333-3333-3333-3333-333333333333},
    OwnerId = {00000000-0000-0000-0000-00000000000A},
    ElementTags = ElementTag
    {
      {
        ElementKeyId = {33333333-3333-3333-3333-333333333333}, 
        Tag = "c"
      }
    }
  }
]

However, looking at the logs from Dommel, It would actually print out the proper query in the logs:

...
MultiMapAsync<Element>: select * from `elements` left join `element_tags` on `elements`.`element_key_id` = `element_tags`.`element_key_id` where `elements`.`owner_id` = @p1
MultiMapAsync<Element>: select * from `elements` left join `element_tags` on `elements`.`element_key_id` = `element_tags`.`element_key_id` where `elements`.`owner_id` = @p1

It is very flaky, sometimes it will work, sometimes not. So I'm not sure if it's related to how it works with the cache key, but this actually blocks the upgrade. There is no issue on 3.3.2

henkmollema commented 2 weeks ago

Thanks for reporting this. Seems like the new way to combine primary key values for the cache key don't work correctly for non-numeric ID's. I'll look into it.

henkmollema commented 2 weeks ago

Are your ID's actually 11111111-1111-1111-1111-111111111111, 22222222-2222-2222-2222-222222222222 and 33333333-3333-3333-3333-333333333333? Because their hash codes are all 0. However I'd suspect that this would lead to flaky mapping before, since that logic also used the hash code.

bromike commented 1 week ago

Are your ID's actually 11111111-1111-1111-1111-111111111111, 22222222-2222-2222-2222-222222222222 and 33333333-3333-3333-3333-333333333333? Because their hash codes are all 0. However I'd suspect that this would lead to flaky mapping before, since that logic also used the hash code.

Not always, these were some examples but they are also usually generated via Guid.NewGuid() in the context of unit tests.

henkmollema commented 1 week ago

I can't reproduce the issue with actual generated GUID's (eg Guid.NewGuid()). Do you have an example of that?