[Bug]: SharePoint Graph update methods (PR #8318) - follow-up corrections

[JesperSchulz]

Follow-up to #8318 (SharePoint Graph Module - extend functionality with additional methods), which was merged before these review points were addressed. Volodymyr Dvernytskyi (@Drakonian), could you pick these up in a follow-up PR?

Describe the issue

Code review of #8318 found one behavioural defect and several convention issues in the new UpdateListItem / UpdateDriveItem / RenameDriveItem code paths.

1. Silent field loss in UpdateListItem (main issue)

In SharePoint Graph Client Impl. (codeunit 9120), UpdateListItem does:

GraphListItem.Init();
GraphListItem.Id := CopyStr(ItemId, 1, MaxStrLen(GraphListItem.Id));
GraphListItem.ListId := CopyStr(ListId, 1, MaxStrLen(GraphListItem.ListId));
SharePointGraphParser.ParseListItemFieldValueSet(JsonResponse, GraphListItem);
if not GraphListItem.Insert() then
    GraphListItem.Modify();

The PATCH to .../items/{id}/fields returns only a fieldValueSet, so it carries no ContentType, WebUrl, CreatedDateTime or LastModifiedDateTime. When a caller reuses a buffer that was previously populated by GetListItem, the Init() blanks those fields and the Modify() writes the blanks back over data that was already correct.

The XML doc says the record "is refreshed", which understates this - the record is effectively replaced, and four previously-valid fields are silently cleared.

2. ItemBufferCollisionTelemetryErr is missing Locked = true

ItemBufferCollisionTelemetryErr: Label 'The record already contains item from a different list. Use a separate record variable per list.';

It is only ever passed to Session.LogMessage, so it should be locked. It is the only telemetry-only label in the file without it (compare OperationSuccessTelemetryMsg and GraphSharePointCategoryLbl). As-is it gets sent for translation and emits localized telemetry.

3. Hand-rolled query string in GetListItem

Endpoint := SharePointGraphUriBuilder.GetListItemByIdEndpoint(ListId, ItemId);
if not GraphOptionalParameters.GetODataQueryParameters().ContainsKey(Format(Enum::"Graph OData Query Parameter"::expand)) then
    Endpoint += '?$expand=fields';

This is the only raw query-string concatenation in the module. It works, but the idiomatic form is GraphOptionalParameters.SetODataQueryParameter(Enum::"Graph OData Query Parameter"::expand, 'fields') - the parameter is passed by value, so mutating the local copy is safe and it keeps all URI assembly inside the builder.

4. ParseListItemFieldValueSet has no return value

Every other parser in SharePoint Graph Parser returns Boolean and callers check it. This one does not, so UpdateListItem cannot detect a malformed PATCH response and will report success on garbage input.

5. Telemetry event IDs are shared across four public entry points

UpdateDriveItem, UpdateDriveItemByPath, RenameDriveItem and RenameDriveItemByPath all funnel through UpdateDriveItemAtEndpoint, so all four log 0000UKR / 0000UKS / 0000UKT with the operation name hard-coded as 'UpdateDriveItem'. A RenameDriveItemByPath failure is indistinguishable from an UpdateDriveItem failure in telemetry.

Separately, it is still worth confirming that 0000UKB-0000UKV are actually allocated to this area - this is the question Magnus Hartvig Grønbech (@Groenbech96) raised on the PR in July that was never answered.

Expected behavior

1. UpdateListItem should either preserve the fields the PATCH response does not return, or the XML doc on both the facade and the implementation should state plainly that the record is replaced and that ContentType, WebUrl, CreatedDateTime and LastModifiedDateTime are cleared. Preserving them is the better outcome.
2. ItemBufferCollisionTelemetryErr carries Locked = true.
3. GetListItem sets the default expand through Graph Optional Parameters rather than string concatenation.
4. ParseListItemFieldValueSet returns Boolean and UpdateListItem checks it, matching the surrounding parser contract.
5. Each public update/rename entry point is distinguishable in telemetry.

Steps to reproduce

For the UpdateListItem field loss:

1. Call GetListItem(ListId, ItemId, TempGraphListItem) and observe WebUrl, ContentType, CreatedDateTime and LastModifiedDateTime are populated.
2. Call UpdateListItem(ListId, ItemId, FieldsJson, TempGraphListItem) with the same record variable.
3. Observe those four fields are now blank, while Title and the fields JSON reflect the update.

TestUpdateListItem_BufferReusedFromGet in codeunit 132984 covers this exact sequence but only asserts Id, Title and the request count, so the loss is not caught. Extending that test with assertions on WebUrl / ContentType reproduces it.

The remaining points are visible by inspection of SharePointGraphClientImpl.Codeunit.al and SharePointGraphParser.Codeunit.al.

Additional context

Two further review points from #8318 that are no longer actionable in code, noted here only so the thread is complete:

• The PR description's validation checklist described a follow-up GET after the PATCH and a UpdateListItem_FollowUpGetFails test. Neither exists in the merged code - UpdateListItem issues a single PATCH. It also stated 22 new tests where there are 25. The description appears to have been written against an earlier iteration; worth keeping the checklist in sync with the final diff on future PRs, since reviewers lean on it.
• Roughly 837 of the 862 added README lines are a new architecture document (dependency graphs, object registry, test-coverage counts, pattern catalogue) unrelated to the linked issue. The object-registry and test-coverage sections in particular will drift out of date quickly - worth deciding whether they should be maintained or trimmed.

One consistency observation for whoever picks this up: the new methods use if not Insert() then Modify(), while the pre-existing GetDriveItem uses a bare Insert() and hard-errors on a reused buffer. TestStickyModeStillWorks works around this with a TempDriveItem.DeleteAll(). Not introduced by this PR, but the client now has two different buffer-reuse contracts, and aligning them would be worthwhile.

Original review context: #8318, fixes #8317.

[github-actions]

🤖 AI triage completed. Product group, please validate the triage report (Microsoft internal access only, use Ctrl+Click to open in new tab).

---

_{Was this triage helpful? React with 👍 or 👎 on this comment to provide feedback.}

[Drakonian]

Thanks, I will create PR soon Jesper Schulz-Wedde (@JesperSchulz)

About points:

1. Valid
2. Valid
3. I think it's incorrect

the parameter is passed by value, so mutating the local copy is safe

Codeunits are reference types in AL, so SetODataQueryParameter on the parameter mutates the caller's instance and leaks $expand=fields into subsequent unrelated requests; the endpoint form was introduced deliberately to fix exactly that.

4. I think it's incorrect

The claimed scenario ("cannot detect a malformed PATCH response and reports success on garbage") cannot occur as stated:

• SharePointGraphRequestHelper.Patch -> ProcessJsonResponse already fails on any non-2xx or non-JSON body, so the parser always receives a JSON object.
• A fieldValueSet has no mandatory keys - unlike ParseListItemDetail, which requires id. There is no condition on which this parser could fail; a Boolean return would be always-true dead weight with an unreachable error branch in UpdateListItem.

At the same time I agree about small inconsistency on contract level. I will think about it.

5. Valid

I also verified that IDs 0000UKB-0000UKV appears only once. Ofc, I can't be sure if these IDs are valid in general Magnus Hartvig Grønbech (@Groenbech96)