U4-4880 - SEO Checker fails after upgrading from 7.1.1 to 7.1.2

Created by Sebastiaan Janssen 13 May 2014, 15:48:39 Updated by Shannon Deminick 16 May 2014, 02:11:27

Relates to: U4-4926

We must've broken something that used to work on 7.1.1, see screenshot for more details

3 Attachments

Comments

Shannon Deminick 14 May 2014, 03:06:44

This error is coming from their library: PreValueCollectionExtensions, @Richard.Soeteman (@rsoeteman ) you should definitely be ensuring he's not adding duplicate keys to your dictionary in that extension method.

SEOChecker.DataTypes.Extensions.PropertyValueCollectionExtensions.Add - this is the culprit. Which is called by another SEO checker class: SEOCheckerPropertyWrapper.

The only change we made previously that I can see is the change to DataTypeService.GetPreValuesByDataTypeId(int id) in rev: cefc319bfb665c38f08347f8aa059a38c417c862

This change is required because we would have got other YSODs since pre-values are either an array or a dictionary, if you use the property PreValuesAsArray and it is a dictionary you will get a ysod. I'm very surprised that this exception never showed up for SEO Checker.

Even if pre-values are an array, there is zero guarantee that there are not duplicate values in there and we we're not filtering them to be distinct anyways, so

Lastly, I cannot replicate this error since I don't know where it's coming from. I've clicked on every SEO checker thing i can find in the back office after it's been installed.


Shannon Deminick 14 May 2014, 03:22:53

I've tried another test which is to add the SEO Checker data type to a content item, i do not get this error either. I've actually tried setting each property to the same name in the data type pre-values to attempt to get this ysod but i can't because the value is stored as xml like:

<SEOCheckerDataTypeConfig><TitleMappingAlias>anotherMediaPicker</TitleMappingAlias><KeywordsMetaAlias>anotherMediaPicker</KeywordsMetaAlias><DescriptionMetaAlias>anotherMediaPicker</DescriptionMetaAlias><ShowKeywordOption>true</ShowKeywordOption></SEOCheckerDataTypeConfig>

I've also tested what would happen with the codebase before this rev: cefc319bfb665c38f08347f8aa059a38c417c862 and it would have thrown a YSOD there because the pre-values are not an array but a dictionary. Anytime new pre-values are saved with the new API they are always a dictionary, this has been the case since 7.0.

If you can give me any more ideas on how to replicate, i can try to see what is going on, perhaps it is something else we've changed. However, in the meantime, the SEO Checker should absolutely update it's codebase to ensure it's not trying to add duplicate keys to it's own dictionaries.


Shannon Deminick 14 May 2014, 03:57:09

Ok, have now managed to replicate this issue, but i don't know why it is happening. We are giving SEO checker the same data we gave it before, we don't even have duplicate data in the data were giving it so it's an issue in their logic in SEOChecker.DataTypes.Extensions.PropertyValueCollectionExtensions.Add

I've attached a screenshot of the breakpoint that is causing the error, you can see the pre-val with key SEOCheckerReadOnly has a value of true and the other prevalue (SEOCheckerPreValueProperty) has a value of the XML listed above. In the other screen shot the property editor that is accepting these pre-values is the SEOCheckerPropertyWrapper. I don't know what it is doing with the data but it is throwing the exception and from what i can tell with our code history we have not changed anything that would have presented a different data structure during this call in previous versions.

I've actually reverted this commit (cefc319bfb665c38f08347f8aa059a38c417c862) and tried with the old code and I get far more YSODs regarding pre-values not being formatted as an array (which is what I expected).


Shannon Deminick 14 May 2014, 08:41:34

The other thing to note is that this is intermittent. You'll get this ysod on a certain content item, then go to another one, then back and you don't get it, but then sometimes when you return to it you'll get it again. Pretty sure it's something to do with that 'true' parameter in the pre-values, which I don't know how it gets there, something internally of seo checker.


Richard Soeteman 14 May 2014, 09:01:04

Yep it's the pre-value that gets added twice. Patch release will be out this week. In case you need it now send me an email richard@soetemansoftware.nl


Shannon Deminick 14 May 2014, 09:02:55

So can you confirm that it's an SEO checker issue, not a core one ? Or if it's a core one can you let me know where it is?


Richard Soeteman 14 May 2014, 09:09:08

Well I'm using The PropertyValueEditorWrapper and the ConfigureForDisplay methods gets called with the prevalues collection filled since 7.1.2. I'm adding the SEOCheckerReadOnly item there. But since I am propably the only one using this I wouldn't bother too much.


Shannon Deminick 14 May 2014, 09:11:52

Yup, so I'm wondering what has changed? Isn't this the same as pre 7.1.2 ?


Richard Soeteman 14 May 2014, 09:14:00

No then it worked fine. Since 7.1.2 was released various people mentioned this and I would have seen it in 7.1.1 also since that was my main development environment.


Shannon Deminick 14 May 2014, 09:16:54

Ok, so is there any chance you can tell me what has changed? I'm trying to figure out what we've changed that is causing you this problem.


Shannon Deminick 16 May 2014, 02:11:15

Both richard and I have implemented fixes, in the seo checker package and in the core.

The core fixes relate to this: U4-4926


Priority: Normal

Type: Bug

State: Fixed

Assignee: Shannon Deminick

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted:

Affected versions: 7.1.2

Due in version: 7.1.3

Sprint:

Story Points:

Cycle: