Add Azure File Services API module #23710

Krajzys · 2023-06-06T08:40:51Z

Okay, this is a bit of a big PR, but I hope you can help me out.
We want to add a library to handle Azure File Share service in mostly the same way as Azure Blob Service is currently implemented.
We based this module heavily on the Azure Blob Services API module and it follows the same patterns.

I'm not sure how to go about the automatic testing process, because as far as I know azurite doesn't support Azure File Shares, so any tips on that would be helpful.

darjoo

Very nice module that complements the Azure Blob Storage! Great work and looking forward to when we can merge this and ship!
Some comments here and there, but my biggest thought (which is also a comment somewhere) is how much can we combine between this and ABS? It definitely looked like some overlap which we may be able to extract without having to duplicate them and when we get Queue/Table services fx, they can hook into that.

Inregards to testing, I'm a little unsure on that point as well. At least when it comes to unit-testing.
We could come to terms and create tests which will only be run manually. Where if we make any changes, we'll run them against an actual storage account. Not the most convenient but it'll at least allow us to test everything simply by running the tests.

Modules/System/Azure File Services API/src/AFSClientImpl.Codeunit.al

darjoo · 2023-06-06T14:26:13Z

Modules/System/Azure File Services API/src/AFSClientImpl.Codeunit.al

+        CreateFileOperationNotSuccessfulErr: Label 'Could not create file %1 in %2.', Comment = '%1 = File Name; %2 = File Share Name';
+        PutFileOperationNotSuccessfulErr: Label 'Could not put file %1 ranges in %2.', Comment = '%1 = File Name; %2 = File Share Name';
+        CreateDirectoryOperationNotSuccessfulErr: Label 'Could not create directory %1 in %2.', Comment = '%1 = Directory Name; %2 = File Share Name';
+        GetFileOperationNotSuccessfulErr: Label 'Could not get File %1.', Comment = '%1 = File';


%1 = File Path

darjoo · 2023-06-06T14:28:08Z

Modules/System/Azure File Services API/src/AFSClientImpl.Codeunit.al

+        PutFileOperationNotSuccessfulErr: Label 'Could not put file %1 ranges in %2.', Comment = '%1 = File Name; %2 = File Share Name';
+        CreateDirectoryOperationNotSuccessfulErr: Label 'Could not create directory %1 in %2.', Comment = '%1 = Directory Name; %2 = File Share Name';
+        GetFileOperationNotSuccessfulErr: Label 'Could not get File %1.', Comment = '%1 = File';
+        CopyFileOperationNotSuccessfulErr: Label 'Could not copy File %1.', Comment = '%1 = File';


%1 = File Path

darjoo · 2023-06-06T14:28:43Z

Modules/System/Azure File Services API/src/AFSClientImpl.Codeunit.al

+        CopyFileOperationNotSuccessfulErr: Label 'Could not copy File %1.', Comment = '%1 = File';
+        DeleteFileOperationNotSuccessfulErr: Label 'Could not %3 File %1 in file share %2.', Comment = '%1 = File Name; %2 = File Share Name, %3 = Delete/Undelete';
+        DeleteDirectoryOperationNotSuccessfulErr: Label 'Could not delete directory %1 in file share %2.', Comment = '%1 = File Name; %2 = File Share Name';
+        AbortCopyFileOperationNotSuccessfulErr: Label 'Could not abort copy of File %1.', Comment = '%1 = File';


%1 = File Path

darjoo · 2023-06-06T14:30:29Z

Modules/System/Azure File Services API/src/AFSClientImpl.Codeunit.al

+        AFSOperationPayload.Initialize(StorageAccountName, FileShare, Path, Authorization, ApiVersion);
+    end;
+
+    internal procedure SetBaseUrl(BaseUrl: Text)


All procedures in the impl are internal. internal keyword not required here, same for the rest in this codeunit

Modules/System/Azure File Services API/src/Helper/AFSWebRequestHelper.Codeunit.al

Modules/System/Azure File Services API/src/Helper/AFSURIHelper.Codeunit.al

Modules/System/Azure File Services API/src/Helper/AFSHttpHeaderHelper.Codeunit.al

Modules/System/Azure File Services API/src/Helper/AFSHttpContentHelper.Codeunit.al

Modules/System/Azure File Services API/src/Helper/AFSFormatHelper.Codeunit.al

Add copyright section, some more documentation and some properties.

Krajzys · 2023-06-19T09:38:35Z

Regarding the point with similarities between Azure Blob Service API and Azure File Service API, I compiled a small list of object I think have some chance to be made common.

Codeunits:

AFS/ABS Web Request Helper:
- Could be made common (no changes)
AFS/ABS Optional Parameters:
- Could be made common, not sure if it would reduce complexity
- Some headers are the same in both services
- Some only occur in one of them
AFS/ABS Operation Payload:
- Probably best to leave it separate
- Could be made common if optional parameters codeunit and operations enum were common
- ABS uses container name and blob name, AFS uses file share and path, Query and Table could use something else
AFS/ABS Operation Response:
- Could be made common (no major changes)
- Both are a universal wrapper for HttpResponseMessage
AFS/ABS HttpContent Helper:
- Could be made common but would require a different implementation of procedures
- Currently only two universal procedures
  - ContentSet
  - GetContentLength
AFS/ABS HttpHeader Helper:
- Could be made common (some changes)
- AFS uses a superset of ABS (some procedures to handle response headers)
AFS/ABS Format Helper:
- Could be made common
- AFS uses a subset of ABS (with an exception of one procedure)

Enums:

AFS/ABS Lease Action:
- Could be made common
- They are basically the same

Summary:

We could probably create a module that would have the following:

Universal Web Request Helper - Codeunit
Universal Operation Response - Codeunit
Universal HttpHeader Helper - Codeunit
Universal Format Helper - Codeunit
Universal Lease Action - Enum

I think it's not enough to justify it for now, what's your take on it?

JesperSchulz · 2023-07-07T09:56:52Z

Time to get this rolling! We'll try to get this ready for release with 2023 Wave 2!

darjoo · 2023-07-25T13:12:04Z

@Krajzys Sorry for such a delay in response, summer and all.

Agreed, and it would require more thinking of how we would maybe even combine HttpContent (Through interfaces?).. For a later discussion.

I'll start processing the PR internally, but do you perhaps have a small app or something that you tested the module with?
I'll at least add that to our internal E2E testing, just so that we can ensure this works :) Since doing unit tests with these modules don't work well without mocking capabilities. Fx ABS.

…into itintegro/add-azure-file-share-library

Krajzys · 2023-07-25T15:49:20Z

No problem :)
Yes, I wrote some basic tests in the meantime. I will push those changes here.

pri-kise · 2023-08-18T06:49:56Z

Modules/System/Azure File Services API/src/API Enums/AFSFilePermissionCopyMode.Enum.al

+/// </summary>
+enum 8956 "AFS File Permission Copy Mode"
+{
+    Extensible = false;


Access Should be delcared explicit.

pri-kise · 2023-08-18T06:50:24Z

Modules/System/Azure File Services API/src/API Enums/AFSFileAttribute.Enum.al

+/// </summary>
+enum 8955 "AFS File Attribute"
+{
+    Extensible = false;


Access should be delcared explicit, similiar to the other objects.

pri-kise · 2023-08-18T06:50:53Z

Modules/System/Azure File Services API/src/AFSOptionalParameters.Codeunit.al

+/// Holds procedures to format headers and parameters to be used in requests.
+/// </summary>
+codeunit 8956 "AFS Optional Parameters"
+{


Access should be delcared explicit, similiar to the other objects.

pri-kise · 2023-08-18T06:53:40Z

Modules/System/Azure File Services API/src/Helper/AFSWebRequestHelper.Codeunit.al

+        HttpResponseMessage: HttpResponseMessage;
+    begin
+        if not HttpClient.Send(HttpRequestMessage, HttpResponseMessage) then
+            Error(OperationNotSuccessfulErr);


Should we mention this error on the summary of the public methods of the Codeunit AFS Client?

JesperSchulz · 2023-08-18T07:19:21Z

@Krajzys, I hope you don't mind me asking, but in which cases is it better to use Azure File Storage than Azure Blob Storage? I've found a few articles, but they haven't really given me a solid answer... In which scenarios are you planning to use the File Storage? Forgive my ignorance, but storage isn't one of my strong suits and I'm trying to learn a little while processing the many storage related pull requests 😊

pri-kise · 2023-08-18T07:26:03Z

@Krajzys, I hope you don't mind me asking, but in which cases is it better to use Azure File Storage than Azure Blob Storage? I've found a few articles, but they haven't really given me a solid answer... In which scenarios are you planning to use the File Storage? Forgive my ignorance, but storage isn't one of my strong suits and I'm trying to learn a little while processing the many storage related pull requests 😊

@JesperSchulz we use Azure File Share for our customers, since it can be mapped to drives in windows and can be used from there like any other network drive.
https://appsource.microsoft.com/en-us/product/dynamics-365-business-central/PUBID.prisma_informatik_1762332%7CAID.pi-explorerforazurefileshare%7CPAPPID.d9dc02ea-b2c0-460c-b2be-219f1036249b?tab=Overview (See attached screenshots).

JesperSchulz · 2023-08-18T07:36:41Z

OK, that's pretty cool! I should play with it 🥳

Krajzys · 2023-08-18T08:13:44Z

@Krajzys, I hope you don't mind me asking, but in which cases is it better to use Azure File Storage than Azure Blob Storage? I've found a few articles, but they haven't really given me a solid answer... In which scenarios are you planning to use the File Storage? Forgive my ignorance, but storage isn't one of my strong suits and I'm trying to learn a little while processing the many storage related pull requests 😊

@JesperSchulz Just as @pri-kise mentioned 😃. We want to use azure file share drives to map them to windows drives. Some customers have file-based processes and it can be hard to convince them to switch to blob storage. That's when azure file share can shine, because you can make it look like a regular network drive or even map it to folder on a physical drive (the user then can interact with it as with a regular folder). In the end it's just another option to use in our toolkit.

JesperSchulz · 2023-08-18T12:08:29Z

That makes a lot of sense! Very cool. Let's do this!
23.0 is beginning to look a little tight, but 23.1 is definitely within reach!

BW-PA · 2023-08-20T21:35:56Z

@Krajzys, I hope you don't mind me asking, but in which cases is it better to use Azure File Storage than Azure Blob Storage? I've found a few articles, but they haven't really given me a solid answer... In which scenarios are you planning to use the File Storage? Forgive my ignorance, but storage isn't one of my strong suits and I'm trying to learn a little while processing the many storage related pull requests 😊

Just adding my two cents. We use Azure Blob Storage now as a service to service integration between BC and a process that generates files for BC to import. To move the file into Blob Storage we needed to write a utility that monitors a local folder for new files and uploads them to Blob Storage. With Azure File Storage we can remove the need for this Utility app and map the Azure File Storage to a local folder.

GhOnBc · 2023-08-24T07:22:11Z

That makes a lot of sense! Very cool. Let's do this! 23.0 is beginning to look a little tight, but 23.1 is definitely within reach!

great news, really looking forward to see this available!
We also have a lot of scenarios where we would really benefit of being able to use azure file share instead of azure blob storage.

GhOnBc · 2023-10-02T15:43:33Z

That makes a lot of sense! Very cool. Let's do this! 23.0 is beginning to look a little tight, but 23.1 is definitely within reach!

Hi Jesper,
Is 23.1 still a possibility to have this PR included?

JesperSchulz · 2023-10-03T07:41:06Z

Yes, I've started processing the PR internally and have received quite a few code review comments. I'll ask the developers to relay those comments here on GitHub (or I will do it). If we get those comments addressed before mid-October, the module could make the 23.1 release. Alternatively, we're looking at 23.2.

AndreasMoth · 2023-10-03T12:26:29Z

Modules/System/Azure File Services API/src/AFSDirectoryContent.Table.al

+        {
+            DataClassification = SystemMetadata;
+            Caption = 'Entry No.', Locked = true;
+            Access = Internal;


Minor: Any reason this is internal? Would be great with a comment on why. It should still be possible to access and modify this through RecordRef + FieldRef.

Good question, not sure to be honest. As I mentioned this module was based on the module for blob storage, so it might be a leftover from there. I think the access modifier can be safely removed.

AndreasMoth · 2023-10-03T12:30:20Z

Modules/System/Azure File Services API/src/AFSClientImpl.Codeunit.al

+// Licensed under the MIT License. See License.txt in the project root for license information.
+// ------------------------------------------------------------------------------------------------
+
+codeunit 8951 "AFS Client Impl."


Minor: "AFS File Client Impl." seems like a missing rename (aligning the interface and implementation codeunit)

Right, I will fix this. Initially I wanted to split the interface codeunit into one for files and one for directories, but I came to conclusion that one codeunit will be more convenient.

AndreasMoth · 2023-10-03T12:36:46Z

Modules/System/Azure File Services API/src/Helper/AFSDirectoryContentHelper.Codeunit.al

+        EntryNo := GetNextEntryNo(AFSDirectoryContent);
+
+        AFSDirectoryContent.Init();
+        AFSDirectoryContent."Parent Directory" := CopyStr(DirectoryPath, 1, MaxStrLen(AFSDirectoryContent."Parent Directory"));


If we can put a limit on the input parameter we should always do that, I.E. "DirectoryPath: Text[MaxLength]". Otherwise we truncate text that may be unexpected. Someone extending this module and calling this may create a bug because they expect this could be any length.

AndreasMoth · 2023-10-03T12:39:27Z

Modules/System/Azure File Services API/src/Helper/AFSDirectoryContentHelper.Codeunit.al

+                AFSDirectoryContent.Insert(true);
+            end;
+
+            CurrentParent := CopyStr(AFSDirectoryContent."Full Name" + '/', 1, MaxStrLen(AFSDirectoryContent.Name));


if adding the forwardslash causes this to overflow, wouldn't this fail? I.E. maybe we should throw an error in this case?

AndreasMoth · 2023-10-03T12:46:20Z

Modules/System/Azure File Services API/src/Helper/AFSHelperLibrary.Codeunit.al

+        Clear(FieldNo);
+        Field.Reset();
+        Field.SetRange(TableNo, TableNo);
+        Field.SetRange("Field Caption", CopyStr(FieldCaption, 1, MaxStrLen(Field.FieldName)));


You can ignore this for now but in the future it would be good to limit the input FieldCaption variable even if it's obvious that you will truncate it

AndreasMoth · 2023-10-03T13:12:04Z

Modules/System/Azure File Services API/src/Helper/AFSFormatHelper.Codeunit.al

+    InherentPermissions = X;
+
+    [NonDebuggable]
+    procedure AppendToUri(var Uri: Text; ParameterIdentifier: Text; ParameterValue: Text)


This should use UriBuilder. The current implementation does not encode the values, it may also be possible for non-admin users to mess up with the call if they have access to set just one of these parameters.

AndreasMoth · 2023-10-03T13:12:52Z

Modules/System/Azure File Services API/src/Helper/AFSFormatHelper.Codeunit.al

+    procedure ConvertToEnum(FieldName: Text; PropertyValue: Text): Variant
+    begin
+        if FieldName = 'Resource Type' then
+            case PropertyValue of


LowerCase PropertyValue

AndreasMoth · 2023-10-03T13:16:33Z

Modules/System/Azure File Services API/src/Helper/AFSFormatHelper.Codeunit.al

+            end;
+    end;
+
+    procedure GetRfc1123DateTime(MyDateTime: DateTime): Text


You can ignore this for now but in the future it would be good to move to another module along with FormatDateTime (something others may also use).

AndreasMoth · 2023-10-03T13:32:16Z

Modules/System/Azure File Services API/src/Helper/AFSFormatHelper.Codeunit.al

+        NewDateTime := 0DT;
+        // PropertyValue is something like the following: 'Mon, 24 May 2021 12:25:27 GMT'
+        // 'Evaluate' converts these correctly
+        Evaluate(NewDateTime, PropertyValue);


Did you verify this with different locales? We have commonly seen issues when evaluating dates if you are using different date times. The date does look like it should work but just to be sure.

I did some tests with the two formats of dates that the azure file share api can send us back (RFC 1123 and ISO 8601) and changing up regions, time zones etc. in BC. Everything looked fine.

AndreasMoth · 2023-10-03T13:34:03Z

Modules/System/Azure File Services API/src/AFSOperationPayload.Codeunit.al

+        Clear(StorageBaseUrl);
+        Clear(UriParameters);
+        Clear(RequestHeaders);
+        Clear(ContentHeaders);


Looks like AFSOperation is not cleared.

Krajzys · 2023-11-14T12:08:21Z

I'm coming back with two questions 😃

I've seen that the new repo BCApps went live.
Should I move the PR to there, or is it okay if it stays here?

Also could we get a status update on the PR processing?

JesperSchulz · 2023-11-15T09:15:20Z

I'm coming back with two questions 😃

I've seen that the new repo BCApps went live. Should I move the PR to there, or is it okay if it stays here?

Also could we get a status update on the PR processing?

@Krajzys, I was just about to ask you, if you would like to port the PR to the BCApps repository! I could do that too, but I'd prefer that you do it, as you would then be listed as the owner of the initial PR for this module ⭐
When the PRs is in BCApps, it will get processed further until we can merge it. And then the job is finally done 😊
Sorry that this has dragged out a little, but we had to prioritize a few other efforts. But now those are done and we're ready to get this finalized!
Let me know what you prefer.

Krajzys · 2023-11-15T15:50:03Z

@JesperSchulz Thanks for the reply, I created an issue (microsoft/BCApps#340) and a PR (microsoft/BCApps#341) on the new repo .
If there is anything more I need to do please help me out.

JesperSchulz · 2023-11-16T11:41:09Z

That was all which needed to be done. Thanks a lot for your help! Closing this PR.

# 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](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/477758)

Krajzys added 9 commits May 31, 2023 12:09

Add Azure File Share Library files

45869b1

Cleanup AFS objects

cc5305b

Add documentation and renumber objects

3925471

Fix typos

067080d

Update app.json

e3ded72

Add README documentation

d67f79b

Remove commented code

58f606f

Add missing dependencies

c81df99

Fix code cop warnings

2d68123

darjoo reviewed Jun 7, 2023

View reviewed changes

Krajzys added 2 commits June 12, 2023 11:38

Cleanup code

b83b80f

Add copyright section, some more documentation and some properties.

Add get/set file metadata requests

4d301a9

Fix code cop errors

deaede9

Krajzys requested a review from darjoo June 20, 2023 11:55

JesperSchulz added the processing-PR The PR is currently being reviewed label Jul 7, 2023

JesperSchulz assigned darjoo Jul 7, 2023

darjoo previously approved these changes Jul 25, 2023

View reviewed changes

Krajzys added 2 commits July 25, 2023 17:42

Merge branch 'main' of https://github.com/IT-integro/ALAppExtensions …

27b936f

…into itintegro/add-azure-file-share-library

Add Azure File Share library tests

cf93f5a

Krajzys dismissed darjoo’s stale review via cf93f5a July 26, 2023 06:50

pri-kise reviewed Aug 18, 2023

View reviewed changes

Add explicit access modifiers

1843dd2

AndreasMoth reviewed Oct 3, 2023

View reviewed changes

Fix comments after PR review

83673e2

Krajzys requested a review from a team as a code owner October 11, 2023 12:00

JesperSchulz added the system application This PR is related to the system application and must be completed asap (migration in progress) label Nov 7, 2023

This was referenced Nov 15, 2023

[BC Idea]: Azure File Share API Module microsoft/BCApps#340

Closed

Add Azure File Services API module microsoft/BCApps#341

Merged

JesperSchulz closed this Nov 16, 2023

Add Azure File Services API module #23710

Add Azure File Services API module #23710

Conversation

Krajzys commented Jun 6, 2023

darjoo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Krajzys commented Jun 19, 2023

Codeunits:

Enums:

Summary:

JesperSchulz commented Jul 7, 2023

darjoo commented Jul 25, 2023 • edited Loading

Krajzys commented Jul 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JesperSchulz commented Aug 18, 2023

pri-kise commented Aug 18, 2023

JesperSchulz commented Aug 18, 2023

Krajzys commented Aug 18, 2023

JesperSchulz commented Aug 18, 2023

BW-PA commented Aug 20, 2023 • edited Loading

GhOnBc commented Aug 24, 2023

GhOnBc commented Oct 2, 2023

JesperSchulz commented Oct 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Krajzys commented Nov 14, 2023

JesperSchulz commented Nov 15, 2023

Krajzys commented Nov 15, 2023

JesperSchulz commented Nov 16, 2023

darjoo left a comment •

edited

Loading

darjoo commented Jul 25, 2023 •

edited

Loading

BW-PA commented Aug 20, 2023 •

edited

Loading