Add Deserialize impls, a new Client constructor, and some QoL features #49

AzureMarker · 2020-07-24T18:20:22Z

Serde's Deserialize is implemented for APS and its sub-structs.
Added Client::certificate_parts for creating a client from the raw certificate and private key data.
Made the properties of LocalizedAlert public (and all fields optional), to make constructing notifications easier.
Implemented Default for some APS structs, to make constructing notifications easier.

This helps when creating a localized APS manually.

This helps when manually building an APS instance.

AzureMarker · 2020-08-10T18:04:09Z

@pimeys Can you look into merging these PRs? We currently have to rely on a fork.

pimeys · 2020-08-10T18:06:25Z

Hey! Sorry for delay, I've been (and still will be) on vacation with only an old openbsd laptop with me. I can try to test and release a new version, but it might be complex...

pimeys · 2020-08-10T18:33:00Z

src/request/notification/localized.rs

 #[serde(rename_all = "kebab-case")]
 pub struct LocalizedAlert<'a> {
-    title: &'a str,


Instead of having these as public fields, we should utilize the builders as we do with notifications. The API to set optional fields directly is never that great, and will pollute the code with None if not needing to set the fields.

Could you add these as builders instead?

If you utilize Default, it is pretty concise:

LocalizedAlert { title_loc_key: Some("SentTab.NoTabArrivingNotification.title"), loc_key: Some("SentTab.NoTabArrivingNotification.body"), ..Default::default() }

The whole point of these structs are to map 1:1 to the JSON that APNS expects. It makes sense to at least make the fields public.

pimeys · 2020-08-10T18:34:34Z

src/request/notification/localized.rs

@@ -84,8 +87,8 @@ impl<'a> LocalizedNotificationBuilder<'a> {
    {
        LocalizedNotificationBuilder {
            alert: LocalizedAlert {
-                title: title,
-                body: body,
+                title: Some(title),


Are these actually optional fields? If so, they should be separate builder methods...

pimeys · 2020-08-10T18:38:58Z

src/client.rs

+    /// Create a connection to APNs using the raw PEM-formatted certificate and
+    /// key, extracted from the provider client certificate you obtain from your
+    /// [Apple developer account](https://developer.apple.com/account/).
+    pub fn certificate_parts(cert: &[u8], key: &[u8], endpoint: Endpoint) -> Result<Client, Error> {


Could the cert and key be impl Read instead, allowing one to load them directly from files? &[u8] should also implement Read.

AlpnConnector::with_client_cert takes in &[u8] so we don't benefit from using impl Read here.

pimeys · 2020-08-12T07:34:49Z

So... I'll be back to Berlin to my work computer on 26.8. I don't have a good enough computer to review and test this. Could some other long-time a2 user like @dbrgn maybe see if this makes sense?

AzureMarker · 2020-09-26T02:26:06Z

@pimeys Are you still interested in these changes?

jrconlin · 2020-12-07T22:36:24Z

Is this PR blocked? I see that there are changes requested, are those still valid?

There has been no action on reown-com/a2#49 since Aug 12 2020. Using a moz controlled branch for now. Closes #236

* chore: move a2 under mozilla-services There has been no action on reown-com/a2#49 since Aug 12 2020. Using a moz controlled branch for now. Closes #236

pimeys · 2020-12-22T13:57:23Z

As you probably noticed, I'm quite busy currently and not really having time on older crates that I'm not working with anymore. Sorry about that. I'm still not convinced we should make the fields of the struct public. And also not using this crate anymore, but I know there are active users such as @dbrgn, who'd be much better saying should we change the API or not.

dbrgn · 2020-12-22T14:09:41Z

I haven't touched the codebase that uses a2 in a while. I may be able to take a look in early January (I made a note for that), but at the moment I'm quite busy as well 🙂

pimeys · 2020-12-22T14:11:35Z

Haha, no worries! Like, also first vacation for me in ages. I'm a bit worried merging and touching a codebase I'm not actively using anymore, especially when it changes a public-facing api...

dbrgn · 2020-12-22T14:20:34Z

Yeah, and I'm wary of updating dependencies on a service that's been running perfectly fine for months 😅

However, I should be able to judge the API changes independently from that (this PR only changes the API, not how network events are handled or anything like that).

pimeys · 2020-12-22T14:29:21Z

What, you aren't excited about upgrading to Tokio 0.3 or 1.0 already?-)

Jokes aside, what would be cool to do here is what we did in Tiberius, make it runtime-independent. Although it's quite off-topic already... :D

jrconlin · 2020-12-22T16:39:07Z

heh, well, first off, FOR THE LOVE OF WHATEVER YOU DEEM HOLY, PLEASE GO ENJOY YOUR BREAKS!

(Sorry, I do a lot of back end work and may be a bit triggered about seeing folks doing work when there's other, more enriching things to do.)

Next: Honestly, this gives me a lot more info to work with. We're using this library as part of our back-end service. We've already vendored a fork of it, so no worries about any blocks. Also totally fine if y'all treat this as semi-abandonware or archive it. I kinda had the feeling that was the case anyway.

In anycase, go, shoo, have a festive Yule, Happy Saturnalia, Merry Christmas, Joyous Hanukkah, reasonably OK Festivus or whatever else y'all want to have, and all the best in the New Year.

pimeys · 2020-12-22T16:41:25Z

If you made a fork of it, what about taking the ownership of the whole crate?

jrconlin · 2020-12-22T16:51:59Z

Heh, yeah, no. We have enough stuff <https://github.com/mozilla-services> we have to support for a team of three engineers. I mean, it's public in our repos <https://github.com/mozilla-services/a2>, and we'll do regular updates on it so long as we use it, of course, but we're probably not going to address anything in the issue backlog. I suppose we could clean up the inheritance chain a bit to make the link a bit more direct, and I'm going to guess we should probably legacy the "Contact" info if you want... Ok, let me at least talk it over with the rest of the team in January and see what we might be able to do, but we also might just switch off to a different library if we find one.

…

On Tue, Dec 22, 2020 at 8:41 AM Julius de Bruijn ***@***.***> wrote: If you made a fork of it, what about taking the ownership of the whole crate? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#49 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAIXK7FLCOOAXWT7S2UZCTSWDD4NANCNFSM4PG6RZ5A> .

pimeys · 2020-12-22T17:47:02Z

I mean, I doubt there will be that much of changes to a2, at least when tokio stabilizes finally.

We did the same thing as an organization to tiberius, basically rewrote it and negotiated with the original author to get access to the crates.io crate and make our fork the main one. I'd be willing to do the same for this and my fcm + web-push crates, if there'd be an interested community keeping them alive and moving.

HarryET · 2022-08-15T09:37:45Z

👋🏼 The project has new maintainers now. I'm getting up-to speed with everything and trying to tidy up the PRs and Issues; are the changes here still relevant?

jrconlin · 2022-08-15T14:35:55Z

@HarryET Welcome! (And thank you!)

Yes, these are still valid, in that we still use them in our fork and it might improve things for others.

HarryET · 2022-08-15T15:13:07Z

Hey @jrconlin, thanks! Can you move the PR to point into v0.7 instead of master? and I'll get these reviewed asap.

jrconlin · 2022-08-15T15:32:50Z

hrm... I don't have access to AsureMarker's account, so I can't post from that. I'll close this PR and open a new one from ours.

HarryET · 2022-08-15T15:34:51Z

@jrconlin looks like I can switch it don't worry. But if it would be easier long term I can close this and work on a new PR with you?

jrconlin · 2022-08-15T15:50:53Z

Yeah, I think it would.

Mark did this original work while he was an intern. He's now off to better things. The other repo has shared ownership.

HarryET · 2022-08-15T16:21:11Z

Yeah, I think it would.

Mark did this original work while he was an intern. He's now off to better things. The other repo has shared ownership.

👌🏼 I'll close this out and will review a new PR once open

AzureMarker added 4 commits July 24, 2020 10:46

Derive Deserialize for APS and sub-structs

1dd1ef3

Add Client::certificate_parts to support more use cases

54760f6

Make the properties of LocalizedAlert public

2a1129b

This helps when creating a localized APS manually.

Implement Default for APS structs

55a974f

This helps when manually building an APS instance.

AzureMarker mentioned this pull request Jul 27, 2020

feat: APNS Router mozilla-services/autopush-rs#201

Merged

pimeys suggested changes Aug 10, 2020

View reviewed changes

Move localized alert title and body to builder methods

424cd51

jrconlin added a commit to mozilla-services/autopush-rs that referenced this pull request Dec 7, 2020

chore: move a2 under mozilla-services

4e12567

There has been no action on reown-com/a2#49 since Aug 12 2020. Using a moz controlled branch for now. Closes #236

jrconlin mentioned this pull request Dec 15, 2020

chore: move a2 under mozilla-services mozilla-services/autopush-rs#245

Merged

HarryET closed this Aug 15, 2022

jrconlin mentioned this pull request Aug 15, 2022

Feat/deserialize and certificate parts mozilla-services/a2#3

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Deserialize impls, a new Client constructor, and some QoL features #49

Add Deserialize impls, a new Client constructor, and some QoL features #49

AzureMarker commented Jul 24, 2020

AzureMarker commented Aug 10, 2020

pimeys commented Aug 10, 2020

pimeys Aug 10, 2020

AzureMarker Aug 10, 2020

pimeys Aug 10, 2020

pimeys Aug 10, 2020

AzureMarker Aug 10, 2020

pimeys commented Aug 12, 2020

AzureMarker commented Sep 26, 2020

jrconlin commented Dec 7, 2020

pimeys commented Dec 22, 2020

dbrgn commented Dec 22, 2020

pimeys commented Dec 22, 2020

dbrgn commented Dec 22, 2020

pimeys commented Dec 22, 2020

jrconlin commented Dec 22, 2020

pimeys commented Dec 22, 2020

jrconlin commented Dec 22, 2020 via email

pimeys commented Dec 22, 2020

HarryET commented Aug 15, 2022

jrconlin commented Aug 15, 2022

HarryET commented Aug 15, 2022

jrconlin commented Aug 15, 2022

HarryET commented Aug 15, 2022

jrconlin commented Aug 15, 2022

HarryET commented Aug 15, 2022

Add Deserialize impls, a new Client constructor, and some QoL features #49

Add Deserialize impls, a new Client constructor, and some QoL features #49

Conversation

AzureMarker commented Jul 24, 2020

AzureMarker commented Aug 10, 2020

pimeys commented Aug 10, 2020

pimeys Aug 10, 2020

Choose a reason for hiding this comment

AzureMarker Aug 10, 2020

Choose a reason for hiding this comment

pimeys Aug 10, 2020

Choose a reason for hiding this comment

pimeys Aug 10, 2020

Choose a reason for hiding this comment

AzureMarker Aug 10, 2020

Choose a reason for hiding this comment

pimeys commented Aug 12, 2020

AzureMarker commented Sep 26, 2020

jrconlin commented Dec 7, 2020

pimeys commented Dec 22, 2020

dbrgn commented Dec 22, 2020

pimeys commented Dec 22, 2020

dbrgn commented Dec 22, 2020

pimeys commented Dec 22, 2020

jrconlin commented Dec 22, 2020

pimeys commented Dec 22, 2020

jrconlin commented Dec 22, 2020 via email

pimeys commented Dec 22, 2020

HarryET commented Aug 15, 2022

jrconlin commented Aug 15, 2022

HarryET commented Aug 15, 2022

jrconlin commented Aug 15, 2022

HarryET commented Aug 15, 2022

jrconlin commented Aug 15, 2022

HarryET commented Aug 15, 2022