Page MenuHomePhorge

Store modseq with the synckey
ClosedPublic

Authored by mollekopf on Mar 12 2024, 11:31 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Apr 22, 1:44 AM
Unknown Object (File)
Wed, Apr 17, 11:15 AM
Unknown Object (File)
Wed, Apr 17, 11:15 AM
Unknown Object (File)
Wed, Apr 17, 11:15 AM
Unknown Object (File)
Wed, Apr 17, 11:15 AM
Unknown Object (File)
Wed, Apr 17, 11:15 AM
Unknown Object (File)
Wed, Apr 17, 11:15 AM
Unknown Object (File)
Wed, Apr 17, 11:15 AM
Subscribers

Details

Summary

Test emptySync and flag change

The modseq table expects the device primary key, not the deviceid

Don't forget changes while counting changes

If we update modseq whenever we retrieve the changes,
we have to at least not update if we just count.
This is either way super fragile.

Test for flag change retry.

Test an invalid sync key

Store modseq with the synckey

We store the modseq info belongs to the synckey because we want to be
able to list the changes since the last sync.
By storing the modseq info with the synckey we make sure that the
modseq info remains available as long as the sync key is relevant,
and is then automatically cleaned up with the sync key.

This in particular fixes the cases where a sync request is sent multiple
times, where previously we would overwrite the modseq info after the
first attempt, and subsequently fail to return the changes.

I opted for the generic "extraData" approach, instead of changing the
modseq table with a foreign key constraint on the synckey (which would
also work I think), because I suspect we'll need the same thing for dav.
Performance wise I don't think it matters either way.

Notes:

  • We should probably further remove the idea that we are syncing "time windows". We're really syncing based on sync-key entries with some metadata (modseq, dav's equivalent, worst case fallback to timestamps).
  • The "getExtraData()" approach to store modseq is a bit convoluted, this could perhaps be solved better with some refactoring.

Diff Detail

Repository
rS syncroton
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

mollekopf created this revision.
mollekopf edited the summary of this revision. (Show Details)

Dropping unintended patch

machniak subscribed.
machniak added inline comments.
docs/SQL/mysql.initial.sql
63 ↗(On Diff #13386)

extra_data

lib/ext/Syncroton/Command/Sync.php
1000 ↗(On Diff #13386)

This new method should be added to the interface in lib/ext/Syncroton/Data/IData.php.

lib/ext/Syncroton/Data/AData.php
187 ↗(On Diff #13386)

This is not PHP syntax. We're not using this file, so you didn't notice ;)

lib/kolab_sync_data.php
566 ↗(On Diff #13386)

If something changed since we counted entries, the result may contain more/less entries, but maybe it's not a problem in reality.

671 ↗(On Diff #13386)

4th argument does not exist.

lib/kolab_sync_data_contacts.php
473 ↗(On Diff #13386)

@param for the 4th argument missing.

lib/kolab_sync_storage.php
1138 ↗(On Diff #13386)

It looks like we don't need this argument anymore.

1146 ↗(On Diff #13386)

Missing @param for $extraData.

1201 ↗(On Diff #13386)

Here I'd use json_decode()/json_encode(). We can left Zend_Json use to the Syncroton library only. In future I'd remove Zend_Json use from there too. Probably even the whole outdated Zend Framework.

1270 ↗(On Diff #13386)

I think this method should return an array. It seems to me you're json encoding the value twice.

1277 ↗(On Diff #13386)

Shouldn't that return something like ['modseq' => intval($value)]? In case we have other extras in future.

1281 ↗(On Diff #13386)

This will fail with DAV folders. I suppose for now you have to add public function getExtraData($folderid, $deviceid) { return ''; } in kolab_sync_storage_kolab4.

This revision now requires changes to proceed.Mar 12 2024, 12:23 PM
mollekopf marked 12 inline comments as done.
mollekopf retitled this revision from Test emptySync and flag change to Adjust adapter for phpunit 11 compat.
mollekopf edited the summary of this revision. (Show Details)

Addressed comments

lib/kolab_sync_data.php
566 ↗(On Diff #13386)

I don't think it's an issue.

mollekopf retitled this revision from Adjust adapter for phpunit 11 compat to Store modseq with the synckey.Mar 12 2024, 1:36 PM
------ ------------------------------------------------------------------------------------------ 
  Line   lib/kolab_sync_data.php                                                                   
 ------ ------------------------------------------------------------------------------------------ 
  570    If condition is always false.                                                             
  593    If condition is always false.                                                             
  603    Method kolab_sync_data::getExtraData() should return string|null but returns array|null.  
 ------ ------------------------------------------------------------------------------------------ 

 ------ ---------------------------------------------------------------------------------------------- 
  Line   lib/kolab_sync_storage.php                                                                    
 ------ ---------------------------------------------------------------------------------------------- 
  1276   Method kolab_sync_storage::getExtraData() should return array|null but returns string|false.  
  1284   Method kolab_sync_storage::getExtraData() should return array|null but returns string|false.  
 ------ ----------------------------------------------------------------------------------------------
This revision now requires changes to proceed.Mar 12 2024, 2:05 PM

Also, there's a few print()s in tests that need to be removed.

This revision is now accepted and ready to land.Mar 12 2024, 2:34 PM