Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Support clj-time Intervals/DateTime for :max-age and :expires #62

Merged
merged 1 commit into from Mar 21, 2012

Conversation

Projects
None yet
2 participants
Contributor

KushalP commented Mar 19, 2012

This fixes #55.

Each commit appears with a header and body description for what I've done.

Here's a quick summary:

  1. clj-time is a dependency for ring-core
  2. Tests check that :max-age and :expires accept an integer (as originally outlined)
  3. :max-age now accepts an Interval — this is exercised through tests
  4. :expires now accepts a DateTime — exercised through tests
  5. Pre-conditions make sure that only those two keys take their respective object kinds

Let me know if any further work needs to be done.

Collaborator

weavejester commented Mar 19, 2012

Thanks for the work you've put into this so far! There are a few issues though...

The :expires attribute should not be a Unix timestamp, but in a format like "Wdy, DD Mon YYYY HH:MM:SS GMT". I believe the cookie spec gives some leeway, and as far as I can tell, the :rfc822 format type that clj-time has built in should create a cookie-compatible date string.

I think it would be something like:

(unparse (formatters :rfc822) value)

The valid-attr? function could be refactored a little by writing the cond as part of the and, e.g.

(and (contains? set-cookie-attrs key)
     (not (.contains (str value) ";")
     (case key
       :max-age (or (instance? Interval value) (integer? value))
       :expires (or (instance? DateTime value) (string? value))
       true))

Finally, I think your branch is a little out of date, as GitHub is telling me no automatic merging. You might want to rebase it off master.

@KushalP KushalP Add clj-time Intervals/DateTime for :max-age and :expires (fixes #55)
- Add a test for :max-age and :expires to (wrap-cookies ...). There were
  no real tests to exercise the base input cases (int, string) which the
  comment block states. This test just makes sure it fulfils that
  contract.
- clj-time is now a project dependency
- :max-age accepts an Interval as input. Updated (write-attr-map ...) to
  accept an Interval (from JodaTime) as well as an int. The interface
  from clj-time is used.
- :expires accepts a DateTime object. It converts the DateTime object in
  the equivalent RFC822 which the cookie spec requires.
- Added pre-conditions for :max-age and :expires to make sure that they
  only accept Interval and DateTime, respectively.
75c90aa
Contributor

KushalP commented Mar 19, 2012

The commits have now been squashed into a single commit, rather than the intermediate steps.

@weavejester weavejester added a commit that referenced this pull request Mar 21, 2012

@weavejester weavejester Merge pull request #62 from KushalP/master
Support clj-time Intervals/DateTime for :max-age and :expires
86a0442

@weavejester weavejester merged commit 86a0442 into mmcgrana:master Mar 21, 2012

@jaceklaskowski jaceklaskowski referenced this pull request in jaceklaskowski/ring May 24, 2013

@weavejester weavejester Merge pull request #62 from arkx/master
Update WOFF mime type
cf17e62
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment