-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
System.Data.SqlClient.SqlException: String or binary data would be truncated in table 'Umbraco.dbo.umbracoLog', #9458
Comments
Looks like something is a bit too long when this method gets called https://github.com/umbraco/Umbraco-CMS/blame/v8/contrib/src/Umbraco.Core/Persistence/Repositories/Implement/AuditRepository.cs#L38 This might have been a problem for about 6 years it looks like.. 😅 We'd love some help getting this one fixed up! |
Hi @kimschurmann, We're writing to let you know that we would love some help with this issue. We feel that this issue is ideal to flag for a community member to work on it. Once flagged here, folk looking for issues to work on will know to look at yours. Of course, please feel free work on this yourself ;-). If there are any changes to this status, we'll be sure to let you know. For more information about issues and states, have a look at this blog post Thanks muchly, from your friendly Umbraco GitHub bot :-) |
@nul800sebastiaan any small fix would be helpful here? Trim the text, try/catch it and log the error? Anything.. But an AuditLog that throws an exception is not a good option. |
Yeah any fix would work for me I guess, first of all it needs to be clear what is being inserted that is too long and then it can be fixed depending on what it is. |
@nul800sebastiaan does it matter what is being inserted? If it fails if the length exceeds the size of the column in SQL then do that validation here? Should be a quick fix? |
Something's not right. The truncated string is only 100 characters long, but the column is defined to have a max of 500 characters. Can you check the length of the column in SQL Server? |
Yes, I want to know where we're trying to insert more than 500 chars, knowing that the table doesn't support that. Seems like we made a mistake if that's the case and then that needs to be fixed. 😃
@SteveVaneeckhout which truncated string are you referring to? 🤔 |
@nul800sebastiaan It's on the first line of his stacktrace:
|
@nul800sebastiaan I've made a PoC on this locally with a fresh install.
@SteveVaneeckhout It is the stack trace generation, that is limiting the string to 100 characters. The actual string is over 500 characters. |
@SteveVaneeckhout Ah!! Good point, any idea @kimschurmann if your database schema is wrong here? @kevinstampe Looks fine, but I'd rather figure out first if we need to do anything at all, I still don't know of anything that is trying to insert more than 500 chars and I'd like to understand first what is trying to be insterted and why. The second worst thing after failing an insert is only getting half the data you were trying to log. 😅 |
In src\Umbraco.Core\Services\Implement\ContentService.cs line 809, the code actually puts the langs string into both the Message column (max length 4000) and the Parameters column (max length 500).
@nul800sebastiaan Do we have any reason to fill the data in twice, in the same row? The row it inserts, looks like this: The reason it fails is because alot of countries are being saved at once, then the logger receives a string longer than 500 characters, generated by the 'langs' variable. EDIT: |
Exactly @kevinstampe - the problem is if you try to save too many language variations of a piece of content. |
@nul800sebastiaan When it was decided that the column is only 500 chars - it was also decided that truncated audit logs was a possibility. I think most users are fine with that - but not being able to publish pages is not fine. |
.Wait, you can't publish pages now? That wasn't clear from your initial report, that changes things! So.. add 20 languages and try to publish a page fails?
No that seems a little silly 😊 |
@nul800sebastiaan Yeah, well.. If the pages you try to publish aren't saved first individually, then it fails. It is not the publishing that fails, it is the saving. |
@nul800sebastiaan - sorry (I did not explicitly write that)- the exception is propagated to the client and shown to the user. The stacktrace above is taken from that error dialog. So no save event is executed. We can save them invidiually of course - but not more than what is 500 chars long :) I think some wrapping with try/catch would be nice in these scenarios - no logging events should be able to break (core) functionality imo? |
No of course it shouldn't. We'll fix! |
@nul800sebastiaan thanks - we are pondering on a dirty fix locally - as this might drag on a bit before it is available in a version? Do you have any suggestion on that? We are thinking of perhaps just altering the column to nvarchar(4000) or even MAX? Or creating and injecting a logger that handles this edge case and uses the base functionality for the rest? |
Yeah of course, just make it NVARCHAR(MAX) for now. |
@nul800sebastiaan thanks - just worried you might change the schema too in a migration and that would fail if there are warnings/errors on possible truncated/lost data? |
I think that should be fine, we don't check if the column is exactly 500 chars / 4000 chars first. |
@nul800sebastiaan but SQL does? :) |
🤷♂️ no idea, does it? |
ALTER TABLE table_name
ALTER COLUMN column_name new_data_type(size); I don't see why that would check what the old size was or how it would know that it needs to be an exact amount. 😊 |
@nul800sebastiaan it wouldnt fail on any check, but if you decrease a columns data size to ex. 500 from 1000 in a migration, and you have data that is larger than 500, then the migration would fail |
Ah I see! Well, I don't know what we're going to do. But I'll update here when we do. Until then, there's a small risk we don't set it to max, for sure! 😅 |
Any idea when do we see a fix for this issue? |
We have just run into this issue as well. Of course it's annoying to replicate manually as you need to update say 30+ variants on one node, then save/publish them all in one go. You could argue that people wouldn't be updating that many different variants without saving more frequently! However we hit the problem because uSync was publishing a new piece of content with 71 variants to a different environment. So of course one node was being updated for 71 different languages in one go. And this error appears to be preventing uSync from properly creating all the variants. Perhaps Umbraco Deploy might have the same problem if people try pushing content with a large number of languages? I have implemented the following workaround locally:
I then re-ran the uSync import and this time it all worked fine and all the content variants were created properly. FYI when Umbraco logs the 'PublishVariant' activity as a result of this, the 'parameters' column contains: English (United States), English (United Kingdom), Spanish (Spain), Amharic (Ethiopia), Bulgarian (Bulgaria), Czech (Czech Republic), Danish (Denmark), German (Germany), Greek (Greece), English (Australia), English (Belgium), English (Canada), English (Ghana), English (Ireland), English (India), English (Kenya), English (Nigeria), English (New Zealand), English (Singapore), English (South Africa), Spanish (Chile), Spanish (Colombia), Spanish (Mexico), Spanish (Peru), Spanish (United States), Spanish (Uruguay), Spanish (Bolivarian Republic of Venezuela), Finnish (Finland), French (Belgium), French (Canada), French (Cameroon), French (France), Hindi (India), Croatian (Croatia), Hungarian (Hungary), Indonesian (Indonesia), Italian (Italy), Japanese (Japan), Korean (Korea), Burmese (Myanmar), Dutch (Belgium), Dutch (Netherlands), Norwegian, Polish (Poland), Portuguese, Portuguese (Brazil), Romanian (Romania), Russian (Russia), Swedish (Sweden), Kiswahili (Kenya), Thai (Thailand), Ukrainian (Ukraine), Vietnamese (Vietnam), Chinese (Traditional, Hong Kong S.A.R.), Chinese (Traditional, Taiwan) Those 71 variants have a character count of 1104. If we implement this workaround on our production database, and then this column is changed in a future release to a smaller size it sounds like that migration would fail. Which is not ideal. But this is show stopper for us as it is preventing us from being able to push content to production, so not sure we have any choice. So maybe we should up it to 1500 which is big enough for our use case and just hope for the best?! |
Thanks for the update! Looks like we haven't been able to prioritize this issue. At this point we'd love a pull request to fix this. We'd be looking for a migration that works on both SQLCE and SQL Server, it should take into account that people have already manually updated the column size, so it won't throw an error during the upgrade. If anyone is up for the task then we'd love that! 👍 |
@LottePitcher I am also facing this issue in my application aswell, I just upgraded my aplication from version 10 to 13, are these scripts not executing while the umbraco upgrade? |
The max length still configured 500, it should be more than that, because I had around 50 language variants for my content 500 is not enough for that, is there any reason why its not changing to an higher limit character type? |
We have added a custom component that creates and saves pages on all languages that have english as fallback, to help the editor create default pages. But occasionally this fails with an audit log.
Here is the stack trace:
Umbraco version
I am seeing this issue on Umbraco version: 8.5.5
Reproduction
Create a website with a lot of langauges that fallback to English. Add this component and try to save a new english page.
Here is the custom component:
Bug summary
Specifics
Steps to reproduce
Expected result
Actual result
This item has been added to our backlog AB#9875
The text was updated successfully, but these errors were encountered: