anz-bank / sysl-go

Communication library used by SYSL-generated code written in Go.
Apache License 2.0
10 stars 14 forks source link

Include RestRequestOption interface and OnResult implementation #161

Closed andrewemeryanz closed 4 years ago

andrewemeryanz commented 4 years ago

In some instances, custom handler implementations require access to more information than downstream service calls currently permit.

Downstream service calls currently provide access to deserialised response body on successful requests, however in some instances the status code and headers are also required to be known.

Likewise, downstream services calls currently return an error for unsuccessful requests, however in some instances the status code, headers or response body are required to be known.

RestRequestOption instances can be passed into downstream REST requests to perform custom actions:

response, err := client.GetRestFooBar(ctx, &request,
    restlib.RestRequestOnResult(func(result *restlib.HTTPResult, err error) {
        // access to raw http result and error
    }))

To support this, generated clients subsequently change:

// from
GetRestFooBar func(ctx context.Context, req *foo.GetRestFooBarRequest) 
(*foo.BarResponse, error)

// to
GetRestFooBar func(ctx context.Context, req *foo.GetRestFooBarRequest, opts ...restlib.RestRequestOption) 
(*foo.BarResponse, error)

Closes #105 #159

gkanz commented 4 years ago

I think it is a way to make progress on this problem, but it defeats the design principles of sysl-go. In particular:

only allowing source code dependencies to point "inward"

Following that principle, it should be noted that this is an interim solution and brings risks with it, i.e. testability and portability of the business logic. And should be remediated before wider adoption. For example:

I can think of two options that uphold the principle but would require (possibly considerably) more work:

  1. Get the downstream systems to not return information that business logic depends upon in HTTP headers and status codes. This is not something that the Acceleration team can control.
  2. Map the HTTP headers into the same protocol-agnostic structures used by the HTTP body for consumption by the handlers. Status codes should be mapped into Go-style errors. This would require additional information to be captured in the Sysl spec and updates to the code generation logic.
andrewemeryanz commented 4 years ago

@gkanz Thanks. I agree that a custom abstraction would be better:

// definition: core package
type RestResult struct {
    StatusCode int
    Headers    map[string][]string
    Body       []byte
}

// usage
response, err := client.GetRestFooBar(ctx, &request,
    restlib.RestRequestOnResult(func(result *core.RestResult, err error) {
        // access to http result and error
    }))

However changing anything behaviourally (return value or errors) would require an additional client method to maintain backwards compatibility. I'm not sure I want to pull the trigger on that without a bigger rethink into what the end state looks like.

andrewemeryanz commented 4 years ago

Replaced by https://github.com/anz-bank/sysl-go/pull/165.