Brainstorm: "2500 limit" issue

[tomfulton]

We're storing the configuration of each Archetype in the cmsDataTypePrevalues table, but the field is an nvarchar(2500). This causes a YSOD to bubble up on Save if your Archetype's configuration (JSON blob) ends up being longer than that.

Since we've removed some of the config options, this issue probably isn't as prevalent, but it still happens on some of my test datatypes.

The current workaround is to change the limit in SQL: ALTER TABLE [your-db] ALTER COLUMN [value] nvarchar(MAX)

There's probably not much we can do to fix this, but maybe we can ease the pain somehow so users don't see a YSOD and help guide them towards the solution.

[tomfulton]

Idea: Keep an eye on the length of the model while the user is building the Archetype, and alert them somehow (client-side) when they reach 2500. The alert could possibly contain a link to some info about the issue and/or include the raw SQL statement. Maybe we could also include a webservice that makes the change for them? (Not sure if that's smart)

[kgiszewski]

@tomfulton I think it needs to be a conscious decision by the package installer (person doing it). Just not sure what to do. I'm not 100% sure the core will change this anytime soon.

[mattbrailsford]

I'd suggest raising a bug on issues.umbraco.org if you haven't already asking to make it "text" or "nvarchar(max)". Not sure why it would be given a limit of 2500.

[kgiszewski]

@mattbrailsford I reported it last April: http://issues.umbraco.org/issue/U4-2120

Could you some votes I suppose :)

[leekelleher]

We have a dirty workaround for this (because we had to) ... in ApplicationEventHandler.ApplicationStarted we check if the database table column is nvarchar(2500), then ALTER it to be an ntext.

It feels like an expensive way to handle it, but we were in a desperate position. // @mattbrailsford

[kgiszewski]

@leekelleher We've flirted with the idea of handling it on install. Not sure if that would be any cleaner.

[leekelleher]

@kgiszewski - On install sounds like a better event to do it on. Let us know if you want us to share our code for it.

[kgiszewski]

Please do :)

I think v7 still uses a user control at the end of the package install process.

On 2/13/14, 8:02 AM, Lee Kelleher wrote:

@kgiszewski https://github.com/kgiszewski - On install sounds like a better event to do it on. Let us know if you want us to share our code for it.

— Reply to this email directly or view it on GitHub https://github.com/imulus/Archetype/issues/34#issuecomment-34975639.

[leekelleher]

We can write it as a package-action that runs on install. I wont get chance to look this before next week though.

[kgiszewski]

No worries. I'll merge your other PR in the meantime.

[mattbrailsford]

Just a note on this being done on package install. You'll need to check if UaaS runs package actions when deploying (I'm guessing not) as if it doesn't, then it means your UaaS instances won't be updated. This was the reason our implementation does it at app start.

To be honest, I don't see why the app start check is an issue. It checks whether it needs running before applying the fix, so it's not wasteful. At worst, it one db hit. At most it's 2 :)

Just my 2 cents

Matt

On Thursday, 13 February 2014, Kevin Giszewski notifications@github.com wrote:

No worries. I'll merge your other PR in the meantime.

Reply to this email directly or view it on GitHubhttps://github.com/imulus/Archetype/issues/34#issuecomment-35027982 .

[leekelleher]

@mattbrailsford Good point (you probably told me this yesterday). Guess we was trying to remove the need for the app-start check; but yes UaaS would be a major issue (as it wouldn't run the package-action)

[tomfulton]

Yeah - on install sounds more elegant, but thinking about deployments (non-UAAS), AppStart would seem to be the most reliable way since the package installer will likely never be ran in some environments.

I still feel dirty modifying someone's DB, especially without deliberate consent, but this should be minor and not cause any issues. ...Right? :grimacing:

I wonder if we need to alter the statement at all for MySQL or SQLCE as well. Anyway, your code sounds like a good start if you'd like to share!

[mattbrailsford]

Defo works on ce and azure. Dunno bout MySQL. Do people still use that? ;)

On Thursday, 13 February 2014, Tom Fulton notifications@github.com wrote:

Yeah - on install sounds more elegant, but thinking about deployments (non-UAAS), AppStart would seem to be the most reliable way since the package installer will likely never be ran in some environments.

I still feel dirty modifying someone's DB, especially without deliberate consent, but this should be minor and not cause any issues. ...Right? [image: :grimacing:]

I wonder if we need to alter the statement at all for MySQL or SQLCE as well. Anyway, your code sounds like a good start if you'd like to share!

Reply to this email directly or view it on GitHubhttps://github.com/imulus/Archetype/issues/34#issuecomment-35029756 .

[leekelleher]

Submitted PR #60

[Attackmonkey]

You could always set a config value in the web.config file to say that you've updated the database, that way you can check that first, to avoid the extra DB hit?

[kjac]

Submitted PR #112

[leekelleher]

OK, so I hit an issue with Courier... seems that there's a 4000 character limit!

I believe this is due to how Courier implements NHibernate, defining the PreValue column as an Nvarchar - so it throws an exception before attempting to sync the database (regardless of the fact that the db column itself is actually an Ntext type).

It's difficult to pinpoint the code, (since Courier is closed-source), but this is once-again a show-stopper for me using complex Archetypes with UaaS.

I haven't tried the chunking patch from #113 yet - but it's looking like that's currently the best option.

</rant>

[kgiszewski]

Snap! If you have some time and don't mind testing #113, that might be the reason to go with chunking // @kjac @tomfulton

[leekelleher]

Sure thing, I'll take a look tomorrow (or over the weekend, depending on workload)

[kjac]

Whoops.... that's messed up. Sorry to hear that, and I'm not gonna say "I told you so". Really, extending the size of the column is the most neat solution, as long as Archetype supports doing so during install (and I can see that the DatabaseSizer project was meant for that). I will forward merge the master to #113 and have it ready for you for testing asap.

[larsudengaard]

It seems the Archetype config is persisted in the 'value' column with json-formatting. Line-breaks, whitespaces, indentation and all. Removing the formatting would give some extra leverage regarding the 2500 char limit.

[kjac]

Yep, would give a few extra bytes here and there if we stripped all formatting. But it just postpones the inevitable a little bit, I'm afraid... we need a working solution that will allow Archetypes of any size. Hopefully the chunkify solution can work, or even better - perhaps Courier can be brought to play nice with the schema change.

[kgiszewski]

@kjac We're still trying to urge the core team to do a DB migration as a proper fix :)

[kjac]

:+1: would without a doubt be the best way around this.

[leekelleher]

I meant to chat with you all at CG14 about this. It turns out that the prevalue config for the new Grid feature (in Umbraco 7.2) uses a large-ish JSON structure. I discussed this with Morten and Per at the CG14 Retreat and agreed that the database column should be switched to an Ntext. No discussion of when this would happen, but the seed has been sown.

[kjac]

Awesome...! :)

[kgiszewski]

w00t!

[kgiszewski]

I saw our Umbraco Issue got assigned to v7.2

[kjac]

Yay... finally!

[jamiepollock]

Hey folks. Just to be completely clear. This fix is required for development with Archetype with pre-v7.2 sites because it's to do with core Umbraco rather than Archetype?

[kgiszewski]

While not required, you'll get an error that the field is too small when your Archetype config (stored as JSON) gets larger than 2500 characters (core limitation). But for most uses of Archetype, you'll hit that limit quickly.

To be clear, v7.2 ups the limit to ntext which avoids this issue.

If you manually up the limit to avoid the issue (pre-7.2), we can't be sure of how graceful an upgrade will be going from v7.1.x to v7.2.

[jamiepollock]

Hi Kevin, Thanks for that. I'm already experiencing the problem myself and if it was still the case I was going to share a gist with my solution. I know it's not a particularly difficult solution to fix but if it could save people time.

https://gist.github.com/jamiepollock/8c527fb50fde65ef50e1

If this isn't recommended practice, I can remove the link.

Thanks, Jamie

[kgiszewski]

Thank you so much. @leekelleher has already included a similar idea in the source code: https://github.com/imulus/Archetype/tree/master/app/Umbraco/DatabaseSizer

The DLL is available on the release page.

[jamiepollock]

Doh, trust me and my skim reading! I'll check it out. Thanks again.

[owlyowl]

Hi guys we're getting this behaviour.. we're running 7.2

https://our.umbraco.org/forum/developers/extending-umbraco/66856-custom-property-editor-with-angular-array-to-add-rows-throwing-500-on-save-and-publish

Just wondering if this is the same issue. We're getting an SQL truncated exception

[kjac]

Issue solved on our. @owlyowl please don't post non-Archetype related questions here :) @kgiszewski this issue should probably be closed as it's been fixed in the core since 7.2?

[kgiszewski]

This is now fixed with 7.2.x :)