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.

Sign up for GitHub

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

[octetsream-codec] fix serialization and deserialization of nested objects #1262

Merged
merged 7 commits into from
Apr 5, 2024
+234 −15
Merged

[octetsream-codec] fix serialization and deserialization of nested objects #1262

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c764b96
fix nested object serialization and deserialization
derwehr Apr 2, 2024
3ad5f54
fix format
derwehr Apr 2, 2024
162a37b
remove unused parameter
derwehr Apr 2, 2024
ac36439
clean up
derwehr Apr 2, 2024
7415c62
fix format
derwehr Apr 2, 2024
d4a3888
add check to ensure `ex:bitOffset` is valid
derwehr Apr 4, 2024
1d86750
add checks for more parameters
derwehr Apr 5, 2024
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
91 changes: 76 additions & 15 deletions packages/core/src/codecs/octetstream-codec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,14 +59,41 @@
debug("OctetstreamCodec parsing", bytes);
debug("Parameters", parameters);

const bigEndian = !(parameters.byteSeq?.includes(Endianness.LITTLE_ENDIAN) === true); // default to big endian
let signed = parameters.signed !== "false"; // default to signed
const length =
parameters.length != null
? parseInt(parameters.length)
: (warn("Missing 'length' parameter necessary for write. I'll do my best"), undefined);

if (length !== undefined) {
if (isNaN(length) || length < 0) {
throw new Error("'length' parameter must be a non-negative number");
}
if (length !== bytes.length) {
throw new Error(`Lengths do not match, required: ${length} provided: ${bytes.length}`);
}

Check warning on line 73 in packages/core/src/codecs/octetstream-codec.ts

View check run for this annotation

Codecov / codecov/patch

packages/core/src/codecs/octetstream-codec.ts#L72-L73 

Added lines #L72 - L73 were not covered by tests
}

let signed = true; // default to signed
if (parameters.signed !== undefined) {
if (parameters.signed !== "true" && parameters.signed !== "false") {
throw new Error("'signed' parameter must be 'true' or 'false'");
}
signed = parameters.signed === "true";
}

let bitLength = schema?.["ex:bitLength"] !== undefined ? parseInt(schema["ex:bitLength"]) : bytes.length * 8;

if (isNaN(bitLength) || bitLength < 0) {
throw new Error("'ex:bitLength' must be a non-negative number");
}

const offset = schema?.["ex:bitOffset"] !== undefined ? parseInt(schema["ex:bitOffset"]) : 0;
if (parameters.length != null && parseInt(parameters.length) !== bytes.length) {
throw new Error("Lengths do not match, required: " + parameters.length + " provided: " + bytes.length);

if (isNaN(offset) || offset < 0) {
throw new Error("'ex:bitOffset' must be a non-negative number");
}
let bitLength: number =
schema?.["ex:bitLength"] !== undefined ? parseInt(schema["ex:bitLength"]) : bytes.length * 8;

const bigEndian = !(parameters.byteSeq?.includes(Endianness.LITTLE_ENDIAN) === true); // default to big endian
let dataType: string = schema?.type;

if (!dataType) {
Expand Down Expand Up @@ -214,24 +241,47 @@
const sortedProperties = Object.getOwnPropertyNames(schema.properties);
for (const propertyName of sortedProperties) {
const propertySchema = schema.properties[propertyName];
result[propertyName] = this.bytesToValue(bytes, propertySchema, parameters);
const length = bytes.length.toString();
result[propertyName] = this.bytesToValue(bytes, propertySchema, { ...parameters, length });
}
return result;
}

valueToBytes(value: unknown, schema?: DataSchema, parameters: { [key: string]: string | undefined } = {}): Buffer {
debug(`OctetstreamCodec serializing '${value}'`);

if (parameters.length == null) {
warn("Missing 'length' parameter necessary for write. I'll do my best");
const bigEndian = !(parameters.byteSeq?.includes(Endianness.LITTLE_ENDIAN) === true); // default to big endian

let signed = true; // default to true

if (parameters.signed !== undefined) {
if (parameters.signed !== "true" && parameters.signed !== "false") {
throw new Error("'signed' parameter must be 'true' or 'false'");
}
signed = parameters.signed === "true";
}

let length =
parameters.length != null
? parseInt(parameters.length)
: (warn("Missing 'length' parameter necessary for write. I'll do my best"), undefined);

if (length !== undefined && (isNaN(length) || length < 0)) {
throw new Error("'length' parameter must be a non-negative number");
}

const bigEndian = !(parameters.byteSeq?.includes(Endianness.LITTLE_ENDIAN) === true); // default to big endian
let signed = parameters.signed !== "false"; // default to signed
// byte length of the buffer to be returned
let length = parameters.length != null ? parseInt(parameters.length) : undefined;
let bitLength = schema?.["ex:bitLength"] !== undefined ? parseInt(schema["ex:bitLength"]) : undefined;

if (bitLength !== undefined && (isNaN(bitLength) || bitLength < 0)) {
throw new Error("'ex:bitLength' must be a non-negative number");
}

const offset = schema?.["ex:bitOffset"] !== undefined ? parseInt(schema["ex:bitOffset"]) : 0;

if (isNaN(offset) || offset < 0) {
throw new Error("'ex:bitOffset' must be a non-negative number");
}

let dataType: string = schema?.type ?? undefined;

if (value === undefined) {
Expand Down Expand Up @@ -542,7 +592,18 @@
throw new Error("Missing 'length' parameter necessary for write");
}

result = result ?? Buffer.alloc(parseInt(parameters.length));
const length = parseInt(parameters.length);
const offset = schema["ex:bitOffset"] !== undefined ? parseInt(schema["ex:bitOffset"]) : 0;
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to to a more sophisticated check.. not only for undefined but also NaN and/or invalid parameters?

Copy link
Contributor Author

@derwehr derwehr Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I added a check in the last commit.

The bytesToValue and valueTobytes functions could probably check most of the DataSchema values and the length parameter more thoroughly as well.

Copy link
Member

Choose a reason for hiding this comment

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

If you want to introduce the checks it would be fine by me.. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added some more checks (and tests making sure the checks work) in my last commit

Copy link
Member

Choose a reason for hiding this comment

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

A "simpler" approach could have been a helper method that does the check (since I think the are almost all the same). Anyhow, fine like this as well 👍


if (isNaN(offset) || offset < 0) {
throw new Error("'ex:bitOffset' must be a non-negative number");
}

Check warning on line 600 in packages/core/src/codecs/octetstream-codec.ts

View check run for this annotation

Codecov / codecov/patch

packages/core/src/codecs/octetstream-codec.ts#L599-L600 

Added lines #L599 - L600 were not covered by tests

if (offset > length * 8) {
throw new Error(`'ex:bitOffset' ${offset} exceeds 'length' ${length}`);
}

Check warning on line 604 in packages/core/src/codecs/octetstream-codec.ts

View check run for this annotation

Codecov / codecov/patch

packages/core/src/codecs/octetstream-codec.ts#L603-L604 

Added lines #L603 - L604 were not covered by tests

result = result ?? Buffer.alloc(length);
for (const propertyName in schema.properties) {
if (Object.hasOwnProperty.call(value, propertyName) === false) {
throw new Error(`Missing property '${propertyName}'`);
Expand All @@ -557,7 +618,7 @@
} else {
buf = this.valueToBytes(propertyValue, propertySchema, parameters);
}
this.copyBits(buf, propertyOffset, result, propertyOffset, propertyLength);
this.copyBits(buf, propertyOffset, result, offset + propertyOffset, propertyLength);
}
return result;
}
Expand Down
158 changes: 158 additions & 0 deletions packages/core/test/ContentSerdesTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,59 @@ class SerdesOctetTests {
},
}
);

checkStreamToValue(
[0x0e, 0x10, 0x10, 0x10, 0x0e],
{
flags1: { flag1: false, flag2: true },
flags2: { flag1: true, flag2: false },
},
"object",
{
type: "object",
properties: {
flags1: {
type: "object",
"ex:bitOffset": 0,
"ex:bitLength": 8,
properties: {
flag1: {
type: "boolean",
title: "Bit 1",
"ex:bitOffset": 3,
"ex:bitLength": 1,
},
flag2: {
type: "boolean",
title: "Bit 2",
"ex:bitOffset": 4,
"ex:bitLength": 1,
},
},
},
flags2: {
type: "object",
"ex:bitOffset": 8,
"ex:bitLength": 8,
properties: {
flag1: {
type: "boolean",
title: "Bit 1",
"ex:bitOffset": 3,
"ex:bitLength": 1,
},
flag2: {
type: "boolean",
title: "Bit 2",
"ex:bitOffset": 4,
"ex:bitLength": 1,
},
},
},
},
},
{ length: "5" }
);
}

@test async "OctetStream to value should throw"() {
Expand Down Expand Up @@ -531,6 +584,55 @@ class SerdesOctetTests {
{ type: "uint8" }
)
).to.throw(Error, "Type is unsigned but 'signed' is true");

expect(() =>
ContentSerdes.contentToValue(
{ type: `application/octet-stream;length=test`, body: Buffer.from([0x36]) },
{ type: "integer" }
)
).to.throw(Error, "'length' parameter must be a non-negative number");

expect(() =>
ContentSerdes.contentToValue(
{ type: `application/octet-stream;length=-1`, body: Buffer.from([0x36]) },
{ type: "integer" }
)
).to.throw(Error, "'length' parameter must be a non-negative number");

expect(() =>
ContentSerdes.contentToValue(
{ type: `application/octet-stream;signed=invalid`, body: Buffer.from([0x36]) },
{ type: "integer" }
)
).to.throw(Error, "'signed' parameter must be 'true' or 'false'");

expect(() =>
ContentSerdes.contentToValue(
{ type: `application/octet-stream`, body: Buffer.from([0x36]) },
{ type: "integer", "ex:bitOffset": "invalid" }
)
).to.throw(Error, "'ex:bitOffset' must be a non-negative number");

expect(() =>
ContentSerdes.contentToValue(
{ type: `application/octet-stream`, body: Buffer.from([0x36]) },
{ type: "integer", "ex:bitOffset": -1 }
)
).to.throw(Error, "'ex:bitOffset' must be a non-negative number");

expect(() =>
ContentSerdes.contentToValue(
{ type: `application/octet-stream`, body: Buffer.from([0x36]) },
{ type: "integer", "ex:bitLength": "invalid" }
)
).to.throw(Error, "'ex:bitLength' must be a non-negative number");

expect(() =>
ContentSerdes.contentToValue(
{ type: `application/octet-stream`, body: Buffer.from([0x36]) },
{ type: "integer", "ex:bitLength": -1 }
)
).to.throw(Error, "'ex:bitLength' must be a non-negative number");
}

@test async "value to OctetStream"() {
Expand Down Expand Up @@ -763,6 +865,39 @@ class SerdesOctetTests {
);
body = await content.toBuffer();
expect(body).to.deep.equal(Buffer.from([0xc0]));

content = ContentSerdes.valueToContent(
{
flags1: { flag1: false, flag2: true },
flags2: { flag1: true, flag2: false },
},
{
type: "object",
properties: {
flags1: {
type: "object",
properties: {
flag1: { type: "boolean", "ex:bitOffset": 3, "ex:bitLength": 1 },
flag2: { type: "boolean", "ex:bitOffset": 4, "ex:bitLength": 1 },
},
"ex:bitLength": 8,
},
flags2: {
type: "object",
properties: {
flag1: { type: "boolean", "ex:bitOffset": 3, "ex:bitLength": 1 },
flag2: { type: "boolean", "ex:bitOffset": 4, "ex:bitLength": 1 },
},
"ex:bitOffset": 8,
"ex:bitLength": 8,
},
},
"ex:bitLength": 16,
},
"application/octet-stream;length=2;"
);
body = await content.toBuffer();
expect(body).to.deep.equal(Buffer.from([0x08, 0x10]));
}

@test "value to OctetStream should throw"() {
Expand Down Expand Up @@ -851,6 +986,29 @@ class SerdesOctetTests {
Error,
"Missing 'type' property in schema"
);
expect(() => ContentSerdes.valueToContent(10, { type: "int8" }, "application/octet-stream;signed=8")).to.throw(
Error,
"'signed' parameter must be 'true' or 'false'"
);
expect(() =>
ContentSerdes.valueToContent(10, { type: "int8" }, "application/octet-stream;length=-1;")
).to.throw(Error, "'length' parameter must be a non-negative number");
expect(() => ContentSerdes.valueToContent(10, { type: "int8" }, "application/octet-stream;length=x;")).to.throw(
Error,
"'length' parameter must be a non-negative number"
);
expect(() =>
ContentSerdes.valueToContent(10, { type: "integer", "ex:bitOffset": -16 }, "application/octet-stream")
).to.throw(Error, "'ex:bitOffset' must be a non-negative number");
expect(() =>
ContentSerdes.valueToContent(10, { type: "integer", "ex:bitOffset": "foo" }, "application/octet-stream")
).to.throw(Error, "'ex:bitOffset' must be a non-negative number");
expect(() =>
ContentSerdes.valueToContent(10, { type: "integer", "ex:bitLength": -8 }, "application/octet-stream")
).to.throw(Error, "'ex:bitLength' must be a non-negative number");
expect(() =>
ContentSerdes.valueToContent(10, { type: "integer", "ex:bitLength": "foo" }, "application/octet-stream")
).to.throw(Error, "'ex:bitLength' must be a non-negative number");
}
}

Expand Down
Loading