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

Collection list refresh: Don't update fetched homesets #1222

Open
wants to merge 5 commits into
base: main-ose
Choose a base branch
from

Conversation

sunkup
Copy link
Member

@sunkup sunkup commented Jan 6, 2025

Purpose

At collection list refresh DAVx5 updates homesets when detected a second time. This leads to homesets detected as personal at first encounter (directly within the user principal), wrongly being overwritten as non-personal homesets (when found in a group principal afterwards).

This PR alters CollectionListRefresher.discCoverHomesets() to remember homesets already saved to database during the same refresh and won't overwrite/update them. They might only be updated at a second refresh if changed on server.

Short description

  • add a set of http urls alreadyFetched property to CollectionListRefresher
  • before saving/updating a homeset check wether it is in the set / has already been fetched
  • after saving/updating a homeset, add it to the alreadyFetched set
  • test the new behaviour

Checklist

  • The PR has a proper title, description and label.
  • I have self-reviewed the PR.
  • I have added documentation to complex functions and functions that can be used by other modules.
  • I have added reasonable tests or consciously decided to not add tests.

@sunkup sunkup added the bug Something isn't working label Jan 6, 2025
@sunkup sunkup self-assigned this Jan 6, 2025
@sunkup sunkup requested a review from ArnyminerZ January 7, 2025 10:17
@sunkup sunkup marked this pull request as ready for review January 7, 2025 10:17
@rfc2822
Copy link
Member

rfc2822 commented Jan 7, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Homesets are not "personal" when they are detected two times (1x personal, 1x non-personal)
3 participants