handlers_user: Include db err in the internal error

[jhrozek]

Errors like these are what we would print e.g. when adding a duplicate
user:

{"level":"error","Resource":{"service":"mediator.v1.UserService","method":"CreateUser"},"Attributes":{"http.code":"Internal","http.content-type":["application/grpc"],"http.duration":"550.55325ms","http.user_agent":["grpc-go/1.58.1"],"exception.message":"rpc
error: code = Internal desc = failed to create
user"},"Timestamp":1695131280165826000}

Now we get:

{"level":"error","Resource":{"service":"mediator.v1.UserService","method":"CreateUser"},"Attributes":{"http.code":"Internal","http.content-type":["application/grpc"],"http.duration":"547.674ms","http.user_agent":["grpc-go/1.58.1"],"exception.message":"rpc
error: code = Internal desc = failed to create user: pq: duplicate key
value violates unique constraint
\"users_organization_id_username_lower_idx\""},"Timestamp":1695131520115208000}

The user still gets just an internal error in the CLI, but at least we
see what's going on in the server logs.

[JAORMX]

We can use %w instead.

[evankanderson]

(also, error implements strings.Stringer, so you could use %s if you just wanted the error string. But Ozz's suggestion is better)

[jhrozek]

I'm not sure we can use %w here because status.Errorf (not to be confused with stdlib's fmt.Errorf) uses fmt.Sprintf internally which IIUC doesn't support the %w formatter.

[jhrozek]

I wasn't sure about %w from the top of my head, but a quick go playground experiment seems to agree.

[jhrozek]

(also, error implements strings.Stringer, so you could use %s if you just wanted the error string. But Ozz's suggestion is better)

Does it? I'm seriously questioning my Go-fu now, but I thought that error only ever had to implement the Error method and that errors were handled internally in the formatter by calling Error in the fmt module.

Anyway, calling Error explicitly is not needed and I'll drop it (even though it would be nice to be consistent, there's a bunch of places where we call Error explicitly)

[evankanderson]

I'm also wondering if just returning err in these cases would be sufficient, given that the log message will include the method name. But I don't feel strongly either way.

[jhrozek]

I'm also wondering if just returning err in these cases would be sufficient, given that the log message will include the method name. But I don't feel strongly either way.

I thought the point was to carry the grpc error code along with the returned error.

[evankanderson]

I'm also wondering if just returning err in these cases would be sufficient, given that the log message will include the method name. But I don't feel strongly either way.

I thought the point was to carry the grpc error code along with the returned error.

I think if you return error, it will map to codes.Internal unless you have a different error type. Also, unless you use util.UserVisibleError, the message text will be hidden from the RPC reply by code in the SanitizingInterceptor in the same file.