Runtime panic occurs if there are multiple Conjure errors with the same name

nmiyake commented 1 year ago

What did you do?

Publish the following Conjure definition (which will be referred to as common-service.conjure.json):

types:
  definitions:
    default-package: com.palantir.test
    errors:
      EntityNotFound:
        namespace: V6
        code: NOT_FOUND
        docs: Thrown when the provided entity ID is not found
        safe-args:
          entityId: string

Create module github.com/palantir/test-library-module that generates code using the common-service.conjure.json definition and references the generated code
Create module github.com/palantir/test-service-module that generates code using the common-service.conjure.json definition and references the generated code AND that has github.com/palantir/test-library-module as a module dependency in go.mod
Run the github.com/palantir/test-service-module

What happened?

The program crashes on startup with the following error:

panic: ErrorName V6:EntityNotFound already registered as v6.EntityNotFound

goroutine 1 [running]:
github.com/palantir/conjure-go-runtime/v2/conjure-go-contract/errors.RegisterErrorType({0x10237d60e, 0x11}, {0x10272f050?, 0x1021e2960})

What did you want to happen?

Program should start normally

nmiyake commented 1 year ago

The issue is that the generated errors.conjure.go file generates the following code:

func init() {
    errors.RegisterErrorType("V6:EntityNotFound", reflect.TypeOf(EntityNotFound{}))
}

The errors.RegisterErrorType function uses shared storage and panics if registration is done with the same key.

In the steps above, the repository ends with with 2 copies of the same errors.conjure.go (one in the consumed/vendored module and one in the local module), so the init function is called twice, which causes the panic at runtime.

It is an intentional design decision to require that error types be globally unique, but this poses a problem in the above scenario. In such a scenario, currently the only workaround is to modify the generated code (basically, to remove the error registration). Although this works, it involves manual edits and also makes it such that typed errors aren't returned for the locally generated code.

Possible Solutions

Associate error registration with particular client

The purpose of registering the errors is to allow conjure-go-runtime to provide a strongly typed error -- it maps the unmarshal of the error to the type provided.

The problem in this circumstance is that there are now 2 separate Go types that both map to the same error. However, in most cases the "scope" should be fairly well-defined -- any calls that use the vendored version of the type (declare the vendored error type as a return error type or operate on it) should use that type, while "local" code should use the local type.

Right now conjure-go-runtime is designed to have a single global lookup table for errors, but theoretically we could namespace this. For example, we could modify the error registration function so that the defining module is provided when registering errors, and could stipulate that typed errors would only be supported if a client is instantiated with a module.

As an example of this proposal, the current function signature:

func RegisterErrorType(name string, typ reflect.Type)

Could be changed to:

func RegisterErrorType(registeringModule, name string, typ reflect.Type)

Then the generated init code for the 2 modules in the example above would be:

func init() {
    errors.RegisterErrorType("github.com/palantir/test-library-module", "V6:EntityNotFound", reflect.TypeOf(EntityNotFound{}))
}

func init() {
    errors.RegisterErrorType("github.com/palantir/test-service-module", "V6:EntityNotFound", reflect.TypeOf(EntityNotFound{}))
}

We could then change the conjure-go-runtime client initialization to either require specifying a module or attempt to infer the module (although not sure how practical the latter would be).

It's not totally clear that this would be the best approach, but it's an illustration of a possible way to solve this general issue.

jamesross3 commented 1 year ago

Updates

I looked into Nick's suggested possible solution today and drafted the beginnings of something I think will work:

conjure-go changes

I opened this PR to add "conjuremoduleregistrar" package generation. This adds one package per conjure project, where the contents of the package record the package's path:

package conjuremoduleregistrar

import (
    "runtime"
)

var ConjureModuleIdentifier string

func init() {
    _, ConjureModuleIdentifier, _, _ = runtime.Caller(0)
}

The value of ConjureModuleIdentifier will depend on the root path of the conjure package, letting us discern between vendored and locally generated code.

conjure-go-runtime changes

I opened this PR adding support for per-module error registries, as well as method for storing retrieving module names from contexts. A later conjure-go PR can use this new method in the following way:

func init() {
  errors.RegisterErrorTypeForModule(conjuremoduleregistrar.ConjureModuleIdentifier, errName, errType)
}

If vendored conjure and locally generated conjure update to use this instead of the existing RegisterErrorType method, they can deconflict their error registration.

still to do

The following is the remaining work I'm aware of in terms of getting this wired up fully:

update conjure-go's client generation such that generated clients add the conjure module as a context parameter:

request = request.WithContext(errors.WithContextErrorRegistryModuleName(conjuremoduleregistrar.ConjureModuleIdentifier)

update CGR's response error decoder middleware to pull this module name off of request contexts so that it can use the appropriate error registry:

moduleName := errors.ErrorRegistryFromContext(resp.Request.Context())
conjureErr, err := errors.UnmarshalErrorForModule(moduleName, respBodyBytes)

I don't love using request contexts for wiring here so I'm open to any alternatives people think of.

tabboud commented 1 year ago

Given the global error registry is strictly used for CGR clients to know how to unmarshal a given error type, what if we instead defined a new interface for a conjure error decoder that knows how to unmarshal a specific/strongly-typed error and return a conjure error. Something like this that takes in the name of the error and the body and returns a conjure error. The name is already parsed out in the errors.UnmarshalError() func (code) which is where we could call the conjure error decoder. Alternatively, we could move the name unmarshaling into the generated conjure error decoder and change the signature slightly.

type ConjureErrorDecoder interface {
    DecodeConjureError(name string, body []byte) (errors.Error, error)
}

Then we can generate an implementation of this ConjureErrorDecoder alongside the error definitions which can be wired into the generated service clients.

func (e errorDecoder) DecodeConjureError(name string, body []byte) (errors.Error, error) {
    switch name {
    case "V6:EntityNotFound":
        var entityNotFoundErr EntityNotFound
        if err := codecs.JSON.Unmarshal(body, &entityNotFoundErr); err != nil {
            return nil, err
        }
        return &entityNotFoundErr, nil
    }
    return nil, errors.New("Unsupported type")
}

Example service client that wires in the ConjureErrorDecoder above.

func (c *v6Client) GetEntity(ctx context.Context, authHeader bearertoken.Token, requestArg GetEntityRequest) (EntityResp, error) {
    var defaultReturnVal EntityResp
    var returnVal *EntityResp
    var requestParams []httpclient.RequestParam
    requestParams = append(requestParams, httpclient.WithRPCMethodName("GetEntity"))
    requestParams = append(requestParams, httpclient.WithRequestMethod("GET"))
    requestParams = append(requestParams, httpclient.WithPathf("/entity"))
    requestParams = append(requestParams, httpclient.WithJSONRequest(requestArg))
    requestParams = append(requestParams, httpclient.WithJSONResponse(&returnVal))
    // This is where we wire in the error decoder above.
    requestParams = append(requestParams, httpclient.WithRequestConjureErrorDecoder(errorDecoder{}))
    if _, err := c.client.Do(ctx, requestParams...); err != nil {
        return defaultReturnVal, werror.WrapWithContextParams(ctx, err, "getEntity failed")
    }
    if returnVal == nil {
        return defaultReturnVal, werror.ErrorWithContextParams(ctx, "getEntity response cannot be nil")
    }
    return *returnVal, nil
}

where the WithRequestConjureErrorDecoder wraps the restErrorDecoder to pass in the new conjure error decoder.

func WithRequestConjureErrorDecoder(ced ConjureErrorDecoder) RequestParam {
    return requestParamFunc(func(b *requestBuilder) error {
        b.errorDecoderMiddleware = errorDecoderMiddleware{
            errorDecoder: restErrorDecoder{
                conjureErrorDecoder: ced,
            },
        }
        return nil
    })
}

This allows us to remove the global registry altogether and more tightly couples the errors defined to the endpoints that use them. I prototyped this approach

One major flaw with this approach is that we do not know the full set of errors that each service/endpoint returns. This could be solved if error attribution (RFC) is implemented in both the conjure IR and the languages, but that's not done yet.

palantir / conjure-go