Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ArgumentOutOfRangeException in TableValueConverter #28

Open
hfloyd opened this issue Feb 13, 2024 · 3 comments · May be fixed by #29
Open

ArgumentOutOfRangeException in TableValueConverter #28

hfloyd opened this issue Feb 13, 2024 · 3 comments · May be fixed by #29

Comments

@hfloyd
Copy link

hfloyd commented Feb 13, 2024 •
edited
Loading

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:
image

Rows has a count of zero.

Rows is supposed to be populated here:
image
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
Copy link
Author

hfloyd commented Feb 13, 2024

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
Copy link
Member

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
Copy link
Author

hfloyd commented Feb 14, 2024

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.

@hfloyd hfloyd linked a pull request Feb 15, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Added support for legacy Imulus.TableEditor formatted data hfloyd/Limbo.Umbraco.Tables
2 participants
@hfloyd @abjerner