Generate RPC Services from Go interfaces

[dennwc]

Generate RPC services from Go interfaces. Also make transform to generate service definition instead of a list of RPCs.

[dennwc]

There will be some more changes to generate client code as well, so don't merge yet.

[codecov[bot]]

Codecov Report

Merging #89 into master will decrease coverage by 0.5%. The diff coverage is 88.68%.

@@            Coverage Diff             @@
##           master      #89      +/-   ##
==========================================
- Coverage   91.84%   91.33%   -0.51%     
==========================================
  Files          13       13              
  Lines        1422     1777     +355     
==========================================
+ Hits         1306     1623     +317     
- Misses         96      126      +30     
- Partials       20       28       +8

Impacted Files	Coverage Δ
protobuf/gen.go	94.17% <100%> (ø)	:arrow_up:
protobuf/protobuf.go	89.58% <100%> (ø)	:arrow_up:
rpc/context.go	100% <100%> (ø)	:arrow_up:
scanner/package.go	90.52% <14.28%> (-6.07%)	:arrow_down:
scanner/scanner.go	88.65% <70.58%> (-2.52%)	:arrow_down:
resolver/resolver.go	91.24% <75%> (-2.15%)	:arrow_down:
protobuf/transform.go	97.39% <91.3%> (-0.51%)	:arrow_down:
rpc/rpc.go	93.16% <91.74%> (+0.91%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a40ce5f...6e0da0d. Read the comment docs.

[Serabe]

We've been talking about this PR IRL. The main problem with this is that interfaces are the translations of oneof in protobuf. If in a future we want to support them somehow, we would have closed that door by accepting this.

We like the idea, though we would need to find a workaround for this. We are considering options for the comment proteus:generate but haven't settled in anything yet (ideas more than welcomed here).

Also, there is the problem that imagine you have the following interface:

type Person interface {
  Name() string
}

This would generate a personRemote struct satisfying this interface that would swallow any error in the called.

My initial proposal would be to apply the following two changes:

• For an interface to be generated as a service, it needs to be called with //proteus:generate:service. This would imply no changes in hasGenerateComment since it checks with strings.HasPrefix.
• For an interface to be generated as a service, all the methods need to return, at least, an error in final position. That would allow us to work with a remote version being able to handle the errors.

As before, an example needs to be added in example/models.go and checked in example_test.go

What do you think, @dennwc, @erizocosmico, and @mcuadros?

[erizocosmico]

@Serabe why forcing all interface methods to return error? Currently we don't enforce that with functions or methods, why do it for interfaces?

[dennwc]

@Serabe I agree with @erizocosmico that logic should match current behavior for global functions. If user wants an interface function with no error, he should be aware that RPC can fail and zero result will be returned. This can be documented, but is a valid use case.

Regarding type unions (interfaces that are implemented by multiple message types), I think we can easily distinguish between them and service interfaces. In any case RPC generation from interfaces requires explicit //proteus:generate annotation. And type unions (generated for protobuf), are useless without a message that contains them as a field type. And if we find a message/struct with such interface we can generate oneof automatically, the same way as dependent structs are generated.

And sure, I will add examples, but I need to commit few more changes first to integrate context support.

---

Review status: 0 of 13 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

Comments from Reviewable

[Serabe]

The matching behaviour for current functions is with an error as last returned value in the client. We allow functions without errors as last returned value because we only generate the server, and therefore, we don't force functions to return an error.

func A(b string) int {
    return 0
}

func AWithError(b string) (int, error) {
    return A(b), nil
}

Since we are going from not having an error (original function) to having an error (in order to satisfy the signature in the server's interface) we are no loosing any information.

This PR adds a sort of client, the remote struct. In this case we have the following:

//proteus:generate
type RemoteType interface {
    Name() string
}

And we are generating something like:

func (c *remoteTypeRemote) Name() string {
    s, _ := c.Client.Name(ctx) // here we discard the error
    return s
}

So we are going the opposite direction, from having an error to not having it. That causes some information loss I would rather not have.

Since this is new behaviour, I would like to be conservative so far.

Thank you!

[dennwc]

I totally understand what you are saying, and I agree that dropping an error in RPC client is a bad idea, but it's not possible to add an error to client's interface definition.

The point I was trying to make is that if client asks to generate an RPC client-server pair for his interface and this interface has a method with no error, RPC client should match this, or it will not be able to implement user's interface. It will render generated code useless in context of user's interface.

I propose printing a warning message in proteus log for these situations and/or generating a FIXME comment in client definition, so user will be aware that omitting an error might cause unexpected behavior.

---

Review status: 0 of 13 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

Comments from Reviewable

[dennwc]

Rebased, integrated with context and added examples. @Serabe please take a look.

---

Review status: 0 of 18 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

protobuf/transform.go, line 157 at r2 (raw file):

Previously, Serabe (Sergio Arbeo) wrote…

There is no test checking what happens if we generate an empty interface. Can we add one to make sure nothing explodes?

Done. Added as an example.

---

Comments from Reviewable

[dennwc]

@Serabe Any updates on this?

[Serabe]

Still would like to only accept interfaces with the error, unless another solution is proposed. Don't like the idea of losing errors.

[dennwc]

@Serabe I already proposed two solutions. What about them?

[dennwc]

Okay, I think I should provide an example to show that this might be a desired behavior for some use cases.

Let's assume we are building a plugin system with microservices. Each plugin can do some useful work, and has an additional method to expose meta-information about itself:

message Info {
  enum State {
     UNAVAILABLE = 0;
     READY = 1;
  }
  State state = 1;
  string name = 2;
}

service Module {
    rpc Info(Empty) return (Info);
    rpc Do(DoReq) returns (DoResp);
} 

This corresponds to a following Go interface:

type Module interface{
   Info() Info
   Do(DoReq) (DoResp, error)
}

As you can see, Info method has no error. But since default value of Info struct already has a valid meaning (status = unavailable), user can still verify that an error happened on the RPC and module cannot be reached.

[Serabe]

We cannot check if an struct has anything that could be considered an unavailable state like in the Info above and still we are losing information (the error returned by gRPC).

To resume: For this PR to be accepted it needs to:

1. Only generate services from interfaces with //proteus:generate:service or something that is not a plain //proteus:generate.
2. Only generate methods that return an error in last place.

Thank you!