sdk-javascript icon indicating copy to clipboard operation
sdk-javascript copied to clipboard

Return type definition of Kafka.structured seems incorrect

Open evanshortiss opened this issue 2 years ago • 8 comments

Describe the Bug

The types for return values from Kafka.structured(cloudEventInstance) seem to be incorrect. The returned object has the following structure if viewed using logs or a debugger:

{
  key: 'fubar',
  value: '{"id":"ef233aa0-00a0-4b5a-9886-f8ff7566ef50","time":"2022-04-14T20:26:32.291Z","type":"foo","source":"bar","specversion":"1.0","partitionkey":"fubar","data":{"hello":"world}}',
  headers: { 'content-type': 'application/cloudevents+json; charset=utf-8' },
  body: '{"id":"ef233aa0-00a0-4b5a-9886-f8ff7566ef50","time":"2022-04-14T20:26:32.291Z","type":"foo","source":"bar","specversion":"1.0","partitionkey":"fubar","data":{"hello":"world}}',
  timestamp: '1649967992291'
}

However, the types come from CE.Message<string>. This results in the following intellisense structure being suggested:

{
  body: unknown,
  headers: CE.Headers
}

It seems as though the headers key is correct, but the body is incorrect, and other keys are missing.

Steps to Reproduce

  1. Create a CloudEvent, e.g new ce.CloudEvent({ type: 'foo', source: 'bar', data: { hello: 'world' }, partitionkey: 'foobar' }
  2. Pass it to, e.g Kafka.structured(theEvent)
  3. Attempt to access the timestamp, value, or key. These are not known properties on the return value.
  4. Try to use the body and you find it is incorrectly typed as unknown

Expected Behavior

I would have expected the types to reflect the Object in the Describe the bug section, e.g:

{
  key: string|undefined
  value: string,
  headers: CE.Headers,
  body: string,
  timestamp: string
}

Is this an invalid assumption to make? Passing the result of Kafka.structured(cloudEventInstance) object to a Kafka client such as KafkaJS requires casting the object to access the necessary properties to satisfy the TS compiler. It seems like this casting should be unnecessary.

Additional context

I'm using version 6.0.1 of the cloudevents module. The image below might help provide some context.

Screenshot 2022-04-14 at 1 45 23 PM

evanshortiss avatar Apr 14 '22 20:04 evanshortiss

After looking a little more at this today, it seems as though the Binding might need to support a generic(s) to pass the necessary structure(s) down to the Serializer? Currently the more specific structures are lost, and the type end up using the plain Message instead of the more specific KafkaMessage.

evanshortiss avatar Apr 18 '22 19:04 evanshortiss

Yep I think I'm seeing the same issue, but in the reverse.

When consuming a Kafka message, I want to convert it back into an abstract CloudEvent. I can use Kafka.toEvent which takes the general Message type. The Message interface can be seen below.

export interface Message<T = string> {
    headers: Headers;
    body: T | string | Buffer | unknown;
}

This is a bit confusing because Kafka.toEvent is bound to deserializeKafkaMessage which checks for a value property on the message parameter. But we need to pass in a Message which doesn't have this property! Same goes for the key.

This means in order to actually generate a valid CloudEvent from the incoming Kafka message I'm forced to cast the argument. Without casting an Error is raised.

const myCloudEvent: CloudEventV1<string> = Kafka.toEvent({
  headers,
  key,
  value,
  body,
} as Message);

Happy to help out if there is consensus this could be improved, I just can't tell if it's me misinterpreting how the SDK was designed or not.

FelixHarvey avatar Apr 29 '22 11:04 FelixHarvey

@FelixHarvey @evanshortiss thank you for your detailed descriptions of the issues you are seeing with the SDK - and I would welcome any contributions you have towards improvement. The interfaces are a bit clumsy.

The Binding interface is what is causing all of this trouble, I think. https://github.com/cloudevents/sdk-javascript/blob/d9ee0e05d1b38c84314d3e0535b17c25470d95a5/src/message/index.ts#L25-L30

I want this to be generic so that various protocols such as HTTP and Kafka, which have differing implementations can all behave similarly when serializing and deserializing CloudEvent messages. The Serializer and Deserializer interfaces that Binding exposes are meant to be the generic API. https://github.com/cloudevents/sdk-javascript/blob/d9ee0e05d1b38c84314d3e0535b17c25470d95a5/src/message/index.ts#L63-L79

However, the result is what you are seeing now. There is some amount of type casting that is inevitable. If you have ideas or suggestions to improve this situation, I'd be more than happy to talk about them.

lance avatar May 10 '22 16:05 lance

This issue is stale because it has been open 30 days with no activity.

github-actions[bot] avatar Jun 10 '22 00:06 github-actions[bot]

This issue is stale because it has been open 30 days with no activity.

github-actions[bot] avatar Jul 14 '22 00:07 github-actions[bot]

This issue is stale because it has been open 30 days with no activity.

github-actions[bot] avatar Aug 14 '22 00:08 github-actions[bot]

I am facing the same issue. I implemented a workaround by casting to any and then to Message.

import { Message } from 'kafkajs';
import { Kafka as CloudEventsKafka } from 'cloudevents';

const message = CloudEventsKafka.structured(cloudEvent) as any as Message;

raman-nbg avatar Jan 29 '23 18:01 raman-nbg

This issue is stale because it has been open 30 days with no activity.

github-actions[bot] avatar Mar 01 '23 00:03 github-actions[bot]