implement HTTP target capability and connector handler #14491
Conversation
jinhoonbang
requested review from
vyzaldysanchez,
DavidOrchard,
bolekk,
cedric-cordenier and
justinkaseman
and removed request for
a team and
vyzaldysanchez
| return capabilities.CapabilityResponse{}, err | ||
| } | ||
|
|
||
| // TODO: check target response format and fields |
There was a problem hiding this comment.
When/how will this TODO get resolved?
There was a problem hiding this comment.
I propose this struct. Thoughts?
type CapabilityResponsePayload struct {
Success bool
ErrorMessage string
StatusCode uint8
Headers map[string]string
Body string
}
| @@ -0,0 +1,168 @@ | |||
| package target | |||
There was a problem hiding this comment.
I remember seeing a discussion about naming and file structure, but I find capabilities/webapi/target/capability.go file exporting package target and tested by target_test.go to be a bit confusing. I'd prefer the filename to be target.go, even though that's redundant. But maybe there's general guidance against duplicating folder and filename.
There was a problem hiding this comment.
Will make this change.
| @@ -15,6 +15,7 @@ import ( | |||
| const ( | |||
| FunctionsHandlerType HandlerType = "functions" | |||
| DummyHandlerType HandlerType = "dummy" | |||
| WebCapabilitiesType HandlerType = "web-capabilities" | |||
There was a problem hiding this comment.
how about webapi-capabilities for consistency?
| if len(spec.StandardCapabilitiesSpec.Config) == 0 { | ||
| return nil, errors.New("config is empty") | ||
| } | ||
| // TODO: check if TOML is the only supported format here |
There was a problem hiding this comment.
How does this TODO get resolved?
| @@ -0,0 +1,6 @@ | |||
| package webcapabilities | |||
There was a problem hiding this comment.
I suggest webapicapabilities for package
| require.NoError(t, err) | ||
| } | ||
|
|
||
| func TestCapability_Execute(t *testing.T) { |
There was a problem hiding this comment.
what about invalid workflow id, workflow execution id, and other error scenarios?
There was a problem hiding this comment.
Will add those scenarios
| c.registeredWorkflowsMu.Lock() | ||
| defer c.registeredWorkflowsMu.Unlock() |
There was a problem hiding this comment.
is it necessary to hold the lock for the duration of this method or can it be released after reading the map?
There was a problem hiding this comment.
yep can release it after reading the map although i imagine contention on this lock is very low
| registry core.CapabilitiesRegistry | ||
| config Config | ||
| registeredWorkflows map[string]struct{} | ||
| registeredWorkflowsMu sync.Mutex |
| gatewayIDs := c.gc.GatewayIDs() | ||
| sort.Strings(gatewayIDs) | ||
|
|
||
| err := c.gc.SignAndSendToGateway(ctx, gatewayIDs[0], body) |
There was a problem hiding this comment.
is gatewayIDs guaranteed to have length?
There was a problem hiding this comment.
sounds like a fatal set up error if it doesn't. But I wasn't able to find such validation. I can handle it more gracefully here.
| l.Errorw("no response channel found") | ||
| return | ||
| } | ||
| ch <- msg |
There was a problem hiding this comment.
allow bypass of message send if ctx cancels even if channel is buffered
| func TestCapability_Execute(t *testing.T) { | ||
| th := setup(t) | ||
| ctx := testutils.Context(t) | ||
| th.connector.On("DonID").Return("donID") |
There was a problem hiding this comment.
use .EXPECT() for type safety
| type ConnectorHandler struct { | ||
| gc connector.GatewayConnector | ||
| lggr logger.Logger | ||
| responseChs map[string]chan *api.Message |
There was a problem hiding this comment.
how big do we expect this map to get? creates a new entry for every request, we probably need a cleanup after each read
There was a problem hiding this comment.
makes sense. will make that change
Co-authored-by: Street <[email protected]>
|
SingleNodemode, which forwards HTTP calls via the first available gateway. This is done through a newSignAndSendToGateway()method in the gateway connector that selects an available gateway and signs the request before sending it to the gatewaycapability.Execute()JIRA: https://smartcontract-it.atlassian.net/browse/CM-469
Doc: https://docs.google.com/document/d/19PvTwosdwqNztwP6vcXSdt_s81xZXpjKU-5bpwhsd2M/edit#heading=h.4e7ng0tekbkc