refactor(updater): replace manual parsing with struct definitions

[JonasKruckenberg]

What kind of change does this PR introduce?

• Bugfix
• Feature
• Docs
• New Binding issue #___
• Code style update
• Refactor
• Build-related changes
• Other, please describe:

Does this PR introduce a breaking change?

• Yes, and the changes were approved in issue #___
• No

Checklist

• When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
• A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
• I have added a convincing reason for adding this feature, if necessary

Other information

There a number of chances to further tighten down the interface, but they would introduce a breaking change (something @nothingismagick explicitly wanted to avoid)

• Strongly type target. Currently target is a String. By creating a custom type UpdateTarget { os, arch } we could constrain the target to valid target combinations only and outright reject unsupported/malformed targets. This would also help to solve [bug] UpdateBuilder::target weird behavior when specifying target #4161.
• Require version field to be strict semver. Currently we accept both v1.0.0 and 1.0.0 but only the latter is valid semver (the semver crate will reject the former). Requiring strict semver would allow us to get rid of the custom deserialize fn. This would also have the advantage that the should_update callback would receive proper Version structs instead of &str and could perform comparisons much easier (and reliably).
• Remove name alias. Currently the version field can be called version or name. We never have name in any docs and it's rather obscure. Getting rid of it would simplify the interface.
• Add chrono type for pub_date. Currently pub_date is a String, and we pass it through as-is. chrono::DateTime is ISO 8601 and implements Deserialize. This would make updates more secure as malformed dates would be rejected during deseralization as soon as possible.

[lucasfernog]

@JonasKruckenberg I know you'll think the deserialize impl is too ugly, but it's needed to have better error messages. #[serde(flatten)] just gives us useless errors (data did not match any variant of untagged enum RemoteReleaseInner).

[JonasKruckenberg]

@JonasKruckenberg I know you'll think the deserialize impl is too ugly, but it's needed to have better error messages. #[serde(flatten)] just gives us useless errors (data did not match any variant of untagged enum RemoteReleaseInner).

Hmmm, you're right, that is very ugly... 😂 Can't we use something like https://github.com/dtolnay/path-to-error and simply convert the serde_json errors into something nicer, without weird whacky struct workarounds?

[lucasfernog]

Hmmm, you're right, that is very ugly... joy Can't we use something like dtolnay/path-to-error and simply convert the serde_json errors into something nicer, without weird whacky struct workarounds?

We can't, serde doesn't give us any information with the flatten attribute, so the path_to_error crate just returns . for the error path.

[JonasKruckenberg]

Stupid flatten 🙄 another idea, we just duplicate version, pub_date and notes fields and have RemoteRelease be an enum?

[lucasfernog]

We also need to duplicate the ReleaseManifestPlatform fields right? url, signature and with_elevated_task since that's where the flatten is used.

[JonasKruckenberg]

Hmmm the Dynamic variant would either have to duplicate or use use flatten yeah 🤔

But would flatten in that context still be a problem?

[lucasfernog]

I think flatten is always a problem for the error message.

[tweidinger]

Looks good to me. I don't see breaking or security impact changes. The PR mentions additionally (breaking) changes which sound like good improvements. Especially the chrono type for the date.

[lucasfernog]

@JonasKruckenberg I tried the enum change, but this is what serde_json gives me: SerdeJson(Error("data did not match any variant of untagged enum RemoteRelease", line: 0, column: 0)). Seems like the custom deserializer is the only choice :D

[JonasKruckenberg]

@JonasKruckenberg I tried the enum change, but this is what serde_json gives me: SerdeJson(Error("data did not match any variant of untagged enum RemoteRelease", line: 0, column: 0)). Seems like the custom deserializer is the only choice :D

It very much looks like it yeah :/