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

Url parameter names with characters that cannot be used in language identifiers do not work. #1445

Closed
Tracked by #25 ...
darrelmiller opened this issue Mar 23, 2022 · 7 comments · Fixed by #1498
Closed
Tracked by #25 ...
Assignees
Labels
fixed generator Issues or improvements relater to generation capabilities. type:bug A broken experience

Comments

@darrelmiller
Copy link
Member

e.g. query parameters like $expand or start-date cannot be used

We need the ability to map the property names on the request parameters type to the names that should be used in the URL.

This will allow us to get rid of the middleware that fixes the issue with $ that is required by GraphV1.

@baywet baywet added enhancement New feature or request generator Issues or improvements relater to generation capabilities. labels Mar 24, 2022
@baywet baywet added this to Kiota Mar 24, 2022
@baywet baywet moved this to Todo in Kiota Mar 24, 2022
@baywet
Copy link
Member

baywet commented Apr 4, 2022

I've spent some time looking at this. These parameters are not going to work in their current shape as they are not valid RFC 6570 parameter names.

Assuming GetUrlTemplate has percent encoded the parameter name in the URL template and SetPathAndQueryParameters strips invalid characters for query parameters.

We could add a virtual GetParameterName method in the QueryParameterBase class of the abstractions which accepts a string (parameter name as class property) and returns a string (parameter name as expected by the API).
The default implementation there would be just returning the passed value, if during generation kiota detects a parameter has been "cleaned up" (based on a difference between the property name and the parameter name), it adds an override method with the parameter mappings that have been cleaned up (basically a switch case).

@andrueastman
Copy link
Member

Hey @baywet.

What are your thoughts about potentially using custom annotations like the serialization libraries similar to what System.Text.Json/Json.NET use?

Potentially at generation the property could have a custom annotation added if it's different and the QueryParameterBase would essentially check if the annotation exists to put the right key string in the final URL.

I think this should work for java and dotnet with a custom annotation looking something like this.

[QueryName("$expand")]
public string Expand { get; set; }

Go can take advantage of its property tags feature like this. Ref here

Expand string `$expand`

Typescript can take advantage of property decorators like this. Ref here

@queryName("$expand")
Expand : string;

@baywet
Copy link
Member

baywet commented Apr 5, 2022

this is a great suggestion! I wasn't sure TypeScript would support it. I think we could take an approach where languages that do support annotations use them and languages that don't fallback on a "translation method". What do you think?

@andrueastman
Copy link
Member

I think we could take an approach where languages that do support annotations use them and languages that don't fallback on a "translation method". What do you think?

Yeah, I agree. That makes sense to me.
In my head using annotations will likely result in less code to generate than generating methods. So, we can take advantage if/when we can.

@baywet
Copy link
Member

baywet commented Apr 5, 2022

After some more search on the encoding aspect.
The following template http://example.org/{?$select,api-version}, should result in the following generated template http://example.org/{?%24select,api%2Dversion}

Parameters being passed at runtime will result in the following:

  • %24select : displayName
  • api%2Dversion: 2

http://example.org/?%24select=displayName&api%2Dversion=2

Because the API provider might not be URI decoding and might not understand those param names, we need to decode them at runtime only for specific characters (~, ., -) (unreserved set), we'll need a kiota middleware to decode only those before running the requests.

Microsoft Graph core will have a superset middleware to decode those + $ (odata)

Which will result in the following URL actually being sent over the wire, while being compliant with RFC 6570 and 3986.
http://example.org/?$select=displayName&api-version=2

Note: for the moment the search will run across the entire URL for perf/simplicity. This might have side effect in the following case http://example.org/?%24search=fileName:%2Dfoo (searching for a file with the name that contains -foo) which will result in http://example.org/?$search=fileName:-foo (searching for a file name that does not contain foo).

@baywet
Copy link
Member

baywet commented Apr 6, 2022

it appears that system uri already decodes ~ - . but not $. go Url decodes everything, Java URL decodes nothing. So we'll only have a middleware in kiota core, nothing in graph core with what works for OData services. This way data verse people don't need a dependency on graph core. Anybody not happy with the default can disable the middleware, remove it, or change the set of characters to decode.

@baywet
Copy link
Member

baywet commented Apr 11, 2022

FYI @nikithauc I've had to use a method in the case of typescript since decorators are still experimental. Which means I've had to use a class a the parameter for the executor method. But I don't believe this will be an issue with your work on #1013 and the work I'm planning with #1494

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed generator Issues or improvements relater to generation capabilities. type:bug A broken experience
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants