[JENKINS-52161] Improve creation-date value determination #3525

Wadeck · 2018-06-26T11:15:03Z

In case of migration we put null instead of now and so changed the display in Legacy token monitoring page.
Improve also the difference in days for the Xxx day(s) ago to take calendar day in difference and not the total duration (<24h = 0 days, which seems wrong in UI).
It's only a partial improvement for the ticket, 2 other parts are still not done yet. This is the most "urgent" feature to avoid loosing the information that "we do not know" the creation date of legacy token during migration from XML.

See JENKINS-52161.

Proposed changelog entries

(internal) API token: Improve the creation date value by putting null during migration of legacy token from XML
API token: Display better the difference in terms of days between the creation (or last use) date and now, taking only the number of calendar days into account.
API token: Legacy token monitor page now differentiates Unknown creation date from today.

Submitter checklist

JIRA issue is well described
Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
* Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
Appropriate autotests or explanation to why this change has no tests
[N/A] For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@reviewbybees @jsoref

- in case of migration we put null instead of now - improve also the difference in days for the Xxx day(s) ago to take calendar day in difference and not the total duration (<24h = 0 days, which seems wrong in UI)

jsoref

Thanks, this seems like a good improvement over what's happening today.

jsoref · 2018-06-26T12:29:48Z

...rc/main/resources/jenkins/security/apitoken/LegacyApiTokenAdministrativeMonitor/manage.jelly

                                <j:if test="${legacyStats.lastUseDate != null}">
                                    <i:formatDate var="lastUseDateFormat" value="${legacyStats.lastUseDate}" type="both" dateStyle="medium" timeStyle="medium" />
+                                    <!--TODO convert to "Today", "Tomorrow", "4 days ago", "2 weeks ago", etc-->


Pet peeve: etc.

jsoref · 2018-06-26T12:36:45Z

core/src/main/java/hudson/Util.java

@@ -23,6 +23,7 @@
 */
 package hudson;

+import com.sun.istack.internal.NotNull;


Is this the right import?

I think it is not used, indeed.

jsoref · 2018-06-26T12:38:38Z

core/src/main/java/hudson/Util.java

+     */
+    @Restricted(NoExternalUse.class)
+    public static int differenceInCalendarDay(@Nonnull Date a, @Nonnull Date b){
+        LocalDate aLocal = a.toInstant().atZone(ZoneId.systemDefault()).toLocalDate();


I have half a preference for considering the user's time zone instead of the system's. (This is achievable by passing both dates down through to the jelly and letting it do this math instead.)

IIRC HTTP Headers do not contain any information about timezone. You mean using JavaScript instead of server-side rendering ?

Except for time zones that have managed to skip a day recently (which hasn't happened for decades or possibly centuries IIRC), this logic would work for any time zone.

Yes, I meant JavaScript instead of server-side.

The issue isn't that the logic would work for any time zone, it's that the creature who wants to do the counting has a different perspective.

Date A is created at 1:03am New Zealand time.
https://www.timeanddate.com/worldclock/converter.html?iso=20171230T120300&p1=22&p2=195
Date B is created at 11:55pm New Zealand time on the same calendar date in New Zealand.
https://www.timeanddate.com/worldclock/converter.html?iso=20171231T105500&p1=22&p2=195

User C in New Zealand sees these two events as 0 days apart.

User D in Paris looks at these dates and sees them as 1 day apart.

Oh, I see. Yeah, that would be an actual issue.

To me it seems like we want a more general fix for this, maybe JENKINS-19887 or the linked tickets there rather than introducing complexity here as a one-off case.

Yeah, I'm fine with it not being addressed here. I just want people to be aware. Hence "my preference" as opposed to any stronger request.

The reason to mention it here is that it impacts the "api" perspective. If the values are both exposed to the client, then the client side can be upgraded to get this w/o needing to rebuild the "api".

OTOH, if jenkins treats the strings as the "api" and doesn't see them as "frozen", then evolving it later and doing it server-side is fine. -- I haven't built up a complete sense of how jenkins manages its "apis". (I just know that it has things that are apis.)

@jsoref FYI about API:

As the code that is modified here is @Restricted (actually an inner class of a restricted class) we can consider it as an internal API (i.e. private) and so we could break it without any other consideration.

If there are public methods in a non-@Restricted, they could be used by other plugins / components, that's the developer API we need to care about in terms of binary compatibility.

@Wadeck: k. that covers the java side, but what are the rules for the http side? (Maybe the answer is there aren't any constraints, which is probably a good thing here)

@jsoref yeah in general I would say there is no constraint except that you need to keep consistency, security and compatibility in mind.

fcojfernandez

🐝
LGTM, just a minor comment

fcojfernandez · 2018-06-26T12:50:05Z

core/src/main/java/hudson/Util.java

@@ -23,6 +23,7 @@
 */
 package hudson;

+import com.sun.istack.internal.NotNull;


I think it is not used, indeed.

daniel-beck · 2018-06-26T12:59:07Z

core/src/main/java/jenkins/security/apitoken/ApiTokenStore.java

@@ -371,6 +371,22 @@ private void init() {

            return result;
        }
+
+        public static @Nonnull HashedToken buildNewFromLegacy(@Nonnull HashValue value, boolean migrationFromExistingLegacy) {


@since or @Restricted

as the parent class (ApiTokenStore) is @Restricted, this method is also @Restricted by implication

dwnusbaum · 2018-06-26T13:22:20Z

core/src/main/java/hudson/Util.java

+    public static int differenceInCalendarDay(@Nonnull Date a, @Nonnull Date b){
+        LocalDate aLocal = a.toInstant().atZone(ZoneId.systemDefault()).toLocalDate();
+        LocalDate bLocal = b.toInstant().atZone(ZoneId.systemDefault()).toLocalDate();
+        return Period.between(aLocal, bLocal).getDays();


Period#getDays just returns the number of days less than a month, not the total (i.e. differenceInCalendarDay(2017-01-01, 2018-01-01) will return 0, but I think you want it to return 365 (Yes, I have been bitten by this in the past)). You should use ChronoUnit.DAYS.between(aLocal, bLocal) to get the total number of days.

Nice catch! It's due to things like this that I tend to use Instant more often than the human versions.

dwnusbaum · 2018-06-26T13:24:29Z

core/src/test/java/hudson/UtilTest.java

+        Date day1_11pm55 = new Date(2018, 4, 6, 23, 55);
+        Date day2_01am = new Date(2018, 4, 7, 1, 0);
+        Date day2_11pm = new Date(2018, 4, 7, 1, 0);
+        Date day3_08am = new Date(2018, 4, 8, 8, 0);


Add some Dates with different years, and dates more than a month apart :)

varyvol · 2018-06-26T13:39:50Z

core/src/main/java/hudson/Util.java

+     * even if there are only 3 hours between. As well as "10am" to "2pm" both on the same day, returns 0.
+     */
+    @Restricted(NoExternalUse.class)
+    public static int differenceInCalendarDay(@Nonnull Date a, @Nonnull Date b){


Days instead of day for all these methods?

How about daysBetween()?

jvz

Added a couple notes about naming and boolean usage, but otherwise looks good.

jvz · 2018-06-26T14:11:23Z

core/src/main/java/hudson/Util.java

+     * even if there are only 3 hours between. As well as "10am" to "2pm" both on the same day, returns 0.
+     */
+    @Restricted(NoExternalUse.class)
+    public static int differenceInCalendarDay(@Nonnull Date a, @Nonnull Date b){


How about daysBetween()?

jvz · 2018-06-26T14:12:34Z

core/src/main/java/hudson/Util.java

+     */
+    @Restricted(NoExternalUse.class)
+    public static int differenceInCalendarDay(@Nonnull Date a, @Nonnull Date b){
+        LocalDate aLocal = a.toInstant().atZone(ZoneId.systemDefault()).toLocalDate();


Except for time zones that have managed to skip a day recently (which hasn't happened for decades or possibly centuries IIRC), this logic would work for any time zone.

jvz · 2018-06-26T14:13:24Z

core/src/main/java/hudson/Util.java

+    public static int differenceInCalendarDay(@Nonnull Date a, @Nonnull Date b){
+        LocalDate aLocal = a.toInstant().atZone(ZoneId.systemDefault()).toLocalDate();
+        LocalDate bLocal = b.toInstant().atZone(ZoneId.systemDefault()).toLocalDate();
+        return Period.between(aLocal, bLocal).getDays();


Nice catch! It's due to things like this that I tend to use Instant more often than the human versions.

jvz · 2018-06-26T14:13:59Z

core/src/main/java/hudson/Util.java

+     * @see #differenceInCalendarDay(Date, Date)
+     */
+    @Restricted(NoExternalUse.class)
+    public static int numOfCalendarDayToNow(@Nonnull Date date){


Could name this something like daysSince() or daysElapsedSince(). Also, based on usage, you could also return 0 for future dates to simplify a bit.

jvz · 2018-06-26T14:19:16Z

core/src/main/java/jenkins/security/apitoken/ApiTokenStore.java

@@ -371,6 +371,22 @@ private void init() {

            return result;
        }
+
+        public static @Nonnull HashedToken buildNewFromLegacy(@Nonnull HashValue value, boolean migrationFromExistingLegacy) {


I already packed it away, so I'm not sure which item it is, but Effective Java suggests using enums instead of booleans for situations like this. The context of what exactly that flag does is lost in a language like Java (i.e., no passing parameters by name).

In this case the method is @Restricted and so can be considered "private" for us, i.e. the context is never lost as we are using the source directly.

jvz · 2018-06-26T14:20:22Z

core/src/test/java/hudson/UtilTest.java

+        Date day2_01am = new Date(2018, 4, 7, 1, 0);
+        Date day2_11pm = new Date(2018, 4, 7, 1, 0);
+        Date day3_08am = new Date(2018, 4, 8, 8, 0);
+        assertEquals(0, Util.differenceInCalendarDay(day1_10am, day1_11pm55));


It might be simpler to use a parameterized test for this, but it'd have to be in a separate class thanks to junit 4.

- also add translation for another creationDate - and manage correctly the null creation date instead of 0

jvz

LGTM

jsoref · 2018-06-26T16:10:09Z

...rc/main/resources/jenkins/security/apitoken/LegacyApiTokenAdministrativeMonitor/manage.jelly

@@ -138,14 +144,14 @@ THE SOFTWARE.
                        </j:otherwise>
                    </j:choose>
                </table>
-
+                


I'm not sure of the normal style. I personally am not a fan of trailing whitespace (and apparently the code wasn't using it before).

Yeah I just tried to make it consistent

jsoref · 2018-06-26T16:13:16Z

core/src/test/java/hudson/UtilTest.java

@@ -781,4 +781,29 @@ public void testPermissionsToMode() throws Exception {
        assertEquals(0000, Util.permissionsToMode(PosixFilePermissions.fromString("---------")));
    }

+    @Test
+    public void testDifferenceDays() {
+        Date may_6_10am = new Date(2018 - 1900, 4, 6, 10, 0);


https://docs.oracle.com/javase/8/docs/api/java/util/Date.html#constructor.summary

Date(int year, int month, int date, int hrs, int min, int sec)
Deprecated.
As of JDK version 1.1, replaced by Calendar.set(year + 1900, month, date, hrs, min, sec) or GregorianCalendar(year + 1900, month, date, hrs, min, sec).

In general I do not care really for test code to use deprecated constructors like that because it's not the code that is tested but only the tools we use to test the real things.

As you pointed out this code and take time to retrieve the documentation, I imagine it's important for you, so I changed it 👍

jsoref · 2018-06-26T19:21:15Z

@Wadeck: thanks, that's actually much easier to read.

Wadeck · 2018-06-27T08:09:42Z

@reviewbybees done

@jsoref do you approve this PR ? :)

[JENKINS-52161] Improve creation-date value determination

5f7c50c

- in case of migration we put null instead of now - improve also the difference in days for the Xxx day(s) ago to take calendar day in difference and not the total duration (<24h = 0 days, which seems wrong in UI)

daniel-beck self-requested a review June 26, 2018 11:59

jsoref reviewed Jun 26, 2018

View reviewed changes

Address Josh's comments

0a41e87

fcojfernandez approved these changes Jun 26, 2018

View reviewed changes

daniel-beck reviewed Jun 26, 2018

View reviewed changes

daniel-beck approved these changes Jun 26, 2018

View reviewed changes

dwnusbaum reviewed Jun 26, 2018

View reviewed changes

varyvol reviewed Jun 26, 2018

View reviewed changes

jvz suggested changes Jun 26, 2018

View reviewed changes

Address other comments

e0cc790

- also add translation for another creationDate - and manage correctly the null creation date instead of 0

jvz approved these changes Jun 26, 2018

View reviewed changes

jsoref reviewed Jun 26, 2018

View reviewed changes

Avoid using deprecated constructor in test

d0be7b3

Wadeck added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Jun 27, 2018

jsoref approved these changes Jun 27, 2018

View reviewed changes

daniel-beck merged commit 1b805f8 into jenkinsci:master Jun 29, 2018

daniel-beck mentioned this pull request Jun 29, 2018

Legacy api token ux #3519

Merged

[JENKINS-52161] Improve creation-date value determination #3525

[JENKINS-52161] Improve creation-date value determination #3525

Conversation

Wadeck commented Jun 26, 2018

Proposed changelog entries

Submitter checklist

Desired reviewers

jsoref left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwnusbaum Jun 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fcojfernandez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwnusbaum Jun 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsoref commented Jun 26, 2018

Wadeck commented Jun 27, 2018

dwnusbaum Jun 26, 2018 •

edited

dwnusbaum Jun 26, 2018 •

edited