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

Introduce Oppia proto API v1: Android #1

Open
wants to merge 51 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
1bfefe8
Introduce first iteration of Android backend API.
BenHenning Aug 6, 2021
a69a240
Rename 'model' version to be content, instead.
BenHenning Aug 6, 2021
b41cf7d
structure version -> proto version
anandwana001 Aug 10, 2021
be028d2
StructureDownloadResult to DownloadResult
anandwana001 Aug 10, 2021
0ab3fc3
ClientCompatibilityContext index fixed
anandwana001 Aug 10, 2021
abbdf13
removed all placeholder comments
anandwana001 Aug 10, 2021
c1e97fd
renaming fields of WrittenTranslation
anandwana001 Aug 10, 2021
0e921d1
adding Brazilian and Portuguese in LanguageCode
anandwana001 Aug 10, 2021
f6b3210
remove reserved fields
anandwana001 Aug 10, 2021
47dfb60
added docs for bazel files
anandwana001 Aug 10, 2021
8dc3eb1
brazilian portuguese language fix
anandwana001 Aug 11, 2021
a5edbf5
updated readme
anandwana001 Aug 11, 2021
13fad56
added doc for proto
anandwana001 Aug 13, 2021
fa11822
java package match proto package
anandwana001 Aug 13, 2021
cdf7150
LanguageStructuresVersion to LanguageProtosVersion
anandwana001 Aug 13, 2021
0231af8
added docs
anandwana001 Aug 17, 2021
ab70643
added note
anandwana001 Aug 17, 2021
0b2896c
nit fix
anandwana001 Aug 17, 2021
5d868e2
added docs
anandwana001 Aug 18, 2021
0b937e3
nit fix
anandwana001 Aug 18, 2021
5ceec82
Update most docstrings.
seanlip Aug 23, 2021
78d06fa
Work through topic summary as well.
seanlip Sep 14, 2021
ee332e2
Clean up line comment wrapping.
BenHenning Oct 13, 2021
ee5b117
Modularize WORKSPACE.
BenHenning Oct 13, 2021
b10a3a4
Make protos shareable.
BenHenning Oct 14, 2021
9afead9
Revise topic list API.
BenHenning Oct 14, 2021
97a134d
Annotate protos with their version types.
BenHenning Oct 14, 2021
cdd8c67
Redo a LOT of documentation.
BenHenning Oct 15, 2021
fcf4a60
move dir to org/oppia/
anandwana001 Oct 29, 2021
eabbdf2
adding 4 more exploration
anandwana001 Dec 7, 2021
937c883
added 4 more exploration ad nupdate to Dto suffix
anandwana001 Dec 7, 2021
458d489
nit fixes
anandwana001 Dec 9, 2021
0250c69
nit fixes
anandwana001 Dec 9, 2021
1b3ef9e
nit fixes
anandwana001 Dec 10, 2021
bb559ea
few doc added
anandwana001 Dec 10, 2021
886e1fc
update math interaction rule and added MatchesUpToTrivialManipulation…
anandwana001 Jan 24, 2022
8f3cde8
nit fix for math rules
anandwana001 Jan 25, 2022
4ae1907
updated RevisionCardDto
anandwana001 Feb 10, 2022
bc78dd9
nit fix revision card
anandwana001 Feb 10, 2022
6c3b65a
nit fix
anandwana001 Feb 10, 2022
58b1b7b
Move repo bzl files to correct top-level location.
BenHenning Mar 9, 2022
1c129b7
Add language support for Swahili.
BenHenning Apr 15, 2022
7f766e1
Significant changes.
BenHenning Feb 28, 2023
7219d37
Clarify calendar documentation.
BenHenning Jun 3, 2023
8bcb631
Add support for Nigerian Pidgin.
BenHenning Jun 3, 2023
4ea008b
Add possibility for absence (rather than failure).
BenHenning Jun 3, 2023
cb3434e
Add support for classroom summaries.
BenHenning Mar 20, 2024
87422ba
Add new version to compatibility context.
BenHenning Mar 20, 2024
98d3122
Update rules_proto.
BenHenning May 27, 2024
9cf993e
Fix documentation typo.
BenHenning May 27, 2024
f6d167c
Add structural support for NumberWithUnits.
BenHenning Dec 28, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
"""
Copy link
Member

Choose a reason for hiding this comment

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

@BenHenning flagging this and other similar TODOs for you to look at.

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in the latest changes.

TODO: fill in
"""

package_group(
name = "api_visibility",
packages = [
"//",
],
)

package_group(
name = "proto_impl_visibility",
packages = [
"//proto/...",
],
)

java_library(
name = "android_java_protos",
visibility = ["//visibility:public"],
exports = [
"//proto/v1/api:android_java_proto",
"//proto/v1/structure:structure_java_proto",
],
)

java_library(
name = "android_java_lite_protos",
visibility = ["//visibility:public"],
exports = [
"//proto/v1/api:android_java_proto_lite",
"//proto/v1/structure:structure_java_proto_lite",
],
)
44 changes: 42 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
@@ -1,2 +1,42 @@
# oppia-android-api
Published Android-specific API for the Oppia backend.
# oppia-proto-api
Published common proto API for the Oppia server/client.

This API governs the transfer of data between the Oppia Python server and the Android app.

Copy link
Member

Choose a reason for hiding this comment

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

@BenHenning I think it might be worth adding notes in this README on "If you feel that you need to update a proto version, this is the specific process to follow".

The procedure should specify what things to think about (e.g. maintain backwards compatibility, grep for any comment with the name of the proto you're modifying since there may be dependent protos, etc.), and also include non-coding steps for getting sign-off from the tech lead, communicating with the leads of the client repos, etc. Will proto versions just get incremented or do we need to preserve information about the older versions in some way?

Having this written down would help ensure that everyone is on the same page about the upgrade process -- I'm currently having a bit of trouble auditing it / thinking through it during review, because I don't quite know what it is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack. I plan to take a stab at writing a full README in a later revision (didn't quite get to it since covering the rest of the documentation was rather involved).

### API Versions
Each version of this API is formatted as a major & minor version (e.g. 1.0) and corresponds to a separate release.

- The major version is represented directly in the directory structure (e.g. 'v1'), and is only incremented if a breaking change must be introduced into the API. (Note that this repository might house multiple major versions, in order to maintain long-term backward compatibility with older API versions.)
Copy link
Member

Choose a reason for hiding this comment

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

Is the major version specified in code anywhere? (If so, where? Suggest explaining in the doc.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack. I plan to take a stab at writing a full README in a later revision (didn't quite get to it since covering the rest of the documentation was rather involved).

- The minor version is incremented whenever a compatible change to the API is released. It is only ever represented in the release numbers, and never in code form.
Copy link
Member

Choose a reason for hiding this comment

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

Does "change to the API" mean "change to any proto version defined in this repository"? If so, maybe state that explicitly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack. I plan to take a stab at writing a full README in a later revision (didn't quite get to it since covering the rest of the documentation was rather involved).


Because protobuf is backward/forward interoperable, incompatibilities are unlikely as long as proto versions are respected (though, even in those cases, there should be potential for graceful failures).

### Content Versions
Content versions represent the version of the corresponding entity instance (e.g. a topic, skill, or exploration). These are used to track whether a particular structure has content updates. They map to the versions stored in the corresponding structure's VersionedModel in the backend.

### Proto Versions
Proto versions correspond to the proto structures defined in this repository. They only need to be incremented when a proto structure is updated in such a way that the default data will break existing logical assumptions (thus requiring a data migration). Such upgrades should always happen in a compatible way (except for major API version upgrades -- see the "API Version" section above).

Three notes:

1. Some proto versions correspond to **groups** of substructures (such as SubtitledHtml, or other language-based substructures), because these substructures are shared across high-level structures.
2. There is no need to version interactions separately. This is because their structure and how they relate to the exploration/question experience is implied as part of State's structure version.
3. If any changes happen in the proto structure which are getting tracked, the version specified in the comment must be incremented.

The following are the list of structure types whose versions are tracked:
- TopicSummaryProtoVersion
- RevisionCardProtoVersion
- ConceptCardProtoVersion
- ExplorationProtoVersion
- QuestionProtoVersion
- StateProtoVersion
- LanguageProtosVersion
- ImageProtoVersion

## Support
If you have any feature requests or bug reports, please log them on our [issue tracker](https://github.com/oppia/oppia-proto-api/issues/new?title=Describe%20your%20feature%20request%20or%20bug%20report%20succinctly&body=If%20you%27d%20like%20to%20propose%20a%20feature,%20describe%20what%20you%27d%20like%20to%20see.%0A%0AIf%20you%27re%20reporting%20a%20bug,%20please%20be%20sure%20to%20include%20the%20expected%20behaviour,%20the%20observed%20behaviour,%20and%20steps%20to%20reproduce%20the%20problem.%20Console%20copy-pastes%20and%20any%20background%20on%20the%20environment%20would%20also%20be%20helpful.%0A%0AThanks!).

Please report security issues directly to [email protected].

## Licence
The Oppia-Proto-API code is released under the [Apache v2 license](https://github.com/oppia/oppia-proto-api/blob/master/LICENSE).
60 changes: 60 additions & 0 deletions WORKSPACE
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
"""
This file lists and imports all external dependencies needed to build Oppia Proto API.
"""

load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

# Set up Python (as a prerequisite dependency for Protobuf).

PYTHON_SHA256_HASH = "934c9ceb552e84577b0faf1e5a2f0450314985b4d8712b2b70717dc679fdc01b"
PYTHON_VERSION = "0.3.0"

http_archive(
name = "rules_python",
url = "https://github.com/bazelbuild/rules_python/releases/download/{0}/rules_python-{0}.tar.gz".format(PYTHON_VERSION),
sha256 = PYTHON_SHA256_HASH,
)

# Set up proto & its toolchain.

PROTOBUF_SHA256_HASH = "c6003e1d2e7fefa78a3039f19f383b4f3a61e81be8c19356f85b6461998ad3db"
PROTOBUF_VERSION = "3.17.3"

http_archive(
name = "com_google_protobuf",
sha256 = PROTOBUF_SHA256_HASH,
strip_prefix = "protobuf-%s" % PROTOBUF_VERSION,
urls = ["https://github.com/protocolbuffers/protobuf/archive/v%s.tar.gz" % PROTOBUF_VERSION]
)

# Set up rules_proto to allow defining proto libraries.

# Note that rules_proto doesn't have any shipped versions, so the workspace needs to pin to a
# specific commit hash.
RULES_PROTO_VERSION = "97d8af4dc474595af3900dd85cb3a29ad28cc313"
RULES_PROTO_SHA256_HASH = "602e7161d9195e50246177e7c55b2f39950a9cf7366f74ed5f22fd45750cd208"

http_archive(
name = "rules_proto",
sha256 = RULES_PROTO_SHA256_HASH,
strip_prefix = "rules_proto-%s" % RULES_PROTO_VERSION,
urls = ["https://github.com/bazelbuild/rules_proto/archive/%s.tar.gz" % RULES_PROTO_VERSION],
)

load("@rules_proto//proto:repositories.bzl", "rules_proto_dependencies", "rules_proto_toolchains")
rules_proto_dependencies()
rules_proto_toolchains()

# Set up rules_java to enable the Java proto & Java proto lite rules.
RULES_JAVA_VERSION = "0.1.1"
RULES_JAVA_SHA256_HASH = "220b87d8cfabd22d1c6d8e3cdb4249abd4c93dcc152e0667db061fb1b957ee68"

http_archive(
name = "rules_java",
sha256 = RULES_JAVA_SHA256_HASH,
url = "https://github.com/bazelbuild/rules_java/releases/download/{0}/rules_java-{0}.tar.gz".format(RULES_JAVA_VERSION),
)

load("@rules_java//java:repositories.bzl", "rules_java_dependencies", "rules_java_toolchains")
rules_java_dependencies()
rules_java_toolchains()
3 changes: 3 additions & 0 deletions proto/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
"""
Copy link
Member

Choose a reason for hiding this comment

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

@anandwana001 are you planning to add anything here, and ditto for the other similar TODOs?

Choose a reason for hiding this comment

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

For this particular file, I don't have much idea what Ben is planning to add here.

The root BUILD has all the necessary things to build bazel for proto files, but I am not much clear with this BUILD file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in the latest changes.

TODO: fill in
"""
3 changes: 3 additions & 0 deletions proto/v1/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
"""
TODO: fill in
"""
30 changes: 30 additions & 0 deletions proto/v1/api/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
"""
This library contains the proto_library(), java_lite_proto_library() and java_proto_library rules.
The proto_library() rule creates a proto file library for the android API
proto file to be used in multiple languages.
The java_lite_proto_library() rule takes in a proto_library target and generates Java code.
The java_proto_library() rule takes in a proto_library target and generates Java code.
"""

load("@rules_java//java:defs.bzl", "java_lite_proto_library", "java_proto_library")
load("@rules_proto//proto:defs.bzl", "proto_library")

proto_library(
name = "android_proto",
srcs = ["android.proto"],
deps = [
"//proto/v1/structure:structure_proto"
],
)

java_proto_library(
name = "android_java_proto",
visibility = ["//:api_visibility"],
deps = [":android_proto"],
)

java_lite_proto_library(
name = "android_java_proto_lite",
visibility = ["//:api_visibility"],
deps = [":android_proto"],
)
116 changes: 116 additions & 0 deletions proto/v1/api/android.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
syntax = "proto3";

package org.oppia.proto.v1.api;

import "proto/v1/structure/concept_card.proto";
import "proto/v1/structure/exploration.proto";
import "proto/v1/structure/question.proto";
import "proto/v1/structure/revision_card.proto";
import "proto/v1/structure/topic_summary.proto";
import "proto/v1/structure/versions.proto";

option java_package = "org.oppia.proto.v1.api";
option java_multiple_files = true;

message TopicListRequest {
Copy link
Member

Choose a reason for hiding this comment

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

@BenHenning -- FYI I took a stab at documenting this file. Please feel free to edit as needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've revised basically all documentation for the PR, using a lot of the existing material so thanks for taking a first stab it!

TopicListRequestResponseProtoVersion proto_version = 1;

ClientContext client_context = 2;
ClientCompatibilityContext compatibility_context = 3;
ClientDownloadStateContext download_context = 4;
Copy link
Member

Choose a reason for hiding this comment

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

FYI I don't really understand what this field is tracking or how downloads are being managed in general (I don't really get why we're sending this in the topic list request). So I haven't documented it yet.

It seems odd to me that this is part of the topic list request actually. Is it to tell the server what objects we're currently in the process of downloading? If so, why would the server need to know that, for this request? (It seems to introduce additional state that makes things more complicated IMO; the client should be able to de-dup if needed.)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is needed to tell the server what the app currently has downloaded, not what it's downloading (the topic list is always automatically downloaded by the client upon app enter; explicit downloads via TopicContentRequest always comes from the user requesting a topic to be downloaded or updated).

This is needed so that the server can compute an incremental topic list to save on proto size. The client will be merging its current topic list with whatever the server sends in order to detect cases when there are updates available.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you have any follow-up questions or concerns here @seanlip? Also, the latest changes include a lot more documentation here (and some additional structure changes/renames).

}

message TopicListResponse {
TopicListRequestResponseProtoVersion proto_version = 1;

// All topic summaries that are newer & compatible with the client.
Copy link
Member

Choose a reason for hiding this comment

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

...that are newer than the version of the topic currently stored on the client, but are still compatible with the client.

(Otherwise, not clear what "newer" is being compared against.)

Copy link
Member

Choose a reason for hiding this comment

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

(Fixed in a commit I made to document this file.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Can this now be considered resolved @seanlip?

repeated org.oppia.proto.v1.structure.TopicSummary topic_summaries = 2;
}

message TopicContentRequest {
Copy link
Member

Choose a reason for hiding this comment

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

Just to confirm -- it looks like this request doesn't include the topic ID at all. Is that intended?

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't need it since DownloadRequestStructureIdentifier only contains global IDs. The structure is intentionally open--we can download any content payloads that we need (technically including topics if we added them, but we don't need more than the summaries that we get from the topic list in the current client).

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you have any follow-up questions or concerns here @seanlip?

Also, in the latest version we now have the capability of explicitly requesting the topic summary to build a bit more resilience into the Android client for some edge failure scenarios.

TopicContentRequestResponseProtoVersion proto_version = 1;

ClientContext client_context = 2;
repeated DownloadRequestStructureIdentifier identifiers = 3;
int32 requested_max_payload_size = 4;
}

message TopicContentResponse {
TopicContentRequestResponseProtoVersion proto_version = 1;

repeated DownloadResult download_results = 2;

// Structure corresponds to TopicContentRequestResponseProtoVersion.
message DownloadResult {
DownloadRequestStructureIdentifier identifier = 1;

oneof structure_type {
// Indicates that the request corresponding to this particular identifier was skipped
// and suggested to retry by the Oppia backend. Note that the presence of this value in the
// oneof is sufficient to indicate its state, and its actual value does not matter.
bool skipped = 2;
org.oppia.proto.v1.structure.RevisionCard revision_card = 3;
org.oppia.proto.v1.structure.ConceptCard concept_card = 4;
org.oppia.proto.v1.structure.Exploration exploration = 5;
QuestionIdList question_id_list = 6;
org.oppia.proto.v1.structure.Question question = 7;
}
}

// Structure corresponds to TopicContentRequestResponseProtoVersion.
message QuestionIdList {
repeated string question_ids = 1;
}
}

message ClientContext {
Copy link
Member

Choose a reason for hiding this comment

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

The "version name" and "version code" could probably use a prefix and some documentation to explain whether they correspond to an API version, content version, proto version, or some kind of "client version" (that needs to be defined).

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, and added documentation. Please let me know if you think further context is needed.

string version_name = 1;
int64 version_code = 2;
}

message ClientCompatibilityContext {
org.oppia.proto.v1.structure.TopicSummaryProtoVersion topic_summary_proto_version = 1;
org.oppia.proto.v1.structure.RevisionCardProtoVersion revision_card_proto_version = 2;
org.oppia.proto.v1.structure.ConceptCardProtoVersion concept_card_proto_version = 3;
org.oppia.proto.v1.structure.ExplorationProtoVersion exploration_proto_version = 4;
org.oppia.proto.v1.structure.QuestionProtoVersion question_proto_version = 5;
org.oppia.proto.v1.structure.StateProtoVersion state_proto_version = 6;
org.oppia.proto.v1.structure.LanguageProtosVersion language_proto_version = 7;
org.oppia.proto.v1.structure.ImageProtoVersion thumbnail_proto_version = 8;
}

message ClientDownloadStateContext {
Copy link
Member

Choose a reason for hiding this comment

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

In TopicListRequest, what will the download_context field (which is of type ClientDownloadStateContext) contain?

Copy link
Member Author

Choose a reason for hiding this comment

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

See my comment above.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you have any follow-up questions or concerns here @seanlip?

message DownloadState {
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem to include any status field (despite the DownloadState name). Is the idea that if something is represented here, it is a download that's currently in progress? If so then should the message name be CurrentDownload instead (or something similar)?

Copy link
Member Author

Choose a reason for hiding this comment

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

See my comment above--this is representing the current state in the client.

Any suggestions for alternative names to make it clearer?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you have any follow-up questions or concerns here @seanlip?

int32 content_version = 1;
oneof structure_type {
string topic_id = 2;
org.oppia.proto.v1.structure.SubtopicId revision_card_id = 3;
string concept_card_id = 4;
string story_id = 5;
string exploration_id = 6;
string question_id = 7;
string skill_id = 8;
}
}

repeated DownloadState current_downloads = 1;
}

// This structure's version corresponds to TopicContentRequestResponseProtoVersion.
message DownloadRequestStructureIdentifier {
oneof structure_type {
org.oppia.proto.v1.structure.SubtopicId revision_card_id = 1;
Copy link
Member

Choose a reason for hiding this comment

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

Note that this is topic-specific and is not globally unique.

(I think that's probably OK but wanted to point that out just in case.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack. It's for this reason we're using SubtopicId here (which contains both topic ID & index). All IDs in this structure need to be global, hence the need for SubtopicId.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you have any follow-up questions or concerns here @seanlip?

string concept_card_id = 2;
Copy link
Member

Choose a reason for hiding this comment

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

Note that concept_card_id == skill_id (there is no difference). Do you want to call it skill ID instead in case we subsequently download other stuff for the skill?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so; we'd want to have specific oneof cases for those situations. By calling this 'concept_card_id' we're contextualizing it to specifically correspond to concept cards.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you have any follow-up questions or concerns here @seanlip?

string exploration_id = 3;
string question_list_skill_id = 4;
Copy link
Member

Choose a reason for hiding this comment

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

Bearing in mind that field 2 is the skill ID, does this duplicate field 2?

Copy link
Member Author

Choose a reason for hiding this comment

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

In potential overlap, yes, but the meaning is entirely different. If this one is populated it means the client should receive the full list of questions corresponding to the specific skill ID rather than the skill's concept card. Oneofs always give us two pieces of information: the actual data for a case, and the case itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you have any follow-up questions or concerns here @seanlip?

string question_id = 5;
}
}

message TopicListRequestResponseProtoVersion {
int32 version = 1;
}

message TopicContentRequestResponseProtoVersion {
int32 version = 1;
}
38 changes: 38 additions & 0 deletions proto/v1/structure/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
"""
This library contains the proto_library(), java_lite_proto_library() and java_proto_library rules.
The proto_library() rule creates a proto file library for the core proto files to be used in multiple languages.
The java_lite_proto_library() rule takes in a proto_library target and generates Java code.
The java_proto_library() rule takes in a proto_library target and generate Java code.
"""

load("@rules_java//java:defs.bzl", "java_lite_proto_library", "java_proto_library")
load("@rules_proto//proto:defs.bzl", "proto_library")

proto_library(
name = "structure_proto",
srcs = [
"concept_card.proto",
"exploration.proto",
"image.proto",
"languages.proto",
"objects.proto",
"question.proto",
"revision_card.proto",
"state.proto",
"topic_summary.proto",
"versions.proto",
],
visibility = ["//:proto_impl_visibility"],
)

java_proto_library(
name = "structure_java_proto",
visibility = ["//:api_visibility"],
deps = [":structure_proto"],
)

java_lite_proto_library(
name = "structure_java_proto_lite",
visibility = ["//:api_visibility"],
deps = [":structure_proto"],
)
49 changes: 49 additions & 0 deletions proto/v1/structure/concept_card.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
syntax = "proto3";

package org.oppia.proto.v1.structure;

import "proto/v1/structure/image.proto";
import "proto/v1/structure/languages.proto";
import "proto/v1/structure/versions.proto";

option java_package = "org.oppia.proto.v1.structure";
option java_multiple_files = true;

// NOTE: Update ConceptCardProtoVersion if the structure of this proto is modified. The current
// version is 1.
Copy link
Member

Choose a reason for hiding this comment

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

I'm worried about "The current version is 1." going stale. Is there somewhere central we can keep track of it?

Ditto elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Left a similar comment earlier.

The source of truth here is actually going to be Oppia web. How populated fields tie to versions is entirely tied to how the protos are used; we can't stipulate it here (& it would be incorrect to try to). Protos just define structure, not versioning or usage.

Copy link
Member Author

Choose a reason for hiding this comment

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

Latest changes partially mitigate this by annotating protos with the version they correspond to. The actual details for how versioning will work needs to go into the README, so that'll be a follow-up change.

// Proto of a single concept card that can be displayed for a specific skill.
message ConceptCard {
ConceptCardProtoVersion proto_version = 1;

// The ID corresponding to the skill this concept is representing.
string skill_id = 2;

//A human-readable description for the skill.
string description = 3;

// The explanation of the skill that is shown in the concept card.
SubtitledHtml explanation = 4;

// A list of worked examples to present to the learner.
repeated WorkedExample worked_examples = 5;

// The recorded audio to play SubtitledHTML subfields of this concept card.
Copy link
Member

Choose a reason for hiding this comment

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

Unlike written translations, this won't include all possible audio languages, right? If so, should this fact be documented? (It might also result in a dependency on other local context information, since this field wouldn't be as "stateless" as the other ones -- is that an issue?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Excellent point. I don't think we actually need to change these structures, or even their contracts. This is really a contract of the topic list API, so I've updated that documentation to include details on this. However, they're a bit fuzzy right now because we haven't actually figured out how we want to handle translation packs, so I also filed & added a TODO on #5 to track this.

RecordedVoiceovers recorded_voiceovers = 6;

// The translations of SubtitledHTML subfields of this concept card.
WrittenTranslations written_translations = 7;

// The list of all images that are included within SubtitledHTML and WorkedExample
// subfields of this concept card.
ReferencedImageList referenced_image_list = 8;
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering, why ReferencedImageList rather than repeated ReferencedImage (here and elsewhere)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the definition of an image list is a shared substructure, so its version is actually governed as part of ImageStructureVersion. My intent is to design for future iterations on how we represent images (which seems likely to change in coming years as we add more complex image editing in the exploration editor, etc.)

Copy link
Member

Choose a reason for hiding this comment

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

I'm not quite sure I follow, sorry -- wouldn't the same thing still work if you had repeated ReferencedImage and ReferencedImage still includes an ImageProtoVersion? I.e. what's an example of a future change that would affect a list of images that wouldn't just affect each image individually?

Copy link
Member Author

Choose a reason for hiding this comment

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

In your example the version would only handle how ReferencedImage is represented, not the fact that it's represented using a linear list. If we wanted to represent referenced images differently (for example, by mapping them to content or image IDs) then it would change ReferencedImageList but not the structures that contain it.

Copy link
Member

Choose a reason for hiding this comment

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

[Update: Chatted with Ben, I think the idea is that this might possibly change to e.g. a dict in the future. Note that, if this is necessary, the message name can be changed without issue, so it's not a problem that it's currently named "*List".]

Copy link
Member Author

Choose a reason for hiding this comment

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

Though, changing the type will require a new field (and a migration by updating the proto version & having a routine to migrate lists to a map). We would deprecate the old field & add a new one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can this now be considered resolved @seanlip?


// The worked example is an example of question and answer shown in a concept card.
message WorkedExample {

// The question for this worked example.
SubtitledHtml question = 1;

// The explanation for how to solve this worked example.
SubtitledHtml explanation = 2;
}
}
Loading