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

- adds support for raw URLs in request builders #508

Merged
merged 23 commits into from
Aug 25, 2021
Merged

Conversation

baywet
Copy link
Member

@baywet baywet commented Aug 19, 2021

fixes #410

Generation diff

microsoft/kiota-samples#225

Todo

  • replicate in dotnet
  • replicate in java
  • replicate in go
  • replicate in ruby
  • update unit tests
  • bump abstractions versions

@baywet baywet added this to the Beta milestone Aug 19, 2021
@baywet baywet self-assigned this Aug 19, 2021
@baywet
Copy link
Member Author

baywet commented Aug 19, 2021

Todo doc comment new method request info

@baywet baywet force-pushed the feature/raw-url branch 4 times, most recently from aa4dfe6 to aa415fa Compare August 24, 2021 13:22
Signed-off-by: GitHub <noreply@github.com>
baywet added 3 commits August 24, 2021 13:48
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
@baywet baywet marked this pull request as ready for review August 24, 2021 14:09
@baywet baywet enabled auto-merge August 24, 2021 14:09
@nikithauc
Copy link
Contributor

Can you please give some examples of isRawUrl will be used? I checked the generated files the meaning of isRawURL is 'not clear.

src/Kiota.Builder/CodeParameterOrderComparer.cs Outdated Show resolved Hide resolved
if len(keyValue) == 2 {
request.QueryParameters[keyValue[0]] = keyValue[1]
} else if len(keyValue) == 1 {
request.QueryParameters[keyValue[0]] = ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there value in adding the query parameter in the event that the value is absent/null? (I think this applies to all the languages)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this explanation was the best 🤣

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤣 🤣 🤣
If we can, we should probably remember to upvote that answer

abstractions/dotnet/src/RequestInfo.cs Outdated Show resolved Hide resolved
abstractions/dotnet/src/RequestInfo.cs Outdated Show resolved Hide resolved
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

80.9% 80.9% Coverage
0.0% 0.0% Duplication

@baywet
Copy link
Member Author

baywet commented Aug 25, 2021

@nikithauc

The generated code for the fluent API will generate new RequestBuilder(currentPath, httpCore, false).
Somebody using the request builder with a raw URL (next link, delta link, ...) will end up writing new RequestBuilder(rawURl, httpCore) where the default value of the parameter is true.

That parameter is assigned to a property on the request builder which is then is passed to the requestInfo.setUri method (generate request method).

This parameter is used in the setUri method to know whether we should:

  1. Append the current path and the segment to use as a URI
  2. Parse the current path (which is the complete URL in this case) to get the query parameters into the dedicated dictionary, and use the rest as the uri.

Basically of of that is done to avoid appending a path segment when not needed and avoid "loosing" any query parameter that might already be present.
I hope this clarifies things?

@baywet baywet requested a review from andrueastman August 25, 2021 11:44
@baywet baywet merged commit 443f1d8 into main Aug 25, 2021
@baywet baywet deleted the feature/raw-url branch August 25, 2021 12:42
@nikithauc
Copy link
Contributor

I got the purpose of doing this.

I asked for examples on how the code is used because: isRawURL and the documentation Whether the current path is a raw URL is confusing. The naming does not state the purpose at the first glance on the code. isRawURL expects the user to know that the implementation details currentPath gets appended.

Can you please provide a code snippet when isRawURL is true and isRawURL is false?

Please provide simple code snippets of the request builders with the new changes in the PR description as it gives more clarity.

@baywet
Copy link
Member Author

baywet commented Aug 25, 2021

const page1 = await client.me.messages.get(); //raw url is false
const page2 = await (new MessagesRequestBuilder(page1.nextLink, httpCore)).get(); //is raw url is true

Would it be better if the description was changed to Whether the current path is a fully qualified URL. When true, any query parameter passed will be kept for the request and the current path of the request builder won't be appended. ?

@nikithauc
Copy link
Contributor

I would think of a different name instead of isRawURL param.

@baywet
Copy link
Member Author

baywet commented Aug 26, 2021

Suggestions are welcome but what do you think of "IsFullyQualifiedUrl"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for using nextLink in the typescript sdk
3 participants