Improvement: support optional binary (#191)

`optional<binary>` now represented in go as `*ReadCloser` and a 204 status code is treated as nil

changelog/@unreleased/pr-191.v2.yml

@@ -0,0 +1,10 @@
+type: improvement
+improvement:
+  description: Fixes issue where generated Conjure clients and servers did not properly handle empty responses for Conjure
+    endpoints returning `optional<binary>`.
+    The Conjure specification states that endpoints returning `optional<binary>` should return a 204 status code to
+    indicate an empty optional (to distinguish from the case where the response is a present optional of 0 length).
+    This change interprets `optional<binary>` as `*ReadCloser` (rather than `ReadCloser') and updates the server to mark
+    'nil' responses with a 204 status code. Correspondingly, clients return 'nil' when the response has a 204 status code.
+  links:
+  - https://github.com/palantir/conjure-go/pull/191

conjure/conjure_test.go

@@ -1007,6 +1007,25 @@ func NewExampleUnionFromRecursive(v ExampleUnion) ExampleUnion {
 		},
 		"markers" : [ ]
 		}, {
+		"endpointName" : "maybeStreamResponse",
+		"httpMethod" : "GET",
+		"httpPath" : "/catalog/maybe/streamResponse",
+		"auth" : {
+			"type" : "header",
+			"header" : { }
+		},
+		"args" : [ ],
+		"returns" : {
+			"type" : "optional",
+			"optional" : {
+				"itemType" : {
+				  "type" : "primitive",
+				  "primitive" : "BINARY"
+				}
+			}
+		},
+		"markers" : [ ]
+		}, {
 		"endpointName" : "queryParams",
 		"httpMethod" : "GET",
 		"httpPath" : "/catalog/echo",
@@ -1052,6 +1071,7 @@ import (
 	"context"
 	"fmt"
 	"io"
+	"net/http"
 	"net/url"
 
 	"github.com/palantir/conjure-go-runtime/v2/conjure-go-client/httpclient"
@@ -1064,6 +1084,7 @@ type TestServiceClient interface {
 	GetFileSystems(ctx context.Context, authHeader bearertoken.Token) (map[string]int, error)
 	CreateDataset(ctx context.Context, cookieToken bearertoken.Token, requestArg string) error
 	StreamResponse(ctx context.Context, authHeader bearertoken.Token) (io.ReadCloser, error)
+	MaybeStreamResponse(ctx context.Context, authHeader bearertoken.Token) (*io.ReadCloser, error)
 	QueryParams(ctx context.Context, inputArg string, repsArg int) error
 }
 
@@ -1123,6 +1144,23 @@ func (c *testServiceClient) StreamResponse(ctx context.Context, authHeader beare
 	return resp.Body, nil
 }
 
+func (c *testServiceClient) MaybeStreamResponse(ctx context.Context, authHeader bearertoken.Token) (*io.ReadCloser, error) {
+	var requestParams []httpclient.RequestParam
+	requestParams = append(requestParams, httpclient.WithRPCMethodName("MaybeStreamResponse"))
+	requestParams = append(requestParams, httpclient.WithRequestMethod("GET"))
+	requestParams = append(requestParams, httpclient.WithHeader("Authorization", fmt.Sprint("Bearer ", authHeader)))
+	requestParams = append(requestParams, httpclient.WithPathf("/catalog/maybe/streamResponse"))
+	requestParams = append(requestParams, httpclient.WithRawResponseBody())
+	resp, err := c.client.Do(ctx, requestParams...)
+	if err != nil {
+		return nil, err
+	}
+	if resp.StatusCode == http.StatusNoContent {
+		return nil, nil
+	}
+	return &resp.Body, nil
+}
+
 func (c *testServiceClient) QueryParams(ctx context.Context, inputArg string, repsArg int) error {
 	var requestParams []httpclient.RequestParam
 	requestParams = append(requestParams, httpclient.WithRPCMethodName("QueryParams"))
@@ -1146,6 +1184,7 @@ type TestServiceClientWithAuth interface {
 	GetFileSystems(ctx context.Context) (map[string]int, error)
 	CreateDataset(ctx context.Context, requestArg string) error
 	StreamResponse(ctx context.Context) (io.ReadCloser, error)
+	MaybeStreamResponse(ctx context.Context) (*io.ReadCloser, error)
 	QueryParams(ctx context.Context, inputArg string, repsArg int) error
 }
 
@@ -1171,6 +1210,10 @@ func (c *testServiceClientWithAuth) StreamResponse(ctx context.Context) (io.Read
 	return c.client.StreamResponse(ctx, c.authHeader)
 }
 
+func (c *testServiceClientWithAuth) MaybeStreamResponse(ctx context.Context) (*io.ReadCloser, error) {
+	return c.client.MaybeStreamResponse(ctx, c.authHeader)
+}
+
 func (c *testServiceClientWithAuth) QueryParams(ctx context.Context, inputArg string, repsArg int) error {
 	return c.client.QueryParams(ctx, inputArg, repsArg)
 }

conjure/serverwriter.go

@@ -454,11 +454,23 @@ func getReturnStatements(
 	body = append(body, getIfErrNotNilReturnErrExpression())
 
 	var codec types.Typer
+	var respArg string
 	if isBinary, err := isBinaryType(*endpoint.Returns); err != nil {
 		return nil, err
 	} else if isBinary {
+		isOptional, err := visitors.IsSpecificConjureType(*endpoint.Returns, visitors.IsOptional)
+		if err != nil {
+			return nil, err
+		}
+		if isOptional {
+			body = append(body, getOptionalBinaryStatusNoContentExpression())
+			respArg = "*" + responseArgVarName
+		} else {
+			respArg = responseArgVarName
+		}
 		codec = types.CodecBinary
 	} else {
+		respArg = responseArgVarName
 		codec = types.CodecJSON
 	}
 	info.AddImports(codec.ImportPaths()...)
@@ -479,7 +491,7 @@ func getReturnStatements(
 		Values: []astgen.ASTExpr{
 			expression.NewCallFunction(codec.GoType(info), codecEncodeFunc,
 				expression.VariableVal(responseWriterVarName),
-				expression.VariableVal(responseArgVarName),
+				expression.VariableVal(respArg),
 			),
 		},
 	})
@@ -821,6 +833,29 @@ func getIfErrNotNilReturnErrExpression() astgen.ASTStmt {
 	}
 }
 
+// getOptionalBinaryStatusNoContentExpression returns an expression used for optional<binary> endpoints to distinguish
+// between zero length present binaries and empty binaries. Specifically, empty binaries are marked with a StatusNoContent
+// status code per the conjure spec.
+func getOptionalBinaryStatusNoContentExpression() astgen.ASTStmt {
+	return &statement.If{
+		Cond: &expression.Binary{
+			LHS: expression.VariableVal(responseArgVarName),
+			Op:  token.EQL,
+			RHS: expression.Nil,
+		},
+		Body: []astgen.ASTStmt{
+			statement.NewExpression(&expression.CallExpression{
+				Function: &expression.Selector{
+					Receiver: expression.VariableVal(responseWriterVarName),
+					Selector: "WriteHeader",
+				},
+				Args: []astgen.ASTExpr{expression.Type("http.StatusNoContent")}}),
+			statement.NewReturn(
+				expression.Nil,
+			),
+		}}
+}
+
 func getIfErrNotNilExpression() astgen.ASTExpr {
 	return &expression.Binary{
 		LHS: expression.VariableVal(errorName),

conjure/servicewriter.go

@@ -810,10 +810,38 @@ func serviceStructMethodBodyAST(endpointDefinition spec.EndpointDefinition, retu
 	body = append(body, ifErrNotNilReturnHelper(hasReturnVal, valVarToReturnInErr, errVar, nil))
 
 	if returnsBinary {
+		isOptional, err := isReturnTypeSpecificType(endpointDefinition.Returns, visitors.IsOptional)
+		if err != nil {
+			return nil, err
+		}
+		binaryRespVar := respVar
+		if isOptional {
+			// If an endpoint with a return type of optional<binary> provides a response with a code of StatusNoContent
+			// then the return value is empty and nil is returned.
+			body = append(body, &statement.If{
+				Cond: &expression.Binary{
+					LHS: expression.NewSelector(
+						expression.VariableVal(respVar),
+						"StatusCode",
+					),
+					Op:  token.EQL,
+					RHS: expression.Type("http.StatusNoContent"),
+				},
+				Body: []astgen.ASTStmt{
+					statement.NewReturn(
+						expression.Nil,
+						expression.Nil,
+					),
+				},
+			})
+			info.AddImports("net/http")
+			// if endpoint returns binary, return pointer to body of response directly
+			binaryRespVar = "&" + respVar
+		}
 		// if endpoint returns binary, return body of response directly
 		body = append(body, statement.NewReturn(
 			expression.NewSelector(
-				expression.VariableVal(respVar),
+				expression.VariableVal(binaryRespVar),
 				"Body",
 			),
 			expression.Nil,
@@ -1018,26 +1046,17 @@ func returnTypesForEndpoint(endpointDefinition spec.EndpointDefinition, info typ
 	var returnTypes []expression.Type
 	imports := make(StringSet)
 	if endpointDefinition.Returns != nil {
-		var goType string
-		returnBinary, err := isReturnTypeSpecificType(endpointDefinition.Returns, visitors.IsBinary)
+		typer, err := visitors.NewConjureTypeProviderTyper(*endpointDefinition.Returns, info)
 		if err != nil {
 			return nil, nil, err
 		}
-		if returnBinary {
-			// special case: "binary" type resolves to []byte in structs, but indicates a streaming response when
-			// specified as the return type of a service, so use "io.ReadCloser".
-			goType = types.IOReadCloserType.GoType(info)
-			imports.AddAll(NewStringSet(types.IOReadCloserType.ImportPaths()...))
-		} else {
-			typer, err := visitors.NewConjureTypeProviderTyper(*endpointDefinition.Returns, info)
-			if err != nil {
-				return nil, nil, err
-			}
-			goType = typer.GoType(info)
-			if err != nil {
-				return nil, nil, errors.Wrapf(err, "failed to process return type %q", goType)
-			}
-		}
+
+		// special case: "binary" type resolves to []byte in structs, but indicates a streaming response when
+		// specified as the return type of a service, so replace all nested references with "io.ReadCloser".
+		var readCloserImports []string
+		typer, readCloserImports = types.MapBinaryTypeToReadCloserType(typer)
+		imports.AddAll(NewStringSet(readCloserImports...))
+		goType := typer.GoType(info)
 		returnTypes = append(returnTypes, expression.Type(goType))
 	}
 	return append(returnTypes, expression.ErrorType), imports, nil

conjure/types/types.go

@@ -196,23 +196,33 @@ func (t *mapType) ImportPaths() []string {
 	return append(t.keyType.ImportPaths(), t.valType.ImportPaths()...)
 }
 
-type singleGenericValType struct {
-	valType   Typer
-	fmtString string
+type iterableValType struct {
+	valType Typer
 }
 
-func (t *singleGenericValType) GoType(info PkgInfo) string {
-	return fmt.Sprintf(t.fmtString, t.valType.GoType(info))
+func (t *iterableValType) GoType(info PkgInfo) string {
+	return fmt.Sprintf("[]%s", t.valType.GoType(info))
 }
 
-func (t *singleGenericValType) ImportPaths() []string {
+func (t *iterableValType) ImportPaths() []string {
+	return t.valType.ImportPaths()
+}
+
+type optionalGenericValType struct {
+	valType Typer
+}
+
+func (t *optionalGenericValType) GoType(info PkgInfo) string {
+	return fmt.Sprintf("*%s", t.valType.GoType(info))
+}
+
+func (t *optionalGenericValType) ImportPaths() []string {
 	return t.valType.ImportPaths()
 }
 
 func NewListType(valType Typer) Typer {
-	return &singleGenericValType{
-		valType:   valType,
-		fmtString: "[]%s",
+	return &iterableValType{
+		valType: valType,
 	}
 }
 
@@ -228,9 +238,8 @@ func NewSetType(valType Typer) Typer {
 }
 
 func NewOptionalType(valType Typer) Typer {
-	return &singleGenericValType{
-		valType:   valType,
-		fmtString: "*%s",
+	return &optionalGenericValType{
+		valType: valType,
 	}
 }
 
@@ -326,3 +335,19 @@ func (f *funcType) ImportPaths() []string {
 	}
 	return importPaths
 }
+
+// MapBinaryTypeToReadCloserType replaces BinaryType and OptionalType<BinaryType> with IOReadCloserType and
+// OptionalType<IOReadCloserType> respectively. It also returns IOReadCloserType's imports if a binary
+// reference is found.
+func MapBinaryTypeToReadCloserType(valType Typer) (Typer, []string) {
+	if valType == BinaryType {
+		return IOReadCloserType, IOReadCloserType.ImportPaths()
+	}
+	if v, ok := valType.(*optionalGenericValType); ok {
+		typer, importPaths := MapBinaryTypeToReadCloserType(v.valType)
+		return &optionalGenericValType{
+			valType: typer,
+		}, importPaths
+	}
+	return valType, nil
+}

conjure/types/types_test.go

@@ -181,3 +181,61 @@ func TestFunctionTypes(t *testing.T) {
 		})
 	}
 }
+
+func TestReadCloseMapper(t *testing.T) {
+
+	for _, test := range []struct {
+		name         string
+		initialTyper Typer
+		desiredTyper Typer
+		hasImports   bool
+	}{
+		{
+			name:         "String type",
+			initialTyper: String,
+			desiredTyper: String,
+		},
+		{
+			name:         "SafeLong type",
+			initialTyper: SafeLong,
+			desiredTyper: SafeLong,
+		},
+		{
+			name:         "Binary type",
+			initialTyper: BinaryType,
+			desiredTyper: IOReadCloserType,
+			hasImports:   true,
+		},
+		{
+			name:         "Optional String type",
+			initialTyper: NewOptionalType(String),
+			desiredTyper: NewOptionalType(String),
+		},
+		{
+			name:         "Optional SafeLong type",
+			initialTyper: NewOptionalType(SafeLong),
+			desiredTyper: NewOptionalType(SafeLong),
+		},
+		{
+			name:         "Optional Binary type",
+			initialTyper: NewOptionalType(BinaryType),
+			desiredTyper: NewOptionalType(IOReadCloserType),
+			hasImports:   true,
+		},
+		{
+			name:         "List Binary type",
+			initialTyper: NewListType(BinaryType),
+			desiredTyper: NewListType(BinaryType),
+		},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			typer, imports := MapBinaryTypeToReadCloserType(test.initialTyper)
+			assert.Equal(t, typer, test.desiredTyper)
+			if test.hasImports {
+				assert.Equal(t, imports, IOReadCloserType.ImportPaths())
+			} else {
+				assert.Nil(t, imports)
+			}
+		})
+	}
+}