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

carddav: fix PROPFIND for all path layouts #99

Closed

Conversation

bitfehler
Copy link
Collaborator

Refactor the PROPFIND handling to properly support all possible path
layouts (from "everything at /" to "everything at different path"). This
includes adding a new method the Backend interface, as it is the
authority that determines the path layout.

This fixes the recent regression of the broken "everything at /" layout.
It also already provides the needed bits and pieces to support multiple
address books.

Refactor the PROPFIND handling to properly support all possible path
layouts (from "everything at /" to "everything at different path"). This
includes adding a new method the Backend interface, as it is the
authority that determines the path layout.

This fixes the recent regression of the broken "everything at /" layout.
It also already provides the needed bits and pieces to support multiple
address books.
@bitfehler
Copy link
Collaborator Author

So, this is sort of v2 of #87, but completely different. First of all, it fixes the regression I introduced (and un-comments the respective test case). I successfully tested the "all at /" case with both cadaver and TB/CardBook.

The depth handling is a bit confusing, but I think bearable. It won't be simple if we want to support these different layouts. The good news is that all this should already (mostly) work with multiple address books, should we ever get there 😉

I've stared at this for a too long now, and I figured your fresh eyes would surely have some valuable input, so here we go 🙂

// Backend is a CardDAV server backend.
type Backend interface {
CardDAVResourceType(ctx context.Context, path string) (ResourceType, error)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I was unsure about: I would have just named this ResourceType(), but we'd need the same thing for CalDAV eventually, and it's better to allow one type to implement both interfaces (i.e. keep the interfaces conflict-free).

One option might be to make this generic enough, e.g.: user principal, home set, collection, object - and then use that for both? Not sure, let me know if you have ideas...

bitfehler added a commit to bitfehler/go-webdav that referenced this pull request Aug 24, 2022
bitfehler added a commit to bitfehler/go-webdav that referenced this pull request Aug 24, 2022
bitfehler added a commit to bitfehler/go-webdav that referenced this pull request Sep 29, 2022
emersion pushed a commit that referenced this pull request Nov 2, 2022
@bitfehler
Copy link
Collaborator Author

Made obsolete by #101

@bitfehler bitfehler closed this Nov 15, 2022
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

Successfully merging this pull request may close these issues.

None yet

1 participant