Skip to content

Commit

Permalink
Add support for proto3 optional scalars
Browse files Browse the repository at this point in the history
Protobuf 3.15 introduced support for marking scalar fields like
uint32 as optional, and all of our tooling appears to support it
now. This allows us to use simple optional/null checks in our Rust/
TypeScript code, without having to resort to an inner message.

I had to apply a minor patch to protobufjs to get this working with
the json-module output; this has also been submitted upstream:
protobufjs/protobuf.js#1693

I've modified CardStatsResponse as an example of the new syntax.

One thing to note: while the Rust and TypeScript bindings use optional/
null fields, as that is the norm in those languages, Google's Python
bindings are not very Pythonic. Referencing an optional field that is
missing will yield the default value, and a separate HasField() call
is required, eg:

```
>>> from anki.stats_pb2 import CardStatsResponse as R
... msg = R.FromString(b"")
... print(msg.first_review)
... print(msg.HasField("first_review"))
0
False
```
  • Loading branch information
dae committed Feb 27, 2022
1 parent 55c64e5 commit 6941bcc
Show file tree
Hide file tree
Showing 6 changed files with 42 additions and 53 deletions.
8 changes: 4 additions & 4 deletions proto/anki/stats.proto
Expand Up @@ -32,10 +32,10 @@ message CardStatsResponse {
string deck = 4;
// Unix timestamps
int64 added = 5;
generic.Int64 first_review = 6;
generic.Int64 latest_review = 7;
generic.Int64 due_date = 8;
generic.Int32 due_position = 9;
optional int64 first_review = 6;
optional int64 latest_review = 7;
optional int64 due_date = 8;
optional int32 due_position = 9;
// days
uint32 interval = 10;
// per mill
Expand Down
16 changes: 8 additions & 8 deletions proto/protobuf.bzl
Expand Up @@ -33,39 +33,39 @@ def setup_protobuf_binary(name):
http_archive,
name = "protoc_bin_macos",
urls = [
"https://github.com/protocolbuffers/protobuf/releases/download/v3.14.0/protoc-3.14.0-osx-x86_64.zip",
"https://github.com/protocolbuffers/protobuf/releases/download/v3.19.4/protoc-3.19.4-osx-x86_64.zip",
],
sha256 = "699ceee7ef0988ecf72bf1c146dee5d9d89351a19d4093d30ebea3c04008bb8c",
sha256 = "d8b55cf1e887917dd43c447d77bd5bd213faff1e18ac3a176b35558d86f7ffff",
build_file_content = """exports_files(["bin/protoc"])""",
)

maybe(
http_archive,
name = "protoc_bin_linux_x86_64",
urls = [
"https://github.com/protocolbuffers/protobuf/releases/download/v3.14.0/protoc-3.14.0-linux-x86_64.zip",
"https://github.com/protocolbuffers/protobuf/releases/download/v3.19.4/protoc-3.19.4-linux-x86_64.zip",
],
sha256 = "a2900100ef9cda17d9c0bbf6a3c3592e809f9842f2d9f0d50e3fba7f3fc864f0",
sha256 = "058d29255a08f8661c8096c92961f3676218704cbd516d3916ec468e139cbd87",
build_file_content = """exports_files(["bin/protoc"])""",
)

maybe(
http_archive,
name = "protoc_bin_linux_arm64",
urls = [
"https://github.com/protocolbuffers/protobuf/releases/download/v3.14.0/protoc-3.14.0-linux-aarch_64.zip",
"https://github.com/protocolbuffers/protobuf/releases/download/v3.19.4/protoc-3.19.4-linux-aarch_64.zip",
],
sha256 = "67db019c10ad0a151373278383e4e9b756dc90c3cea6c1244d5d5bd230af7c1a",
sha256 = "95584939e733bdd6ffb8245616b2071f565cd4c28163b6c21c8f936a9ee20861",
build_file_content = """exports_files(["bin/protoc"])""",
)

maybe(
http_archive,
name = "protoc_bin_windows",
urls = [
"https://github.com/protocolbuffers/protobuf/releases/download/v3.14.0/protoc-3.14.0-win64.zip",
"https://github.com/protocolbuffers/protobuf/releases/download/v3.19.4/protoc-3.19.4-win64.zip",
],
sha256 = "642554ed4dd2dba94e1afddcccdd7d832999cea309299cc5952f13db389894f8",
sha256 = "828d2bdfe410e988cfc46462bcabd34ffdda8cc172867989ec647eadc55b03b5",
build_file_content = """exports_files(["bin/protoc.exe"])""",
)

Expand Down
25 changes: 8 additions & 17 deletions rslib/src/stats/card.rs
Expand Up @@ -32,12 +32,8 @@ impl Collection {
note_id: card.note_id.into(),
deck: deck.human_name(),
added: card.id.as_secs().0,
first_review: revlog.first().map(|entry| pb::generic::Int64 {
val: entry.id.as_secs().0,
}),
latest_review: revlog.last().map(|entry| pb::generic::Int64 {
val: entry.id.as_secs().0,
}),
first_review: revlog.first().map(|entry| entry.id.as_secs().0),
latest_review: revlog.last().map(|entry| entry.id.as_secs().0),
due_date,
due_position,
interval: card.interval,
Expand All @@ -52,22 +48,17 @@ impl Collection {
})
}

fn due_date_and_position(
&mut self,
card: &Card,
) -> Result<(Option<pb::generic::Int64>, Option<pb::generic::Int32>)> {
fn due_date_and_position(&mut self, card: &Card) -> Result<(Option<i64>, Option<i32>)> {
let due = if card.original_due != 0 {
card.original_due
} else {
card.due
};
Ok(match card.queue {
CardQueue::New => (None, Some(pb::generic::Int32 { val: due })),
CardQueue::New => (None, Some(due)),
CardQueue::Learn => (
Some(pb::generic::Int64 {
val: TimestampSecs::now().0,
}),
card.original_position.map(|u| (u as i32).into()),
Some(TimestampSecs::now().0),
card.original_position.map(|u| u as i32),
),
CardQueue::Review | CardQueue::DayLearn => (
{
Expand All @@ -78,10 +69,10 @@ impl Collection {
let days_remaining = due - (self.timing_today()?.days_elapsed as i32);
let mut due = TimestampSecs::now();
due.0 += (days_remaining as i64) * 86_400;
Some(pb::generic::Int64 { val: due.0 })
Some(due.0)
}
},
card.original_position.map(|u| (u as i32).into()),
card.original_position.map(|u| u as i32),
),
_ => (None, None),
})
Expand Down
22 changes: 9 additions & 13 deletions ts/card-info/CardStats.svelte
Expand Up @@ -4,7 +4,7 @@ License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
-->
<script lang="ts">
import * as tr2 from "../lib/ftl";
import { Stats, unwrapOptionalNumber } from "../lib/proto";
import { Stats } from "../lib/proto";
import { DAY, timeSpan, Timestamp } from "../lib/time";
export let stats: Stats.CardStatsResponse;
Expand All @@ -23,33 +23,29 @@ License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
statsRows.push({ label: tr2.cardStatsAdded(), value: dateString(stats.added) });
const firstReview = unwrapOptionalNumber(stats.firstReview);
if (firstReview !== undefined) {
if (stats.firstReview != null) {
statsRows.push({
label: tr2.cardStatsFirstReview(),
value: dateString(firstReview),
value: dateString(stats.firstReview),
});
}
const latestReview = unwrapOptionalNumber(stats.latestReview);
if (latestReview !== undefined) {
if (stats.latestReview != null) {
statsRows.push({
label: tr2.cardStatsLatestReview(),
value: dateString(latestReview),
value: dateString(stats.latestReview),
});
}
const dueDate = unwrapOptionalNumber(stats.dueDate);
if (dueDate !== undefined) {
if (stats.dueDate != null) {
statsRows.push({
label: tr2.statisticsDueDate(),
value: dateString(dueDate),
value: dateString(stats.dueDate),
});
}
const duePosition = unwrapOptionalNumber(stats.duePosition);
if (duePosition !== undefined) {
if (stats.duePosition != null) {
statsRows.push({
label: tr2.cardStatsNewCardPosition(),
value: duePosition,
value: stats.duePosition,
});
}
Expand Down
11 changes: 0 additions & 11 deletions ts/lib/proto.ts
Expand Up @@ -73,14 +73,3 @@ export const stats = Stats.StatsService.create(serviceCallback as RPCImpl);

export { Tags };
export const tags = Tags.TagsService.create(serviceCallback as RPCImpl);

export function unwrapOptionalNumber(
msg: Generic.IInt64 | Generic.IUInt32 | Generic.IInt32 | null | undefined,
): number | undefined {
if (msg && msg !== null) {
if (msg.val !== null) {
return msg.val;
}
}
return undefined;
}
13 changes: 13 additions & 0 deletions ts/patches/protobufjs+6.11.2.patch
@@ -1,3 +1,16 @@
diff --git a/node_modules/protobufjs/src/field.js b/node_modules/protobufjs/src/field.js
index 20c1cd2..3a1395f 100644
--- a/node_modules/protobufjs/src/field.js
+++ b/node_modules/protobufjs/src/field.js
@@ -270,6 +270,8 @@ Field.prototype.resolve = function resolve() {
this.typeDefault = null;
else // instanceof Enum
this.typeDefault = this.resolvedType.values[Object.keys(this.resolvedType.values)[0]]; // first defined
+ } else if (this.options && this.options.proto3_optional) {
+ this.typeDefault = null;
}

// use explicitly set default value if present
diff --git a/node_modules/protobufjs/src/root.js b/node_modules/protobufjs/src/root.js
index df6f11f..112f9e8 100644
--- a/node_modules/protobufjs/src/root.js
Expand Down

1 comment on commit 6941bcc

@dae
Copy link
Member Author

@dae dae commented on 6941bcc Feb 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hgiesel @RumovZ We don't use a lot of optional fields at the moment, but this should make things a bit more ergonomic when we do

Please sign in to comment.