Fixes the unable to cast to JToken exception

[AaronSadlerUK]

Fixes #39

[AaronSadlerUK]

@abjerner Any news on merging my PR?

[abjerner]

Hi @AaronSadlerUK and sorry for the lack of reply. I haven't really had the time to work much on our packages lately, including this one. While your change is relatively small, I still need to find a bit of time to test it before merging, and I can't say when that will be. Hope that makes sense 😉

[AaronSadlerUK]

@abjerner No problems, the issue is that since the changes to the RTE to support block list it now throws an error on save and cannot rebuild the indexes... So causes a massive performance hit.

The nuget package needs to target the version of v13 and above which introduced blocks in the rte (I can't remember the exact version)

[abjerner]

Although people should ideally move towards the newest version of U13, I don't think raising the version requirements is the correct way to go. Also setting the upper bound to 14) would suggest that the U13 package supports U14 pre-releases, which it doesn't. Can you revert your latest commit?

I haven't really looked into the specifics yet, so the description below is based on how I remember it to work.

But if we assume that earlier versions of U13 would save the RTE value as a string, and newer versions of U13 will save a JSON object with a markup property, we should also take people upgrading from old U13 to new U13 into account. Or people upgrading from U9-12 to U13.

Also assuming that the cell value is <p>Hello World</p>, new U13 would save something like { "markup": "<p>Hello World</p>" }, which with the proposed change, would be indexed to something like { "markup": "Hello World" }. We don't really want the JSON structure as that adds unnecessary noise (e.g. not being able to match Hello).

So instead of just converting the value token to a string, we can check against the type of the JSON token, and thus support both old and new U13 values. I've tried to illustrate this with a quick Razor partial:

@using Newtonsoft.Json.Linq
@{

    JToken sample1 = new JObject {
        {"value", "<p>Hello World</p>"}
    };

    JToken sample2 = new JObject {
        {"value", new JObject {
            {"markup", "<p>Hello World</p>"}
        }}
    };

    <h3>Sample 1</h3>
    string? value1 = sample1.Value<JToken>("value")?.ToString();
    <pre>@value1</pre> // outputs <p>Hello World</p>
    <pre>@value1?.StripHtml()</pre> // outputs Hello World ✅

    <h3>Sample 2</h3>
    string? value2 = sample2.Value<JToken>("value")?.ToString();
    <pre>@value2</pre> // outputs {  "markup": "<p>Hello World</p>" }
    <pre>@value2?.StripHtml()</pre> // outputs { "markup": "Hello World" } ❌

    foreach (JToken cell in new[] { sample1, sample2 }) {

        JToken? value = cell.Value<JToken>("value");

        switch (value?.Type) {

            case JTokenType.String:
                <pre>@(value.Value<string>())</pre> // returns <p>Hello World</p>
                <pre>@(value.Value<string>()?.StripHtml())</pre> // returns Hello World ✅
                break;

            case JTokenType.Object:
                string? markup = ((JObject)value).GetValue("markup")?.Value<string>();
                <pre>@(markup)</pre> // returns <p>Hello World</p>
                <pre>@(markup?.StripHtml())</pre> // returns Hello World ✅
                break;

        }

    }

}

So with this in mind the ProcessRow method should probably look something like this instead:

private static IEnumerable<string?> ProcessRow(JArray row) {

    foreach (JToken cell in row) {

        JToken? value = cell.Value<JToken>("value");

        switch (value?.Type) {

            case JTokenType.String:
                yield return value.Value<string>()?.StripHtml();
                break;

            case JTokenType.Object:
                string? markup = ((JObject)value).GetValue("markup")?.Value<string>();
                yield return markup?.StripHtml();
                break;

        }

    }

}

[AaronSadlerUK]

@abjerner I have reverted the version minimum change

[wardlawj]

Would it be possible to get this PR in? Also hitting this issue on v13.4.0

[abjerner]

@wardlawj as mentioned in #39, I haven't been able to reproduce the issue myself. I suspect the proposed fix isn't 100% correct, so I'd like to see the error myself and do some more testing before merging.

If you can provide some additional details on how to reproduce the error, I'd be happy to have another look 😉

[wardlawj]

Thanks for the reply @abjerner

My setup at the moment:

• Umbraco 13.14.0 install
• Element Doctype: tableBlock
• • With two properties (Title: textstring, table: Table Picker)
• Block list datatype with our tableBlock element added to it
• Create a page docType add a propertype that uses our Block list datatype

Create a page in the CMS add a tableBlock content block to it with dummy content. Save & Publish

In Examine settings, rebuild the External index go to log viewer and you should see the following error in there:

System.InvalidCastException: Cannot cast Newtonsoft.Json.Linq.JObject to Newtonsoft.Json.Linq.JToken.
at Newtonsoft.Json.Linq.Extensions.Convert[T,U](T token)
at Newtonsoft.Json.Linq.JToken.Value[T](Object key)
at Limbo.Umbraco.Tables.PropertyEditors.TablePropertyIndexValueFactory.ProcessRow(JArray row)+MoveNext()

And the rebuild fails

[AaronSadlerUK]

I just want to add we are seeing our issue on a multi-cultural site, not sure if that makes a difference

[abjerner]

That roughly matches my setup, but I still can't reproduce the error. I hadn't tried with variant content, but that doesn't seem to change anything.

Anyways, unless I missed something, none of the error reports mention what version of the Tables package is being used. Can any of you report back on this? This wouldn't be because you're still using the U10 package?

Also, can anyone of you share the property value for the block list of an affected page? I just need to see the general structure, so feel free to blank out any sensitive information.

[wardlawj]

Thanks Anders, appreciate you looking into this. Can confirm we're using v13.0.1

Did a bit of testing just now, this content is triggering the error for me:

[ { "contentTypeKey": "a2b207b0-898a-4461-a5d6-db1261d5cd93", "udi": "umb://element/4c063e28e5bc4f98b6f13347fece2ca2", "title": "", "table": { "rows": [ {}, {}, {}, {} ], "columns": [ {}, {}, {}, {} ], "cells": [ [ { "rowIndex": 0, "columnIndex": 0, "value": { "markup": "", "blocks": {} }, "type": "th", "scope": "col" }, { "rowIndex": 0, "columnIndex": 1, "value": "<h3>What personal data do we collect or proccess</h3>", "type": "th", "scope": "col" }, { "rowIndex": 0, "columnIndex": 2, "value": "<p>How do we use your personal data?</p>", "type": "th", "scope": "col" }, { "rowIndex": 0, "columnIndex": 3, "value": "<p>What is the lawful basis for processing my personal data?</p>", "type": "th", "scope": "col" } ], [ { "rowIndex": 1, "columnIndex": 0, "value": "<h3>What we do</h3>\n<p>Recording calls to and from our administration telephone lines. For more information please read our call recording <a href=\"/\" title=\"test\">privacy statement.</a></p>", "type": "td", "scope": null }, { "rowIndex": 1, "columnIndex": 1, "value": "<p>We’ll collect a recording of the conversation, your phone number, the date and time of the call and your personal details, in order to identify you.</p>", "type": "td", "scope": null }, { "rowIndex": 1, "columnIndex": 2, "value": "<p>We’ll collect a recording of the conversation, your phone number, the date and time of the call and your personal details, in order to identify you.</p>\n<p>We’ll collect a recording of the conversation, your phone number, the date and time of the call and your personal details, in order to identify you.</p>\n<p>We’ll collect a recording of the conversation, your phone number, the date and time of the call and your personal details, in order to identify you.</p>", "type": "td", "scope": null }, { "rowIndex": 1, "columnIndex": 3, "value": "<p>We’ll collect a recording of the conversation, your phone number, the date and time of the call and your personal details, in order to identify you.We’ll collect a recording of the conversation, your phone number, the date and time of the call and your personal details, in order to identify you.</p>", "type": "td", "scope": null } ], [ { "rowIndex": 2, "columnIndex": 0, "value": "<h3>testing some title text that will go here some extra text to make it longer</h3>", "type": "td", "scope": null }, { "rowIndex": 2, "columnIndex": 1, "value": "<p>We’ll collect a recording of the conversation, your phone number, the date and time of the call and your personal details, in order to identify you.We’ll collect a recording of the conversation, your phone number, the date and time of the call and your personal details, in order to identify you.We’ll collect a recording of the conversation, your phone number, the date and time of the call and your personal details, in order to identify you.</p>", "type": "td", "scope": null }, { "rowIndex": 2, "columnIndex": 2, "value": "<p>We’ll collect a recording of the conversation, your phone number, the date and time of the call and your personal details, in order to identify you.</p>\n<p>We’ll collect a recording of the conversation, your phone number, the date and time of the call and your personal details, in order to identify you.We’ll collect a recording of the conversation, your phone number, the date and time of the call and your personal details, in order to identify you.</p>", "type": "td", "scope": null }, { "rowIndex": 2, "columnIndex": 3, "value": "<p>We’ll collect a recording of the conversation, your phone number, the date and time of the call and your personal details, in order to identify you.</p>", "type": "td", "scope": null } ], [ { "rowIndex": 3, "columnIndex": 0, "value": "<p>We’ll collect a recording of the conversation, your phone number, the date and time of the call and your personal details, in order to identify you.</p>", "type": "td", "scope": null }, { "rowIndex": 3, "columnIndex": 1, "value": "<p>We’ll collect a recording of the conversation, your phone number, the date and time of the call and your personal details, in order to identify you.We’ll collect a recording of the conversation, your phone number, the date and time of the call and your personal details, in order to identify you.</p>\n<p>We’ll collect a recording of the conversation, your phone number, the date and time of the call and your personal details, in order to identify you.</p>", "type": "td", "scope": null }, { "rowIndex": 3, "columnIndex": 2, "value": "<p>We’ll collect a recording of the conversation, your phone number, the date and time of the call and your personal details, in order to identify you.</p>", "type": "td", "scope": null }, { "rowIndex": 3, "columnIndex": 3, "value": "<p>We’ll collect a recording of the conversation, your phone number, the date and time of the call and your personal details, in order to identify you.We’ll collect a recording of the conversation, your phone number, the date and time of the call and your personal details, in order to identify you.</p>", "type": "td", "scope": null } ] ], "useFirstRowAsHeader": true, "useFirstColumnAsHeader": false }, "anchorName": "", "disableBlock": "0" } ]

[abjerner]

Thanks @wardlawj

The error is caused by the first cell. I had an idea this was the cause (hence my examples earlier in this PR).

As in your case, I think the issue is caused by adding a empty cell. Getting actual data is key as I can quickly see the error 😄 I hadn't thought about testing with adding empty cells 🤔

When submitting the RTE overlay, this line is called. But since model.prop.value?.markup doesn't have a value, model.prop.value is used instead - which in this case is the object, and not the RTE string as intended.

[abjerner]

@AaronSadlerUK given the data above, the PR isn't entirely correct. Can you incorporate my code from here? #40 (comment)

Shortly described, if the cell value is an object, we should grab the value of the object's markup property, instead of just converting the object to a string (since that will then include some of the JSON structure). This should take care of the C# part.

For the JavaScript part, we also need to have a look at this line. The existing check isn't type safe:

cell.value = model.prop.value?.markup ? model.prop.value.markup : model.prop.value;

It should likely be something like this instead:

cell.value = typeof model.prop.value === "string" ? model.prop.value : model.prop.value?.markup ?? "";

The snippet below should prove that this will work:

var samples = [
    {prop:{value:null}},
    {prop:{value:""}},
    {prop:{value:"Hello World"}},
    {prop:{value:{markup:null}}},
    {prop:{value:{markup:""}}},
    {prop:{value:{markup: "Hello World"}}}
];

samples.forEach(function(model, index) {
    const value = typeof model.prop.value === "string" ? model.prop.value : model.prop.value?.markup ?? "";
    console.log(index, value);
});

[AaronSadlerUK]

@abjerner I plan on working on this soon!

Could you possibly submit the repo to hacktoberfest for Umbraco :P

[abjerner]

@AaronSadlerUK I've just added the hacktoberfest-accepted label to the repo. I can add that to your PR once merged 😉

[AaronSadlerUK]

@abjerner I have added your changes, however, I still get the issue when saving content, this looks to be from another package I think, but I cannot 100% confirm as my break points aren't being hit.

The error doesn't say where it's failing though, so I'm not 100% sure how to get past it

[abjerner]

I'll have a look merging in 😉

Do you have a stack trace for the other error?

[AaronSadlerUK]

Nothing helpful in pointing in any direction, I know it's the same error seen by this package, so it has to be a property value converter somewhere.

The error only happens on multilingual content which doesn't help narrow it down

The error below is returned on the PostSave endpoint

One or more errors occurred. (One or more errors occurred. (The JSON value could not be converted to System.Text.Json.Nodes.JsonArray. Path: $.result | LineNumber: 1 | BytePositionInLine: 13.))

System.AggregateException, System.Private.CoreLib, Version=8.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e: One or more errors occurred. (One or more errors occurred. (The JSON value could not be converted to System.Text.Json.Nodes.JsonArray. Path: $.result | LineNumber: 1 | BytePositionInLine: 13.))

   at Umbraco.Cms.Infrastructure.Scoping.Scope.RobustExit(Boolean completed, Boolean onException)
   at Umbraco.Cms.Infrastructure.Scoping.Scope.DisposeLastScope()
   at Umbraco.Cms.Infrastructure.Scoping.Scope.Dispose()
   at Umbraco.Cms.Core.Services.ContentService.SaveAndPublish(IContent content, String[] cultures, Int32 userId)
   at Umbraco.Cms.Web.BackOffice.Controllers.ContentController.PublishInternal(ContentItemSave contentItem, String defaultCulture, String cultureForInvariantErrors, Boolean& wasCancelled, String[]& successfulCultures)
   at Umbraco.Cms.Web.BackOffice.Controllers.ContentController.PostSaveInternal[TVariant](ContentItemSave contentItem, Func`3 saveMethod, Func`2 mapToDisplay)
   at Umbraco.Cms.Web.BackOffice.Controllers.ContentController.PostSave(ContentItemSave contentItem)
   at lambda_method1914(Closure, Object)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ActionMethodExecutor.AwaitableObjectResultExecutor.Execute(ActionContext actionContext, IActionResultTypeMapper mapper, ObjectMethodExecutor executor, Object controller, Object[] arguments)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeActionMethodAsync>g__Awaited|12_0(ControllerActionInvoker invoker, ValueTask`1 actionResultValueTask)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeNextActionFilterAsync>g__Awaited|10_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Rethrow(ActionExecutedContextSealed context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeInnerFilterAsync()
--- End of stack trace from previous location ---
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeNextExceptionFilterAsync>g__Awaited|26_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)```

[abjerner]

AggregateException is bad for logging 😞

It will have collection of one or more inner exceptions, so if you can get to any of those, you might be able to get some more details.

E.g. if you write your own bit of code so call save and publish on said page, I think you can catch the exception that way, and read the inner exceptions.

[abjerner]

Thanks again. I'll have a new release out soon 😉

[abjerner]

Aaaand new release is out: https://github.com/limbo-works/Limbo.Umbraco.Tables/releases/tag/v13.0.2

[wardlawj]

Great work @abjerner @AaronSadlerUK! Can confirm it's fixed the error for us.

Thanks again :-)