Skip to content
This repository has been archived by the owner on Jan 10, 2024. It is now read-only.

Migration to eat api #1422

Closed
wants to merge 113 commits into from
Closed

Conversation

Liqs-v2
Copy link
Contributor

@Liqs-v2 Liqs-v2 commented Jan 12, 2022

Issue

resolves: #1370

Why this is useful for all students

Migrating to the latest, still maintained API for getting cafeteria data ensures this component will keep functioning for the forseeable future.

Known issues

  1. CafeteriaCard is not being displayed in the landing
    • This is happening, because menus are no longer being downloaded in bulk at application startup and instead on-the-fly on a by cafeteria basis. This leads to the problem that on the home screen intially, there are no menus stored in the RoomDB yet and a download for the default location needs to be initiated to fix this. A first fix is already in the codebase, however I was unable to verify it, because of issue 2.
  2. Cafeteria menus are no longer downloading
    • Previously this was already working, now I am getting a rather cryptic error message telling me that the response was invalid/unable to be fetched via HTTP (for the error messsage see below, this occured on multiple devices and network connections).
    • 2022-06-15 16:06:12.712 14801-14876/de.tum.in.tumcampus E/AuthenticationManager$$ExternalSyntheticLambda1: javax.net.ssl.SSLHandshakeException: Chain validation failed javax.net.ssl.SSLHandshakeException: Chain validation failed at com.android.org.conscrypt.SSLUtils.toSSLHandshakeException(SSLUtils.java:362) at com.android.org.conscrypt.ConscryptEngine.convertException(ConscryptEngine.java:1134) at com.android.org.conscrypt.ConscryptEngine.readPlaintextData(ConscryptEngine.java:1089) at com.android.org.conscrypt.ConscryptEngine.unwrap(ConscryptEngine.java:876) at com.android.org.conscrypt.ConscryptEngine.unwrap(ConscryptEngine.java:747) at com.android.org.conscrypt.ConscryptEngine.unwrap(ConscryptEngine.java:712) at com.android.org.conscrypt.ConscryptEngineSocket$SSLInputStream.processDataFromSocket(ConscryptEngineSocket.java:849) at com.android.org.conscrypt.ConscryptEngineSocket$SSLInputStream.access$100(ConscryptEngineSocket.java:722) at com.android.org.conscrypt.ConscryptEngineSocket.doHandshake(ConscryptEngineSocket.java:238) at com.android.org.conscrypt.ConscryptEngineSocket.startHandshake(ConscryptEngineSocket.java:217) at okhttp3.internal.connection.RealConnection.connectTls(RealConnection.kt:379) at okhttp3.internal.connection.RealConnection.establishProtocol(RealConnection.kt:337) at okhttp3.internal.connection.RealConnection.connect(RealConnection.kt:209) at okhttp3.internal.connection.ExchangeFinder.findConnection(ExchangeFinder.kt:226) at okhttp3.internal.connection.ExchangeFinder.findHealthyConnection(ExchangeFinder.kt:106) at okhttp3.internal.connection.ExchangeFinder.find(ExchangeFinder.kt:74) at okhttp3.internal.connection.RealCall.initExchange$okhttp(RealCall.kt:255) at okhttp3.internal.connection.ConnectInterceptor.intercept(ConnectInterceptor.kt:32) at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:109) at okhttp3.internal.cache.CacheInterceptor.intercept(CacheInterceptor.kt:95) at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:109) at okhttp3.internal.http.BridgeInterceptor.intercept(BridgeInterceptor.kt:83) at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:109) at okhttp3.internal.http.RetryAndFollowUpInterceptor.intercept(RetryAndFollowUpInterceptor.kt:76) at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:109) at de.tum.in.tumcampusapp.api.app.ChaosMonkeyInterceptor.intercept(ChaosMonkeyInterceptor.kt:27) at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:109) at de.tum.in.tumcampusapp.api.app.ApiHelper.lambda$getDeviceInterceptor$2(ApiHelper.java:97) at de.tum.in.tumcampusapp.api.app.ApiHelper.$r8$lambda$Ed6Z45oC4dmTmrD4C58bWLoLAoM(Unknown Source:0) at de.tum.in.tumcampusapp.api.app.ApiHelper$$ExternalSyntheticLambda1.intercept(Unknown Source:4) at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:109) at de.tum.in.tumcampusapp.api.app.ApiHelper.lambda$disableGzip$1(ApiHelper.java:72) at de.tum.in.tumcampusapp.api.app.ApiHelper.$r8$lambda$OUC4myTOllc53YNAK_DOFdZp_xU(Unknown Source:0) at de.tum.in.tumcampusapp.api.app.ApiHelper$$ExternalSyntheticLambda2.intercept(Unknown Source:0) at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:109) at okhttp3.internal.connection.RealCall.getResponseWithInterceptorChain$okhttp(RealCall.kt:201) at okhttp3.internal.connection.RealCall.execute(RealCall.kt:154) at retrofit2.OkHttpCall.execute(OkHttpCall.java:204) at retrofit2.adapter.rxjava2.CallExecuteObservable.subscribeActual(CallExecuteObservable.java:46) at io.reactivex.Observable.subscribe(Observable.java:12267) at retrofit2.adapter.rxjava2.BodyObservable.subscribeActual(BodyObservable.java:35) at io.reactivex.Observable.subscribe(Observable.java:12267) at io.reactivex.internal.operators.observable.ObservableFlatMap$MergeObserver.subscribeInner(ObservableFlatMap.java:165) 2022-06-15 16:06:12.712 14801-14876/de.tum.in.tumcampus E/AuthenticationManager$$ExternalSyntheticLambda1: at io.reactivex.internal.operators.observable.ObservableFlatMap$MergeObserver.onNext(ObservableFlatMap.java:139) at io.reactivex.internal.operators.observable.ObservableDoOnEach$DoOnEachObserver.onNext(ObservableDoOnEach.java:101) at io.reactivex.internal.operators.observable.ObservableSubscribeOn$SubscribeOnObserver.onNext(ObservableSubscribeOn.java:58) at io.reactivex.internal.operators.observable.ObservableFilter$FilterObserver.onNext(ObservableFilter.java:52) at io.reactivex.internal.operators.observable.ObservableScalarXMap$ScalarDisposable.run(ObservableScalarXMap.java:248) at io.reactivex.internal.operators.observable.ObservableJust.subscribeActual(ObservableJust.java:35) at io.reactivex.Observable.subscribe(Observable.java:12267) at io.reactivex.internal.operators.observable.ObservableFilter.subscribeActual(ObservableFilter.java:30) at io.reactivex.Observable.subscribe(Observable.java:12267) at io.reactivex.internal.operators.observable.ObservableSubscribeOn$SubscribeTask.run(ObservableSubscribeOn.java:96) at io.reactivex.Scheduler$DisposeTask.run(Scheduler.java:578) at io.reactivex.internal.schedulers.ScheduledRunnable.run(ScheduledRunnable.java:66) at io.reactivex.internal.schedulers.ScheduledRunnable.call(ScheduledRunnable.java:57) at java.util.concurrent.FutureTask.run(FutureTask.java:266) at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:301) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641) at java.lang.Thread.run(Thread.java:923) Caused by: java.security.cert.CertificateException: Chain validation failed at com.android.org.conscrypt.TrustManagerImpl.verifyChain(TrustManagerImpl.java:724) at com.android.org.conscrypt.TrustManagerImpl.checkTrustedRecursive(TrustManagerImpl.java:554) at com.android.org.conscrypt.TrustManagerImpl.checkTrustedRecursive(TrustManagerImpl.java:575) at com.android.org.conscrypt.TrustManagerImpl.checkTrustedRecursive(TrustManagerImpl.java:620) at com.android.org.conscrypt.TrustManagerImpl.checkTrusted(TrustManagerImpl.java:510) at com.android.org.conscrypt.TrustManagerImpl.checkTrusted(TrustManagerImpl.java:428) at com.android.org.conscrypt.TrustManagerImpl.getTrustedChainForServer(TrustManagerImpl.java:356) at android.security.net.config.NetworkSecurityTrustManager.checkServerTrusted(NetworkSecurityTrustManager.java:94) at android.security.net.config.RootTrustManager.checkServerTrusted(RootTrustManager.java:90) at com.android.org.conscrypt.ConscryptEngineSocket$2.checkServerTrusted(ConscryptEngineSocket.java:161) at com.android.org.conscrypt.Platform.checkServerTrusted(Platform.java:250) at com.android.org.conscrypt.ConscryptEngine.verifyCertificateChain(ConscryptEngine.java:1644) at com.android.org.conscrypt.NativeCrypto.ENGINE_SSL_read_direct(Native Method) at com.android.org.conscrypt.NativeSsl.readDirectByteBuffer(NativeSsl.java:568) at com.android.org.conscrypt.ConscryptEngine.readPlaintextDataDirect(ConscryptEngine.java:1095) at com.android.org.conscrypt.ConscryptEngine.readPlaintextData(ConscryptEngine.java:1079) ... 58 more Caused by: java.security.cert.CertPathValidatorException: Response is unreliable: its validity interval is out-of-date at sun.security.provider.certpath.PKIXMasterCertPathValidator.validate(PKIXMasterCertPathValidator.java:135) at sun.security.provider.certpath.PKIXCertPathValidator.validate(PKIXCertPathValidator.java:222) at sun.security.provider.certpath.PKIXCertPathValidator.validate(PKIXCertPathValidator.java:140) at sun.security.provider.certpath.PKIXCertPathValidator.engineValidate(PKIXCertPathValidator.java:79) at java.security.cert.CertPathValidator.validate(CertPathValidator.java:301) at com.android.org.conscrypt.TrustManagerImpl.verifyChain(TrustManagerImpl.java:720) ... 73 more 2022-06-15 16:06:12.713 14801-14876/de.tum.in.tumcampus E/AuthenticationManager$$ExternalSyntheticLambda1: Caused by: java.security.cert.CertPathValidatorException: Response is unreliable: its validity interval is out-of-date at sun.security.provider.certpath.OCSPResponse.verify(OCSPResponse.java:619) at sun.security.provider.certpath.RevocationChecker.checkOCSP(RevocationChecker.java:709) at sun.security.provider.certpath.RevocationChecker.check(RevocationChecker.java:363) at sun.security.provider.certpath.RevocationChecker.check(RevocationChecker.java:337) at sun.security.provider.certpath.PKIXMasterCertPathValidator.validate(PKIXMasterCertPathValidator.java:125) ... 78 more Suppressed: java.security.cert.CertPathValidatorException: Unable to determine revocation status due to network error at sun.security.provider.certpath.RevocationChecker.checkCRLs(RevocationChecker.java:562) at sun.security.provider.certpath.RevocationChecker.checkCRLs(RevocationChecker.java:465) at sun.security.provider.certpath.RevocationChecker.check(RevocationChecker.java:394) ... 80 more Caused by: sun.security.provider.certpath.PKIX$CertStoreTypeException: java.io.IOException: Cleartext HTTP traffic to crl4.digicert.com not permitted at sun.security.provider.certpath.URICertStore.engineGetCRLs(URICertStore.java:430) at java.security.cert.CertStore.getCRLs(CertStore.java:190) at sun.security.provider.certpath.DistributionPointFetcher.getCRL(DistributionPointFetcher.java:245) at sun.security.provider.certpath.DistributionPointFetcher.getCRLs(DistributionPointFetcher.java:189) at sun.security.provider.certpath.DistributionPointFetcher.getCRLs(DistributionPointFetcher.java:121) at sun.security.provider.certpath.RevocationChecker.checkCRLs(RevocationChecker.java:552) ... 82 more Caused by: java.io.IOException: Cleartext HTTP traffic to crl4.digicert.com not permitted at com.android.okhttp.HttpHandler$CleartextURLFilter.checkURLPermitted(HttpHandler.java:127) at com.android.okhttp.internal.huc.HttpURLConnectionImpl.execute(HttpURLConnectionImpl.java:462) at com.android.okhttp.internal.huc.HttpURLConnectionImpl.getResponse(HttpURLConnectionImpl.java:411) at com.android.okhttp.internal.huc.HttpURLConnectionImpl.getInputStream(HttpURLConnectionImpl.java:248) at sun.security.provider.certpath.URICertStore.engineGetCRLs(URICertStore.java:396) ... 87 more
  3. Cafeteria Pricing is not displayed next to dishes
    • As a result of the migration this section was broken and still needs updating (Fetching, storing somehow and updating frontend code)
  4. FavoriteDish functionality no longer works after migration
    • As a result of the migration this section was broken and still needs updating (Fetching, storing somehow and updating frontend code)
  5. Location table and its API are outdated
    • Because the eat-API uses new ids for cafeterias (and also uses slugs) the Location table, that is also referencing cafeteria locations needs to be updated.
    • Unfortunately this data is provided by an API that still provides the outdated ids and I was thus unable to migrate this table as part of this PR.

I would suggest to tackle issue 1+2 in a single PR or potentially add the fix to this one and 3-5 as separate PRs.

Caveat

There probably are some performance issues with the downloading of the menus, that could potentially be moved into a seperate thread and done asynchronously. I have not implemented it in this way so far, due to my inexperience in Android and Kotlin development.

Screenrecordings

Migrated version (before issue 2 started occuring)

eat-api_migration

@Liqs-v2
Copy link
Contributor Author

Liqs-v2 commented Jan 12, 2022

This is still a very rough draft with lots missing, however I would still appreciate some feedback on the direction I have taken with changing the CafeteriaMenu table as well as the migration class that was added. As potential changes needed here are rather easy to do now, but will be increasingly more work as go through with migrating more of the codebase.

@joschahenningsen @kordianbruck

@joschahenningsen
Copy link
Member

This looks great already! You're definitely going in the right direction. The only concern I have is the request size: all.json is 3.5MB (130KB gzipped) whereas the old API is just 205 KB (15 gzipped). Loading this payload in the ubahn on my way to Garching sounds painful :D Also, all.json will grow indefinitely.

This is a little more work but I think it would be best to load only the weeks that are needed right now (perhaps with a week or two padding).

@Liqs-v2
Copy link
Contributor Author

Liqs-v2 commented Jan 12, 2022

Good point, will look into that.

@Liqs-v2
Copy link
Contributor Author

Liqs-v2 commented Jan 12, 2022

So it seems like the all.json from the eat-api already only contains the current week, the two past and two upcoming weeks in terms of data it provides. It seems like it just contains a lot more data in general.

For example

  • labels: New field containing an array of dish labels (typically allergens) for each dish
  • price: Is now provided for each dish instead of once and in a standalone field

all.json contains about 1000 more dishes than the old api. Old API: ~1500 entries vs Eat-API: ~2900 entries, which is what could be attributed to the 2 week padding that you suggested, as the old API has no backwards padding and only provides the next two weeks from the current date onwards. I will check in with the Eat-API people and ask whether Eat-API all.json always provides a 2 week padding or if this is some behaviour that one would better not rely on.

all_ref.json could be an alternative, as its backwards padding is only 1 day (ie yesterday) and apart from that only provides future menus. It however still is 1.9 MB.

@Liqs-v2
Copy link
Contributor Author

Liqs-v2 commented Jan 13, 2022

@joschahenningsen Okay, so all.json apparently does not have a specified padding. I would propose using all_ref.json as that is already very close to what you are suggesting.

Sadly that is still a lot larger than the old API, but since it is already only using 2 days padding for the future and one day backwards, I dont really know how to reduce it even further.

Potentially only using 1 week could shave off another couple hundred KB, but would also be a lot more work to implement.

@joschahenningsen
Copy link
Member

Ahh thanks for clearing that up @Liqs-v2

The IOS app seems to load the canteen-specific week when needed:
https://github.com/TUM-Dev/Campus-iOS/blob/38a3606ed3d043305724b4bb44a127dfe3afda2f/TUM%20Campus%20App/Cafeteria/MealPlanTableViewController.swift#L40
(e.g. https://tum-dev.github.io/eat-api/mensa-garching/2022/02.json, which is just 70kb (4 gzipped))

If that's too much work, all_ref should do as well though.

@Liqs-v2
Copy link
Contributor Author

Liqs-v2 commented Jan 13, 2022

Alright I ll take a look at it. This will probably be more difficult to do though, because the old API used to just load everything, meaning this will be more of a rewrite.

@Liqs-v2
Copy link
Contributor Author

Liqs-v2 commented Jan 13, 2022

But yea I agree loading for a canteen when needed is a better way of handling this.

@joschahenningsen
Copy link
Member

Alright I ll take a look at it. This will probably be more difficult to do though, because the old API used to just load everything, meaning this will be more of a rewrite.

Lmk if there's any way I can assist here :)

@joschahenningsen
Copy link
Member

Seems like using all_ref might be a problem:
TUM-Dev/eat-api#108

Side note: I opened TUM-Dev/eat-api#109

@Liqs-v2
Copy link
Contributor Author

Liqs-v2 commented Jan 13, 2022

Actually there is a thing that I need help with. The Eat-API uses a cafeteria slug as id (e.g: mensa-garching). That means I would need to migrate the Cafeteria table too, since it currently uses an integer cafeteriaId from the old API. However the endpoint this table gets its data from is also outdated then. But I do not have access and or dont know where to find the code for that to change it. So I would be very grateful if u could point me to the repo and give me access or help me update the API. Thanks :)

@joschahenningsen
Copy link
Member

joschahenningsen commented Jan 13, 2022

Sure thing! The old api deployed is not open source, that's why we currently rebuild the backend over at https://github.com/TUM-Dev/Campus-Backend/ I can take care of a new endpoint that returns the right cafeteria ids, or we schedule a meeting and I show you around the backend if you want to implement this yourself :)

Once I got #1423 to compile it should be fairly easy to implement a new endpoint.

@Liqs-v2
Copy link
Contributor Author

Liqs-v2 commented Jan 13, 2022

Lets have a meeting sometime during the weekend, preferably Saturday. Always good to know how to do that for the future :)

@Liqs-v2
Copy link
Contributor Author

Liqs-v2 commented Jan 13, 2022

Sure thing! The old api deployed is not open source, that's why we currently rebuild the backend over at https://github.com/TUM-Dev/Campus-Backend/ I can take care of a new endpoint that returns the right cafeteria ids, or we schedule a meeting and I show you around the backend if you want to implement this yourself :)

Once I got #1423 to compile it should be fairly easy to implement a new endpoint.

Alright, I guess that means the only real way of doing it is rewriting to do lazy fetching.

@joschahenningsen
Copy link
Member

Saturday works for me, but I think https://tum-dev.github.io/eat-api/canteens.json could be used instead of adapting the API.

@Liqs-v2
Copy link
Contributor Author

Liqs-v2 commented Jan 13, 2022

That seems promising indeed, I ll see if I can change the fetching to use this enpoint instead.

@Liqs-v2
Copy link
Contributor Author

Liqs-v2 commented Jan 20, 2022

The switch from an integer cafeteriaId to a string cafeteriaId is causing me some problems, as it comes with a huge cascade of changes throught the entire project and database. Might it be better to introduce some sort of a mapping table for the old cafeteriaId to the new cafeteriaId and leave the components mostly alone?

I am not sure what the best practice is in this case and would appreciate some input. @joschahenningsen

@CommanderStorm
Copy link
Member

@Liqs-v2 What exactly is still needed on this issue?
What are concrete tasks I can do to get this PR to a state where this can be merged into master?

@Liqs-v2
Copy link
Contributor Author

Liqs-v2 commented Jun 8, 2022

Theres a bug causing the cafeteria card to not be shown which I am still investigating. Apart from that there are some other known issues that should be addressed once this PR is merged. Please be patient for a bit longer.

@CommanderStorm
Copy link
Member

I am sorry If you persieved my message as threatening.
It was not meant that way. I just thought that maybe I can help out with one of the remaining issues. xD
Sorry..

@Liqs-v2
Copy link
Contributor Author

Liqs-v2 commented Jun 8, 2022

I didnt, no worries. All i meant to say is that I think it makes more sense to work on new PRs once this one is merged in, since it is rather old by now. For the other issues seperate PRs would probably be better to keep things small-ish.

@Liqs-v2 Liqs-v2 force-pushed the issue/1370_switch_to_eat-api branch from 0adfb27 to edf9aca Compare July 2, 2022 11:02
@Liqs-v2
Copy link
Contributor Author

Liqs-v2 commented Jul 2, 2022

Alright, so I finally found some time to take another look at this over the past few weeks. I found out why the CafeteriaCard is no longer showing in the Home (Landing) screen and also attempted to implement a fix, but now the Cafeteria component is no longer able to download the eat-api data either. The error seems like some certificate issue, so I was wondering if there were any changes to the eat-api in that regard.

Regardless of this, because I atleast know which section is causing the issue and in the interest of time, I have decided to just put this PR up for review and add this issue to the list of known issues, that I would have put up as a disclaimer anyways.

This means that now would be a good time to jump in and help, if ur still down for that @CommanderStorm :)

@Liqs-v2 Liqs-v2 marked this pull request as ready for review July 2, 2022 11:26
@CommanderStorm CommanderStorm changed the title Issue/1370 switch to eat api Migrate to eat api Jul 4, 2022
@CommanderStorm CommanderStorm changed the title Migrate to eat api Migration to eat api Jul 4, 2022
@CommanderStorm
Copy link
Member

The file permmissions for gradlew seem to be changed in edf9aca. 😉
Could you drop the commit to make shure the build (incl klint) runs?

@CommanderStorm
Copy link
Member

One of the reasons, why this issue exists you noticed, is that I dont think we currently give feedback to the user, that a menu is not availiable (e.g. mediziner mensa).
This looks suspiciously like a certificate error in the log.
How should this be handled? Retry 3 times and then show an error message or immedeatly show one?

@Liqs-v2
Copy link
Contributor Author

Liqs-v2 commented Jul 5, 2022

The file permmissions for gradlew seem to be changed in edf9aca. 😉 Could you drop the commit to make shure the build (incl klint) runs?

Ah yea, I'll take another look at that. My project was being all weird since I am trying out Windows again since April and I had Linux before, so it didnt really like that.

@Liqs-v2
Copy link
Contributor Author

Liqs-v2 commented Jul 5, 2022

One of the reasons, why this issue exists you noticed, is that I dont think we currently give feedback to the user, that a menu is not availiable (e.g. mediziner mensa). This looks suspiciously like a certificate error in the log. How should this be handled? Retry 3 times and then show an error message or immedeatly show one?

I'll try to see if this error can be reduced to occuring for certain cafeterias and menu availability. But it has been occuring for mensa-garching, which typically does pretty decently with providing menus.

@CommanderStorm
Copy link
Member

But it has been occuring for mensa-garching, which typically does pretty decently with providing menus.

Yep, this is why I said one of the issues ;)

@COM8
Copy link
Member

COM8 commented Sep 22, 2022

Any updates on this topic?

@@ -2,18 +2,12 @@
"formatVersion": 1,
"database": {
"version": 6,
"identityHash": "f743f786b0866afd26a4cc1c1fafc8d1",
"identityHash": "07b6e7b518ae4df546549d9a64c87963",
Copy link
Member

Choose a reason for hiding this comment

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

As far as I know, existing migrations should not be changed. Could you have another look into why this was changed?

@Liqs-v2
Copy link
Contributor Author

Liqs-v2 commented Sep 22, 2022

No, not really. I tried what @CommanderStorm mentioned, but it didn't work and I also didn't have any other ideas.

I can take another look in the time after my exam and before the semester starts, but quite frankly, my priorities have shifted and I have other commitments at this time and will probably not be contributing much in the coming semester.

@COM8 COM8 mentioned this pull request Sep 30, 2022
10 tasks
@CommanderStorm
Copy link
Member

closing in favor of #1583

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

Successfully merging this pull request may close these issues.

Switch to Eat API
4 participants