Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 95 additions & 20 deletions src/implementation/Client/GRPCClient/binding.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,47 @@ limitations under the License.
*/

import GRPCClient from './GRPCClient';
import { InvokeBindingRequest, InvokeBindingResponse } from '../../../proto/dapr/proto/runtime/v1/dapr_pb';
import {
InvokeBindingRequest as AutogeneratedInvokeBindingRequest,
InvokeBindingResponse as AutogeneratedInvokeBindingResponse,
} from '../../../proto/dapr/proto/runtime/v1/dapr_pb';
import IClientBinding from '../../../interfaces/Client/IClientBinding';
import { types } from 'util';
import { InvokeBindingResponse } from '../../../types/binding/InvokeBindingResponse.type';

export class BindingRequest {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to export BindingRequest and BindingResponse?

Can we use parameter properties instead of members? Class definition will be cleaner.

Copy link
Member

@shubham1172 shubham1172 Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, do we need these classes at all? send function can directly call makeRequest (instead of going via invoke), and BindingRequest or BindingResponse won't be needed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for this is that we do not want to make destructive changes to the send function. The long-term plan is to add invoke(req: BindingRequest): Promise<BindingResponse> and invoke(name: string, operation: string, data: any, metadata: Record<string, string>): Promise<BindingResponse>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you talking about changing the API name from client.binding.send to client.binding.invoke? https://docs.dapr.io/reference/api/bindings_api/#invoking-output-bindings we technically need only one method in our IClientBinding, which we have as send today.

Do we really need the BindingRequest class? Isn't it simpler to use the params name, operation, data, and metadata directly? It makes sense from a consumer point of view and eliminates the need to pack and unpack the data. The class BindingRequest itself provides no value (it's more like a struct).

Similar argument for BindingResponse. Since we already have InvokeBindingResponse, do we need to introduce yet another class here?

/cc @XavierGeerinck

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, deprecating send in favor of invoke is not something we want to do at this moment.

Regarding classes, I personally also would refrain from adding extra abstraction classes. The gRPC Classes build from dapr/dapr already provide a DTO, this would just add another DTO on top.

I can however see where this is coming from as the Java SDK does it through this way.

public readonly bindingName: string;
public readonly operation: string;
public data?: any;
public metadata: Record<string, string>;

constructor(bindingName: string, operation: string) {
this.bindingName = bindingName;
this.operation = operation;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove extra line

this.metadata = {};
}

public setData(data: any) {
this.data = data;
}

public setMetadata(metadata: Record<string, string>) {
this.metadata = metadata;
}
}

export class BindingResponse implements InvokeBindingResponse {
public readonly operation: string;
public readonly data: Uint8Array | string;
public readonly metadata: Record<string, string>;

constructor(operation: string, data: Uint8Array | string, metadata: Record<string, string>) {
this.operation = operation;
this.data = data;
this.metadata = metadata;
}
}

// https://docs.dapr.io/reference/api/bindings_api/
export default class GRPCClientBinding implements IClientBinding {
Expand All @@ -23,29 +62,65 @@ export default class GRPCClientBinding implements IClientBinding {
this.client = client;
}

// Send an event to an external system
// @todo: should pass the metadata object
// @todo: should return a specific typed Promise<TypeBindingResponse> instead of Promise<object>
async send(bindingName: string, operation: string, data: any, _metadata: object = {}): Promise<object> {
const msgService = new InvokeBindingRequest();
msgService.setName(bindingName);
msgService.setOperation(operation);
msgService.setData(Buffer.from(JSON.stringify(data), "utf-8"));
private makeRequest(
name: string,
operation: string,
data: any,
metadata: Record<string, string>,
): Promise<AutogeneratedInvokeBindingResponse> {
const envelope = new AutogeneratedInvokeBindingRequest();
envelope.setName(name);
envelope.setOperation(operation);
envelope.setData(data);
Object.keys(metadata).forEach((key) => {
envelope.getMetadataMap().set(key, metadata[key]);
});

return new Promise((resolve, reject) => {
const client = this.client.getClient();
client.invokeBinding(msgService, (err, res: InvokeBindingResponse) => {
if (err) {
return reject(err);
}

// https://docs.dapr.io/reference/api/bindings_api/#payload
return resolve({
data: res.getData(),
metadata: res.getMetadataMap(),
operation
});
client.invokeBinding(envelope, (err, res) => {
if (err) return reject(err);
return resolve(res);
});
});
}

private keyValuePairList2Record(kvpArray: Array<[string, string]>): Record<string, string> {
const record: Record<string, string> = {};
for (const kvp of kvpArray) {
if (kvp.length !== 2) {
throw new Error('Invalid key-value pair');
}
record[kvp[0]] = kvp[1];
}
return record;
}

async invoke(request: BindingRequest): Promise<BindingResponse> {
// Sometimes we need to upload files to the object storage through binding, do not serialize binary data.
let payload = request.data;
if (!types.isTypedArray(request.data)) {
payload = Buffer.from(JSON.stringify(request.data), 'utf-8');
}

const response = await this.makeRequest(request.bindingName, request.operation, payload, request.metadata);
return new BindingResponse(
request.operation,
response.getData(),
this.keyValuePairList2Record(response.getMetadataMap().getEntryList()),
);
}

// Send an event to an external system
async send(
bindingName: string,
operation: string,
data: any,
metadata: Record<string, string>,
): Promise<BindingResponse> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be this?

Suggested change
): Promise<BindingResponse> {
): Promise<InvokeBindingResponse> {

const request = new BindingRequest(bindingName, operation);
request.setData(data);
request.setMetadata(metadata);
return await this.invoke(request);
}
}
11 changes: 8 additions & 3 deletions src/implementation/Client/HTTPClient/binding.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ limitations under the License.

import HTTPClient from './HTTPClient';
import IClientBinding from '../../../interfaces/Client/IClientBinding';
import { InvokeBindingResponse } from '../../../types/binding/InvokeBindingResponse.type';

// https://docs.dapr.io/reference/api/bindings_api/
export default class HTTPClientBinding implements IClientBinding {
Expand All @@ -23,7 +24,7 @@ export default class HTTPClientBinding implements IClientBinding {
}

// Send an event to an external system
async send(bindingName: string, operation: string, data: any, metadata: object = {}): Promise<object> {
async send(bindingName: string, operation: string, data: any, metadata: object = {}): Promise<InvokeBindingResponse> {
const result = await this.client.execute(`/bindings/${bindingName}`, {
method: 'POST',
headers: {
Expand All @@ -32,10 +33,14 @@ export default class HTTPClientBinding implements IClientBinding {
body: JSON.stringify({
operation,
data,
metadata
metadata,
}),
});

return result as object;
return {
data: result,
operation,
metadata: {},
};
}
}
4 changes: 3 additions & 1 deletion src/interfaces/Client/IClientBinding.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import { InvokeBindingResponse } from "../../types/binding/InvokeBindingResponse.type";

export default interface IClientBinding {
send(bindingName: string, operation: string, data: any, metadata?: object): Promise<object>;
send(bindingName: string, operation: string, data: any, metadata?: object): Promise<InvokeBindingResponse>;
}
18 changes: 18 additions & 0 deletions src/types/binding/InvokeBindingResponse.type.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/*
Copyright 2022 The Dapr Authors
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

export type InvokeBindingResponse = {
data: any;
operation: string;
metadata: Record<string, string>;
}