change LogDto Comment and Parameters to nvarcharmax

[LottePitcher]

Prerequisites

• I have added steps to test this contribution in the description below

This fixes issue #9458

Description

This pull request is a result of a Discord Hack session earlier today with Seb, Warren and some other community folk.

To replicate the issue you would need to have say 40+ languages configured in your back office, and then try to save or publish all variants in one go. This causes a SQL error, which therefore prevents the node being saved/published. This is because Umbraco is trying to write a record to the UmbracoLogs table with a comma delimited list of the affected variants in the parameters column, and that column has a max length of only 500 chars.

We checked where the 'parameters' column is used. Turns out it's displayed on the 'info' tab as indicated in this screenshot:

We discussed truncating the string to 500 characters before saving but this would mean that there was missing information on the info tab, and it is important to know who published which variant.

We discussed that having such a long list of language names isn't ideal, especially as the language list is hard-coded to the English name. But those enhancements would need proper discussion, so we agreed to keep the scope of this PR to be just fixing the issue that is preventing people from saving content (which does actually happen when you use a tool such as uSync or Translation Manager to create this many variants in one go!).

People who have hit this problem already may have increased the size of the column on the database themselves, so we set the string to be nvarchar(max) as this wouldn't cause those people any truncation issues during an upgrade.

We also thought it was a good idea to change the logComment column at the same time to be nvarchar(max) as its existing length of 4000 felt rather arbitrary.

Once the database column has been increased in size you are then able to save/publish 40+ variants in one go. This was shown on the Discord session earlier today.

As this commit contains database schema changes, this PR has been tested on:

• A fresh install using SQL db
• An upgrade of existing instance using SQL db
• A fresh install using SQL CE db
• An upgrade of existing instance using SQL CE db

In all tests above the database gets created / updated correctly.

Oh and @nul800sebastiaan said to add the migration to a folder called 'V_8_18_0', so I did 😉

[umbrabot]

Hi there @LottePitcher, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

• It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
• The automated tests all pass (see "Checks" tab on this PR)
• The level of security for this contribution is the same or improved
• The level of performance for this contribution is the same or improved
• Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
• If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
• The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

[mikecp]

Hi @LottePitcher , thanks for the PR, with max 2Gb those fields should indeed be long enough now 😅😁

I noticed that one of the data persistence tests failed, I'll run them again just to make sure, but if it remains, it might be worth investigating if it's due to this change.

Cheers!

[LottePitcher]

Thanks @mikecp

System.Data.SqlServerCe.SqlCeException : The ntext and image data types cannot be used in WHERE, HAVING, GROUP BY, ON, or IN clauses, except when these data types are used with the LIKE or IS NULL predicates.

Considering this PR introduced an nvarchar(max), which I believe is ntext in SQL CE, it does sound like it is the culprit!

Sounds like further investigation is required 😢. But yay for tests 😂

[mikecp]

Yes I saw the ntext issue in the log too, and indeed feared that this would be the CE interpretation for nvarchar(max) 😥
But it's indeed good that we have tests to tackle this before it goes live 😅

[mikecp]

Code changes look all good but some further investigation is needed about the failing test(s).

[nzdev]

Have a look at https://github.com/umbraco/Umbraco-CMS/pull/11014/files for how to work around SQLCE. That pr fixes the rest of the ntext columns

[p-m-j]

During lunch hack today we looked into whether we can update ExpressionVisitorBase.cs to write the operand as 'like' as opposed to = for SqlCe when a where clause is checking against a ntext column.

We proved this works by always using like as opposed to = but the code needs to be more context aware.

[umbrabot]

Hi there @LottePitcher!

Thanks for the contribution here and apologies if it has been a while since you heard from us. We have been in the very fortunate position of having lots of work to do. With this in mind, we are writing to let you know that with the release of the Long Term Support (LTS) version, 8.18, we have now moved into the support phase of Umbraco 8. You can read all about that here but to surmise, we will be keeping Umbraco 8 safe and well by releasing patching for security or regression issues if they arise but no longer will we do that for bug fixes. The same is still true for features, although we stopped merging those some time ago.

We'd love for you to keep contributing and while we are not able to merge this to Umbraco 8, if this is still something you'd like to see in Umbraco 9, please take a look and either create an issue to say so or find an issue that already exists. We'll be happy to give you some input around how you can adjust your pull request to target Umbraco 9. Even better, it might be something that Umbraco 9 already does or has. In which case, enjoy!

Once again, a huge thank you for the time you have spent working with us.

#H5YR
Your friendly Umbraco GitHub bot 🤖 🙂