Dev #255
Dev #255
Conversation
…xDepth") to be thrown from ctor SiteMapNodeModel
Unittests to support correct behaviour when looking for Nodes to include in the model when visibilityAffectsDescendants is set to false and descending to a lower level
|
This looks good. I particularly like how you made the node hierarchy documentation clear in your test case - I will update the other test cases to follow the same conventions. While there are no apparent problems with the code in its current state, there are a few additions that would be extremely helpful.
The visibilityAffectsDescendants behavior in XmlSiteMapResult has been on my radar (item 10 in the list), but I haven't had time to clean it up. In hindsight, it would make more sense to have a SiteMap level setting "visibilityAffectsDescendants" (in ISiteMapSettings that is exposed as a public readonly property on ISiteMap) that would affect all of the HTML helpers and XmlSiteMapResult at once. For backward compatibility with the current HTML helper overloads, when a visibilityAffectsDescendants parameter is specified, it would override this SiteMap-wide setting. The nice thing about that change is that it would mean you could use the original overload you wanted Anyway, this isn't a requirement to accept your pull request, but if you are interested in the benefits of 1) making visibilityAffectsDescendants work on XmlSiteMapResult and/or 2) making visibilityAffectsDescendants a SiteMap-wide setting and you have the available time, we would certainly appreciate the contribution. I will begin by making unit tests for items 2 and 3 above. |
|
I'll have a look later this week. I don't have a clue about how much time it would take me to fix these items, I can spent a couple of hours per week, but I'll start with the XmlSiteMapResultFactory and take it from there one step at a time. Is item 10 the best place to ask any questions I might have? Or is there some other forum for general questions? Or we could just send emails. |
|
No problem. FYI - I merged your changes into the dev branch already and added a few more commits there (and will likely keep adding), so make sure to pull the latest if/when you start. I will probably release a patch after I have completed my unit tests, and the rest of these changes will need to go into a minor version release. The XmlSiteMapResultFactory is basically just there to prevent the need for multiple constructor overloads on XmlSiteMapResult when calling it in a controller action. XmlSiteMapResult is what does all of the heavy lifting for the /sitemap.xml endpoint. The rough equivalent of the SiteMapNodeModel.Children property is the XmlSiteMapResult.FlattenHierarchy() method and it will require similar branching logic to implement the VisibilityAffectsDescendants = true case. Personally, I don't think there should be a visibilityAffectsDescendants=true case, but something needs to be done because the default behavior of the Menu, SiteMapPath and SiteMap HTML helpers is visibilityAffectsDescedants=true and the only behavior of XmlSiteMapResult is visibilityAffectsDescendants=false, It is not possible to change these defaults in v4 because it would break backward compatibility (although I would argue that we should change the XmlSiteMapResult to visibilityAffectsDescendants=true to be consistent with the others). Having a global visibilityAffectsDescendants setting seems to be the best compromise to ensure there is consistent behavior while still allowing visibilityAffectsDescendants=true, since there is a performance benefit to that setting. When we move on to version 5, we can then flip the default setting back to visibilityAffectsDescendants=false and leave the global setting in place so it can be used to mimic v4 behavior, if needed. I am not sure if the performance benefit is worth the confusion caused by the visibility not toggling on and off like it should. Anyway, we can correspond in this pull request (whether open or closed) or you can post your comments in another issue if they are on topic there. We don't have a forum and I prefer to communicate here rather than by email since it will keep Maarten and other contributors in the loop as well. |
|
Finally had some time to look into XmlSiteMapResult - visibilityAffectsDescendants issue. The problem is clear but I'm not sure I understand your solution correctly. Should it be possible to override the ISiteMapSetting in XmlSiteMapResultFactory like in the HTML helper overloads? |
That is a good question. Another good question is should it be possible to do in every overload combination? Personally, I think I made a wrong turn by making this an overload setting instead of a global one because it makes little sense to allow this behavior in one location but not another. In addition, I took a shortcut by only making a small subset of the overloads (the ones with the most parameters in each "branch" of functionality) rather than doubling the number of Menu overloads (which had already been inflated from 21 overloads in v3 to 63 overloads in v4). Actually, my plan was not to touch the menu logic at all, but before the v4 release we had a contributor make a performance enhancement on the menu, which primarily involved changing the visibility behavior in the menu. It wasn't until after v4 was released that I realized the visibility behavior had changed, so visibilityAffectsDescendants was created as a way to re-enable v3 behavior (which in my opinion is the correct behavior). With that in mind, I would rather not create any additional overloads in XmlSiteMapResultFactory. There doesn't seem to be any reason to, as in the extremely rare case where someone wanted different behavior between the HTML helpers and XmlSiteMapResult, they could work it out by adjusting the global setting to set XmlSiteMapResultFactory and using the overloads to set the HTML helpers. XmlSiteMapResult Scalability Issue #258 When making changes to XmlSiteMapResult, you should be aware that there are plans to fix a scalability problem which arises from storing the complete set of XElement items in an in memory collection before sending them to the output stream. I am not asking you also take on this additional task (you can if you wish), but just review it so to structure the visibilityAffectsDescendants logic so it can be easily converted later. See my comments for a potential solution. The visibilityAffectsDescendants logic should be processed in the first stage - that is the stage that builds the stream (or streams) of raw data. After thinking this over a little more, it probably makes sense to make a series of "named" streams, one for each page number (or more likely page index so the first one is 0). That way, it could correspond to separate files on disk, each with a maximum payload of 35,000 URLs. The only thing that we would want to keep around in memory is the total count of (visible) URLs, which would be cached for 12 hours. The expiration of the cache could be used to signal the regeneration of the streams on the next incoming request, the same way we do for the SiteMap object. Each incoming request could then use the total node count and some math to determine which page is being requested and which named (or numbered) stream to serve the URL data from or whether to generate the sitemaps index file, which could be done on the fly. We could potentially use the XElement object as the type that stores the URL payload rather than something custom, assuming the type is serializable. The IEnumerable result of FlattenHierarchy() should also be addressed at some point, but it is not contributing to the main performance bottleneck of XmlSiteMapResult because all we are doing is creating pointers to objects that already exist in memory in the SiteMap object. At some point we need to address the scalability problem of storing the entire SiteMap in memory as well, but since there are a few workarounds to that problem it is not as critical to fix. |
|
FYI - Your patch is now live in v4.4.7. Also, the scalability problem doesn't seem to be with the XmlSiteMapResult, but rather with the SiteMap itself (which has now been greatly improved). So if you still intend to work on this you don't have to worry about that part. We have another contributor working now on adding a fluent API and if possible I would like to roll the visibilityAffectsDescendants fix into the same release. If it helps, I could add the global configuration setting so you can see where to read the value from. Thanks for all of your help so far. |
|
I allready produced some code. I will do a pull Request sometime tomorrow
|
|
Closing this issue as we now have another open pull request that deals with the same thing (and this is already merged into master). |
Fix and unit tests for #254