Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential issue with full synchronization 'since' timestamp #17

Closed
JonOfUs opened this issue Aug 12, 2021 · 6 comments
Closed

Potential issue with full synchronization 'since' timestamp #17

JonOfUs opened this issue Aug 12, 2021 · 6 comments

Comments

@JonOfUs
Copy link
Collaborator

JonOfUs commented Aug 12, 2021

On full synchronization, since timestamp is set to 0 and so only episode and subscription changes from within the last week will be synchronized.

Expected Behavior

Full synchronization after first synchronization

Current Behavior

Only episode actions and subscriptions from within last week will be synchronized

Possible Solution

If not intended, repair since-parameter or correct createDateTimeFromTimestamp to be able to deal with since=0

Steps to Reproduce

  1. Install AntennaPod debug with current build 3edd01e (12-08-2021) & Install gpoddersync 1.0.9.
  2. Do some subscription changes and create some episode actions
  3. Wait >1 week
  4. Delete app data, re-synchronize everything

Context (Environment)

Using MariaDB 10.3, php8.0, Nextcloud 22, gpoddersync 1.0.9, Raspberry Pi OS 64bit, nginx 1.14.2

Detailed Description

I recognized that after deleting AntennaPod app data and re-synchronizing subscriptions and episode changes, some episode changes won't synchronize.
After some digging I found out that the full synchronization feature calls /subscription_change/list with since=0 and therefore createDateTimeFromTimestamp will always return a timestamp from a week ago (because 0 is false).

The problem with this is that episode changes and subscriptions that are older than one week won't get synchronized.

Is this behaviour intended?

/**
* @param int|null $since
*
* @return DateTime
*/
private function createDateTimeFromTimestamp(?int $since): DateTime {
return ($since)
? (new \DateTime)->setTimestamp($since)
: (new \DateTime('-1 week'));
}

If not intended, I could PR a fix, already found one.

@JonOfUs JonOfUs changed the title Potential issue with full synchronization since timestamp Potential issue with full synchronization 'since' timestamp Aug 12, 2021
@thrillfall
Copy link
Owner

Oh yes. The condition in line 101 should check for $since not being null.

A PR would very welcome

@thrillfall
Copy link
Owner

@JonOfUs you really don't need to go through the ordeal of filling any issue template if you clearly found the issue and can just shortly point out the bug in the code. Thanks for digging into this

@JonOfUs
Copy link
Collaborator Author

JonOfUs commented Aug 12, 2021

You're welcome!
Okay, I will try to hold me back next time ^^

@thrillfall
Copy link
Owner

@JonOfUs I'll take this commit as the opportunity to add automated releases with github actions. Your fix will then be the first to be released in that way. Please bear with me if it takes a few days

@JonOfUs
Copy link
Collaborator Author

JonOfUs commented Aug 13, 2021

Okay. I'll try to find some other things, a new version for just such a small patch always sounds a bit overkill.

@thrillfall
Copy link
Owner

Not at all. This is a bug. The fix must be released. Size doesn't matter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants