Add interfaces for Requests and Responses

[google]

Add interfaces for Requests and Responses

[eliben]

It's not immediately clear to me how you intend to use this. Do you have a concrete example of the code this would enable writing that's hard to write today?

[dbaumgarten]

If I currently want to write a function that accepts (for example) any kind of Request-type as argument, I would have to use the Message-interface as argument-type. But if I do this, I do not get access to the Request-struct that is embedded into the provided Request-type. All I get is the getSeq() method from the Message interface. But if I need access to the fields Request.Command or Request.ProtocollMessage.Seq I have to resort to reflection (or giant switches).

Currently:

func GetCommandOfRequest(req Message)string{
// this does not work. The field is there, but I can not access it because the current type is the Message-interface. 
return req.Request.Command
}

What I would like to have:

func GetCommandOfRequest(req RequestMessage)string{
// the new interface method exposes the embedded Request, so I can access it's fields 
// Also it is impossible to pass anything then a Request-type to this function (which is good)
return req.GetRequest().Command
}

[eliben]

Sorry, I should have made it clearer that I'm interested in understanding the deeper level of what code needs this. We've written code using this package and haven't needed the capability, hence my question. Interfaces are easy to add but much harder to take away, so I want to be extra cautious here.

Specifically to the example you've provided. It's not clear to me what you gain from having access to Command or to Seq. The latter you already have through the Message interface's GetSeq, right?

As for Command, it's just an untyped string name of the command, which seems little better than asking reflect for a type name, so again, not clear what added capability there is here that cannot be done without the interface.

I can sort of see how there's extra information in Response that would be interesting, though you've specifically talked about Request here.

[dbaumgarten]

What I am trying to do is properly separating the actual business logic of the debug adapter (debugging stuff) from the protocol logic (sequence numbers, filling out fields like command and request etc.).

I used the example server you provided in this repo and started from there. My code is basically split in two parts.

• A handler, which implements an Interface containing a method for each request-type, that receives the arguments for the request and returns a response-body or an error.

• A session, which reads requests, figures out what kind of request it is, calls the correct handler method and passes the arguments. It then receives the response-body (or the error) from the handler-method and (this is the part specific to this issue) fills in the protocol-specific fields (like seq, command, event etc.).

The part "filling in protocol specific fields" for arbitrary responses with data obtained from arbitrary requests is shorter, easier and less error prone when there are interfaces for the different message-types that allow access to the protocol specific fields.

Here is what I am currently doing, using a fork of this repo that already has these interfaces: https://github.com/dbaumgarten/yodk/blob/master/pkg/debug/session.go#L413

While these interfaces are not strictly "necessary" to do anything (if you have reflection and type-switches), I do think they make things easier, while breaking nothing and harming no one.

[polinasok]

This topic has come up in our DAP-in-delve development as well. We needed generic helpers that would operate on any request and access only common Request fields. Instead of passing in the specific request, we had to use request.Request instead (see here). This worked, but did make me miss OO languages with inheritance for cosmetic/abstraction reasons as I would have preferred to just pass request to each helper call.

Then in the default case of our giant request-switch, we did not have access to the decoded type and could not get to the fields needed to populate an ErrorResponse in that case, so we ended up adding GetSeq() method to the message interface and having all Requests, Responses and Events implement it before fixing the TODO - see here.

Since this is a rare corner case that could be hit if the server handles something other than a familiar request (including a non-request message), we kept things very simple and only connected the ErrorResponse to the seq number of the originating message. But strictly speaking this code should be checking if the received message is a request (by checking the 'type' field) and if it is, then also copying out 'command' into the ErrorResponse. This was not possible to do without additional functionality like the one proposed in this issue, but also did not seem critical given the rare nature of this.

From @dbaumgarten's code, it looks like the goal is to avoid code duplication like having the following identical code in every single handler in our mock server: response.Response = *newResponse(request.Seq, request.Command). I remember having to do quite a bit of code massaging to reduce such duplicate lines to a minimum, but unable to completely eliminate them without more interfaces. However, do note that you might not be able to factor this out cleanly even with the new interfaces because sometimes additional logic needs to occur after the response is sent out - see here.

Also, do take a look at the WIP implementation of another DAP server in go here: https://github.com/go-delve/delve/blob/master/service/dap/server.go

[eliben]

@dbaumgarten we're trying to figure out how to address this in the best possible way. I'm wondering about the example you provided in https://github.com/dbaumgarten/yodk/blob/master/pkg/debug/session.go#L413

Before you go into the type-switch for dispatching on the request type, could you create a Response and populate it, and later assign it when you create the specific response types, e.g.:

	case *dap.BreakpointLocationsRequest:
		body, err := ds.handler.OnBreakpointLocationsRequest(&r.Arguments)
		resperr = err
		if body != nil {
			response = &dap.BreakpointLocationsResponse{
                                Response: baseResponse,
				Body: *body,
			}
		}

The real question is what would baseResponse be, since it needs to extract the command and sequence from the request, right?

So perhaps the real underlying issue is that dispatchRequest is taking a dap.Message in the first place. Suppose we had RequestMessage per your suggestion, and dispatchRequest would take a RequestMessage, would this - in addition to the baseResponse assignment mentioned above - help resolve your issue?

Note that while I'm proposing that RequestMessage is sufficient, I'm not saying we should add only that and not the other interface. I'm just wondering what limitation led you to do the pointer assignment after creating the response. As a matter of preference, I like seeing a struct created in a single place with a literal rather than being back-patched later.

[dbaumgarten]

You are absolutely correct. It would be perfectly possible to set the Response-field in every single case. But that would add one line per case to an already gigantic switch-statement (and in case of responses without a body it would add two lines). That's a large amount of code-duplication and I was looking for a cleaner solution.

What you suggest could even be done without any new interfaces. Inside the case-blocks you have access to the Request-field of the incoming request and can therefore access Command and Seq. But that would also add large amounts of duplicate code. Also, if you still need Command and Seq outside a case-block (like here ) you would need two additional lines per case.

In the end, the new interfaces would mainly reduce code-duplication and allow for cleaner (at least from my point of view) and shorter code.

[eliben]

Sorry for the delay. After some deliberation, we agree that the benefits outweigh the concerns here. Feel free to rebase your PR (and use a similar approach to #43 to avoid too much boilerplate) and ping it for a new review when ready.

[eliben]

Closed by #42

Thanks!