Details

Reviewers

Group Reviewers

Commits

rSf24013a7c193: Store modseq with the synckey
rS1ada8e248093: Test an invalid sync key
rSdf97485265bb: Test for flag change retry.
rSc15d6a80dbb3: Don't forget changes while counting changes
rS3151f139e327: The modseq table expects the device primary key, not the deviceid
rS138ebc0c7853: Test emptySync and flag change
rS6f4acbae3f37: Adjust adapter for phpunit 11 compat

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 requested review of this revision.Mar 12 2024, 11:31 AM

mollekopf created this revision.

Harbormaster completed remote builds in B46749: Diff 13383.Mar 12 2024, 11:31 AM

Dropping unintended patch

Harbormaster completed remote builds in B46752: Diff 13386.Mar 12 2024, 11:34 AM

mollekopf added a reviewer: Syncroton Developers.Mar 12 2024, 11:34 AM

machniak requested changes to this revision.Mar 12 2024, 12:23 PM

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

Addressed comments

Harbormaster completed remote builds in B46755: Diff 13389.Mar 12 2024, 1:36 PM

mollekopf added inline comments.Mar 12 2024, 1:36 PM

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

Removed debug message

Harbormaster completed remote builds in B46758: Diff 13392.Mar 12 2024, 1:39 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.