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
AL Rest Client #24803
AL Rest Client #24803
Conversation
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 really awesome, AJ. I did find some security considerations, but I can't wait until this is in BC
Modules/System/AL Rest Client/src/ALHttpResponseMessage.Codeunit.al
Outdated
Show resolved
Hide resolved
Modules/System/AL Rest Client/src/ALHttpResponseMessage.Codeunit.al
Outdated
Show resolved
Hide resolved
Modules/System/AL Rest Client/src/ALHttpRequestMessage.Codeunit.al
Outdated
Show resolved
Hide resolved
Modules/System/AL Rest Client/src/Authentication/HttpAuthenticationBasic.Codeunit.al
Outdated
Show resolved
Hide resolved
Looks like a logo is missing. I'll remove the reference to it (no need for logos in modules). Would love to make the compilation succeed to start with :-) |
Remove reference to logo and update version no.'s.
Remove reference to logo and update version no.s.
Replaced the usage of enum "Http Request Type" from the OAuth module with a new enum "Http Method". I figured it was more appropriate for the module to have its own enum instead of taking a dependency on the OAuth module. |
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.
Overall a very good module. We could benefit a lot from using it.
Modules/System/AL Rest Client/src/ALHttpRequestMessage.Codeunit.al
Outdated
Show resolved
Hide resolved
Modules/System/AL Rest Client/src/ALHttpRequestMessage.Codeunit.al
Outdated
Show resolved
Hide resolved
Modules/System/AL Rest Client/src/Authentication/HttpAuthenticationBasic.Codeunit.al
Outdated
Show resolved
Hide resolved
Modules/System Tests/AL Rest Client/src/ALHttpContentTests.Codeunit.al
Outdated
Show resolved
Hide resolved
Modules/System Tests/AL Rest Client/src/ALRestClientTests.Codeunit.al
Outdated
Show resolved
Hide resolved
Modules/System Tests/AL Rest Client/src/ALRestClientTests.Codeunit.al
Outdated
Show resolved
Hide resolved
Modules/System/AL Rest Client/src/ALHttpRequestMessage.Codeunit.al
Outdated
Show resolved
Hide resolved
Modules/System/AL Rest Client/src/ALHttpRequestMessage.Codeunit.al
Outdated
Show resolved
Hide resolved
Thank you all for the comments so far. Much appreciated and great feedback! |
Just create a fairly large commit to resolve almost all comments. But most importantly, I've introduced an interface "Http Client Handler". A codeunit "Default Client Handler" is also provided, that can also serve as boilerplate code for apps. The main purpose of this handler codeunit is to let the original app send the actual http request. As a result, telemetry signal RT0019 will be sent to the ISV's AppInsight and the feature to block outgoing web requests is applied to the original app. The Http Client Handler must be specified when initializing the Rest Client. In case no handler codeunit is specified, it will take the default one provided in the module. This pattern can also be used for tests to create fake responses or a mocking codeunit to handle the request differently. |
It is not, no. But don't sweat it. As long as it compiles on 2023 Wave 2 / 12.0, it's all good. Hopefully I'll soon be able to give this a spin in our internal build system! |
I've tested with a 23.0 preview build, and all tests succeed. It's great to have the SecretText implemented from the beginning, that's why I needed v23. |
One small issue is missing, which was reported to @KennieNP a while ago: the HttpRequest object in the AL platform doesn't support relative URLs, while the underlying .Net object does. I've worked around it, but I'd be more happy if it would be supported in the platform object. |
@KennieNP also suggested we add some retry-logic to this wrapper, while at it (I could not spot it when rushing through the code today). It would greatly improve the HttpClient call stability in the app if we did. He even provided some sample code to illustrate his intend ;-) `
} This doesn't have to be part of the initial PR, but we'll probably add it eventually, as it's a great suggestion. |
That's a great suggestion! I agree it can be added later. If time permits I may add it to the current PR. |
Feel free to extend the number range 😊 There should be plenty of available numbers... |
over all this PR looks very promising. |
Let's see what our build system has to say to this PR! Processing internally now. |
Short update: Tomorrow we are internally opening up for PRs through the new BCApps repository. I will make this PR the first official AL PR to be processed in the new repository 🥳 From November 1st, the BCApps repository will open to the public too. By then, I hope this PR has successfully been processed though. |
Adding namespaces Fixing CodeCop issues Fixing compilation issues in tests Adding open source headers
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 is in a good enough shape to get submitted. Further improvements can be made in chaser PRs. With this module in, we can start uptake 🥳
We'll have to wait with merging this, until we've rolled out the 23.0 bits on Monday, as this doesn't compile against 22.5 due to namespaces and the use of secure string. |
The PR went into the main branch (24.0) 🥳 As soon as we release the 23 bits, we can merge this PR. Further improvements can then be done to the added module. |
Provides functionality to easily call REST web services
The module contains methods to support
Some examples:
Remarks: