Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Binary data in binary format is not binary? #343

Closed
evankanderson opened this issue Sep 28, 2020 · 6 comments · Fixed by #344
Closed

Binary data in binary format is not binary? #343

evankanderson opened this issue Sep 28, 2020 · 6 comments · Fixed by #344
Assignees
Labels
module/lib Related to the main source code module/transport/http Issues related to the HTTP transport protocol implementation type/bug Something isn't working

Comments

@evankanderson
Copy link

Describe the Bug
I'm trying to send a cloudevent with datacontenttype of image/jpeg and the actual payload being the bytes of the image. It seems like the current code in asData will always base-64 the content when fetching data. (Possibly twice, if I read the code at line 12 of http/index.js correctly -- once for event.data, and once for asData.)

Steps to Reproduce
I'd like the following code to work, but it appears that ArrayBuffer and Blob are not supported types for data:

  fetch(mediaUrl).then(async (resp) => {
    let newEvent = event.cloneWith({
      source: sourceUrl,
      type: responseEventType,
      datacontenttype: resp.headers.get("Content-Type")
    })
    // I want this. This definitely does not work, and an empty object gets encoded.
    newEvent.data = await resp.arrayBuffer();  // or .blob()
    const response = HTTP.binary(newEvent);
    res.status(200).set(response.headers).send(response.body);
  });

If I try to convert the arrayBuffer to a Uint32Array (newEvent.data = new Uint32Array(await resp.arrayBuffer());), I get the following exception:

TypeError: Cannot assign to read only property 'data_base64' of object '[object Object]'

Expected Behavior
I'd like the above code to work, or to be able to find an example of sending a binary file (i.e. an image or a proto or something) as an HTTP CloudEvent.

Additional context
I am terrible at Javascript, so feel free to point out I'm holding it wrong.

@lance
Copy link
Member

lance commented Sep 28, 2020

Hi @evankanderson can you please try the data assignment in the cloneWith() method?

let newEvent = event.cloneWith({
      source: sourceUrl,
      type: responseEventType,
      datacontenttype: resp.headers.get("Content-Type"),
      data: new Uint32Array(await resp.arrayBuffer())
    });

This will get around the assignment errors. But you are correct that when serializing an event, it will be encoded as base64. This is based on a reading of the JSON format spec.

If the implementation determines that the type of data is Binary, the value MUST be represented as a JSON string expression containing the Base64 encoded binary value, and use the member name data_base64 to store it inside the JSON object.

The assumption made in this module is that when serializing for HTTP, the event is converted into a JSON representation, and that data should be encoded as base64. But your use case clearly shows that this should not happen.

@lance lance self-assigned this Sep 28, 2020
@lance lance added module/lib Related to the main source code module/transport/http Issues related to the HTTP transport protocol implementation type/bug Something isn't working version/4.x Issues related to the 4.0 release of this library and removed version/4.x Issues related to the 4.0 release of this library labels Sep 28, 2020
@evankanderson
Copy link
Author

(I'm trying to avoid the overheads of base64 in my image pipeline, and my next step was expecting an image in the POST body.)

Thanks for the suggestion on assigning inside cloneWith, that seems to work. In general, it would be nice if I could assign a ReadableStream to data for the binary case, but I realze that's probably a larger feature request.

@lance
Copy link
Member

lance commented Sep 28, 2020

Thanks for the suggestion on assigning inside cloneWith, that seems to work.

Sure thing. The idea behind cloneWith is that an event should be immutable. So that's why you were getting that particular error.

it would be nice if I could assign a ReadableStream to data for the binary case, but I realze that's probably a larger feature request.

It is. But I should have a fix soon so that the data attribute is unmodified.

lance added a commit to lance/sdk-javascript that referenced this issue Sep 28, 2020
When setting an event's data attribute we were trying to be really clever
and this is problematic. Instead, keep the data attribute unchanged. Per
the 1.0 specification, the data attribute is still inspected to determine
if it is binary, and if so, a data_base64 attribute is added with the
contents of the data property encoded as base64.

Fixes: cloudevents#343

Signed-off-by: Lance Ball <lball@redhat.com>
lance added a commit to lance/sdk-javascript that referenced this issue Sep 28, 2020
When setting an event's data attribute we were trying to be really clever
and this is problematic. Instead, keep the data attribute unchanged. Per
the 1.0 specification, the data attribute is still inspected to determine
if it is binary, and if so, a data_base64 attribute is added with the
contents of the data property encoded as base64.

Fixes: cloudevents#343

Signed-off-by: Lance Ball <lball@redhat.com>
@lance
Copy link
Member

lance commented Sep 29, 2020

Hi @evankanderson - I have a fix in this PR #344. If you can, would you mind giving it a try with your use case?

@lance
Copy link
Member

lance commented Sep 29, 2020

it would be nice if I could assign a ReadableStream to data for the binary case

I think you should be able to do that with the changes in #344, but you could not convert it to a Message. You would need to do something like this.

  fetch(mediaUrl).then(async (resp) => {
    const newEvent = new CloudEvent({
      source: sourceUrl,
      type: responseEventType,
      datacontenttype: resp.headers.get("Content-Type"),
      data: resp.body
    })
    // Do whatever you need to do to read newEvent.data as a ReadableStream,
    // eventually setting a variable with the resultant value
    const data = ...
    const response = HTTP.binary(
      newEvent.cloneWith( { data } );
    );
    res.status(200).set(response.headers).send(response.body);
  });

@evankanderson
Copy link
Author

I have a demo that I'm wrapping up, but I'll try figuring out how to patch this in on a branch probably tomorrow morning once I've got the demo recorded. 😁

@lance lance closed this as completed in #344 Oct 6, 2020
lance added a commit that referenced this issue Oct 6, 2020
* fix: do not alter an event's data attribute

When setting an event's data attribute we were trying to be really clever
and this is problematic. Instead, keep the data attribute unchanged. Per
the 1.0 specification, the data attribute is still inspected to determine
if it is binary, and if so, a data_base64 attribute is added with the
contents of the data property encoded as base64.

Fixes: #343

Signed-off-by: Lance Ball <lball@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module/lib Related to the main source code module/transport/http Issues related to the HTTP transport protocol implementation type/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants