Introduce Oppia proto API v1: Android #1
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 |
|---|---|---|
| @@ -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. @anandwana001 are you planning to add anything here, and ditto for the other similar TODOs? There was a problem hiding this comment. 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. 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,29 @@ | ||
| """ | ||
| 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. | ||
|
There was a problem hiding this comment. In Oppia Android, are we following a typical line length limit for BUILD files? There was a problem hiding this comment. Not exactly, this is no support for enforcing the 100 char length rule for Bazel files. There was a problem hiding this comment. We generally follow a 100 character limit in all files, but @anandwana001 is correct that our linters don't enforce this today (yet). I've revised all lines that can be wrapped to consistently follow the 100 character line limit (both for proto & BUILD/bzl files). |
||
| 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. | ||
|
There was a problem hiding this comment. Obsolete now that this documentation has been rewritten. PTAL. |
||
| """ | ||
|
|
||
| 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,113 @@ | ||
| 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.v1.api"; | ||
| option java_multiple_files = true; | ||
anandwana001 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| message TopicListRequest { | ||
|
There was a problem hiding this comment. @BenHenning -- FYI I took a stab at documenting this file. Please feel free to edit as needed. There was a problem hiding this comment. I've revised basically all documentation for the PR, using a lot of the existing material so thanks for taking a first stab it! |
||
| TopicListRequestResponseStructureVersion structure_version = 1; | ||
|
There was a problem hiding this comment. @anandwana001 These do not seem to have been updated to "proto version". Please fix throughout. Where is the documentation for the proto fields? There was a problem hiding this comment. Update: talked with @BenHenning, please leave just this file for him to document (@anandwana001 it would be great if you did the other files). Thanks! There was a problem hiding this comment. Can this be considered resolved with the latest changes @seanlip? |
||
|
|
||
| ClientContext client_context = 2; | ||
| ClientCompatibilityContext compatibility_context = 3; | ||
| ClientDownloadStateContext download_context = 4; | ||
|
There was a problem hiding this comment. 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. 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. 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 { | ||
| TopicListRequestResponseStructureVersion structure_version = 1; | ||
|
|
||
| // All topic summaries that are newer & compatible with the client. | ||
|
There was a problem hiding this comment. ...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. (Fixed in a commit I made to document this file.) There was a problem hiding this comment. 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. 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. 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. 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. |
||
| TopicContentRequestResponseStructureVersion structure_version = 1; | ||
|
|
||
| ClientContext client_context = 2; | ||
| repeated DownloadRequestStructureIdentifier identifiers = 3; | ||
| int32 requested_max_payload_size = 4; | ||
| } | ||
|
|
||
| message TopicContentResponse { | ||
| TopicContentRequestResponseStructureVersion structure_version = 1; | ||
|
|
||
| repeated DownloadResult download_results = 2; | ||
|
|
||
| // Structure corresponds to TopicContentRequestResponseStructureVersion. | ||
seanlip marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| message DownloadResult { | ||
| DownloadRequestStructureIdentifier identifier = 1; | ||
|
|
||
| oneof structure_type { | ||
| bool skipped_suggest_retry = 2; | ||
seanlip marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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 TopicContentRequestResponseStructureVersion. | ||
| message QuestionIdList { | ||
| repeated string question_ids = 1; | ||
| } | ||
| } | ||
|
|
||
| message ClientContext { | ||
|
There was a problem hiding this comment. 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. 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.TopicSummaryStructureVersion topic_summary_structure_version = 1; | ||
| org.oppia.proto.v1.structure.RevisionCardStructureVersion revision_card_structure_version = 2; | ||
| org.oppia.proto.v1.structure.ConceptCardStructureVersion concept_card_structure_version = 3; | ||
| org.oppia.proto.v1.structure.ExplorationStructureVersion exploration_structure_version = 4; | ||
| org.oppia.proto.v1.structure.QuestionStructureVersion question_structure_version = 5; | ||
| org.oppia.proto.v1.structure.StateStructureVersion state_structure_version = 6; | ||
| org.oppia.proto.v1.structure.LanguageStructuresVersion language_structures_version = 7; | ||
| org.oppia.proto.v1.structure.ImageStructureVersion thumbnail_structure_version = 8; | ||
| } | ||
|
|
||
| message ClientDownloadStateContext { | ||
|
There was a problem hiding this comment. In TopicListRequest, what will the download_context field (which is of type ClientDownloadStateContext) contain? There was a problem hiding this comment. See my comment above. There was a problem hiding this comment. Do you have any follow-up questions or concerns here @seanlip? |
||
| message DownloadState { | ||
|
There was a problem hiding this comment. 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. 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. 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 TopicContentRequestResponseStructureVersion. | ||
| message DownloadRequestStructureIdentifier { | ||
| oneof structure_type { | ||
| org.oppia.proto.v1.structure.SubtopicId revision_card_id = 1; | ||
|
There was a problem hiding this comment. 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. 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. Do you have any follow-up questions or concerns here @seanlip? |
||
| string concept_card_id = 2; | ||
|
There was a problem hiding this comment. 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. 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. 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. Bearing in mind that field 2 is the skill ID, does this duplicate field 2? There was a problem hiding this comment. 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. Do you have any follow-up questions or concerns here @seanlip? |
||
| string question_id = 5; | ||
| } | ||
| } | ||
|
|
||
| message TopicListRequestResponseStructureVersion { | ||
| int32 version = 1; | ||
| } | ||
|
|
||
| message TopicContentRequestResponseStructureVersion { | ||
| 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,28 @@ | ||
| 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.v1.structure"; | ||
| option java_multiple_files = true; | ||
|
|
||
| message ConceptCard { | ||
| ConceptCardStructureVersion structure_version = 1; | ||
|
|
||
| string skill_id = 2; | ||
| string description = 3; | ||
| SubtitledHtml explanation = 4; | ||
| repeated WorkedExample worked_examples = 5; | ||
| RecordedVoiceovers recorded_voiceovers = 6; | ||
| WrittenTranslations written_translations = 7; | ||
| ReferencedImageList referenced_image_list = 8; | ||
|
There was a problem hiding this comment. Just wondering, why There was a problem hiding this comment. 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. 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. 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. [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. 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. Can this now be considered resolved @seanlip? |
||
|
|
||
| // Proto version corresponds to ConceptCardStructureVersion. | ||
seanlip marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| message WorkedExample { | ||
| SubtitledHtml question = 1; | ||
| SubtitledHtml explanation = 2; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| syntax = "proto3"; | ||
|
|
||
| package org.oppia.proto.v1.structure; | ||
|
|
||
| import "proto/v1/structure/image.proto"; | ||
| import "proto/v1/structure/state.proto"; | ||
| import "proto/v1/structure/versions.proto"; | ||
|
|
||
| option java_package = "org.oppia.v1.structure"; | ||
| option java_multiple_files = true; | ||
|
|
||
| message Exploration { | ||
| ExplorationStructureVersion structure_version = 1; | ||
|
|
||
| string id = 2; | ||
| string title = 3; | ||
| string init_state_name = 4; | ||
| map<string, State> states = 5; | ||
| int32 content_version = 6; | ||
| ReferencedImageList referenced_image_list = 7; | ||
seanlip marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| syntax = "proto3"; | ||
|
|
||
| package org.oppia.proto.v1.structure; | ||
|
|
||
| import "proto/v1/structure/versions.proto"; | ||
|
|
||
| option java_package = "org.oppia.v1.structure"; | ||
| option java_multiple_files = true; | ||
|
|
||
| message Thumbnail { | ||
| ImageStructureVersion structure_version = 1; | ||
|
|
||
| string filename = 2; | ||
| int32 background_color_rgb = 3; | ||
| } | ||
|
|
||
| message ReferencedImageList { | ||
| ImageStructureVersion structure_version = 1; | ||
|
|
||
| repeated string referenced_image_filenames = 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.