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

[Protobuf] Optional properties not removed #848

Closed
lewispham opened this issue Oct 18, 2023 · 7 comments
Closed

[Protobuf] Optional properties not removed #848

lewispham opened this issue Oct 18, 2023 · 7 comments
Assignees
Labels
good first issue Good for newcomers invalid This doesn't seem right question Further information is requested

Comments

@lewispham
Copy link

Bug Report

It looks like typia is unexpectedly include optional property with value undefined after decoding with protobuf. Technically, it's considered a valid behavior by typescript but I think as a protobuf parser then typia should remove those properties instead.

📝 Summary

Write a short summary of the bug in here.

  • Typia Version: 5.2.2dev(@next)
  • Expected behavior: No property after decoding
  • Actual behavior: included undefined properties

Write detailed description in here.

// Expected
{}

// Actual
{
  a: undefined,
  b: undefined
}

⏯ Playground Link

https://typia.io/playground/?script=JYWwDg9gTgLgBDAnmYBDOAzKERwERIqp4DcAUGcAHYwCmUGqAxrXAAqpSogDOcA3mThxUALjg8YUagHNywgEbiqAVxAL65AL5lCrAPJgYwCFVQAbDl15wAvO07GLAHivceAPnJkmpyXFoqXwATWmC3G3tCNAA6MGwYCAUVDBjAkNpnQ2NTCwjPAAp+LQBKcl8qfzBOdzsEZFj4iETk1NCMrKMTM0sa3g8C9IhQ8L6eMp8-CHNaGPMIGQLq63GSIA

💻 Code occuring the bug

import typia from "typia";

interface Params {
  a: string;
  b: number;
}
type OptionalParams = Partial<Params>;

const encodedParams = typia.protobuf.encode<OptionalParams>({});
const params = typia.protobuf.decode<OptionalParams>(encodedParams);
console.log(params);
@samchon samchon self-assigned this Oct 27, 2023
@samchon samchon added the question Further information is requested label Oct 27, 2023
@samchon
Copy link
Owner

samchon commented Oct 27, 2023

It is possible to remove the optional property, and it is not hard for me.

However, in that case, it makes the decoded object to be dynamic object, so that it cannot take advantage of hidden class optimization. Considering the purpose of protocol buffer for performance boosting, I think such dynamic object construction is not suitable for protocol buffer functions.

How do you think about it?

image

Dynamic key deletion

image

Dynamic key insertion

@samchon samchon added good first issue Good for newcomers invalid This doesn't seem right labels Oct 27, 2023
@lewispham
Copy link
Author

@samchon The problem with this issue is that it unnecessarily increases the buffer size. And I think protobuf function should aim for networking performance over runtime performance. This issue obviously won't break the code but it's better to produce the smallest possible buffer for maximizing transferring speeds, while still guaranteeing type-safety. Normally, using JSON will require compressions as an extra step for minimizing data size. This is the data lifecycle when using JSON format.

Receiving data -> Input buffer -> decompression -> JSON decode -> Input validation -> processing data -> Output validation -> JSON encode -> compression -> Output buffer -> Transferring data

This is the developer's expected lifecycle.

Receiving data -> Input buffer -> protobuf decode -> processing data -> protobuf encode -> Output buffer -> Transferring data

So even a novice developer can see that there will be a massive performance boost when applying the second approach.

Most developers know that it's very bad to use JSON instead of protobuf in terms of performance. But most protobuf libraries require defining an extra static schema for each message, which is extremely incovenient. typia is actually the developer's dream when it comes to handling this problem. I personally think that typescript should natively support type-casting and validation as well as providing runtime type metadata via plugins. But it seems that it won't happen in near future.

Back to this issue, I think you should check for the existence of the undefined properties in input object before deleting it. Checking a === undefined will fail if the property is defined like {a: undefined}.

// input
{a: undefined, b: 1}

// expected output
{a: undefined, b: 1}

//input 
{b: 1}

// expected output
{b: 1}

So the the code should be like this.

if (!Object.getOwnPropertyDescriptor(input, "a")) {
  delete output.a;
}

@samchon
Copy link
Owner

samchon commented Oct 28, 2023

If I adapt the second way of above picture, the dynamic property assignment, your goal would be accomplished.

By the way, before determining whether accept your suggestion or not, I can't understand your word "increase the buffer size". In the Protocol Buffer spec, if do not insert anything to the buffer about a specific property, it means null or undefined about the specific property. Pre-assigning undefined value does not increase the buffer size of Protocol Buffer.

@lewispham
Copy link
Author

I've just double-check and typia indeed removes keys with null or undefined in the encoded buffer. So the buffer size is irrelevant. If that's the case then I think checking those properties directly with if(prop === undefined || prop === null) {delete output.prop} is good enough.

I personally think that removing undefined keys makes sense but the null value. Sometimes we will need null properties for debugging purposes. But if it's in the spec then it should be fine.

@samchon
Copy link
Owner

samchon commented Oct 31, 2023

I will provide an option that turning on and off the delete statement at Saturday.

@samchon
Copy link
Owner

samchon commented Nov 16, 2023

This feature would be supported after TypeScript v5.3 release with ts-patch supporting

@samchon
Copy link
Owner

samchon commented Nov 19, 2023

If you want to use it earlier, then install like below:

npm install typia@next

samchon added a commit that referenced this issue Nov 19, 2023
Close #848 - `exactOptionalPropertyTypes` option for `typia.protobuf.decode<T>()` function.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers invalid This doesn't seem right question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants