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.

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

JSON not minified & errors out by persisting to dataNvarchar in cmsPropertyData #92

Open
jamiehowarth0 opened this issue Sep 23, 2019 · 7 comments

Comments

@jamiehowarth0
Copy link

Variant data is persisted to the dataNvarchar column in Umbraco 7.15, which is capped at 500 characters. Also, the JSON is not minified (unsure if this is just a debug/release issue), which leads to a ridiculous amount of whitespace, as per https://our.umbraco.com/packages/website-utilities/tea-commerce/tea-commerce-support/99189-bug-in-data-truncation-on-cmspropertydata.

PR incoming shortly.

@jamiehowarth0
Copy link
Author

OK... ValueType is set to JSON on the PropertyEditor attribute, which in theory should persist as ntext. This could be an Umbraco bug...

@jamiehowarth0
Copy link
Author

OK this appears not to be an Umbraco bug, but a dependency problem with Teacommerce, as the built-in Image Cropper functions correctly and persists to the dataNtext column in cmsPropertyData.

Interestingly, the variant editor is the only datatype that's not using the new package.manifest format. I'm wondering if that may fix the issue.

@mattbrailsford
Copy link
Contributor

Some good investigation work @benjaminhowarth1 Pretty interesting.

If the same can be achieved with the manifest file, that is currently done via the code file, I'm happy to move it. The only thing we need to consider however is what happens for people upgrading? Will it loose the value because it's now looking elsewhere? Is there a way to move the value over? We have an in build migration system, so that might be what we need to use.

@jamiehowarth0
Copy link
Author

We should be able to identify any propertyType using the editor, then update the cmsPropertyData accordingly. That would do it.

@jamiehowarth0
Copy link
Author

Minification of JSON in Umbraco is covered here & I've submitted a corresponding PR.

@nul800sebastiaan
Copy link

Isn't the main problem here that something is persisted to nvarchar instead of ntext?

The datatype should have a ValueType = "JSON" on it, like, for example, Nested Content has:

[PropertyEditor(Constants.PropertyEditors.NestedContentAlias, "Nested Content", "nestedcontent", ValueType = "JSON", Group = "lists", Icon = "icon-thumbnail-list")]

From reading this forum post, adding that now will unfortunately not work for existing installs, the only way to fix it is to remove and re-add the datatype.

Of course you could probably go into the cmsDatatype table and update the type, but make sure to also migrate existing content for this datatype in cmsPropertyData from the Nvarchar to the Ntext column.

FYI modifying the database schema as you did here will probably lead to upgrade problems in the future.

@jamiehowarth0
Copy link
Author

The issue is the PropertyEditor attribute does have ValueType = "JSON" on it, and it's still not persisting to dataNtext. I'm thinking it's a bug in the TeaCommerce dependency somehow, but I have no idea why. However, I've extracted it from a C# datatype to one in the package.manifest, given that it doesn't have any complex data storage requirements, and I've written a Gist to update the cmsPropertyData table.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants