Add Microsoft Graph API Support to SharePoint Connector#5833
Add Microsoft Graph API Support to SharePoint Connector#5833tinestaric wants to merge 9 commits intomicrosoft:mainfrom
Conversation
- Introduced a new field "Use Graph API" in Ext. SharePoint Account table and page to toggle between SharePoint REST API and Microsoft Graph API. - Updated tooltips for clarity on folder paths for both APIs. - Refactored Ext. SharePoint Connector implementation to utilize Graph API for file operations, including ListFiles, GetFile, CreateFile, CopyFile, MoveFile, FileExists, and DeleteFile. - Created Ext. SharePoint Graph Helper codeunit to encapsulate Graph API logic for file and directory operations. - Added Ext. SharePoint REST Helper codeunit for existing REST API operations. - Ensured compatibility with existing functionality while providing an option to leverage Microsoft Graph API for enhanced performance and capabilities.
There was a problem hiding this comment.
Pull request overview
This PR adds Microsoft Graph API support to the SharePoint Connector as an alternative to the existing SharePoint REST API implementation. The enhancement addresses Business Central's 150 MB HTTP response limit by enabling chunked downloads through Microsoft Graph API, which supports this capability unlike SharePoint REST APIs.
Key Changes:
- Introduces a "Use Graph API" toggle to switch between REST and Graph API implementations
- Refactors connector logic to route operations based on API selection
- Adds new helper codeunits to encapsulate Graph and REST API operations separately
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| ExtSharePointAccount.Table.al | Adds "Use Graph API" boolean field with improved tooltips explaining path requirements for both APIs |
| ExtSharePointAccount.Page.al | Updates UI to display the new "Use Graph API" toggle |
| ExtSharePointConnectorImpl.Codeunit.al | Implements routing logic to delegate operations to appropriate helper based on API selection |
| ExtSharePointGraphHelper.Codeunit.al | New codeunit encapsulating all Microsoft Graph API file operations |
| ExtSharePointRestHelper.Codeunit.al | New codeunit encapsulating existing SharePoint REST API operations |
| GraphAuthClientCredentials.Codeunit.al | Bug fix for certificate parameter assignment in authentication |
Note: Since this PR depends on System Application changes (PR #3655) and I cannot view the actual diff of changes, I cannot provide specific code-level review comments. The PR appears well-structured based on the description, maintains backward compatibility by keeping REST API as the default, and follows a clean separation of concerns by creating dedicated helper codeunits for each API implementation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@IceOnly @StefanSosic @JesperSchulz |
IceOnly
left a comment
There was a problem hiding this comment.
Looks good!
I’ll try to test it at the end of the week.
Only suggestion:
It might make sense to replace the "Use Graph API" boolean with an enum, add an interface, and use that interface to select the correct helper. This could make the design more flexible and easier to extend later.
Including bug fix here should be fine! |
I will also take a look. No holidays for me yet 😊 |
Thanks! This comment isn't a no; I'm open to switching to an interface, but it's more of a discussion. Where would you see it extended in the future? |
...W1/External File Storage - SharePoint Connector/App/src/ExtSharePointGraphHelper.Codeunit.al
Outdated
Show resolved
Hide resolved
.../External File Storage - SharePoint Connector/App/src/ExtSharePointConnectorImpl.Codeunit.al
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
| field(Disabled; Rec.Disabled) { } | ||
| field("Use Graph API"; Rec."Use Graph API") { } |
There was a problem hiding this comment.
If someone selects or deselects this field then Base Relative Folder Path maybe needs to be updated.
I'm not sure about the best behaviour.
The less intrusive would be a notification to check if the Base Relative Folder Path is still correct setup.
This comment is explicit for the Card page that someone opens for an existing setup.
There was a problem hiding this comment.
@pri-kise I now added a warning. But mostly in allot of cases the field is blanket out so the user need to fill it in again.
| end; | ||
| } | ||
|
|
||
| field("Use Graph API"; Rec."Use Graph API") |
There was a problem hiding this comment.
I understand why you added the field at the end.
It's a new setup field.
Since this fields has an influence on the Base Relative Folder Path I would suggest to add this field before the field Base Relative Folder Path. Otherwise a user might need to updat the path field once again afterwards.
I did even thought if it would be better to have an additional step before the common setup for both where a user must select this new option. (Currently I'm not sure if this would be really better)
JesperSchulz
left a comment
There was a problem hiding this comment.
This looks good to me! Once Kilian's comments have been addressed, I think we're ready to push this forward!
| if Path = '' then | ||
| Path := '/'; | ||
|
|
||
| SharePointGraphClient.GetItemsByPath(Path, GraphDriveItem); |
There was a problem hiding this comment.
In all other calls, we use the return value:
Response := SharePointGraphClient.DeleteItemByPath(Path);
if not Response.IsSuccessful() then
ShowError(Response);
Just checking that you're deliberately not doing that here.
|
|
||
| internal procedure ListFiles(SharePointAccount: Record "Ext. SharePoint Account"; Path: Text; FilePaginationData: Codeunit "File Pagination Data"; var TempFileAccountContent: Record "File Account Content" temporary) | ||
| var | ||
| GraphDriveItem: Record "SharePoint Graph Drive Item"; |
There was a problem hiding this comment.
In ListDirectories you're using a temp record. Here not. Is that intentional?
|
@JesperSchulz since @tinestaric is on holiday for 3 months I have talked to him that I would resolve any comments. |
|
@Bertverbeek4PS made you a collaborator on the fork. :) |
Thanks allot @tinestaric |
|
Syncing repo. |
| AadTenantId := NewAadTenantId; | ||
| ClientId := NewClientId; | ||
| Certificate := Certificate; | ||
| Certificate := NewCertificate; |
There was a problem hiding this comment.
Great find! I think this is a very important fix that can't wait. Maybe we should get it in separate PR as soon as possible? In case if this PR will be merged a little bit later
| Error(ErrorOccurredErr, Response.GetError()); | ||
| end; | ||
|
|
||
| local procedure SplitPath(FullPath: Text; var FolderPath: Text; var ItemName: Text) |
There was a problem hiding this comment.
I think we can simplify this function. Instead of using ReverseString with a loop, we can simply call:
LastSlashPos := FullPath.LastIndexOf('/');| LastSlashPos: Integer; | ||
| begin | ||
| // Find the last slash to split path | ||
| LastSlashPos := StrPos(ReverseString(FullPath), '/'); |
There was a problem hiding this comment.
Could it be that FullPath has a trailing slash? In that case, I'm not sure the code will work correctly.
There was a problem hiding this comment.
Mostly indeed it has a trailing slash.
| var | ||
| CheckBasePathMsg: Label 'The API type has been changed. Please verify that the Base Relative Folder Path is still correct for the selected API type.'; | ||
| begin | ||
| Message(CheckBasePathMsg); |
There was a problem hiding this comment.
Should we show this message when check it from true to false?
There was a problem hiding this comment.
Ok, I get it now. Basically, you control both connector options with this boolean
I understand that there are no other implementations besides Legacy REST API and Graph API.
But why don't we just make it an enum value? Without using interfaces. Just to be more readable. In this case, we can, for example, make Graph API the default value, and it will look much better than boolean = true by default.
Also, maybe it will be another implementation in future, low chance, but worth to mention
There was a problem hiding this comment.
Not sure if there allot of other implementation will be there and if an enum is a overkill for this
| AllowInCustomizations = Never; | ||
| DataClassification = SystemMetadata; | ||
| } | ||
| field(13; "Use Graph API"; Boolean) |
There was a problem hiding this comment.
Should be DataClassification = SystemMetadata; ?
…I usage and improve path handling in Graph Helper
|
Added some changes of @Drakonian. @JesperSchulz fyi |
Summary
Add Microsoft Graph API Support to SharePoint Connector
BC Idea
Implements Idea #21d4bf5c-d7b4-f011-aa43-7c1e52a6c1f4
Problem Statement
Business Central limits the maximum HTTP response content size to 150 MB by default (controlled by the
NavHttpClientMaxResponseContentSizeserver setting). This limitation causes file downloads from SharePoint to fail for files larger than this threshold.While chunked downloads would solve this issue, SharePoint REST APIs do not support chunked downloads (only uploads). Microsoft Graph API, however, does support this capability.
Solution
This PR introduces Microsoft Graph API support as an alternative implementation alongside the existing SharePoint REST API, with a backward-compatible toggle to switch between the two.
Changes
ExtSharePointAccount.Table.alandExtSharePointAccount.Page.alto control API selectionExtSharePointConnectorImpl.Codeunit.alto route operations based on API selectionExtSharePointGraphHelper.Codeunit.al: Encapsulates Graph API operations for files and directoriesExtSharePointRestHelper.Codeunit.al: Encapsulates existing REST API operationsGraphAuthClientCredentials.Codeunit.althat prevented certificate-based authentication from working correctlyBackward Compatibility
The existing SharePoint REST API implementation remains the default option, ensuring no breaking changes for current users. The Graph API can be enabled per account configuration.
Dependencies
Depends on SharePoint module additions in the System Application (see #3655)
Benefits
Work Item(s)
Fixes #5383
Fixes AB#612932