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

feat: support bindary buffer that has been JSON.parse(JSON.strinified()) #2960

Merged
merged 1 commit into from
Aug 27, 2023
Merged

feat: support bindary buffer that has been JSON.parse(JSON.strinified()) #2960

merged 1 commit into from
Aug 27, 2023

Conversation

kassah
Copy link
Contributor

@kassah kassah commented Jun 28, 2023

What?
This adds to binary type handler the ability to utilize node.JS's built in Buffer JSON encoding and appropriate tests for the functionality.

What issues does this address?
#2954

What can be improved?

  • Validate the numeric data array to ensure numbers in the data array were between 0 and 255. I don't think Joi can be called within Joi, and so unsure of validating this without quite a bit of code. Hoping maintainers have a better idea of how this can be done without adding significant cognitive load to this change.

Why?
As a part of a larger request, it is helpful to be able to pass a binary value over a larger JSON model. I ran into this recently when passing request handlers over an HTTP call initiating a browser session that then had an array of request replacements. It worked great for JS and CSS, but small images would fail to decode properly.

Available workarounds?
This can also be achieved via doing the following in with the schema:

Joi.alternatives(
   Joi.binary(),
   Joi.object({
      type: 'Buffer',
      data: Joi.array().items(Joi.number().min(0).max(255))
   }).and('type','data').custom(Buffer.from)
)

Why should this be apart of the core framework?
I believe that the inclusion of allowing this to be coerced via Buffer.from() indicates that node.js meant for this data type to be both encoded and decoded. I feel like this change follows the "least surprise" API: If a buffer snuck into a data structure that was passed to a JSON encoder (like when posting an HTTP call), then parsed by a HTTP reciever's middleware, then the least surprising outcome would be to have it be a Buffer in the end. I believe that if projects were to add the workaround to their projects, it would add to the cognitive load of their projects.

Arguments against this being apart of Joi?
Argument: @Marsup brought up the inefficiency of encoding binary in JSON.
Response: While I agree it is inefficient overall, but I pose the purpose of JSON was never extreme efficiency. It's strength is in clarity and easy of reading. I agree that there are other formats that are better for binary transfer, but I think preventing this based on efficiency argument forces premature optimization of software.

Conclusion:
I appreciate your consideration of this PR. If you don't want to commit to adding this to the project, please indicate such and decline this PR, so that downstream projects can act upon that information. Thank you for the awesome tool!

@Marsup Marsup self-assigned this Aug 27, 2023
@Marsup Marsup added the feature New functionality or improvement label Aug 27, 2023
@Marsup Marsup added this to the 17.10.0 milestone Aug 27, 2023
@Marsup
Copy link
Collaborator

Marsup commented Aug 27, 2023

While I may not agree with the practice, people will do it anyway, so I'm going to bet on individual responsibility. I'll just modify your implementation slightly in a further commit.

About your comment on validating the array, Buffer.from seems to be pretty permissive on what it allows, as long as I'm not exposing node to a crash while doing it, I think it's OK not to check the array.

Thanks for the PR!

@Marsup Marsup merged commit dee2978 into hapijs:master Aug 27, 2023
Marsup added a commit that referenced this pull request Aug 27, 2023
Marsup added a commit that referenced this pull request Aug 27, 2023
@kassah kassah deleted the feature/handleJSONdecodedBuffer branch August 28, 2023 17:34
@kassah
Copy link
Contributor Author

kassah commented Aug 28, 2023

Thank you for accepting the feature and adding those tests.

@Nargonath Nargonath mentioned this pull request Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants