Unexport RESTCodec #94
Conversation
| codec := newCodec(protoregistry.GlobalTypes) | ||
| if _, supportsREST := codec.(restCodec); !supportsREST { | ||
| return nil, fmt.Errorf( | ||
| "configuration uses REST protocol but codec for %q format is does not support REST-specific operations: %T", |
There was a problem hiding this comment.
REST can still be used but marshalling maps, lists and single field JSON payloads cannot. This might be fine for most users. Error instead if needed via the interface cast?
There was a problem hiding this comment.
That feels like over-fitting. If they encounter this error (already like to be extremely rare) and they don't actually need the add'l methods, they could easily implement them on their codec as no-ops that always return a error. I don't think we need to get too sophisticated for such an unlikely edge case.
There was a problem hiding this comment.
My suggestion here was to remove this special case checking and just let it error if used. Less code. But fine either way.
There was a problem hiding this comment.
Surely over-fitting is forcing the user to care about the edge case without them first requiring it. Let the Codec interface cast error inform the user that it's required. I think it's weird to have a forced interface REST, via no-ops in methods as the solution, when the subset implemented by Codec would suffice.
There was a problem hiding this comment.
I don't agree. Ignoring likely configuration errors and letting them turn into runtime errors after the server has started taking traffic is a terrible operational experience.
Comments now direct users to use
vanguard.JSONCodecbecause of the add'l operations it provides.NewTranscodernow checks that the codec registered for "json" provides the needed methods if/when REST protocol is used.