pimeys / rust-web-push

A Web Push library for Rust
Apache License 2.0
114 stars 21 forks source link

Add support to create a VapidSignatureBuilder from contents of a PEM file #46

Closed ranile closed 1 year ago

ranile commented 1 year ago

At the moment VapidSignatureBuilder::from_pem takes in and impl Read and copies it's contents to a Vec<u8>. This is not ideal because it requires a Clone of a Vec<u8> at best and a OS file read syscall at worst. That's not ideal when the a &[u8] can be passed. This allows the private key can be read beforehand and the allocation can be reused.

I suggest:

I would be happy to PR this change

andyblarblar commented 1 year ago

PRs are always welcome! I wouldn't want to change from_pem as is though, as that would be a breaking change effecting pretty much every consumer for no reason. Instead, I would suggest adding a from_pem_ref or similar for a borrowed equivelent.

Honestly I don't think the clone here is a very big deal, since I intented reusable builders to use PartialVapidKeyBuilder anyhow to solve this same pattern. At the end of the day, the key will need to be copied for use in the final message, so all this addition would do is remove that inital clone. I'm not against this addition though because it would increse flexibility in the creation of the inital builder, if the user had a key in bytes for some reason. Just keep in mind not to expect significant improvements from this.

ranile commented 1 year ago

It's not just the clone; ergonomics are improved too. Vec doesn't implement Read so the value needs to be wrapped in a Cursor, and then passed to the function, which then copies it into its own Vec.

I agree that from_pem would be a breaking change and should be avoided. A new function like from_pkey or something would be better.

I hadn't looked at the partial builder, maybe that solves the issue. It's good to have that function regardless though

andyblarblar commented 1 year ago

It's not just the clone; ergonomics are improved too. Vec doesn't implement Read so the value needs to be wrapped in a Cursor, and then passed to the function, which then copies it into its own Vec.

I agree that from_pem would be a breaking change and should be avoided. A new function like from_pkey or something would be better.

I hadn't looked at the partial builder, maybe that solves the issue. It's good to have that function regardless though

&[u8] impls Read, so I'm not sure how valid this concern is.

ranile commented 1 year ago

&[u8] impls Read, so I'm not sure how valid this concern is.

Oh, right. I looked at Vec but didn't see the slice impls