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

[BUG]: Android controller endpoint doesn't seem to support fetching multiple versions of structures #18241

Closed
BenHenning opened this issue May 17, 2023 · 3 comments · Fixed by #18298
Assignees
Labels
bug Label to indicate an issue is a regression Impact: High Blocks or significantly slows down a core workflow. Work: High It's not clear what the solution is; will need investigation.

Comments

@BenHenning
Copy link
Sponsor Member

Describe the bug

The Android asset fetching controller (/android_data endpoint) doesn't seem to correctly return multiple structure versions when requested.

Steps To Reproduce

Using curl:

curl "https://<base_oppia_url>/android_data?activity_type=subtopic&activities_data=%5B%7B%22id%22%3A%22C4fqwrvqWpRm-4%22%2C%22version%22%3A1%7D%2C%7B%22id%22%3A%22C4fqwrvqWpRm-4%22%2C%22version%22%3A2%7D%2C%7B%22id%22%3A%22C4fqwrvqWpRm-4%22%2C%22version%22%3A3%7D%2C%7B%22id%22%3A%22C4fqwrvqWpRm-4%22%2C%22version%22%3A4%7D%2C%7B%22id%22%3A%22C4fqwrvqWpRm-4%22%2C%22version%22%3A5%7D%2C%7B%22id%22%3A%22C4fqwrvqWpRm-4%22%2C%22version%22%3A6%7D%2C%7B%22id%22%3A%22C4fqwrvqWpRm-4%22%2C%22version%22%3A7%7D%2C%7B%22id%22%3A%22C4fqwrvqWpRm-4%22%2C%22version%22%3A8%7D%2C%7B%22id%22%3A%22C4fqwrvqWpRm-4%22%2C%22version%22%3A9%7D%5D" -H "X-ApiKey: <redacted_api_key>"

The above request is the following JSON string encoded:

"[{\"id\":\"C4fqwrvqWpRm-4\",\"version\":1},{\"id\":\"C4fqwrvqWpRm-4\",\"version\":2},{\"id\":\"C4fqwrvqWpRm-4\",\"version\":3},{\"id\":\"C4fqwrvqWpRm-4\",\"version\":4},{\"id\":\"C4fqwrvqWpRm-4\",\"version\":5},{\"id\":\"C4fqwrvqWpRm-4\",\"version\":6},{\"id\":\"C4fqwrvqWpRm-4\",\"version\":7},{\"id\":\"C4fqwrvqWpRm-4\",\"version\":8},{\"id\":\"C4fqwrvqWpRm-4\",\"version\":9}]"

Or, in formatted JSON:

[
  {
    "id":"C4fqwrvqWpRm-4",
    "version":1
  },
  {
    "id":"C4fqwrvqWpRm-4",
    "version":2
  },
  {
    "id":"C4fqwrvqWpRm-4",
    "version":3
  },
  {
    "id":"C4fqwrvqWpRm-4",
    "version":4
  },
  {
    "id":"C4fqwrvqWpRm-4",
    "version":5
  },
  {
    "id":"C4fqwrvqWpRm-4",
    "version":6
  },
  {
    "id":"C4fqwrvqWpRm-4",
    "version":7
  },
  {
    "id":"C4fqwrvqWpRm-4",
    "version":8
  },
  {
    "id":"C4fqwrvqWpRm-4",
    "version":9
  }
]

As shown, the request is trying to fetch versions 1-9 of subtopic C4fqwrvqWpRm-4 (subtopic 4 of topic C4fqwrvqWpRm).

However, the above curl request returns the following from the server:

{
  "C4fqwrvqWpRm-4":{
    "id":"C4fqwrvqWpRm-4",
    "topic_id":"C4fqwrvqWpRm",
    <pagedata...>
    "page_contents_schema_version":4,
    "language_code":"en",
    "version":9
  }
}

As seen, the response is only returning version 9.

Expected Behavior

The response should return all versions of the requested subtopic, not just the latest (or potentially last--I didn't verify this). I have verified that these other versions both exist on the server that was tested (backup migration server) and that they fetch through the standard subtopic controller.

Screenshots/Videos

No response

What device are you using?

Desktop

Operating System

Linux

What browsers are you seeing the problem on?

Other

Browser version

N/A

Additional context

No response

@BenHenning BenHenning added triage needed bug Label to indicate an issue is a regression labels May 17, 2023
@seanlip
Copy link
Member

seanlip commented May 17, 2023

I confirmed, through manual testing, that only the dict corresponding to the last item in the list is returned.

Here is the reason: https://github.com/oppia/oppia/blob/develop/core/controllers/android.py#L184

The handler seems to be assuming that activity_data versions don't repeat for the same ID. Therefore it is overwriting activities[activity_data['id']] with a new version each time. Here, activity_data['id'] is the same for all versions you're requesting, so that's why the return dict only has one element.

@BenHenning This should be fairly easy to fix, though I would advise not gating on it. What do you want the format of the return dict to be (here and in the other cases)?

@BenHenning
Copy link
Sponsor Member Author

Nice catch @seanlip.

I think that, in order to make the API more predictable, we should do away with the dict-like response and instead return a list of 1:1 responses for the requested ID/version pairs, in the same order as the request. Failures can be represented using null.

For the request structure:

[
  {
    "id": "<value>",
    "version": <version>
  },
  ...
]

We should have a corresponding response structure:

[
  {
    "id": "<value>",
    "version": <version>,
    "payload": {...}
  },
  ...
]

If a particular ID/version request cannot be fulfilled, the payload should be 'null'. The above format would allow for duplicate requests, but I think we should detect that and fail with a 400 in that case (since the script should make an effort not to request duplicates).

Do you think the above would work?

@seanlip
Copy link
Member

seanlip commented May 18, 2023

Yup this structure seems fine to me. Thanks!

@kshitij01042002 kshitij01042002 added Impact: High Blocks or significantly slows down a core workflow. Work: High It's not clear what the solution is; will need investigation. labels May 25, 2023
@seanlip seanlip self-assigned this May 28, 2023
BenHenning added a commit to oppia/oppia-android that referenced this issue May 31, 2023
Specifically:
- Temporarily disables version fallback (to detect issues with the
  script).
- Fixed multiple incompatibility issues, including adding a couple of
  temporary exemptions for the upcoming release. This also included
  fixing the invalid HTML tag regex pattern.
- Disabled classroom support since the production math classroom isn't
  yet available via the new classroom API.
- Added strong progress tracking for all expensive operations for a much
  easier time when using the script.
- Added on-disk server result caching for much faster subsequent runs
  (and to put much less stress on the remote server). This included
  adding support for converting downloaded structures back to JSON.
- Enabled image downloading.
- Added outputting proto v2 pb and textproto files (legacy proto
  conversion still needs to be added).
- Tuned parallelization to better fit hardware since otherwise a lot of
  strain is put on the OS's scheduler with the script's coroutine
  behaviors.
- Sped up some of the slower parts (JSON to proto conversion) of the
  script by better utilizing parallel coroutines.
- Added support for batch version fetching from remote, though it's
  currently disabled since there's an issue with using this:
  oppia/oppia#18241.
- Silence illegal reflection access warning caused by Retrofit and made
  other quality-of-life improvements to script output.

There's still more analysis required before this script can be
considered done from an alpha perspective, including:
- Double-checking constraint checking is working as expected (current
  reasoning suggests the language comprehension check isn't working).
- Adding legacy proto conversion & output support.
- Verifying that all failing responses from the emulated server are
  correctly failing.
- Verifying that the structures and images import correctly in the app.
@gp201 gp201 closed this as completed in 88231e1 Jun 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Label to indicate an issue is a regression Impact: High Blocks or significantly slows down a core workflow. Work: High It's not clear what the solution is; will need investigation.
Projects
Archived in project
3 participants