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

Adopt downloadconfig API v2 #85

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bionade24
Copy link
Collaborator

Fix key for downloadconfig URL: make it dynamic dependent on the setting of the country code in AppSettings

@bionade24 bionade24 changed the title Remove hardcoded country code in downloadconfig.cpp Bugfix: Remove hardcoded country code in downloadconfig.cpp Feb 1, 2021
@llewelld
Copy link
Owner

llewelld commented Feb 1, 2021

Thank you for this. However, it seems there's no app_config file available under the EUR path, only under the DE path. I assume the DE configuration is intended to be used in all cases.

Doesn't serve a file for me: https://svc90.main.px.t-online.de/version/v1/configuration/country/EUR/app_config
Compared to this: https://svc90.main.px.t-online.de/version/v1/configuration/country/DE/app_config

@bionade24
Copy link
Collaborator Author

bionade24 commented Feb 1, 2021

Yeah, I wasn't sure about that (see my other comment at the commit related to this). I then digged in the Android code once again with my zero Kotlin knowledge and couldn't see that this URL shouldn't be affected by EUR. I'll convert to draft and close if I can't find anything supporting my intention in the official code.

@bionade24 bionade24 marked this pull request as draft February 1, 2021 22:58
@bionade24
Copy link
Collaborator Author

Ok, I now digged more into the Android code and was suprised that the URL above is never used! I then looked for it at the server code and immedately found this test:
https://github.com/corona-warn-app/cwa-server/blob/ea3bf192aa27f686427aaf70a49f741214b60a66/services/distribution/src/test/java/app/coronawarn/server/services/distribution/objectstore/publish/LocalFileTest.java

Is app_config even used and correct?

@bionade24
Copy link
Collaborator Author

Ok, I looked into the S3 code of the server and it seems like this API called here is just somehow unused or deprecated, the iOS and Android specific paths don't cotain country codes, so they maybe just forgot it.

@bionade24 bionade24 closed this Feb 2, 2021
@bionade24
Copy link
Collaborator Author

Ok, I looked at https://github.com/corona-warn-app/cwa-server/blob/ea3bf192aa27f686427aaf70a49f741214b60a66/services/distribution/api_v1.json

There is https://svc90.main.px.t-online.de/version/v1/configuration/app_config
Which looks similar to version/v1/app_config_android but doesn't return anything.
Are you @llewelld sure that the DE API contains the values for EUR, too?

@bionade24
Copy link
Collaborator Author

I got a bit more information. Yes, DE is correct for every country now. They have a version V2 of the API, which uses the data with the os suffix. I fear they'll drop the old scheme sooner or later and we have so see where it's going.

I'll try to get more information on why version/v1/configuration/app_config doesn't work despite it's described. If it would work, it would be the go-to solution.

@bionade24 bionade24 reopened this Feb 3, 2021
@bionade24 bionade24 changed the title Bugfix: Remove hardcoded country code in downloadconfig.cpp Adopt downloadconfig API v2 Feb 4, 2021
@llewelld
Copy link
Owner

llewelld commented Feb 6, 2021

Thanks for looking in to this and also for flagging up this difference between the v1 and v2 APIs. Yes, it looks like we should be transitioning to the v2 API. There are similarities between the two, but the main differences seem to relate to the risk calculation parameters.

Unfortunately the documentation still references the values from the v1 file, so it's not clear to me how the calculation needs to be updated to make use of the values in the v2 files at the moment. The parameters provided are very different, and their names don't offer enough hints, to me at least.

For reference, the proto files needed for decoding both the v1 app_config and the v2 app_config_android files can be found in the Android app source proto buffer folder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants