-
Notifications
You must be signed in to change notification settings - Fork 308
Add Microsoft Graph API Support to SharePoint Connector #5833
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add Microsoft Graph API Support to SharePoint Connector #5833
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Thanks!
JesperSchulz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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