Skip to content
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

Add Azure File Services API module #341

Conversation

Krajzys
Copy link
Contributor

@Krajzys Krajzys commented Nov 15, 2023

Summary

This is a port of the pull request from the old repository, most of the convsersation and reviews are also there (microsoft/ALAppExtensions#23710).
The goal of the PR is to add a module that facilitates working with Azure File Share service (which is a part of the Azure Cloud).
The module is heavily based on the Azure Blob Storage Service API module, and most of the architecture is similair.

There are also tests added for the module, but they should be ran manually, as they require passing credentials.

Fixes #340

Work item: AB#477758

@pri-kise
Copy link
Contributor

pri-kise commented Nov 15, 2023

We need now Namespaces (and optional using statements)

I'd suggest something like namespace System.Azure.Storage.Files.Shares;

Additionnaly we should maybe discuss the three character Affix AFS, that is used.

This could be potentially a reserved affix.

Maybe we can remove this affix and rely solely on namespaces??

@Krajzys
Copy link
Contributor Author

Krajzys commented Nov 16, 2023

Thanks for the tip. How about just System.Azure.Storage.Files? Adding shares might be a bit redundant (the only file service on the Azure storage is File Share).

Regarding second point, yes I think we can remove the AFS from most files, but should I then replace it with more descriprive name or just remove the prefix? e.g. AFS Handle -> File Handle.

@JesperSchulz JesperSchulz changed the title Add Azure FIle Services API module Add Azure File Services API module Nov 16, 2023
@github-actions github-actions bot added this to the Version 24.0 milestone Nov 16, 2023
@JesperSchulz
Copy link
Contributor

Uh, naming and namespace discussions. I'm curious what @AndreasMoth would suggest 😁

@DevHarmonize
Copy link

@JesperSchulz

Hi Jesper,
we spoke during Directions Lyon about this PR, Is it still possible to have this in the product in one of the next minors?

kr

@JesperSchulz
Copy link
Contributor

JesperSchulz commented Nov 29, 2023

I would say the January release is an option. We tried to get it in the December release, but did not make it in time. As this module is communicating with an external service, it needs a security review. I am hence working on creating a threat model for this module. When done, I'll get the module and model reviewed and signed off, and then we're good to release.
I'll do my best to get this approved in the next days!

@JesperSchulz
Copy link
Contributor

Let' get this into a state where it compiles again. Running CI one more time after latest changes. We should be getting close!

@JesperSchulz
Copy link
Contributor

JesperSchulz commented Dec 5, 2023

Security Review scheduled for Thursday, 14th of December.

@JesperSchulz
Copy link
Contributor

Security review conducted with @darjoo. Darrick will comment on outcome.

@darjoo
Copy link
Contributor

darjoo commented Dec 15, 2023

After performing the security review with Jesper, the following is the outcome that needs to be fixed.

  • Telemetry for all usages

Fx, telemetry when calling CreateFile

  • OnCall, OnSuccess, OnError

@JesperSchulz
Copy link
Contributor

@Krajzys, if we get the missing telemetry added next week, we can release this module with 23.3 in January!

@Krajzys
Copy link
Contributor Author

Krajzys commented Dec 18, 2023

@JesperSchulz great! I will get on to it. I should add it through the LogMessage procedure right? I have a questions then. How should I detemine the EventIDs?

@Krajzys
Copy link
Contributor Author

Krajzys commented Dec 18, 2023

I have added the telemetry messages, for now with temporary eventIDs (0000AFS) that need to be changed. Please let me know if the telemetry is enough and how to proceed with the eventIDs 😃.

@JesperSchulz
Copy link
Contributor

@JesperSchulz great! I will get on to it. I should add it through the LogMessage procedure right? I have a questions then. How should I detemine the EventIDs?

We've got a script which determines the IDs. I'll assign the fitting IDs to your PR! However, we're currently discussing that file names and folder names are customer content and hence cannot get emitted to telemetry. But without that information, what good does telemetry then do? We'll post the outcome of that discussion to this PR. If worst comes to worst, we temporarily revert your last commit and submit the module to the repo in its former state. Then we can create a chaser PR, if we come up with meaningful telemetry!

The 2 things pending for sure though are the disabled tests and the Test Libraries in the Test project. Once those have been fixed, I'm tempted to push the "merge button" 😎

@Krajzys
Copy link
Contributor Author

Krajzys commented Dec 21, 2023

@darjoo @JesperSchulz Okay, I updated the telemetry messages and renamed the library codeunits to helper.
Regardind the disabled tests. What @pri-kise wrote was correct. We cannot mock requests with current architecture so the module would need to use RestClient. Otherwise we would need to have a regular azure file share setup only for testing purposes and somehow get the credentials for authorizing requests without hardcoding them.

@JesperSchulz
Copy link
Contributor

JesperSchulz commented Dec 21, 2023

@Krajzys
Alright: so here's the plan. Address the remaining comments made by Gert and we'll merge. All HttpClient related code seems to be nicely wrapped in the internal codeunit 8954 "AFS Web Request Helper", which we afterwards can refactor to use the AL Rest Client - which again enables us to better test this.
Does that sound reasonable?

@Krajzys
Copy link
Contributor Author

Krajzys commented Dec 22, 2023

@JesperSchulz Yes, that sounds great. I fixed things pointed out by @grobyns.

@JesperSchulz
Copy link
Contributor

Awesome! You're REALLY responsive - wow! Let's do this 🥳
Starting CI!

@JesperSchulz JesperSchulz enabled auto-merge (squash) December 22, 2023 08:18
@JesperSchulz
Copy link
Contributor

CI passed! I got a few more comments internally, which I'll address - nothing major. I expect to get this merged before 2024 and in time for the 2023 Update 3 in January.

@JesperSchulz
Copy link
Contributor

@Krajzys, I have addressed some minor code review comments in my internal 23.3 pull request, which I'd like to push to this branch before merging. Could you give me write access to the branch? When I've pushed my changes, I'd love for you to take one final look, and then we merge. But I've made sure this releases in January 🥳

@JesperSchulz
Copy link
Contributor

I'll just merge and submit a chaser pack :-)

@JesperSchulz JesperSchulz merged commit 19e9067 into microsoft:main Dec 28, 2023
18 checks passed
JesperSchulz added a commit that referenced this pull request Dec 28, 2023
<!-- Thank you for submitting a Pull Request. If you're new to
contributing to BCApps please read our pull request guideline below
* https://github.com/microsoft/BCApps/Contributing.md
-->
#### Summary <!-- Provide a general summary of your changes -->
Addresses a few code review comments to the initial pull request #341 

#### Work Item(s) <!-- Add the issue number here after the #. The issue
needs to be open and approved. Submitting PRs with no linked issues or
unapproved issues is highly discouraged. -->
Fixes
[AB#477758](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/477758)
@JesperSchulz
Copy link
Contributor

@Krajzys, here a small post-merge request 😋
Any chance you could provide some very simple code samples on how to use this module for the release notes? I could create them myself and get inspired by the tests you've written, but maybe you have some great code already which would show just how simple it is to work with Azure Files?

@Krajzys
Copy link
Contributor Author

Krajzys commented Jan 5, 2024

@JesperSchulz yes, no problem, I could provide some examples. I will try to send them here before the end of today.

@Krajzys
Copy link
Contributor Author

Krajzys commented Jan 8, 2024

Sorry for the delay, I prepared three simple sample pieces of code.

procedure GetFileAsStream(StorageAccount: Text; FileShare: Text; Authorization: Interface "Storage Service Authorization"; FilePath: Text)
var
    AFSFileClient: Codeunit "AFS File Client";
    AFSOperationResponse: Codeunit "AFS Operation Response";
    FileContents: InStream;
begin
    AFSFileClient.Initialize(StorageAccount, FileShare, Authorization);
    AFSOperationResponse := AFSFileClient.GetFileAsStream(FilePath, FileContents);
    if not AFSOperationResponse.IsSuccessful() then
        Error(AFSOperationResponse.GetError());

    DownloadFromStream(FileContents, '', '', '', FilePath);
end;

procedure SendStreamFile(StorageAccount: Text; FileShare: Text; Authorization: Interface "Storage Service Authorization"; FilePath: Text; var FileContent: InStream)
var
    AFSFileClient: Codeunit "AFS File Client";
    AFSOperationResponse: Codeunit "AFS Operation Response";
begin
    AFSFileClient.Initialize(StorageAccount, FileShare, Authorization);
    AFSOperationResponse := AFSFileClient.CreateFile(FilePath, FileContent);
    if not AFSOperationResponse.IsSuccessful() then
        Error(AFSOperationResponse.GetError());

    AFSOperationResponse := AFSFileClient.PutFileStream(FilePath, FileContent);
    if not AFSOperationResponse.IsSuccessful() then
        Error(AFSOperationResponse.GetError());
end;

procedure GetFileNamesInDirectory(StorageAccount: Text; FileShare: Text; Authorization: Interface "Storage Service Authorization"; DirectoryPath: Text[2048]): List of [Text[2048]];
var
    AFSDirectoryContent: Record "AFS Directory Content";
    AFSFileClient: Codeunit "AFS File Client";
    AFSOperationResponse: Codeunit "AFS Operation Response";
    FileNames: List of [Text[2048]];
begin
    AFSFileClient.Initialize(StorageAccount, FileShare, Authorization);
    AFSOperationResponse := AFSFileClient.ListDirectory(DirectoryPath, AFSDirectoryContent);
    if not AFSOperationResponse.IsSuccessful() then
        Error(AFSOperationResponse.GetError());

    AFSDirectoryContent.SetRange("Resource Type", AFSDirectoryContent."Resource Type"::File);
    if not AFSDirectoryContent.FindSet() then
        exit;
    repeat
        FileNames.Add(AFSDirectoryContent."Full Name");
    until AFSDirectoryContent.Next() = 0;

    exit(FileNames);
end;

Here is also how I call them:

action(SendFile)
{
    ApplicationArea = All;
    Caption = 'Send File';
    Image = SendAsPDF;
    ToolTip = 'Sends a file to the FileShare';

    trigger OnAction()
    var
        StorageServiceAuthorization: Codeunit "Storage Service Authorization";
        FileStream: InStream;
        FileName: Text;
    begin
        if not UploadIntoStream('', '', '', FileName, FileStream) then
            exit;
        SampleAFSOperations.SendStreamFile(StorageAccount, FileShare, StorageServiceAuthorization.UseReadySAS(SASToken), Path, FileStream);
        Message('File %1 sent succesfully.', FileName);
    end;
}
action(GetFileAsStream)
{
    ApplicationArea = All;
    Caption = 'Get File as Stream';
    ToolTip = 'Gets file as a stream.';
    Image = Download;

    trigger OnAction()
    var
        StorageServiceAuthorization: Codeunit "Storage Service Authorization";
    begin
        SampleAFSOperations.GetFileAsStream(StorageAccount, FileShare, StorageServiceAuthorization.UseReadySAS(SASToken), Path);
    end;
}

@JesperSchulz
Copy link
Contributor

Thanks a lot! Those examples should come in handy!

@PJohnstonSysco
Copy link

@Krajzys @JesperSchulz Hi, I'm messing about with this in the most recent release and it works great with SAS Token but when I try to use a Shared Key for authentication I run into issues uploading a file to the File Share. I have attached the error. Any ideas? Authentication using shared key works fine for downloading and listing directory requests.

image

@DevHarmonize
Copy link

I also receive this error when using shared key for upload operation.
But otherwise it works perfectly.

Thanks for your effort in making this @Krajzys !

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BC Idea]: Azure File Share API Module
7 participants