Allow to specify examples in OpenAPI when skip encode decode body set #2858

nitinmohan87 · 2021-05-26T18:31:10Z

Allows to specify examples for HTTP endpoints with skip request/response encode/decode body as below

                Method("test", func() {
                        Payload(Any, func() {
                               Example("my example")
                        })
                        Result(Any, func() {
                                Example("my result")
                        })
			HTTP(func() {
				POST("/")
				SkipRequestBodyEncodeDecode()
				SkipResponseBodyEncodeDecode()
			})
		})

raphael · 2021-05-31T17:50:09Z

http/codegen/openapi/v3/types.go

+				// SkipRequestBodyEncodeDecode. In this case, we set the request body
+				// example from the Payload.
+				req = sf.schemafy(e.MethodExpr.Payload)
+			}


Should the code remove any header, cookie or param from the payload for the body example?

raphael · 2021-05-31T17:50:31Z

http/codegen/openapi/v3/types.go

+					// it is possible for design to have request Body as Empty and set
+					// SkipRequestBodyEncodeDecode. In this case, we set the request body
+					// example from the Payload.
+					js = sf.schemafy(e.MethodExpr.Result)


Sames as above

raphael

Looks good, thank you - I left a few comments and questions

raphael · 2021-05-31T17:52:06Z

codegen/service/example_svc.go

@@ -106,10 +106,10 @@ func basicEndpointSection(m *expr.MethodExpr, svcData *Data) *codegen.SectionTem
 		MethodData:     md,
 		ServiceVarName: svcData.VarName,
 	}
-	if m.Payload.Type != expr.Empty {
+	if md.Payload != "" {


Can you explain in which cases these two tests are different?

Method Payload can be Empty when SkipRequestEncodeDecode is set. In that case, we should generate the example service correctly without the payload argument.

That makes sense, thank you. Would it make sense/be clearer to use mustInitPayload instead?

raphael · 2021-05-31T17:58:59Z

codegen/service/service_data.go

+		for _, r := range httpMet.Responses {
+			if !r.BodyOnly() {
+				// one of the response defines header/cookie.
+				bodyOnly = false


could add a break here

raphael · 2021-05-31T18:05:50Z

http/codegen/openapi/v3/builder.go

+				Extensions:  openapi.ExtensionsFromExpr(e.Body.Meta),
+			}}
+		} else if e.SkipRequestBodyEncodeDecode {
+			// it is possible for design to have request Body as Empty and set


I don't understand this case. If SkipXXX is used then it seems that the OpenAPI request body schema should be computed by looking up all the attributes of the payload that are not used in headers, cookies and params? If the body type is set to empty then maybe we should honor that in the OpenAPI schema?

raphael · 2021-05-31T18:06:07Z

http/codegen/openapi/v3/response.go

+				// it is possible for design to have response Body as Empty and set
+				// SkipResponseBodyEncodeDecode. In this case, we set the response body
+				// example from the Result.
+				content = make(map[string]*MediaType)


Same as above

raphael · 2021-05-31T18:07:44Z

http/codegen/testdata/openapi_dsls.go

+			})
+		})
+	})
+}


Do you think we also need to test for the cases where the body has multiple attributes and some are used by other elements than the body (e.g. headers, params etc.)?

raphael · 2021-06-09T20:44:30Z

codegen/service/service_data.go

+		httpMet = httpSvc.Endpoint(m.Name)
+		for _, r := range httpMet.Responses {
+			if httpMet.SkipResponseBodyEncodeDecode && r.HasBodyOnly() {
+				return false


Should this function only return false if all responses only define a body?

raphael · 2021-06-09T20:52:25Z

expr/http_endpoint.go

+		}
+		// with SkipResponseBodyEncodeDecode at most one success response can be defined
+		if r.StatusCode < 400 {
+			if successResp && e.SkipResponseBodyEncodeDecode {


This a pre-existing bug but it seems the test should also make sure that there is only a body defined (in which case there won't be a result and thus no way for Goa to know what status code to use because no field to test for the Tag value). If there are headers defined then I believe it's OK to have multiple responses.

raphael · 2021-06-09T20:54:04Z

expr/http_endpoint.go

@@ -536,9 +499,6 @@ func (e *HTTPEndpointExpr) Validate() error {
 				verr.Add(e, "Payload type is array but HTTP endpoint defines both a body and headers. At most one of these must be defined and it must be an array.")
 			}
 		}
-		if !hasParams && !hasHeaders && e.SkipRequestBodyEncodeDecode {


I don't see this case being covered in the SkipXXX validation functions.

raphael · 2021-06-09T20:54:38Z

expr/http_endpoint.go

@@ -573,9 +533,6 @@ func (e *HTTPEndpointExpr) Validate() error {
 				verr.Add(e, "Payload type is map but HTTP endpoint defines both a body and route or query string parameters. At most one of these must be defined and it must be a map.")
 			}
 		}
-		if !hasParams && e.SkipRequestBodyEncodeDecode {
-			verr.Add(e, "Payload type is map but HTTP endpoint uses SkipRequestBodyEncodeDecode and does not define headers.")


Same as above, I don't see this validation being tested below.

raphael · 2021-06-09T21:04:09Z

expr/http_endpoint.go

@@ -609,9 +566,6 @@ func (e *HTTPEndpointExpr) Validate() error {
 	}

 	body := httpRequestBody(e)
-	if e.SkipRequestBodyEncodeDecode && body.Type != Empty {


Same as above

raphael · 2021-06-09T21:19:41Z

http/codegen/service_data.go

+				}
+			}
+		}
+		if hasResult {


Could that just be in the same block as above?

raphael · 2021-06-09T21:21:02Z

http/codegen/service_data.go

@@ -1539,7 +1548,7 @@ func buildResponses(e *expr.HTTPEndpointExpr, result *expr.AttributeExpr, viewed
 						break
 					}
 				}
-				if needInit(result.Type) {
+				if needInit(result.Type) && !(e.SkipResponseBodyEncodeDecode && resp.HasBodyOnly()) {


Are there cases where we call needInit without testing for SkipResponseBodyEncodeDecode? If not maybe needInit could be refactored to do that test as well?

github-actions · 2021-09-08T05:40:00Z

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

nitinmohan87 added 5 commits May 26, 2021 11:28

Allow to specify examples when skip encode decode body

6168e02

Fix request example

a4fa35b

Merge branch 'v3' into add_examples_skip_encode_decode

0cf02fa

Set example for SkipXX in Payload/Result

fc7b79e

Fix code generation

edf7874

raphael reviewed May 31, 2021

View reviewed changes

raphael mentioned this pull request Jun 2, 2021

Add http.Replace() and use it for file servers #2856

Merged

Address review comments

40d3b90

raphael reviewed Jun 9, 2021

View reviewed changes

http/codegen/service_data.go

}

}

}

if hasResult {

Copy link

Member

raphael Jun 9, 2021

Choose a reason for hiding this comment

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

Could that just be in the same block as above?

raphael reviewed Jun 9, 2021

View reviewed changes

github-actions bot added the no-pr-activity label Sep 8, 2021

github-actions bot closed this Sep 23, 2021

raphael deleted the add_examples_skip_encode_decode branch June 13, 2022 20:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to specify examples in OpenAPI when skip encode decode body set #2858

Allow to specify examples in OpenAPI when skip encode decode body set #2858

nitinmohan87 commented May 26, 2021 •

edited

Loading

raphael May 31, 2021 •

edited

Loading

raphael May 31, 2021

raphael left a comment

raphael May 31, 2021

nitinmohan87 Jun 4, 2021

raphael Jun 9, 2021

raphael May 31, 2021

raphael May 31, 2021

raphael May 31, 2021

raphael May 31, 2021

raphael Jun 9, 2021

raphael Jun 9, 2021

raphael Jun 9, 2021

raphael Jun 9, 2021

raphael Jun 9, 2021

raphael Jun 9, 2021

raphael Jun 9, 2021

github-actions bot commented Sep 8, 2021

Allow to specify examples in OpenAPI when skip encode decode body set #2858

Allow to specify examples in OpenAPI when skip encode decode body set #2858

Conversation

nitinmohan87 commented May 26, 2021 • edited Loading

raphael May 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raphael left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 8, 2021

nitinmohan87 commented May 26, 2021 •

edited

Loading

raphael May 31, 2021 •

edited

Loading