Skip to content

Conversation

@Drakonian
Copy link
Contributor

@Drakonian Drakonian commented May 7, 2025

Summary

This is the first version of the SharePoint Graph API module that leverages the Microsoft Graph module.

After analyzing the current REST implementation, I concluded that it cannot be changed without breaking backward compatibility. In addition, the REST and Graph APIs have completely different data models and sets of fields, which makes it impossible to reuse the same tables for entities that REST uses.

Therefore, adding a separate SharePoint Graph Client seemed the simplest and most natural solution. I also considered factory/builder patterns and dependency injection, but for the reasons mentioned above these approaches did not seem optimal, they don’t fit well and would greatly complicate the overall use of the SharePoint module.

I have already tested this code locally in various scenarios, and it behaves well. My plans are:

  • Detail review of all the code with improvements where necessary
  • Gathering feedback from the community and Microsoft
  • Test pagination
  • Test large file uploads
  • Restructuring the folders so that REST and Graph reside in separate, peer-level folders
  • Writing tests

Examples of how to use:

local procedure GetDrivesFromSite()
var
    TempGraphDrive: Record "SharePoint Graph Drive" temporary;
    SharePointGraphClient: Codeunit "SharePoint Graph Client";
    GraphAuthorization: Codeunit "Graph Authorization";
    GraphAuthInterface: Interface "Graph Authorization";
begin
    GraphAuthInterface := GraphAuthorization.CreateAuthorizationWithClientCredentials(
        TenantIdValue,
        ClientIdValue,
        ClientSecretValue,
        GraphScopeLbl);

    SharePointGraphClient.Initialize(SharePointUrlValue, GraphAuthInterface);

    if not SharePointGraphClient.GetDrives(TempGraphDrive) then
        Message('No drives found.');

    if TempGraphDrive.FindSet() then
        repeat
            Message('Drive Name: %1\Type: %2\Owner: %3\Quota Used: %4 of %5\Created: %6',
                TempGraphDrive.Name,
                TempGraphDrive.DriveType,
                TempGraphDrive.OwnerName,
                TempGraphDrive.QuotaUsed,
                TempGraphDrive.QuotaTotal,
                TempGraphDrive.CreatedDateTime);
        until TempGraphDrive.Next() = 0;
end;
local procedure UploadFileToRootDriveFolder()
var
    TempGraphDriveItem: Record "SharePoint Graph Drive Item" temporary;
    SharePointGraphClient: Codeunit "SharePoint Graph Client";
    GraphAuthorization: Codeunit "Graph Authorization";
    GraphAuthInterface: Interface "Graph Authorization";
    Diagnostics: Interface "HTTP Diagnostics";
    FileInStream: InStream;
    FileName: Text;
    FolderPath: Text;
begin
    GraphAuthInterface := GraphAuthorization.CreateAuthorizationWithClientCredentials(
        TenantIdValue,
        ClientIdValue,
        ClientSecretValue,
        GraphScopeLbl);

    SharePointGraphClient.Initialize(SharePointUrlValue, GraphAuthInterface);

    FolderPath := 'TestFolder'; //To root drive folder as we not specified DriveId
    if not UploadIntoStream('Select a file to upload', '', '', FileName, FileInStream) then
        exit;

    if SharePointGraphClient.UploadFile(FolderPath, FileName, FileInStream, TempGraphDriveItem) then
        Message('File uploaded successfully: %1', TempGraphDriveItem.Name)
    else begin
        Diagnostics := SharePointGraphClient.GetDiagnostics();
        Error('Error: %1 - %2', Diagnostics.GetHttpStatusCode(), Diagnostics.GetResponseReasonPhrase());
    end;
end;

Work Item(s)

Fixes #3198

Fixes AB#568746

@github-actions github-actions bot added AL: System Application From Fork Pull request is coming from a fork labels May 7, 2025
@Drakonian
Copy link
Contributor Author

@pri-kise

@github-actions github-actions bot added the Linked Issue is linked to a Azure Boards work item label May 7, 2025
@github-actions github-actions bot added this to the Version 27.0 milestone May 7, 2025
Drakonian added 2 commits May 15, 2025 13:31
…ific requests

Bound SharePointDiagnostics with response information
Add ability to upload large files in 4mb chunks
@JesperSchulz
Copy link
Contributor

Very interesting contribution! Please allow me to take a much closer look at your code in the coming days. Unfortunately I will be on the road next week, so this might take me a short while to get to - but I'll be on it soon!

@JesperSchulz JesperSchulz self-assigned this May 22, 2025
@JesperSchulz JesperSchulz added the Integration GitHub request for Integration area label May 22, 2025
@Drakonian
Copy link
Contributor Author

Very interesting contribution! Please allow me to take a much closer look at your code in the coming days. Unfortunately I will be on the road next week, so this might take me a short while to get to - but I'll be on it soon!

Thank you, I’ll look forward to your feedback -)

Copy link
Contributor

@pri-kise pri-kise left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all: I really appreciate the work you have done for this PR.

I'm unsure if maybe some of the functionalities in the graph helper codeunits should maybe better be placed into the graph client module.
I only took a short look into the new code so far. I will need some more time to look into the changes in more detail.

Update:
I'm not sure if it's really helpful to provide a boolean value as return value on all those functions.
How should anyone know why the got false.

I think that it would be more helpful to return errors when something didn't went well.

Furthermore on your planned improvements for pagination:
I'm not sure if this is possible with the GraphOptionalParameters, but I think that it would be helpful if we could specify a range from outside, too.
What I mean is that sometimes we might want to control the pagination steps from outside.

Return repsonse object instead of unclear boolean
Additional inetrnal methods for testing
@Drakonian
Copy link
Contributor Author

@pri-kise

Thank you for such a helpful review of the PR. Your remarks are very useful.

  1. I added an HTTP client handler, of course, it is absolutely essential here.
  2. I completely agree with your point about the boolean, it's a relic from an older code version where I was experimenting with building a SharePoint Graph module (trying to merge with the SharePoint REST module). I prefer the approach used in the Azure Blob Storage API, where a method returns an object containing detailed information about the operation, so I’ve applied a similar pattern here.

Regarding managing pagination externally, that’s an excellent observation, but I’m still considering the best way to implement it. I’ll think through an approach that is convenient.

I believe pagination would be best handled at the Microsoft Graph module level, this needs some further thought.

My plans: at the moment I intend to keep improving the code and finish writing the tests.

@JesperSchulz JesperSchulz changed the title SharePoint Graph API support [DRAFT] SharePoint Graph API support Jun 4, 2025
@JesperSchulz
Copy link
Contributor

@Drakonian, wow, that's impressive work! I must admit that I got "code review fatigue" about 1/3 into the review process - that's A LOT of code 🤣 I will need to see the code in action, rather than just read my way through it.

  1. I will try to compile and use your module updates
  2. I will wait for your tests to fully understand how the code is to be used in praxis

Generally though, I have no objections of accepting your changes. They seem to follow all of our best practices for system application modules!

@Drakonian
Copy link
Contributor Author

@Drakonian, wow, that's impressive work! I must admit that I got "code review fatigue" about 1/3 into the review process - that's A LOT of code 🤣 I will need to see the code in action, rather than just read my way through it.

  1. I will try to compile and use your module updates
  2. I will wait for your tests to fully understand how the code is to be used in praxis

Generally though, I have no objections of accepting your changes. They seem to follow all of our best practices for system application modules!

Thanks, I honestly spent quite a bit of effort to make the code match the quality and style of System Application :)

Of course, the best thing to look at is in action. I'll tag you when I'm done with the tests and also will prepare several examples of usage.

@JesperSchulz
Copy link
Contributor

@Drakonian, wow, that's impressive work! I must admit that I got "code review fatigue" about 1/3 into the review process - that's A LOT of code 🤣 I will need to see the code in action, rather than just read my way through it.

  1. I will try to compile and use your module updates
  2. I will wait for your tests to fully understand how the code is to be used in praxis

Generally though, I have no objections of accepting your changes. They seem to follow all of our best practices for system application modules!

Thanks, I honestly spent quite a bit of effort to make the code match the quality and style of System Application :)

Of course, the best thing to look at is in action. I'll tag you when I'm done with the tests and also will prepare several examples of usage.

I can tell! Really nice work! Let's get this reeled in in time for the 2025 wave 2 release 🥳 Of course we will need documentation updates and release notes, but we can work together on that!

Friday I'll try to take this for an actual spin! Exciting 👏

@tinestaric
Copy link
Contributor

@Drakonian, to echo @JesperSchulz, really, really nice work! And it's precisely what I'll need soon! Thank you! :D

I'll take this module for a spin; however, I see a couple of gaps in the code.

  1. UploadLargeFile sends the file by chunks, but there's no DownloadLargeFile equivalent. I recently discovered that BC has a limit on the size of the response content HttpClient can receive, set to 150 MB by default (controlled by the NavHttpClientMaxResponseContentSize server setting). That means that you can't download files larger than that, so we'd need a chunking option for download as well.

  2. I'm missing any Delete operations for either Files or Folders

  3. No existence check operations for files or folders

And there are two nice-to-haves, which I can work around with a combination of existing functions, but it would be cleaner to use the Graph API directly
4. Copy operation (but I can use Download+Upload)
5. Move operation - via PATCH parentReference (but Download+Upload+Delete gets me there as well)

@Drakonian
Copy link
Contributor Author

@tinestaric
Excellent observations!

I expect the next few weeks to be less busy, and I will be able to work on this PR. Thank you for your comments, they will help make the module much better.

@JesperSchulz
Copy link
Contributor

@JesperSchulz

I see build failed with new error:

Error: Unexpected error when running action. Error Message: ::Error:: Missing execute permissions for:  Codeunit SharePoint Graph Client , StackTrace: at <ScriptBlock>, D:\a\BCApps\BCApps\build\scripts\VerifyExecutePermissions.ps1: line 82 <- at <ScriptBlock>, D:\a\BCApps\BCApps\build\projects\System Application Modules\.AL-Go\PreCompileApp.ps1: line 11 <- at <ScriptBlock>, C:\ProgramData\BcContainerHelper\6.1.11-preview1370\BcContainerHelper\AppHandling\Run-AlPipeline.ps1: line 2228 <- at <ScriptBlock>, C:\ProgramData\BcContainerHelper\6.1.11-preview1370\BcContainerHelper\AppHandling\Run-AlPipeline.ps1: line 1739 <- at <ScriptBlock>, C:\ProgramData\BcContainerHelper\6.1.11-preview1370\BcContainerHelper\AppHandling\Run-AlPipeline.ps1: line 1734 <- at <ScriptBlock>, C:\ProgramData\BcContainerHelper\6.1.11-preview1370\BcContainerHelper\AppHandling\Run-AlPipeline.ps1: line 1285 <- at Run-AlPipeline, C:\ProgramData\BcContainerHelper\6.1.11-preview1370\BcContainerHelper\AppHandling\Run-AlPipeline.ps1: line 1270 <- at <ScriptBlock>, D:\a\_actions\microsoft\AL-Go-Actions\v8.1\RunPipeline\RunPipeline.ps1: line 477 <- at <ScriptBlock>, D:\a\_temp\7c77ea81-b981-4f9a-8c5d-e7c60c029aba.ps1: line 3 <- at <ScriptBlock>, D:\a\_actions\microsoft\AL-Go-Actions\v8.1\Invoke-AlGoAction.ps1: line 21 <- at <ScriptBlock>, D:\a\_temp\7c77ea81-b981-4f9a-8c5d-e7c60c029aba.ps1: line 2 <- at <ScriptBlock>, <No file>: line 1

But I don't really understand why, because if you look at the permissionset “SharePoint API - Objects,” “SharePoint Graph Client” is there.

namespace System.Integration.Sharepoint;

permissionset 9100 "SharePoint API - Objects"
{
    Access = Internal;
    Assignable = false;

    Permissions = codeunit "SharePoint Client" = X, codeunit "SharePoint Graph Client" = X;
}

What am I missing?

Sorry about the late reply. My notification settings for this repo are off :-(

That's indeed a little strange! You did add your public codeunit to the objects PS, which should do the trick! Let me investigate. It will probably be in the new year, but we've got plenty of time before we branch for the 2026 wave 1 release 😊

@JesperSchulz
Copy link
Contributor

@Drakonian, I think the new compilation issues make a lot more sense ;-) Let's get on those!

@Drakonian
Copy link
Contributor Author

@JesperSchulz
Yeah, this error is clear :)

I added missed dependency of Rest Client.

Copy link
Contributor

@JesperSchulz JesperSchulz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This generally looks very good to me! There are still some compilation issues, but once those are resolved we're entering the hot phase of this PR 😉

@Aleyenda Aleyenda assigned Drakonian and unassigned JesperSchulz Jan 7, 2026
@Drakonian
Copy link
Contributor Author

@JesperSchulz

Great, could you trigger pipeline again?)

@JesperSchulz
Copy link
Contributor

You need to sort some using directives. They now must be alphabetically ordered.

@Drakonian
Copy link
Contributor Author

@JesperSchulz

Should be ok now)

@Drakonian
Copy link
Contributor Author

@JesperSchulz

Progress is being made 😄
I added the missing dependencies to the SharePoint Test application.

@Drakonian
Copy link
Contributor Author

@JesperSchulz

Same for SharePoint test library application 🦖

Make sharepoint test library internals visible to sharepoint test application
@Drakonian
Copy link
Contributor Author

@JesperSchulz

The next step, I would gladly check locally, but as it is ⌛

@Drakonian
Copy link
Contributor Author

@JesperSchulz

Additional missing internals visible to Test App.

@Drakonian
Copy link
Contributor Author

@JesperSchulz

Ok, I don't if it's good, but this error is totally unclear to me

On System Application level compilation it's complain about access level of Codeunit from Test Library, but Internals should be visible to Test App
image

@pri-kise
Copy link
Contributor

@JesperSchulz

Ok, I don't if it's good, but this error is totally unclear to me

On System Application level compilation it's complain about access level of Codeunit from Test Library, but Internals should be visible to Test App image

The problem is that the System Application Test Library don't provide access to the System Application Tests app via InternalsVisibleTo
https://github.com/microsoft/BCApps/blob/main/src/System%20Application/Test%20Library/app.json

Eventually we might need to remove the Access=Internal from those codeunits or those codeunits need to be moved to the SharePoint Test App.

@Drakonian
Copy link
Contributor Author

@pri-kise

Thank you, that clarifies a lot. In that case, the best solution would be to remove Access = Internal for these objects in the Test Library App.

@JesperSchulz Let's try again

…int API

this makes it impossible to test the CopyItemByPath method
this method has been tested locally
@Drakonian
Copy link
Contributor Author

@JesperSchulz

Compilation successful!

There are some test failures, the one that mine I fixed. Other failures are completely unrelated to this PR.

The CopyItemByPath uses two consecutive calls to the SharePoint API, which makes it impossible to run test TestCopyItemByPath. This method has been tested locally and added to DisabledTests.json

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AL: System Application From Fork Pull request is coming from a fork Integration GitHub request for Integration area Linked Issue is linked to a Azure Boards work item

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BC Idea]: SharePoint - add graph API support

6 participants