Skip to content

remove http_signature_test from samples when not supported #15221

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

Closed
wants to merge 1 commit into from
Closed

remove http_signature_test from samples when not supported #15221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bin/configs/java-okhttp-gson.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
generatorName: java
outputDir: samples/client/petstore/java/okhttp-gson
library: okhttp-gson
inputSpec: modules/openapi-generator/src/test/resources/3_0/java/petstore-with-fake-endpoints-models-for-testing-with-http-signature-okhttp-gson.yaml
inputSpec: modules/openapi-generator/src/test/resources/3_0/java/petstore-with-fake-endpoints-models-for-testing-okhttp-gson.yaml
templateDir: modules/openapi-generator/src/main/resources/Java
additionalProperties:
artifactId: petstore-okhttp-gson
Expand Down
10 changes: 0 additions & 10 deletions ...ting-with-http-signature-okhttp-gson.yaml → ...oints-models-for-testing-okhttp-gson.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ paths:
'405':
description: Invalid input
security:
- http_signature_test: []
- petstore_auth:
- 'write:pets'
- 'read:pets'
Expand All @@ -63,7 +62,6 @@ paths:
'405':
description: Validation exception
security:
- http_signature_test: []
- petstore_auth:
- 'write:pets'
- 'read:pets'
Expand Down Expand Up @@ -110,7 +108,6 @@ paths:
'400':
description: Invalid status value
security:
- http_signature_test: []
- petstore_auth:
- 'write:pets'
- 'read:pets'
Expand Down Expand Up @@ -151,7 +148,6 @@ paths:
'400':
description: Invalid tag value
security:
- http_signature_test: []
- petstore_auth:
- 'write:pets'
- 'read:pets'
Expand Down Expand Up @@ -1197,12 +1193,6 @@ components:
type: http
scheme: bearer
bearerFormat: JWT
http_signature_test:
# Test the 'HTTP signature' security scheme.
# Each HTTP request is cryptographically signed as specified
# in https://datatracker.ietf.org/doc/draft-cavage-http-signatures/
type: http
scheme: signature
schemas:
Foo:
type: object
Expand Down
29 changes: 0 additions & 29 deletions ...or/src/test/resources/3_0/javascript/petstore-with-fake-endpoints-models-for-testing.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1169,32 +1169,6 @@ paths:
application/json:
schema:
$ref: '#/components/schemas/HealthCheckResult'
/fake/http-signature-test:
get:
tags:
- fake
summary: test http signature authentication
operationId: fake-http-signature-test
parameters:
- name: query_1
in: query
description: query parameter
required: optional
schema:
type: string
- name: header_1
in: header
description: header parameter
required: optional
schema:
type: string
security:
- http_signature_test: []
requestBody:
$ref: '#/components/requestBodies/Pet'
responses:
200:
description: The instance started successfully
servers:
- url: 'http://{server}.swagger.io:{port}/v2'
description: petstore server
Expand Down Expand Up @@ -1272,9 +1246,6 @@ components:
type: http
scheme: bearer
bearerFormat: JWT
http_signature_test:
type: http
scheme: signature
Copy link
Member

Choose a reason for hiding this comment

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

@tiffmaelite thanks for the PR but why not still include the test anyway as it shouldn't break the client/server, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main reason is that it is misleading to have sample openapi.yaml files suggest that the HTTP signature security scheme is supported by many more generators than those actually supporting it.
However, testing and showing in samples the behavior of all generators when faced with unsupported schemes is important. In particular for the ones throwing exceptions when faced with unsupported security schemes.

As a result, I plan to submit a new PR once this one is merged which explicitly includes an invalid scheme (named "unsupported_test" rather than "http_signature_test", for documentation purposes) and a few additional tests in order to avoid that the issue described in #14876 occurs again.
I can also submit both set of changes in one go, but that will possibly be harder to read and review.

Copy link
Member

Choose a reason for hiding this comment

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

can you please PM me via Slack to further discuss this PR when you've time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wing328 will do somewhen tomorrow or early next week.

I also wanted to add a last argument here: what is the point of having files named petstore-with-fake-endpoints-models-for-testing.yaml and petstore-with-fake-endpoints-models-for-testing-with-http-signature.yaml if both contain the http-signature scheme?

But maybe it does not make sense to have both of them side by side, because they are quite hard to maintain (and actually one can see that their content on some endpoints diverges already for no other reason than "someone forgot about the other file")

schemas:
Foo:
type: object
Expand Down
Loading