Extract the page-update rules from PageUpdateManager and inject IClock (#369 part 2a)

[sambetts]

Summary

First of two PRs finishing #369 (the App Insights importer half of #381). This one lifts PageUpdateManager's pure decisions into testable classes and injects IClock. The persistence ports (I*PersistenceManager, IImportDbMaintenance), HitPatch/HitUpdatesSqlExtension and PageViewSaveResult follow in part 2b — this PR touches no SQL, no staging schema and no merge script.

Part 1 (PR #386) extracted PageViewStagingRules, SearchTermRules and ClickEventRules from the save extensions. This PR does the same job for the page-update manager, which the issue calls out as the largest remaining piece.

What moved

Four new classes under WebJob.AppInsightsImporter.Engine/PageUpdates/Rules/:

Class	Rule	Why it matters
PageUpdateGroupingRules.GroupByUrl	Bucket a chunk of page-update events by their URL's base address	Keeps the StringComparer.OrdinalIgnoreCase comparer that stops a casing-only difference producing a second urls row — the same class of bug as #380/#385. Also keeps the "build the buckets once" property that replaced an O(events × urls) scan.
PageUpdateRefreshPolicy.StaleBeforeUtc	The metadata refresh-suppression window	A sign flip here makes every URL look stale and rewrites the whole page-metadata set every cycle.
PageUserEventRules	Validation + de-duplication of the comments and likes on a page-update event, plus the sentiment-enrichment gate	Decides what gets created and what triggers a billable cognitive-services call.
UrlMetadataPropertyRules.IsImportableSimpleProp	The "ignore system and over-sized fields" filter	Moved verbatim.

PageUpdateManager now takes an optional IClock (#368), defaulting to SystemClock.Instance, and uses it for all six DateTime.UtcNow sites it owned: the refresh cutoff, the MetadataLastRefreshed stamp in SaveAll, the two FileMetadataPropertyValue.Updated stamps, and the Created fallbacks for a comment and for a like. Both constructors gained the parameter as a trailing optional, so no call site breaks.

Ordering is preserved deliberately

ProcessCustomAppInsightsEvents used to interleave "log this invalid event" and "create this new record" as it walked the input. PageUserEventRules.Classify therefore returns decisions in input order and the caller still walks them one at a time, so:

• the operator-facing WARNING: Invalid comment/like metadata in event: … lines appear in the same sequence relative to the record creations;
• if a creation throws (it can — the callback inserts users and languages), exactly the same set of earlier events has been logged and created as before.

Pre-classifying up front is safe because dbValues is a materialised list from the database that the callbacks never mutate — they add to db.UrlLikes / the newComments dictionary, not to it.

Two pieces of existing behaviour are documented rather than "fixed", because #369 forbids behavioural change:

• matching is by SharePoint id only (there is deliberately no unique index on (url, sp_id) — a user really can comment twice on a page);
• duplicate ids within one batch are each reported as new, because the stored set is not updated as records are created. In practice the page-update compile step has already de-duplicated likes and comments by SharePoint id before this is reached.

UrlMetadataPropertyRules.IsImportableSimpleProp is likewise verbatim, including two pre-existing quirks I deliberately did not change and have documented in its XML comment: a null field name throws (unreachable from the importer — a JSON object cannot have a null key), and StartsWith(string) is the culture-sensitive overload.

The one deliberate divergence

The staleness cutoff is now computed in C# and passed as a parameter:

// before
u.MetadataLastRefreshed < DbFunctions.AddMinutes(DateTime.UtcNow, minusMetadataRefreshMinutes)
// after
var staleBeforeUtc = PageUpdateRefreshPolicy.StaleBeforeUtc(_config.MetadataRefreshMinutes, _clock.UtcNow);
u.MetadataLastRefreshed < staleBeforeUtc

Corrected after review — my first explanation of this was wrong. EF6 does not evaluate a bare DateTime.UtcNow inside a LINQ-to-Entities query on the client: it maps it to the canonical function CurrentUtcDateTime(), which the SQL Server provider renders as SysUtcDateTime(). So the old predicate compared against the database server's clock; the new one compares against the web-job host's clock.

The real change is therefore the clock source, not datetime-vs-datetime2 parameter typing. The impact is bounded by app-to-database clock skew — sub-second on NTP-synced Azure — against a 24-hour staleness window, and it is arguably more self-consistent: MetadataLastRefreshed is stamped from the host clock in SaveAll, so the comparison now uses one clock rather than two. Note there is no form of this predicate that keeps the database clock and is injectable — passing _clock.UtcNow into DbFunctions.AddMinutes parameterises the host clock just the same. The PageUpdateRefreshPolicy XML doc records this.

Testing

New: 16 tests in AppInsightsPageUpdateRulesTests — zero SQL Server, zero cognitive services, zero wall clock. What each pins:

• casing-only URL differences share a bucket; query string and #fragment are ignored; events with no usable URL are dropped; bucket order is preserved (the compile step takes Name/Username from the first update); a Greek SharePoint URL is grouped rather than dropped;
• the stale cutoff is before now, scales with the configured window, and is driven by the supplied instant rather than the wall clock (a third, near-tautological zero-window test was dropped on review advice rather than padded);
• the field-name length boundary is exclusive at 100 (99 in, 100 out), vti_x005f… is dropped but vti_… is not, and a Greek field name is kept;
• an event with no e-mail or no SharePoint id is invalid with a null normalised e-mail; the e-mail is lower-cased for the user lookup; an already-stored SharePoint id is not recreated; decisions come back in input order; likes classify identically to comments;
• sentiment is requested only when the client is configured and there is at least one new comment.

Pre-existing, unchanged, still passing (DB-backed): PageUpdateManagerTests, PageUpdateManagerMultithreadTests, PageUpdateManagerMultithreadSamePropNameValTests, PageUpdateManagerSamePropNameValTests, CommentsSaveTests, PageUpdateManagerCommentsAndLikesTests, DupTaxonomoyPropertiesWithDifferentGuidsPageUpdateManagerTests, PageUpdateEventAppInsightsQueryResultMergeTest, PageUpdateEventAppInsightsQueryResultCommentsAndLikesMergeTest, and both PageUpdateManagerPerfTests.

Area run: 111 passed / 1 failed of 112 — the failure is the pre-existing CommentsCognitiveTests, which cannot resolve the placeholder example-cognitive.cognitiveservices.azure.com endpoint in the local dev config and passes in CI. Full solution builds (Debug).

Reviewer hotspots

1. The datetime parameter-typing divergence above — is the ≤3.33 ms rounding acceptable, or should the DbFunctions.AddMinutes form be restored?
2. PageUserEventRules.Classify versus the original loop — in particular that pre-classifying cannot observe a stale dbValues, and that ordering/partial-failure semantics really are identical.
3. The tests — for each, what realistic regression makes it fail? I have tried not to add anything that only asserts Math.Max-grade arithmetic; call out anything that reads as theatre and I will delete it rather than pad it.

Still to come in part 2b

IPageViewsPersistenceManager / ISearchesPersistenceManager / IClicksPersistenceManager / IPageUpdatePersistenceManager / IHitUpdatePersistenceManager / IImportDbMaintenance with Sql* adapters and in-memory fakes, PageViewSaveResult carrying the counts PageViewStagingPlan already computes, and removing new SPOInsightsEntitiesContext() + Console.WriteLine from HitPatch.cs. IImportDbMaintenance is shared with #374, so it will be defined once — whichever of the two lands first.

Addresses #369