ArgumentOutOfRangeException in TableValueConverter

hfloyd commented 6 months ago

I am using Limbo Models Builder for model generation. In the generated model file, the property value converter is called:

=> this.Value<global::Limbo.Umbraco.Tables.Models.TableModel>("ReturnsCalendarYear");

but it is failing:

ArgumentOutOfRangeException: Index was out of range. Must be non-negative and less than the size of the collection. (Parameter 'index')
System.Collections.Generic.List<T>.get_Item(int index)
Limbo.Umbraco.Tables.Models.TableModel.ParseCellRow(int index, JArray array, TablesHtmlParser htmlParser, bool preview)
Limbo.Umbraco.Tables.Models.TableModel+<>c__DisplayClass18_0.<.ctor>b__2(int i, JArray x)
Limbo.Umbraco.Tables.Models.TablesDataExtensions.ForEach<TResult>(JArray array, Func<int, JArray, TResult> callback)
Limbo.Umbraco.Tables.Models.TableModel..ctor(JObject json, TableConfiguration config, TablesHtmlParser htmlParser, bool preview)
Limbo.Umbraco.Tables.Models.TableModel.Parse(JObject json, TableConfiguration config, TablesHtmlParser htmlParser, bool preview)
Limbo.Umbraco.Tables.PropertyEditors.TableValueConverter.ConvertIntermediateToObject(IPublishedElement owner, IPublishedPropertyType propertyType, PropertyCacheLevel referenceCacheLevel, object inter, bool preview)
Umbraco.Cms.Core.Models.PublishedContent.PublishedPropertyType.ConvertInterToObject(IPublishedElement owner, PropertyCacheLevel referenceCacheLevel, object inter, bool preview)
Umbraco.Cms.Infrastructure.PublishedCache.Property.GetValue(string culture, string segment)
Umbraco.Extensions.PublishedPropertyExtension.Value<T>(IPublishedProperty property, IPublishedValueFallback publishedValueFallback, string culture, string segment, Fallback fallback, T defaultValue)
Umbraco.Extensions.PublishedContentExtensions.Value<T>(IPublishedContent content, IPublishedValueFallback publishedValueFallback, string alias, string culture, string segment, Fallback fallback, T defaultValue)
Umbraco.Extensions.FriendlyPublishedContentExtensions.Value<T>(IPublishedContent content, string alias, string culture, string segment, Fallback fallback, T defaultValue)

Limbo.Umbraco.Tables Version 1.1.3

Stepping through the code, it appears the issue is here:

Rows has a count of zero.

Rows is supposed to be populated here: but doesn't find anything.

This site is an upgrade form Umbraco 7, and the prior Table editor property type. The back-office display of the data works fine, so I assumed that the data model hadn't changed... but I will need to look closer, I guess.

hfloyd commented 6 months ago

This is a sample of the v7 data:

{
    "useFirstRowAsHeader": false,
    "useLastRowAsFooter": false,
    "tableStyle": null,
    "columnStylesSelected": [
        null,
        null
    ],
    "rowStylesSelected": [
        null,
        null,
        null
    ],
    "cells": [
        [
            {
            ...

There is no "rows" element, just a "cells" element.

When I create a new node and manually add table data, I get this:

{
    "rows": [
        {},
        {}
    ],
    "columns": [
        {},
        {}
    ],
    "cells": [
        [
            {
                "rowIndex": 0,
                "columnIndex": 0,
                "value": "<p>Test Col 1 Row 1</p>",
                "type": "td",
                "scope": null
            },
            {
                "rowIndex": 0,
                "columnIndex": 1,
                "value": "<p>Test Col 2 Row 1</p>",
                "type": "td",
                "scope": null
            }
        ],
        [
            {
                "rowIndex": 1,
                "columnIndex": 0,
                "value": "<p>Test Col 1 Row 2</p>",
                "type": "td",
                "scope": null
            },
            {
                "rowIndex": 1,
                "columnIndex": 1,
                "value": "<p>Test Col 1 Row 2</p>",
                "type": "td",
                "scope": null
            }
        ]
    ],
    "useFirstRowAsHeader": false,
    "useFirstColumnAsHeader": false
}

So it seems that there is a bit of a different format.

abjerner commented 6 months ago

Hi @hfloyd

Limbo.Umbraco.Tables has only been available for Umbraco 10, and it's predecessor, Limbo.Umbraco.StructuredData was only available for Umbraco 9. We have never supported Umbraco 7, so I'm unsure where you have the U7 data from.

The package is based on some internal code we had been using for Umbraco 8, which I then turned into a package for Umbraco 9. I didn't write the original code, and I'm not aware that my colleagues based it on some other package, but given the JSON format looks alike, maybe they did 🤷‍♂️

So to sum things up, your issue is not a bug in this package, but due to a different JSON format from another package. One could argue that maybe the rows and columns properties aren't necessary for parsing the cell values and building the C# model, but as things are now, it's not something that I have the time to look into. This could however be a good candidate for a PR, so if you're able to look at this, I'd be happy to review a PR that changes the value converter. Alternatively you could look into changing the source data to match this package.

hfloyd commented 6 months ago

Hi @abjerner 👋🏻

The v7 site I'm upgrading was using Imulus.TableEditor, and when I was doing the update, and I installed your package and just set those properties to use a Limbo.Tables-backed datatype, the tables appeared beautifully in the back-office, which made me think perhaps it was a direct update of that codebase. Thanks for sharing the history. 🙂

I have a fork and plan to work on a PR for you, I just ran out of time last night and wanted to document my findings. I think it will be easy to support both formats for reading legacy data. Look for something later today.

limbo-works / Limbo.Umbraco.Tables

ArgumentOutOfRangeException in TableValueConverter #28