[swift5] distinguish (un-)authenticated requests#13318
[swift5] distinguish (un-)authenticated requests#13318Jonas1893 wants to merge 2 commits intoOpenAPITools:masterfrom Jonas1893:feat/authenticated-request-builder
Conversation
Jonas1893
changed the title
|
Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/OpenAPITools/openapi-generator/graphs/contributors. Let me know if you need help fixing it. |
There was a problem hiding this comment.
@Jonas1893 Hey, thanks for creating this PR.
This is an issue that some people have, so thanks for taking the time to improve the generator.
Could you please provide an example of an implementation of getDecodableBuilderAuthenticated?
This solution is nice, but a bit verbose, because we need to provide 4 different builder implementations.
Did you consider alternatives?
For example passing a bool requiresAuthentication to this method?
By doing this, we only need the current 2 builders, so it's not a breaking change, and the we can get the endpoint configuration without doing any request.
There was a problem hiding this comment.
Hi @4brunu. I updated the BearerRequestBuilderFactory implementations in the PetStore samples to show the difference. The *authenticated builder version stays the same as before, always injecting a token into the request. For the unauthenticated builder we just use the already provided default AlamofireRequestBuilders that does not inject anything.
I did consider the alternative of distinguishing via parameter, however still this would result in a breaking API change - as long as we don't use a default parameter which I don't think we should, as this would be "guessing" and it might lead to bugs later if the default behavior is forgotten:
internal protocol RequestBuilderFactory {
func getNonDecodableBuilder<T>() -> RequestBuilder<T>.Type
func getBuilder<T: Decodable>() -> RequestBuilder<T>.Type
}
->
internal protocol RequestBuilderFactory {
func getNonDecodableBuilder<T>(isRequestAuthenticated: Bool) -> RequestBuilder<T>.Type
func getBuilder<T: Decodable>(isRequestAuthenticated: Bool) -> RequestBuilder<T>.Type
}
Adoption on app side would result in a if else in each method, e.g.:
func getBuilder<T: Decodable>(isRequestAuthenticated: Bool) -> RequestBuilder<T>.Type {
if isRequestAuthenticated {
return BearerRequestBuilder<T>.self
} else {
return AlamofireRequestBuilder<T>.self
}
}
At this point, I don't think there is any benefit to either approach and we can just chose whatever we feel looks nicer. If you prefer the parameter approach I can also adjust it that way, no problem
There was a problem hiding this comment.
Hi @Jonas1893, sorry if I miscommunicated.
I was not suggesting passing the bool to the getBuilder function.
I was suggesting passing it to the init of the the RequestBuilder.
return localVariableRequestBuilder.init(method: "GET", URLString: (localVariableUrlComponents?.string ?? localVariableURLString), parameters: localVariableParameters, headers: localVariableHeaderParameters, requiresAuthentication: true)
This way it's not a breaking change, and we change even check if a specific endpoint requires authentication or not by calling the getPetByIdWithRequestBuilder(), and we only need the current 2 builders.
3 participants
Summary
This adds the ability to easily distinguish between endpoints that should be requested authenticated and those that should be requested unauthenticated.
Motivation
Currently the intended way of authentication according to the FAQs is to implement a custom
RequestBuilderoverridingexecute(URLSession) or to inject a customRequestInterceptor(AF library). The drawback here is that the information from the swagger spec (via thesecurityscheme annotation), whether an endpoint needs to be requested authenticated or not is not available anymore at this level in the code. There were discussion on how to handle this properly but without definite outcome. Currently, it is necessary to maintain a whitelist of requests that need to be requested (un-)authenticated in the app logic.Changes in this PR
This PR adds two new request builder variations for authenticated requests and calls the appropriate factory method according to the presence of a security annotation in the spec. For endpoint that are intended to be requested authenticated the *authenticated Builder will be used and vice versa. This is a breaking change for people that are already using the request builders, however adoption should be very fast.
Other effects
PR checklist
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*.For Windows users, please run the script in Git BASH.
master(6.1.0) (minor release - breaking changes with fallbacks),7.0.x(breaking changes without fallbacks)Declaration
The program was tested solely for our own use cases, which might differ from yours.
Link to provider information
https://github.com/mercedes-benz