I'm still not sure that I understand the difference between "queried" and "saved". However maybe this example illustrates it:

• We query a principal and find N homesets. So we mark one URL (the principal) as "queried", but all the N homesets as "saved".
• We query another related URL and find again M homesets. So we mark the related URL as "queried", but all the M homesets as "saved". The homesets from the step before won't be added again, because they're already "saved". So "saved" applies to the operation of saving, while query makes sure that we don't query twice. Which are different things.

Is that correct?

But shouldn't the "saved" then apply to everything we find/save? There are other calls to homeSetRepository.insertOrUpdateByUrl() which are not checked against "alreadySaved". I wonder if we even should add/check the collections to "alreadySaved", too?

I understand that the change fixes our current issue. But I wonder if one would still understand the code / what it does without knowing about the current issue. However this should be the aim.

Another thing: we get the service and thus servcice.type as argument. Can we take the

val homeSetClass: Class<out HrefListProperty>
val properties: Array<Property.Name>
when (service.type) {
    Service.TYPE_CARDDAV -> {
        homeSetClass = AddressbookHomeSet::class.java
// …

block out of the discoverHomesets and make it properties of the object? It's not need to evaluate it more than once and I think it makes the discoverHomesets even more difficult to understand.

Is that correct?

That's exactly right. I renamed alreadyFetched to alreadySaved to make it more clear. alreadySaved should hold the URLs of home sets which have been saved to the database already. alreadyQueried should hold the URLs of principals wich we have made propfinds on and don't want to send a propfind to (query) again.

But shouldn't the "saved" then apply to everything we find/save? There are other calls to homeSetRepository.insertOrUpdateByUrl() which are not checked against "alreadySaved". I wonder if we even should add/check the collections to "alreadySaved", too?

I don't think so. I think by that argument already"Queried" would then have to apply to (remember) everything we query too - which we don't, since it does not really make sense to do that.

I can see however that both alreadyQueried and alreadySaved are only used in discoverHomesets() - so do not necessarily need to be properties accessible by other methods in CollectionListRefresher. I think alreadyQueried was made a property because discoverHomesets() is recursive, but we could simply add them both as parameters to the function. That makes their usage clearer and the class less confusing.

Furthermore I will rename alreadySaved to alreadySavedHomeSets, since it will only store home sets and alreadyQueried should only contain URLs of queried principals so that will be alreadyQueriedPrincipals. This should make it clear that other queries and "saves" are not held by these sets of URLs.

See the changes in 1f45c60

Can we take the [...] block out of the discoverHomesets and make it properties of the object?

I guess we could, but by aforemented reasoning I would leave it in the discoverHomesets method. Semantically it belongs there and that is also the only method where it's used. Or what we could do is to create another discoverHomesets method which takes care of the "setup" and then starts the actual recursion.

See the changes in 7247691

Shouldn't we use homeSetPropertyNames here, too, instead of *RefreshCollectionsWorker.DAV_COLLECTION_PROPERTIES?

However a bit later we also check whether the response is a usable collection. So we should actually query all properties (homeSetPropertyNames + DAV_COLLECTION_PROPERTIES), right?
Or maybe it's wise to put all relevant properties into DAV_COLLECTION_PROPERTIES and always query everything?

Since we query for collections we will have to use the collection properties in the propfind. We don't need to combine them. I don't see where that would be beneficial to us besides having a single property list.

It's an interesting idea to query all properties at once, however it is of little advantage for us and may put unecessary additional strain on some servers? I would be surprised if there was no RFC standard defining whether we are allowed to do that in our propfinds. Seems like allprop existed but is now deprecated.

Also both DAV_COLLECTION_PROPERTIES and DAV_PRINCIPAL_PROPERTIES are defined in RefreshCollectionsWorker, but only used in CollectionListRefresher. Should probably moved.

True. I did that now, renamed them both with camelcase and made all properties private.