-
Notifications
You must be signed in to change notification settings - Fork 1
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
base: develop
Are you sure you want to change the base?
Changes from 18 commits
1bfefe8
a69a240
b41cf7d
be028d2
0ab3fc3
abbdf13
c1e97fd
0e921d1
f6b3210
47dfb60
8dc3eb1
a5edbf5
13fad56
fa11822
cdf7150
0231af8
ab70643
0b2896c
5d868e2
0b937e3
5ceec82
78d06fa
ee332e2
ee5b117
b10a3a4
9afead9
97a134d
cdd8c67
fcf4a60
eabbdf2
937c883
458d489
0250c69
1b3ef9e
bb559ea
886e1fc
8f3cde8
4ae1907
bc78dd9
6c3b65a
58b1b7b
1c129b7
7f766e1
7219d37
8bcb631
4ea008b
cb3434e
87422ba
98d3122
9cf993e
f6d167c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
""" | ||
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", | ||
], | ||
) |
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. | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). |
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() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in the latest changes. |
||
TODO: fill in | ||
""" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
""" | ||
TODO: fill in | ||
""" |
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"], | ||
) |
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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Fixed in a commit I made to document this file.) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my comment above. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} |
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"], | ||
) |
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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just wondering, why There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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".] There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.