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.

Sign up for GitHub

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

Jump to bottom

Add procedures to Get/Remove for UriBuilder Query Paramaters/Flags #2247

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Add procedures to Get/Remove for UriBuilder Query Paramaters/Flags #2247

wants to merge 8 commits into from

Conversation

bjarkihall
Copy link

@bjarkihall bjarkihall commented Oct 22, 2024 •
edited by github-actions bot
Loading

Summary

UriBuilder procedures can only Add/Replace Query Flags/Parameters, while the internal procedure can parse them and set them, so I want to be able to use that to only Get/Remove from the collection.

There are different ways to implement this and many ways to test this, I just wanted to propose this approach which reuses what's already there for AddQueryFlag/Parameter, figuring we might need something like this for the upcoming HttpClient mocking: https://www.yammer.com/dynamicsnavdev/#/threads/show?threadId=3029127257333760 (why expose a special QueryParameter dictionary if we already have Uri/UriBuilder codeunits that are supposed to expose these structures for us?)

The new procedures added are:

procedure RemoveQueryFlag(Flag: Text; DuplicateAction: Enum "Uri Query Duplicate Behaviour")
procedure RemoveQueryFlag(Flag: Text)
procedure RemoveQueryParameter(ParameterKey: Text; ParameterValue: Text; DuplicateAction: Enum "Uri Query Duplicate Behaviour")
procedure RemoveQueryParameter(ParameterKey: Text; ParameterValue: Text)
procedure RemoveQueryParameters()
procedure GetQueryFlags(): List of [Text]
procedure GetQueryParameters(): Dictionary of [Text, List of [Text]]
procedure GetQueryParameter(ParameterKey: Text): List of [Text]

Work Item(s)

Fixes #2248
Fixes AB#555623

@bjarkihall bjarkihall requested a review from a team as a code owner October 22, 2024 14:10
@github-actions github-actions bot added AL: System Application From Fork Pull request is coming from a fork labels Oct 22, 2024
@bjarkihall bjarkihall mentioned this pull request Oct 22, 2024
Open
1 task
@bjarkihall
Copy link
Author

Issue #2248 is not valid. Please make sure you link an issue that exists, is open and is approved.

I guess we have an edge case for the contribution workflow, since I had an issue/approval for the idea in another repo. I hope this is okay?

@JesperSchulz JesperSchulz added the Linked Issue is linked to a Azure Boards work item label Oct 23, 2024
@JesperSchulz JesperSchulz self-assigned this Oct 23, 2024
@JesperSchulz
Copy link
Contributor

JesperSchulz commented Oct 23, 2024 •
edited
Loading

Let's take this for spin 🥳

@github-actions github-actions bot added this to the Version 26.0 milestone Oct 23, 2024
@bjarkihall
Copy link
Author

Thanks @JesperSchulz, I was thinking about feedback on the signatures and if the DuplicateAction enum should be accepted and if ShouldRemove parameter should simply be added to AddQueryParameterInternal and AddQueryFlag, instead of roughly cloning these 2 methods into 2 new procedures which would only focus on the removal?
From there, the summary-comments and tests can also be made more detailed, I just wanted to start with a quick draft and tried to match the code which was already there.

@JesperSchulz JesperSchulz added the Integration GitHub request for Integration area label Oct 24, 2024
JesperSchulz
JesperSchulz previously approved these changes Oct 25, 2024
View reviewed changes
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.

Looks good to me!

@JesperSchulz
Copy link
Contributor

There seems to be a small issue in the tests during compilation. Once addressed, I think we're good to go 🥳

@bjarkihall
Copy link
Author

@JesperSchulz sorry about that, I was trying to use the opportunity to get AL help from Github Copilot to write the extra test scenario for me since it was a simple case, since I was told it was really smart about that kind of scenario, but turns out it wasn't.
I've fixed the compilation error. I'm also a bit surprised I didn't get feedback in vscode either, but I hope everything's okay now.

@JesperSchulz
Copy link
Contributor

Let's give this another run! 🚀

@bjarkihall
Unused Uri variable in 4 tests
714ffb5
@bjarkihall
Fix(AT): Uri.GetAbsoluteUri implicitly adds forward slash if missing.…
53acced 
….. added slash to all UriBuilder.Init, so Init is closer to the value of Expected (the slash is irrelevant, since the focus is on the query-parameters)
@bjarkihall
Copy link
Author

@JesperSchulz I noticed the tests were failing, since Uri.GetAbsoluteUri implicitly adds a forward-slash before the query-parameters, and this was taken care of in some of the existing tests before this PR:
image

But judging by how hard it is to catch this little hardcoded detail (for us humans and apparently AI as well), I changed them and the new tests so the "Init-URI" is closer to the final "Expected-URI", so they both have the forward-slash in all scenarios.
It added a little bit of diff from lines I didn't intend to change originally.

Hopefully it's finally ready for the pipelines now, I didn't want to spend too much time on the autotests initially, in case the PR (or parts of it) would be rejected on first review.

@JesperSchulz
Copy link
Contributor

@JesperSchulz I noticed the tests were failing, since Uri.GetAbsoluteUri implicitly adds a forward-slash before the query-parameters, and this was taken care of in some of the existing tests before this PR: image

But judging by how hard it is to catch this little hardcoded detail (for us humans and apparently AI as well), I changed them and the new tests so the "Init-URI" is closer to the final "Expected-URI", so they both have the forward-slash in all scenarios. It added a little bit of diff from lines I didn't intend to change originally.

Hopefully it's finally ready for the pipelines now, I didn't want to spend too much time on the autotests initially, in case the PR (or parts of it) would be rejected on first review.

That's a fine strategy 😊 I don't mind at all taking a few turns to get things right. Let's try again and hope this time it passes 😎

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

@JesperSchulz JesperSchulz JesperSchulz left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees

@JesperSchulz JesperSchulz

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
Milestone
Version 26.0
Development

Successfully merging this pull request may close these issues.

[Bug]: It's not possible to remove/get query parameters in UriBuilder
2 participants
@bjarkihall @JesperSchulz