Fix #27 by using java.util.Date for storing expiration time #50

gerdint · 2023-08-15T13:49:03Z

Hopefully we can get this fix merged soonish, this is the 3rd PR regarding this.

The testing code changes are copied from PR #47, I hope @jamiepratt does not mind.

Requires Java 8 due to use of java.time.Instant in code for clarity

weavejester

Thanks for the PR. Just some minor formatting changes for consistency are required, plus changing the new function to private as its not part of the API.

Could you also change the commit message to:

Replace clj-time with native java.time (fixes #27)

Updates minimum required Java version to 8 due to use of java.time.Instant.

weavejester · 2023-08-15T14:17:02Z

src/ring/middleware/oauth2.clj

+  (:import (java.time Instant)
+           (java.util Date)))


Can you use square brackets here, i.e.

(:import [java.time Instant] [java.util Date])

weavejester · 2023-08-15T14:18:59Z

src/ring/middleware/oauth2.clj

+(defn seconds-from-now-to-date [secs]
+  (-> (Instant/now)
+      (.plusSeconds secs)
+      (Date/from)))


The function body probably all be on one line without losing clarity, and should be a private function as it's not part of the public API:

(defn- seconds-from-now-to-date [secs] (-> (Instant/now) (.plusSeconds secs) (Date/from)))

weavejester · 2023-08-15T14:21:14Z

test/ring/middleware/oauth2_test.clj

@@ -75,12 +74,12 @@
                         (assoc :session {::oauth2/state "xyzxyz"})
                         (assoc :query-params {"code" "abcabc", "state" "xyzxyz"}))
            response (test-handler request)
-            expires  (-> 3600 time/seconds time/from-now)]
+            expires (oauth2/seconds-from-now-to-date 3600)]


Can you align it to match the removed like? So:

response (test-handler request) expires (oauth2/seconds-from-now-to-date 3600)]

weavejester · 2023-08-15T14:32:16Z

test/ring/middleware/oauth2_test.clj

@@ -130,12 +129,13 @@
            request  (-> (mock/request :get "/oauth2/test/callback")
                         (assoc :session {::oauth2/state "xyzxyz"})
                         (assoc :query-params {"code" "abcabc", "state" "xyzxyz"}))
-            response (handler request)]
+            response (handler request)
+            expires (oauth2/seconds-from-now-to-date 3600)]


response (handler request) expires (oauth2/seconds-from-now-to-date 3600)

Align vars to be consistent with other lines in the let form.

gerdint · 2023-08-16T08:26:28Z

@weavejester I amended the commit with the requested changes

weavejester

Almost there! Just one more change necessary to avoid tying the tests to private functions.

weavejester · 2023-08-16T08:47:12Z

test/ring/middleware/oauth2_test.clj

@@ -75,12 +74,12 @@
                         (assoc :session {::oauth2/state "xyzxyz"})
                         (assoc :query-params {"code" "abcabc", "state" "xyzxyz"}))
            response (test-handler request)
-            expires  (-> 3600 time/seconds time/from-now)]
+            expires  (#'oauth2/seconds-from-now-to-date 3600)]


Accessing a private var here saves a little code, but also ties the test case to the internals of the namespace being tested. Ideally we test only the public API. So instead, just inline the function:

expires (-> (Instant/now) (.plusSeconds 3600) (Date/from))

OK, though since identical code is used at four places in the test suite I might as well introduce a helper function for it in the tests I guess?

I don't think I mind. You could duplicate seconds-from-now-to-date in the test namespace if you wanted.

I did it the other way, see what you think.

That works, too. Can you ensure that the lines modified in ring.middleware.oauth are within 80 characters?

I believe the modified lines are all within 80 characters.

I think there's one line in your patch that might be 85?

As you perhaps noted the 2nd commit made that like shorter.

Oh, weird. Is the GitHub diff incorrect then? In any case, it looks like there are other lines over from previous commits, so I can always fix it with a later formatting commit. I just need the commits to be squashed down and then we're ready to merge.

jamiepratt · 2023-08-16T16:28:52Z

Haven't had time to look at the code, but thanks for the work bringing it up to par and merging this! Jamie -- *My homepage* <https://jamiep.org/> and *resume* <https://www.jamiep.org/work/james-robert-pratt/>. Phone no: *+48505206918 <https://wa.me/48505206918> (link messages me on WhatsApp)* Skype Name: *jamiepratt <https://join.skype.com/invite/ben7orDUhbwH>*

…

On Wed, 16 Aug 2023 at 11:04, James Reeves ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test/ring/middleware/oauth2_test.clj <#50 (comment)> : > @@ -75,12 +74,12 @@ (assoc :session {::oauth2/state "xyzxyz"}) (assoc :query-params {"code" "abcabc", "state" "xyzxyz"})) response (test-handler request) - expires (-> 3600 time/seconds time/from-now)] + expires (#'oauth2/seconds-from-now-to-date 3600)] I don't think I mind. You could duplicate seconds-from-now-to-date in the test namespace if you wanted. — Reply to this email directly, view it on GitHub <#50 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEINPDUCHJX2LQKGTAQCATXVSELXANCNFSM6AAAAAA3RDCVHM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

weavejester · 2023-08-24T14:49:35Z

Looks good! Can you squash your commits, and it should be ready to merge.

gerdint · 2023-08-28T19:47:58Z

Squashed

weavejester · 2023-08-28T23:17:56Z

Thanks! Can you also fix the commit message? It has "Implement changes from review feedback" added to the end of it now.

Requires Java 8 due to use of java.time.

gerdint · 2023-08-29T13:18:39Z

Oh sorry, I thought I had removed that. Fixed now.

gerdint · 2023-08-31T09:05:42Z

@weavejester Anything else?

weavejester · 2023-08-31T20:42:58Z

Nope! That's all good. Thanks for your work on this.

gerdint · 2023-09-01T11:11:29Z

@weavejester Good. Perhaps it would be worth making a 0.2.1 release with this fix since it seemed like a highly anticipated change.

weavejester · 2023-09-05T23:55:56Z

Released 0.2.1.

weavejester requested changes Aug 15, 2023

View reviewed changes

gerdint force-pushed the feature/java-time-date branch from 6bb0174 to 631ef60 Compare August 15, 2023 15:14

gerdint requested a review from weavejester August 15, 2023 15:15

gerdint force-pushed the feature/java-time-date branch from 631ef60 to ad4fb13 Compare August 15, 2023 15:19

weavejester requested changes Aug 16, 2023

View reviewed changes

gerdint force-pushed the feature/java-time-date branch from f3907ea to a4aac7c Compare August 28, 2023 19:47

Replace clj-time with native java.time (fixes weavejester#27)

82a7d1b

Requires Java 8 due to use of java.time.

gerdint force-pushed the feature/java-time-date branch from a4aac7c to 82a7d1b Compare August 29, 2023 13:17

weavejester merged commit f8727c3 into weavejester:master Aug 31, 2023
1 check passed

gerdint deleted the feature/java-time-date branch September 1, 2023 11:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #27 by using java.util.Date for storing expiration time #50

Fix #27 by using java.util.Date for storing expiration time #50

gerdint commented Aug 15, 2023

weavejester left a comment •

edited

weavejester Aug 15, 2023

weavejester Aug 15, 2023

weavejester Aug 15, 2023

weavejester Aug 15, 2023

gerdint commented Aug 16, 2023

weavejester left a comment

weavejester Aug 16, 2023

gerdint Aug 16, 2023

weavejester Aug 16, 2023

gerdint Aug 17, 2023

weavejester Aug 17, 2023

gerdint Aug 18, 2023

weavejester Aug 24, 2023

gerdint Aug 27, 2023

weavejester Aug 27, 2023

jamiepratt commented Aug 16, 2023 via email

weavejester commented Aug 24, 2023

gerdint commented Aug 28, 2023

weavejester commented Aug 28, 2023

gerdint commented Aug 29, 2023

gerdint commented Aug 31, 2023

weavejester commented Aug 31, 2023

gerdint commented Sep 1, 2023

weavejester commented Sep 5, 2023

Fix #27 by using java.util.Date for storing expiration time #50

Fix #27 by using java.util.Date for storing expiration time #50

Conversation

gerdint commented Aug 15, 2023

weavejester left a comment • 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

gerdint commented Aug 16, 2023

weavejester 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

Choose a reason for hiding this comment

jamiepratt commented Aug 16, 2023 via email

weavejester commented Aug 24, 2023

gerdint commented Aug 28, 2023

weavejester commented Aug 28, 2023

gerdint commented Aug 29, 2023

gerdint commented Aug 31, 2023

weavejester commented Aug 31, 2023

gerdint commented Sep 1, 2023

weavejester commented Sep 5, 2023

weavejester left a comment •

edited