refactor: change use of pyrfc3339 to use ciso8601

[EdmilsonRodrigues]

Description

This change was mentioned in the issue #1101 . There is just one change that is not simply replacing, and that one is the generation of the rfc3339, but I created a simple function to handle this conversion.

\Replaced old pyrfc3339 that has not received an updated for the last 6 years and is not typed to use ciso8601 instead

QA Steps

<Commands / tests / steps to run to verify that the change works:>

tox -e py3 -- tests/unit/...

tox -e integration -- tests/integration/...

All CI tests need to pass.

<Please note that most likely an additional test will be required by the reviewers for any change that's not a one liner to land.>

Notes & Discussion

Should I also write the tests for this one? Like assert pyrfc3339.parse(timestamp) == ciso8601.parse_rfc339(timestamp) and assert pyrfc3339.generate(datetime) == generate_rfc3339_from_unix_time(datetime)?

[dimaqq]

What's your personal opinion on ciso8601 vs. backports.datetime_fromisoformat?
https://github.com/closeio/ciso8601/blob/master/why_ciso8601.md

[dimaqq]

Adding @Aflynn50 for the Juju perspective.

[dimaqq]

I see that there's a solid-looking set of tests in tests/unit/test_gocookies.py.
Could you clarify why the custom parses function was needed, what specific inputs could not be parsed?

[EdmilsonRodrigues]

ciso8601 does not have a function to generate a rfc3339 timestamp from a datetime, as this one of the pyrfc3339 library, so I implemented my own.

[dimaqq]

I wonder if datetime.isoformat() would be enough.
I'll need someone who knows about go-style cookies and why are those important, if Juju relies on those in some way.

[dimaqq]

/build

[dimaqq]

Fixes #1101

[dimaqq]

________________________ TestGoCookieJar.test_roundtrip ________________________

self = <tests.unit.test_gocookies.TestGoCookieJar testMethod=test_roundtrip>

    def test_roundtrip(self):
        jar = self.load_jar(cookie_content)
        filename2 = os.path.join(self.dir, "cookies2")
>       jar.save(filename=filename2)

tests/unit/test_gocookies.py:201: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
juju/client/gocookies.py:46: in save
    go_cookies.append(py_to_go_cookie(cookie))
juju/client/gocookies.py:107: in py_to_go_cookie
    go_cookie["Expires"] = generate_rfc3339_from_unix_time(unix_time)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

unix_time = datetime.datetime(2345, 11, 15, 18, 16, 8)

    def generate_rfc3339_from_unix_time(unix_time: datetime.datetime) -> str:
>       rfc, discard = unix_time.isoformat().split(".")
E       ValueError: not enough values to unpack (expected 2, got 1)

Please run unit tests 🙇🏻
The easiest way is uvx tox -e unit

[EdmilsonRodrigues]

Re: What's your personal opinion on ciso8601 vs. backports.datetime_fromisoformat?
https://github.com/closeio/ciso8601/blob/master/why_ciso8601.md

My personal perspective is that backports.datetime_fromisoformat is a better use since it doesn't need an external library. But as I am new to the code base and still learning how it works, I wouldn't dare make this change by myself since it would need to change the rfc3339 format to use ISO format. I don't know if this would impact how the GoJuju library works, so I used the suggestion stated in the issue and used the ciso8601. But this is based in lack of experience with the library.

[EdmilsonRodrigues]

Re:
________________________ TestGoCookieJar.test_roundtrip ________________________

self = <tests.unit.test_gocookies.TestGoCookieJar testMethod=test_roundtrip>

def test_roundtrip(self):
    jar = self.load_jar(cookie_content)
    filename2 = os.path.join(self.dir, "cookies2")

  jar.save(filename=filename2)

tests/unit/test_gocookies.py:201:

---

juju/client/gocookies.py:46: in save
go_cookies.append(py_to_go_cookie(cookie))
juju/client/gocookies.py:107: in py_to_go_cookie
go_cookie["Expires"] = generate_rfc3339_from_unix_time(unix_time)

---

unix_time = datetime.datetime(2345, 11, 15, 18, 16, 8)

def generate_rfc3339_from_unix_time(unix_time: datetime.datetime) -> str:

  rfc, discard = unix_time.isoformat().split(".")

E ValueError: not enough values to unpack (expected 2, got 1)
Please run unit tests 🙇🏻
The easiest way is uvx tox -e unit

Thank you!! I didn't know yet how to run the unit tests. Fixing this.

[james-garner-canonical]

When #1101 was raised, I think we assumed that ciso8601 would be essentially a drop-in replacement. Thank you @EdmilsonRodrigues for taking a stab at this, and identifying that we not only need to go from rfc 3339 strings to datetimes, but in one case also from http.cookiejar.Cookie.expires (integer seconds) to a rfc 3339 string. I think it would be best if we didn't have to roll our own date time string generation here. Unfortunately as far as I can tell, ciso8601 strictly parses date time strings, and doesn't provide methods for generating them (anyone want to double check this?).

I'm +1 on using backports.datetime_fromisoformat instead of ciso8601 if it works for our use case, but this doesn't take care of generating rfc 3339 date time strings.

I haven't familiarised myself with the differences between the rfc 3339 and iso 8601 format @dimaqq, but is the output of datetime's isoformat compliant enough for us?

import datetime, time
timestamp = time.time()
timezone = datetime.timezone.utc
# or do we need local timezone?
# utcoffset = datetime.datetime.now().astimezone().utcoffset()  # can be None apparently
# timezone = datetime.timezone(utcoffset) if utcoffset is not None else datetime.timezone.utc
datetime.datetime.fromtimestamp(timestamp, tz=timezone).isoformat()
# '2024-12-17T23:34:38.780403+00:00'

[dimaqq]

@hpidcock commented on Matrix:

https://pkg.go.dev/time#Time.MarshalJSON
It just has to be rfc3339
https://github.com/juju/persistent-cookiejar is the library that was created for the juju client, if pylibjuju is to be able to open the cookie jar saved by the juju client, it just needs to be the same.

[james-garner-canonical]

Here is a useful but overwhelming reference to the differences and overlap between rfc 3339 and iso 8601.

I believe that this should be a perfectly valid rfc 3339 string, so I recommend we use datetime's isoformat for writing the string out:

datetime.datetime.fromtimestamp(timestamp, tz=timezone).isoformat()
# '2024-12-17T23:34:38.780403+00:00'

For parsing the date time strings we get from Juju, I suggest that we try backports.datetime_fromisoformat, and if it works in practice then we should just use it (and assume that juju/persistent-cookiejar won't be updated any time soon to use an esoteric form of rfc 3339 that's incompatible with iso 8601). However I'm not opposed to playing it safe and using ciso8601 if others would prefer that -- and if backports.datetime_fromisoformat doesn't work in our tests then we should just use ciso8601.

[EdmilsonRodrigues]

https://github.com/EdmilsonRodrigues/python-libjuju/blob/de772290a4582083fbd5f431efabada8374b12d4/tests/unit/test_gocookies.py#L206-L227

I don't know how to refer the part of the code the way you do, but I mean this:

    def test_expiry_time(self):
        content = """[
            {
                "CanonicalHost": "bar.com",
                "Creation": "2017-11-17T08:53:55.088820092Z",
                "Domain": "bar.com",
                "Expires": "2345-11-15T18:16:08Z",
                "HostOnly": true,
                "HttpOnly": false,
                "LastAccess": "2017-11-17T08:53:55.088822562Z",
                "Name": "bar",
                "Path": "/",
                "Persistent": true,
                "Secure": false,
                "Updated": "2017-11-17T08:53:55.088822562Z",
                "Value": "bar-value"
            }
        ]"""
        jar = self.load_jar(content)
        got_expires = tuple(jar)[0].expires
        want_expires = int(ciso8601.parse_rfc3339("2345-11-15T18:16:08Z").timestamp())
        self.assertEqual(got_expires, want_expires)

In this test, I think this is an example of a juju/persitent-cookiejar cookie.

This format is compatible with fromisoformat():

from datetime import datetime
datetime.fromisoformat("2345-11-15T18:16:08Z")
datetime.datetime(2345, 11, 15, 18, 16, 8, tzinfo=datetime.timezone.utc)
datetime.fromisoformat("2017-11-17T08:53:55.088822562Z")
datetime.datetime(2017, 11, 17, 8, 53, 55, 88822, tzinfo=datetime.timezone.utc)
So the parsing would work using fromisoformat.

I agree with using the backports.datetime_fromisoformat and test generating using the .isoformat().
I will wait for other opinions before doing anything, but I agree with the current point of view.

[dimaqq]

Would you consider making an alternative PR using backports?

[EdmilsonRodrigues]

Sure! I will open it in some minutes!

The tests are failing I'll fix this and open it tomorrow.