Store time in session as java.util.Date rather than joda-time

[Heliosmaster]

The newer version of clj-time supports tagged literals for clj-time/date-time. Fixes #27

[weavejester]

Thanks for the PR. Can you add a small test to ensure that the middleware works with Ring's cookie sessions store?

[Heliosmaster]

Hey @weavejester I have reworked this according to your comments on #27 . Now the expiration is stored internally as a java.util.Date which should be a bit more robust. It adds a bit of code to do the conversion back and forth. I have also taken the liberty of deleting a couple of unused expression in tests.

[mtravers]

Any reason this has been in limbo for so long? This problem is biting me and I guess I will make my own fork.

[weavejester]

Occassionally a PR will slip through the net. It's why in the contributing guidelines I added a line saying that it's okay to pester me about PRs that have gotten stuck. Overall the PR looks fine, it just needs some formatting updates to fit in with the style guidelines.

[Heliosmaster]

@weavejester I've merged master in, could you point where formatting is not up to standard? It should follow the normal clojure style guide already 🤔

[weavejester]

There are a few lines that exceed 80 characters that need to be fixed, but I think that's the only style issue. However, there look to be some lines in the test code already that hit 85, so as long as the lines are 80-ish it should be okay, and we can be stricter in a future formatting pass.

The ending newline of the file has been accidentally removed and should be put back. Also since the other let clauses are aligned, the one this patch adds should also have its symbols/values aligned into columns.

[malyn]

I need this fix as well and would love to push @Heliosmaster's PR over the finish line. I don't think that there is a way for me to add my own commit to the PR, so I took the PR branch, rebased to master, and then added a commit that addresses the formatting issues. The resulting branch is here: https://github.com/malyn/ring-oauth2/tree/patch-1

@weavejester Let me know if this addresses your formatting comments, and if so, how to get this extra commit merged into the PR. I can submit a PR against @Heliosmaster's patch branch if that helps, for example.

And just to be clear, this is all @Heliosmaster's great work in addressing the underlying issue! I just made some style edits.

[weavejester]

Your patch has some non-standard formatting, @malyn. I follow the Clojure Style Guide for my projects, so the indentation for macros like -> should be consistent.

I think we can also do something like:

(defn- coerce-token-expires-date [tokens]
  (into {} (for [[id t]] [id (update t :expires coerce/from-date)])))

[malyn]

Thanks, @weavejester, for the quick response! I factored out the coerce-tokens-expires-date function and I agree that looks a lot better.

I really struggled with how to handle -> in this patch. Normally I would have vertically aligned that (per the Clojure Style Guide) and that is what I do in my own code So these lines from the patch would become this:

(-> response
    :session
    ::oauth2/access-tokens
    :test
    :expires
    time-coerce/from-date)

I didn't see that vertical form used (on keywords) anywhere else in the file though. Instead, I saw the indentation that I ultimately ended up using (from these lines in master, for example). I am happy to change the patch to use the vertical form, I was just trying to follow the existing code as closely as possible.

Please let me know how you would like to proceed and I will make the requisite change.

[weavejester]

This would be fine:

(-> response :session ::oauth2/access-tokens
    :test :expires time-coerce/from-date)

Or this:

(-> response
    :session ::oauth2/access-tokens
    :test :expires time-coerce/from-date)

Or however you want to break it up.

[weavejester]

Also the time-coerce alias could reasonably just be coerce for brevity.

[malyn]

Very good, thank you for that guidance, @weavejester! I changed time-coerce to coerce and then reformatted the code according to your first proposal. I felt that this better grouped the threading phrase into two sections: get the access tokens, then pull a specific thing out of one of those tokens. This matched what was in nearby lines as well. I also took the liberty of reformatting a few of those nearby lines to get them under 80 characters in width, and to make everything look visually consistent in these updated code blocks.

I recommend you look at the full diff against master (instead of just the recent commit) to get a sense of the final sense of the final patch: https://github.com/weavejester/ring-oauth2/compare/master...malyn:patch-1

Let me know if you have any other changes. If not, I can submit a PR for these style changes to @Heliosmaster's original branch, which, if accepted, should automatically update this PR with these style changes. I have never done this before though, so we may have to take another approach if that fails somewhere along the chain.

[Heliosmaster]

Sorry, I totally missed this. Thanks @malyn! Submit the PR and i'll accept it and this should be updated too 👏

[Heliosmaster]

I've merged @malyn 's branch patch-1 into my patch-1

[jeroenvandijk]

FYI, I had the same issue and this branch has fixed it. I've created a fork with this branch plus a deps.edn so people can try it out in their own deps project.

ring/ring-oauth {:git/url "https://github.com/jeroenvandijk/ring-oauth2.git" :sha "9af66afc6827db18d17eef4cdf64532c9d351cce"}

[lockie]

@weavejester Hey, do you mind merging this PR for those of us who use lein and can't install from github forks?

[gerdint]

So has this PR stalled? This issue is biting me.

[weavejester]

The PR is waiting on the author to address the findings from the code review. I can take a look at addressing those myself (or someone else can), but typically I remove PRs that I've reviewed from my todo list until I get a response.

[gerdint]

Right, well it's been more than a year so I guess @Heliosmaster is up to other things. But I'm using deps so I'll just try the fork for now. But should there not be any major issues it would be nice to get this merged and not have to put up with JodaTime.

[gerdint]

Since clj-time is deprecated it would also be good to migrate away from it I guess, provided Java 8+ is OK. I noticed that I still had issues with malli complaining even with jeroenvandijk's branch (though I checked that date serialization and deserialization looked ok, so it must be something else).