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

[TS][Angular] Avoid strictNullChecks errors for apiKeys #1611

Merged
merged 6 commits into from
Dec 6, 2018

Conversation

grktsh
Copy link
Contributor

@grktsh grktsh commented Dec 4, 2018

fix #1607

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 3.4.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

I'd like to add this.configuration.apiKeys && to the following conditions to avoid strictNullChecks errors for apiKeys:

from typescript-angular/api.service.mustache@master

{{#isKeyInHeader}}
        if (this.configuration.apiKeys["{{keyParamName}}"]) {

from typescript-inversify/api.service.mustache@master

{{#isKeyInHeader}}
        if (this.APIConfiguration.apiKeys["{{keyParamName}}"]) {

@TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @nicokoenig (2018/09) @topce (2018/10)

- ./bin/typescript-angular-petstore-all.sh
- ./bin/typescript-inversify-petstore.sh
- ./bin/security/typescript-angular.sh
- ./bin/security/typescript-angular2.sh
- ./bin/security/typescript-inversify.sh
Copy link
Member

@macjohnny macjohnny 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

public testCodeInjectEndRnNR(testCodeInjectEndRnNR?: string, observe?: 'response', reportProgress?: boolean): Observable<HttpResponse<any>>;
public testCodeInjectEndRnNR(testCodeInjectEndRnNR?: string, observe?: 'events', reportProgress?: boolean): Observable<HttpEvent<any>>;
public testCodeInjectEndRnNR(testCodeInjectEndRnNR?: string, observe: any = 'body', reportProgress: boolean = false ): Observable<any> {
public testCodeInjectEndRnNR(UNKNOWN_BASE_TYPE?: UNKNOWN_BASE_TYPE, observe?: 'body', reportProgress?: boolean): Observable<any>;
Copy link
Member

Choose a reason for hiding this comment

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

any idea why UNKNOWN_BASE_TYPE occurs here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

./bin/security/typescript-angular.sh had warned:

[main] WARN  o.o.codegen.DefaultCodegen - The following schema has undefined (null) baseType. It could be due to form parameter defined in OpenAPI v2 spec with incorrect consumes. A correct 'consumes' for form parameters should be 'application/x-www-form-urlencoded' or 'mul
tipart/form-data' 

And I changed modules/openapi-generator/src/test/resources/2_0/petstore-security-test.yaml:

       consumes:
-        - application/json
-        - "*/ ' \" =end -- \r\n \n \r"
+        - application/x-www-form-urlencoded

But invalid identifier was generated:

-    public testCodeInjectEndRnNR(UNKNOWN_BASE_TYPE?: UNKNOWN_BASE_TYPE, observe?: 'body', reportProgress?: boolean): Observable<any>;
-    public testCodeInjectEndRnNR(UNKNOWN_BASE_TYPE?: UNKNOWN_BASE_TYPE, observe?: 'response', reportProgress?: boolean): Observable<HttpResponse<any>>;
-    public testCodeInjectEndRnNR(UNKNOWN_BASE_TYPE?: UNKNOWN_BASE_TYPE, observe?: 'events', reportProgress?: boolean): Observable<HttpEvent<any>>;
-    public testCodeInjectEndRnNR(UNKNOWN_BASE_TYPE?: UNKNOWN_BASE_TYPE, observe: any = 'body', reportProgress: boolean = false ): Observable<any> {
+    public testCodeInjectEndRnNR(testCodeInject*#39;&quot;&#x3D;endRnNR?: string, observe?: 'body', reportProgress?: boolean): Observable<any>;
+    public testCodeInjectEndRnNR(testCodeInject*&#39;&quot;&#x3D;endRnNR?: string, observe?: 'response', reportProgress?: boolean): Observable<HttpResponse<any>>;
+    public testCodeInjectEndRnNR(testCodeInject*&#39;&quot;&#x3D;endRnNR?: string, observe?: 'events', reportProgress?: boolean): Observable<HttpEvent<any>>;
+    public testCodeInjectEndRnNR(testCodeInject*&#39;&quot;&#x3D;endRnNR?: string, observe: any = 'body', reportProgress: boolean = false ): Observable<any> {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think AbstractTypeScriptClientCodegen#toParamName is broken:

-        name = sanitizeName(name, "\\W-[\\$]");
+        name = sanitizeName(name, "[^\\w$]");

Copy link
Member

Choose a reason for hiding this comment

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

@grktsh can you include this fix, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@macjohnny I will!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@macjohnny Done. Could you review again?

Copy link
Member

Choose a reason for hiding this comment

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

"*/ ' \" =end -- \r\n \n \r" is for detecting code injection (malicious input in consumes). I'll add it back with a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

Done via d57009d

@wing328 wing328 merged commit ced6e05 into OpenAPITools:master Dec 6, 2018
@wing328
Copy link
Member

wing328 commented Dec 6, 2018

@grktsh @macjohnny Do we need the similar fix for other TS generators? (this is not to say either one of you will need to submit the fix or own the issue. Just want to know if other TS generators have a similar bug)

@wing328 wing328 added this to the 4.0.0 milestone Dec 6, 2018
@wing328 wing328 changed the title [typescript] Avoid strictNullChecks errors for apiKeys [TS][Angular] Avoid strictNullChecks errors for apiKeys Dec 6, 2018
@grktsh grktsh deleted the null-check-api-keys branch December 6, 2018 11:31
@grktsh
Copy link
Contributor Author

grktsh commented Dec 6, 2018

@wing328 For sanitization, I think there is no need to do it because this fixes AbstractTypeScriptClientCodegen of #694. For strictNullChecks errors, I don't know...

@wing328
Copy link
Member

wing328 commented Jan 2, 2019

@grktsh thanks for the contribution, which has been included in the 4.0.0-beta release: https://twitter.com/oas_generator/status/1079727020374806529.

Happy New Year and looking forward to more collaboration and contributions in 2019!

A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
…#1611)

* [typescript] Avoid strictNullChecks errors for apiKeys

fix OpenAPITools#1607

* Run ./bin/{LANG}-petstore.sh

- ./bin/typescript-angular-petstore-all.sh
- ./bin/typescript-inversify-petstore.sh

* Run ./bin/security/{LANG}-petstore.sh

- ./bin/security/typescript-angular.sh
- ./bin/security/typescript-angular2.sh
- ./bin/security/typescript-inversify.sh

* [typescript] Fix parameter name sanitization

* Fix invalid consumes of petstore-security-test.yaml

* Run ./bin/security/typescript-*.sh
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[typescript] strictNullChecks errors for Configuration.apiKeys
3 participants