Respect expires header when checking for new versions

[XiangRongLin]

What is it?

• Bugfix (user facing)
• Feature (user facing)
• Codebase improvement (dev facing)
• Meta improvement to the project (dev facing)

Description of the changes in your PR

• Save expires header into preferences on version check
  • Coerce it between 6 and 72 hours
• Check expires date before making network request to check for new versions

Fixes the following issue(s)

• Update check is run too often #5467

Due diligence

• I read the contribution guidelines.

I'd to do some refactoring and add some test if time permitts.

APK

https://github.com/XiangRongLin/NewPipe/actions/runs/501846770
XiangRongLin#10

this apk ignores usual checks for when to run the request

[TobiGr]

Looks good to me.
Can you also add a key that stores the time of the last request? That allows to hande a misconfiguration of the expires header. I'd say the last request should be not older than 72 hours, but also at least 2 hours old.

expires and isGitHubApk could be class variables.

[XiangRongLin]

Can you also add a key that stores the time of the last request? That allows to hande a misconfiguration of the expires header. I'd say the last request should be not older than 72 hours, but also at least 2 hours old.

Right, right there was something about that in the issue... Completly forgot about that 😅

expires and isGitHubApk could be class variables.

As in static?

[XiangRongLin]

@TheAssassin Anything against using the cache-control header, since that one is currently also set with max-age=21600 (in seconds=>6hours)

With that one the app can calculate the expiry datetime itself. During this calculation it can take into account the lower/upper limits. So i only have to save a single preference and as a whole i think the code is cleaner because of less if conditions to check

[Stypox]

I'd say the last request should be not older than 72 hours, but also at least 2 hours old.

Citing @TheAssassin from #5467

I could imagine having a lower limit of 6 hours, as well as a top limit of 48 or even 72 hours.

[TheAssassin]

@XiangRongLin I don't really care. I remembered Expires as it's an HTTP 1.0 header, but HTTP 1.1's Cache-Control serves basically the same purpose. I could imagine that the expires directive in nginx sets both headers. I presume max-age is easier to parse.

Just let me know which one you want to use, we can set up either of them. But if you go with Cache-Control, you should implement all its options. Otherwise, if, e.g., nginx changes and provides different values, your parser won't be able to deal with the data any more. Expires is a lot simpler and only provides timestamps in a defined format.

Usually, both are set anyway by webservers. Browsers, which implement a proper parser, usually prefer Cache-Control, as it provides more options and is also more suitable for dynamic content. I often preferred to use Expires for static and API stuff. As said, it's easier to implement IMO.

Edit: yes, I'd go for a 6 hours lower boundary. That's more than good enough.

[XiangRongLin]

expires it is then. Before i implement all different types of cache-control, I'm going to add some if conditions

[TheAssassin]

You should add more comments to your code. NewPipe is pretty bad at this. Please check out https://standards.mousepawmedia.com/csi.html.

Here, you could like above state that you check whether a timeout has been met already.

It does seem a bit wasteful to always re-parse the string. But I presume it's just easier to do that, so you can conveniently use isAfter.

[XiangRongLin]

It does seem a bit wasteful to always re-parse the string. But I presume it's just easier to do that, so you can conveniently use isAfter

When i added a log statement to the new version check it is only run at startup of the app and not on rotation, meaning very rarely. Is the tradeof of using a little bit more ram constantly compared to a little bit of cpu once good?

@B0pol How did you come to think thats it's run on device rotation

[TheAssassin]

As said, I don't mind this. In fact, I probably reconstructed your thinking.

It was an example of what kind of comment is missing there. I can see what you are doing, but only guess why you are doing it like that. It showcases perfectly why CSI matters.

[B0pol]

@XiangRongLin I was wrong about device rotation, it is the case for MainFragment, MainActivity and many other classes .onCreate() methods, but not App.onCreate()

[XiangRongLin]

I prefer to write the code in a way that does not require comments. In that case maybe add a seperate method with a name, which makes the comment not needed.

I did not do it here at first, since the issue was labelled as ASAP. But i forgot that i have time until at least saturday when the release needs to be done

[TheAssassin]

I prefer to write the code in a way that does not require comments. In that case maybe add a seperate method with a name, which makes the comment not needed.

This is just not possible. And very much not a matter of preference.

Do I really have to quote CSI? You don't seem to have had a look:

“Self-Commenting Code” is a practice wherein a code’s functionality is self-evident. Through naming, structure, and various other techniques, the immediate purpose becomes obvious to the reader. This is beneficial in any language.

However, “Self-Commenting Code” is seldom capable of expressing the entire intent, or “why”, of the code.

It is nearly impossible to express the intended behavior of the code;

-- https://standards.mousepawmedia.com/csi.html#csi-vs-self-commenting-code

Again, your code tells nothing about why you're doing it like that. It cost me valuable time guess the why, and that's then still just a guess.

In practice, intent-commenting saves me considerably more time than it takes! It's a true investment in the future: an extra 10-20 minutes now saves me literal hours later.

-- https://dev.to/codemouse92/to-comment-or-not-to-comment-3f7h

[TheAssassin]

The entire implementation looks very, very basic. I'd recommend to at least check the HTTP status. Anything other than 200 can be considered an error. You don't have to attempt to parse JSON from plain text. It makes no sense.

Also, I miss a lot of error handling in there. You make a lot of assumptions about how the JSON has to look. Does that JSON parser exception also show up when a certain key is missing in there? I'm not so sure.

The code also lacks a lot of comments that explain what you're trying to accomplish there, and especially why. I recommend most people to read https://standards.mousepawmedia.com/csi.html. CSI is usually perceived as a bit overkill with regards to the amount of comments. But in general, I very much agree with the author's arguments. I try to implement form of "CSI light" in all my projects. That makes them a lot more beginner friendly.

[TobiGr]

I agree with @TheAssassin that there should be a basic status code check.
E.g. when there is an internal server error, we should not check back the next time the method is called, but also implement a short cooldown of e.g. 10 min.

Depending on how much time you have to implement this, we could also ship this improvement with the next update.

I've also suggested two comments to clarify why we check and store expiry.

[XiangRongLin]

I agree with TheAssassin that there should be a basic status code check.
E.g. when there is an internal server error, we should not check back the next time the method is called, but also implement a short cooldown of e.g. 10 min.

Depending on how much time you have to implement this, we could also ship this improvement with the next update.

@TobiGr
For me that's a seperate feature request and should be treated as such by opening a seperate issue/PR for it.
Since i don't know from the top of my head what the possible responses are or where to look them up, i don't see it as small enough to let it just tag along this PR.

[TobiGr]

I don't have permission to push to this branch, will open a new PR which adds an one hour cooldown when there are server side or connectivity problems:

diff --git a/app/src/main/java/org/schabi/newpipe/CheckForNewAppVersion.java b/app/src/main/java/org/schabi/newpipe/CheckForNewAppVersion.java
index 63baef5479..11be734ae9 100644
--- a/app/src/main/java/org/schabi/newpipe/CheckForNewAppVersion.java
+++ b/app/src/main/java/org/schabi/newpipe/CheckForNewAppVersion.java
@@ -10,19 +10,22 @@ import android.content.pm.Signature;
 import android.net.ConnectivityManager;
 import android.net.Uri;
 import android.util.Log;
+
 import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
 import androidx.core.app.NotificationCompat;
 import androidx.core.app.NotificationManagerCompat;
 import androidx.core.content.ContextCompat;
 import androidx.preference.PreferenceManager;
+
 import com.grack.nanojson.JsonObject;
 import com.grack.nanojson.JsonParser;
 import com.grack.nanojson.JsonParserException;
-import io.reactivex.rxjava3.android.schedulers.AndroidSchedulers;
-import io.reactivex.rxjava3.core.Maybe;
-import io.reactivex.rxjava3.disposables.Disposable;
-import io.reactivex.rxjava3.schedulers.Schedulers;
+
+import org.schabi.newpipe.report.ErrorActivity;
+import org.schabi.newpipe.report.ErrorInfo;
+import org.schabi.newpipe.report.UserAction;
+
 import java.io.ByteArrayInputStream;
 import java.io.InputStream;
 import java.security.MessageDigest;
@@ -31,9 +34,13 @@ import java.security.cert.CertificateEncodingException;
 import java.security.cert.CertificateException;
 import java.security.cert.CertificateFactory;
 import java.security.cert.X509Certificate;
-import org.schabi.newpipe.report.ErrorActivity;
-import org.schabi.newpipe.report.ErrorInfo;
-import org.schabi.newpipe.report.UserAction;
+import java.time.Instant;
+import java.time.temporal.ChronoUnit;
+
+import io.reactivex.rxjava3.android.schedulers.AndroidSchedulers;
+import io.reactivex.rxjava3.core.Maybe;
+import io.reactivex.rxjava3.disposables.Disposable;
+import io.reactivex.rxjava3.schedulers.Schedulers;
 
 public final class CheckForNewAppVersion {
     private CheckForNewAppVersion() { }
@@ -236,10 +243,17 @@ public final class CheckForNewAppVersion {
                             }
                         },
                         e -> {
-                            // connectivity problems, do not alarm user and fail silently
+                            // connectivity or server side problems
+                            // do not alarm user and fail silently
                             if (DEBUG) {
                                 Log.w(TAG, "Could not get NewPipe API: network problem", e);
                             }
+                            // wait at least an hour before making the next attempt to get API data
+                            final long newExpiry = Instant.now()
+                                    .plus(1, ChronoUnit.HOURS).getEpochSecond();
+                            prefs.edit()
+                                    .putLong(app.getString(R.string.update_expiry_key), newExpiry)
+                                    .apply();
                         });
     }
 }