-
Notifications
You must be signed in to change notification settings - Fork 304
SharePoint Graph API support #3655
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?
Conversation
…ific requests Bound SharePointDiagnostics with response information Add ability to upload large files in 4mb chunks
|
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 -) |
src/System Application/App/SharePoint/src/graph/SharePointGraphClient.Codeunit.al
Show resolved
Hide resolved
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.
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
|
Thank you for such a helpful review of the PR. Your remarks are very useful.
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. |
|
@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.
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 👏 |
|
@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.
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 |
|
@tinestaric 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. |
src/System Application/App/SharePoint/src/graph/SharePointGraphClient.Codeunit.al
Outdated
Show resolved
Hide resolved
Renumber to new ID range and add that range to test app
|
I have good news, I added automated tests and manually tested the current implementation including new methods from @tinestaric Therefore, I would appreciate any code review |
src/System Application/Test Library/SharePoint/src/graph/SharePointGraphTestLibrary.Codeunit.al
Outdated
Show resolved
Hide resolved
| /// </summary> | ||
| table 9132 "SharePoint Graph Drive Item" | ||
| { | ||
| Access = Public; |
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.
Do we those new tables to extensible (at the first release)?
What I mean is, if it weren't better to make the new tables extensible false at first
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.
To be honest, I am not even sure. Are there any best practices? Why prevent others from extending the temporary tables?
|
@Drakonian |
|
@Drakonian The one notable thing I did spot (but it is not directly related to your changes) is that using the Graph API is considerably slower than the REST API when listing files/folders. The issue is that the REST API retrieves all at once, while the Graph API paginates requests. Pagination is fine, but the default page size set on GetAllPages in Graph Client Impl. is 100, and there's no way to change it because GraphPaginationData isn't passed as a parameter. In my tests, for a folder of 850 files, I'm noticing about 3-4 seconds slower performance. It shouldn't be worth changing in the scope of this PR, but for anyone coming across this PR, it might be worth noting. |
@tinestaric aren't you able to set the page size via Then the page size of the optional parameters is used here: BCApps/src/System Application/App/MicrosoftGraph/src/GraphClientImpl.Codeunit.al Line 107 in 5c5e218
|
That's what I initially thought, and I wanted to set the "top" query parameter for Graph Optional Parameters. So, regardless of what I set in the optional parameters, it's always overwritten by the default page size of 100. |
|
Thank you for your discovery and testing. Overall, I agree that improving pagination most likely relates to the context of the Graph API module. Therefore, I suggest moving forward with this module. I think we are close to completing this PR. |
Let's push forward 🚀 I'm super busy these days, but I'll make room for this one 😊 |
|
@Drakonian, looks like we've got some missing permissions and some issues with accessibility. Let's get all lights 🟢 and then we'll continue! |
|
I've added the missing permissions. |
|
I see build failed with new error: 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 😊 |
src/System Application/App/SharePoint/permissions/SharePointApiObjects.PermissionSet.al
Outdated
Show resolved
Hide resolved
|
@Drakonian, I think the new compilation issues make a lot more sense ;-) Let's get on those! |
|
@JesperSchulz I added missed dependency of Rest Client. |
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:
Examples of how to use:
Work Item(s)
Fixes #3198
Fixes AB#568746