Hard-coded assumption about generic type positions

[Smile-James]

Issue: Hard-Coded Assumption in Sheet.cs Line 76

In Sheet.cs, line 76, there's a hard-coded assumption about generic type positions:

var referenceRowType = node.ValueType.GenericTypeArguments[1];

Problem

This works fine for Sheet<TRow> (aka Sheet<string, TRow>) scenarios where TRow is the second generic type argument, but fails when users declare more complex subclasses. For example:

// References to rows in this Sheet type work because TRow is the second argument (Sheet<string, TRow>)...
public class FooSheet : Sheet<TRow> { /*...*/ }

// ... but not here because TRow isn't in position [1]!
public class FooSheet<TBar, TBaz, TRow> : Sheet<TRow> { /*...*/ }

// ... but this one is OK again
public class FooSheet<TBar, TRow, TBaz> : Sheet<TRow> { /*...*/ }

Because of the [1] assumption, TRow must always be at index [1]. That means that users are restricted in how they structure their types. If they're not careful, they'll experience broken references to rows in their custom subclasses! I hit this issue myself just now, and it took me a while to track this one down.

Suggested Solution

Instead of hard-coding the index, could try dynamically searching for the correct type:

// Note: TValue is the row value type in this code block, in Sheet.cs (`Sheet<TKey, TValue>`)
var referenceRowType = node.ValueType.GenericTypeArguments.FirstOrDefault(x => typeof(TValue).IsAssignableFrom(x));

if (referenceRowType == null)
{
    context.Logger.LogError($"Failed to find reference row type {TValue.Name} in {node.ValueType.Name} GenericTypeArguments");
    continue;
}

This way, it tries to find the correct type dynamically. Note that the code above avoids a potential exception by using FirstOrDefault, hence the extra null check. Also, could go with (x => typeof(TValue) == x) if you'd prefer an exact type match.

---

I’m happy to submit a PR with the above fix or, if you're not digging that change, let's discuss another approach. If a PR is welcome, would you prefer that I add some tests as well, or do you just want the patch with the fix?

Let me know if you'd like any further adjustments!

[cathei]

Hello, @Smile-James, thank you for reporting the issue.

Reading this, I don't think the issue comes from the hardcoded [1]. The sheet reference is the nested value type, and should always be Sheet<TKey, TRow>.Reference, regardless of the child sheet generic arguments. Therefore it is reliable to assume [1] will be the row type.

However, I understand this behavior might not be intuitive. Since references are implemented as nested type of base Sheet<,> class, Reference type references based on the row, not sheet. That means BakingSheet has limitation that you cannot reuse concrete row type over multiple sheets. The common solution is to define derived row type per each sheet.

I'll test a bit and see if I can add more practical error messages. Let me know if you think there is more to this. Thank you!

[Smile-James]

Thanks for the quick response.

However, I see that I made a mistake and withheld critical info. My example was incomplete and this detail escaped me last night as I was writing up this issue.

What you've said here is true, assuming that the user doesn't have their own ISheetReference type. If they also have their own, nested ISheetReference type, then what I've said is true. Check it out:

// Here's my complex Sheet type
public class MyComplexSheet<Foo, Bar, Baz, TKey, TRow> : Sheet<TKey, TRow> 
{
    // Within this class, I've declared my own ISheetReference type:
    // Note that in this type, GenericTypeArguments is:
    // node.ValueType.GenericTypeArguments
    // {System.Type[5]}
    //   [0]: {Foo}
    //   [1]: {Bar}
    //   [2]: {Baz}
    //   [3]: {TKey}
    //   [4]: {MyComplexSheet+Row}
    // ie. typpeof(MyComplexSheetRef).FullName
    // =>   "MyComplexSheet`5+MyComplexSheetRef[[Foo, xyz, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null],[Bar, xyz, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null],[Baz, xyz, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null],[TKey, xyzzy, Version=4.0.0.0, Culture=neutral, PublicKeyToken=abc],[MyComplexTable+Row, xyz, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null]]
    // This is where the true problem is:
    public class MyComplexSheetRef : ISheetReference, IAnotherInterface
    {
       /* ... */
    }
}

So in this case, the MyComplexSheetRef Type has a GenericTypeArguments that looks something like this:

    node.ValueType.GenericTypeArguments
    {System.Type[5]}
      [0]: {Foo}
      [1]: {Bar}
      [2]: {Baz}
      [3]: {TKey}
      [4]: {MyComplexSheet+Row}

And the hard-coded [1] in Sheet.cs fails to find the row type in this case. Note that if I rearrange the generic type arguments in MyComplexSheet it can work again, but I have to be careful about where TKey and TRow are positioned. For example: MyComplexSheet<TKey, TRow, Foo, Bar, Baz> works again!

 node.ValueType.GenericTypeArguments
  {System.Type[5]}
      [0]: {TKey}
      [1]: {MyComplexSheet+Row}
      [2]: {Foo}
      [3]: {Bar}
      [4]: {Baz}
      

What you've said about only being able to use a row type once in a SheetContainer is true, too. However, that isn't what I'm trying to bring attention to with this issue.

I hope this clarifies the problem. Please let me know what you think!

[Smile-James]

Life is busy, and I’m not trying to rush a response from you, but I just wanted to follow up and offer some assistance.

I’m not sure how you feel about accepting contributions directly, but if it would be helpful, I can create a minimal Unity project that demonstrates this issue and push it to GitHub for you to look at.

Or, if you’re open to PRs, I could fork your repo and add an example test on a separate branch that hits this problem. I’m also happy to go ahead and fix it, if you’d like, and then submit a PR for your consideration.

Let me know how I can help! I’m happy to do the heavy lifting on this one if you’re open to contributions.

Just wanted to add that I’ve really enjoyed using BakingSheet so far. It’s easy to use, has clean code, and the project is well-organized. Nice work!

[cathei]

First, thank you for the kind words! And yes, I'm open to accept PRs and contributions!

As you pointed out, if you want to have custom sheet reference the GenericTypeArguments[1] can be a problem. Though, taking FirstOfDefault for generic arguments of concrete type will make another assumption in this case, and will be order sensitive if there are multiple generic arguments that is assignable from row type. I think the "correct way" to solve this is to introduce generic ISheetReference<TKey, TRow> interface and get the generic arguments from the implementing interface.

I'm not fully sure about supporting custom references, but adding an interface wouldn't be a much of a problem.