Skip to content

Non Uint32Array binary data does not set the CloudEvent's data_base64 value #491

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

Closed
matthewchngshopback opened this issue Jun 2, 2022 · 4 comments · Fixed by #494
Closed

Comments

@matthewchngshopback
Copy link

matthewchngshopback commented Jun 2, 2022

When binary data, e.. Buffer or Uint8Array, is not converted to Uint32Array before being set as a CloudEvent's data, the data_base64 will not be set because isBinary() detection fails. This results in structured mode to go out of spec by not having the expected data_base64 attribute but instead include an unexpected data object.

If we convert the Buffer to Uint32Array and set as CloudEvent's data, then binary mode message body will contain wrong byte sequence as it will be that of Uint32Array rather than the expected Uint8Array. This can be overcome by wrapping the body from HTTP.binary() with Buffer.from(body) again before feeding it axios(). Also, Uint32Array binary data in binary mode does not work with the provided emitterFor() as the req.write() only allows Buffer or Uint8Array.

Steps to Reproduce

const { CloudEvent, HTTP } = require("cloudevents")

const x = Buffer.from([0x01, 0x02, 0x03, 0x4])
const ce = new CloudEvent({ type: 'test', source:'testor', data: x })
const { body } = HTTP.structured(ce)

console.log(`ce: ${ce}`)
// ce: {"id":"084afd51-9dd2-4f0c-8f4f-517189d6ca13","time":"2022-06-02T07:34:30.982Z","type":"test","source":"testor","specversion":"1.0","data":{"type":"Buffer","data":[1,2,3,4]}}

console.log(`structured body: ${body}`)
// structured body: {"id":"084afd51-9dd2-4f0c-8f4f-517189d6ca13","time":"2022-06-02T07:34:30.982Z","type":"test","source":"testor","specversion":"1.0","data":{"type":"Buffer","data":[1,2,3,4]}}

const x32 = new Uint32Array(x)
const ceX32 = new CloudEvent({ type: 'test', source:'testor', data: x32 })
const { body: bodyX32 } = HTTP.structured(ceX32)

console.log(`ceX32: ${ceX32}`)
// ceX32: {"id":"ca9fb24f-adc1-4c8a-82e2-ff0f69b26cbd","time":"2022-06-02T07:38:27.833Z","type":"test","source":"testor","specversion":"1.0","data_base64":"AQIDBA==","data":{"0":1,"1":2,"2":3,"3":4}}

console.log(`structured bodyX32: ${bodyX32}`)
// structured bodyX32: {"id":"60c32fbd-3868-4e7a-bbd0-cf2920440ec7","time":"2022-06-02T07:39:00.382Z","type":"test","source":"testor","specversion":"1.0","data_base64":"AQIDBA=="}

const { body: bodyBinary } = HTTP.binary(ce)
const { body: bodyBinaryX32 } = HTTP.binary(ceX32)

console.log(x)
console.log(bodyBinary)                 // matched
console.log(bodyBinaryX32)              // no match
console.log(Buffer.from(bodyBinaryX32)) // matched

// <Buffer 01 02 03 04>
// <Buffer 01 02 03 04>
// Uint32Array(4) [ 1, 2, 3, 4 ]
// <Buffer 01 02 03 04>

Expected Behavior
We should be able to pass TypedArray directly as data and both the structured and binary modes should create valid bodies that matches the spec.

lance added a commit to lance/sdk-javascript that referenced this issue Jun 14, 2022
Previously we only considered `Uint32Array` binary data. This was an
oversight. This fixes that issue.

Fixes: cloudevents#491

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

lance commented Jun 14, 2022

Hi @matthewchngshopback thanks for opening this issue. I have applied a fix in #494 - can you please verify that it addresses your use case?

lance added a commit that referenced this issue Jun 15, 2022
* fix: allow `Uint16|8Array` for binary data

Previously we only considered `Uint32Array` binary data. This was an
oversight. This fixes that issue.

Fixes: #491

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

Hi @matthewchngshopback thanks for opening this issue. I have applied a fix in #494 - can you please verify that it addresses your use case?

@lance Yes, this fix addresses all the issues described. Thanks very much. What do you think the ETA is for 6.0.2?

@lance
Copy link
Member

lance commented Jun 20, 2022

@matthewchngshopback We can probably bump a release this week.

@lance
Copy link
Member

lance commented Jun 21, 2022

@matthewchngshopback We can probably bump a release this week.

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants