Fix time zone handling

[phiresky]

The whole code base uses naive date time and TIMESTAMP WITHOUT TIME ZONE.

Now there's conversion issues appearing in federation where timezones are not handled correctly.

Using NaiveDateTime is ... naive? So this PR removes all instances of naive time and adds defined timezones to everything (usually Utc).

Now, no conversions are needed anymore.

The database migration is extensive but it is free (no rewrite needed) since PG 12.

[phiresky]

done now

[phiresky]

This also needs changes to activitypub-federation: LemmyNet/activitypub-federation-rust#62

There is again now a future post on lemmy.world probably due to this issue:

[dessalines]

I'm def down for adding this, especially since it keeps everything in the UTC timezone.

It will be a breaking change so it'll have to come with v0.19, because clients will now receive all the timestamp columns with a timezone, rather than a UTC one they have to hack on a Z string to.

[NatoBoram]

rather than a UTC one they have to hack on a Z string to

Ah, so that's what I'm missing.

That said, why not just send all dates in ISO 8601 format directly?

[dessalines]

After this, they will. We don't manipulate the columns coming back from postgres, so UTC timestamp without timezone doesn't include timestamp info for the 8601 format.

[Nutomic]

I published version 0.5.0-beta.1 of the activitypub crate which includes your timezone commit. As we have a separate release branch for minor versions now, there should be no problem with merging breaking changes like this one.

[dessalines]

I have the SQL format fixed in #3800

[phiresky]

Most stuff should be fixed. I'll wait for #3800 then fix the rest of the CI

[Nutomic]

Needs cargo fmt

[Nutomic]

You can see the errors here: https://woodpecker.join-lemmy.org/repos/129/pipeline/1977/17

---- protocol::activities::block::tests::test_parse_lemmy_block stdout ----
thread 'protocol::activities::block::tests::test_parse_lemmy_block' panicked at '

json atoms at path ".expires" are not equal:
    expected:
        "2021-11-01T12:23:50.151874+00:00"
    actual:
        "2021-11-01T12:23:50.151874Z"

', crates/apub/src/protocol/mod.rs:122:5

Maybe its the other way round and you need to replace +00:00 with Z

[phiresky]

Ah, I missed those somehow. I've replaced +00:00 with Z in the tests. Those two things should be equivalent in all ISO8601 parsers.

[phiresky]

I'm not sure why the federation tests are failing now.. doesn't really seem related but could be I guess

[Nutomic]

Im seeing some errors in the api-test logs:

ESC[2m2023-08-16T10:18:00.895306ZESC[0m ESC[31mERRORESC[0m ESC[2mlemmy_server::scheduled_tasksESC[0mESC[2m:ESC[0m Failed to update post_aggregates hot_ranks
: function hot_rank(bigint, timestamp with time zone) does not exist
ESC[2m2023-08-16T10:18:00.895347ZESC[0m ESC[32m INFOESC[0m ESC[2mlemmy_server::scheduled_tasksESC[0mESC[2m:ESC[0m Finished process_hot_ranks_in_batches exec
ution for post_aggregates (processed 0 rows)
ESC[2m2023-08-16T10:18:00.895907ZESC[0m ESC[31mERRORESC[0m ESC[2mlemmy_server::scheduled_tasksESC[0mESC[2m:ESC[0m Failed to update comment_aggregates hot_ra
nks: function hot_rank(bigint, timestamp with time zone) does not exist
ESC[2m2023-08-16T10:18:00.895935ZESC[0m ESC[32m INFOESC[0m ESC[2mlemmy_server::scheduled_tasksESC[0mESC[2m:ESC[0m Finished process_hot_ranks_in_batches exec
ution for comment_aggregates (processed 0 rows)
ESC[2m2023-08-16T10:18:00.896439ZESC[0m ESC[31mERRORESC[0m ESC[2mlemmy_server::scheduled_tasksESC[0mESC[2m:ESC[0m Failed to update community_aggregates hot_
ranks: function hot_rank(bigint, timestamp with time zone) does not exist
ESC[2m2023-08-16T10:18:00.896463ZESC[0m ESC[32m INFOESC[0m ESC[2mlemmy_server::scheduled_tasksESC[0mESC[2m:ESC[0m Finished process_hot_ranks_in_batches exec
ution for community_aggregates (processed 0 rows)

Dont think they are related but should still be fixed.

To debug the failures you can edit api_tests/package.json line 12 to "api-test": "jest -i comment.spec.ts -t 'Unlike a comment'" so it only runs a single test, then check the logs in /tmp/lemmy_alpha.out and /tmp/lemmy_gamma.out. However I dont see anything relevant there.

Edit: The test case "Reply to a comment from another instance, get notification" does fail with LemmyError { message: Unknown("function hot_rank(bigint, timestamp with time zone) does not exist"), inner: function hot_rank(bigint, timestamp with time zone) does not exist

[phiresky]

You're right. I've updated the hot_rank function to take a timestamp with time zone.

Note that I haven't really tested this change in general, I've just been relying on the compiler and tests to say if it's good.

[Nutomic]

That should be enough, anyway we need to make some release candidates for testing before releasing the major version with this. Now you just need to resolve conflicts and it should get merged automatically.

[lazyguru]

Hey all, just thought I would confirm this is indeed a breaking change. Due to the fact that the latest tag for the docker images got pointed at the beta releases, we inadvertently updated to beta.8 (which has these changes in it). Many (not all) 3rd party clients are now unable to access our instance. It would be nice if a communication campaign can happen so 3rd party devs will know they need to update their code to handle for this prior to 0.19 (and potentially sooner if any other instances make the mistake we did)

[phiresky]

Hey @lazyguru, thanks for the info. That's interesting, because I wouldn't expect this change to break much since it just means Lemmy now sends and receives timestamps with time zone included. So code that uses a normal ISO8601 date parser should work unchanged afterwards - with worst case an offset of a few hours.

It would be interesting to hear which clients break exactly and how.

[lazyguru]

The ones I know of for sure are Sync and Mlem. Memmy works fine

[phiresky]

@laurencedawson @EricBAndrews @mormaer @aeharding might be interested

[dessalines]

updated to beta.8

👀 Be very careful about upgrading to betas and RC's, you'll very likely end up with a broken database that you'll have to fix manually.

[lazyguru]

Yes, the problem was that "latest" is pointed at the betas. IMO it should only ever be pointed at a "stable" release

[dessalines]

Let me know if there's anywhere in our docs where we use that latest tag, as that's potentially destructive.

[laurencedawson]

This was fixed for the next release of sync.

As the "z" was missing previously sync added it before parsing the timestamp. Now it's added conditionally.

[Morethanevil]

I got updated to this beta too, because of the latest tag in docker compose and I wondered why the connect app doesn't work anymore. Second problem is with this release, I don't see the admin settings button anymore. I am admin in the database and I can access the adminpanel via /admin in the URL, but the button is gone