Compatibility workaround: Accept XML content from calendar server even if it's marked up with content-type text/plain

[niccokunzmann]

When testing with the caldav library, I noticed that just in my tests, the content type got altered.
So, I was debugging for 2 hours in this lib and found that in case of text/plain, the content does not get used.
Then, I read through it and saw you wanted to refactor that anyway. So, here is my attempt at that.

Change:

• always attempt to parse response as XML
• dispatch on error:
  • raise if we expect XML
  • log if we do not expect an error
  • ignore content if we expect an error
• remove parsing code duplication

Related:

• Add CalDAV support niccokunzmann/open-web-calendar#660
• CALDAV synchronization niccokunzmann/open-web-calendar#189

[tobixen]

I feel that every time this part of the code is touched, someone with a weird server setup raises some issue complaining that the new changes breaks things in their end, so I should look carefully into it before pressing the merge-button.

[niccokunzmann]

That is why I proposed #451 (comment)

Then, they can record it when it works and you can test it.
I like TDD because I have no fear of changing the whole architecture. I did that twice for recurring-ical-events.

[tobixen]

Most end-users of the caldav library are not direct users. I believe the HomeAssistant caldav integration is the biggest contribution of end-users. Although quite a lot of HA-users are technically competent, it's not possible to ask all of them to run it ... so for every release, it's important to do as much as possible to ensure compatibility. And then of course it's important to have a stand-alone server checker tool, so that when end-users are having problems, they can run the said tool.

[tobixen]

I generally agree to the TTD-concept (though I've seen it taken too far - program split into too many internal/private methods/functions, all of them having unit tests - meaning that most refactoring efforts would breaks lots of unit tests, but I digress).

It's easy and sometimes even fun to write unit tests for purely logical functions that takes some input and should provide some output. When it comes to communication with databases and communication between different components in a layered architecture it becomes more complicated - but the caldav library is even worse, it's used for communication with arbitrary third-parties that may behave in erratic ways. I believe the way to handle this is through functional tests - and I've written up quite a lot of them for the caldav project. If I could run the functional test towards all possible server implementations that exists out there for every release, it would be ultra-safe to do refactoring work :-) I do have a list of servers that I test towards for every release, but it's a lot of work to maintain it, and it involves quite some time and effort to run the tests towards all the servers, hence the releases for the caldav library comes a lot slower than the releases for the recurring ical library :-)

[niccokunzmann]

Thank you for letting me know all this!

[tobixen]

Some thoughts:

• For some queries, it's expected that the server delivers calendar data. My immediate gut feeling is that it's wrong to try to parse the data from the server as XML if the server states it's calendar - but thinking three times on it, perhaps it's good anyway, make it robust enough to handle really weird servers, and if the response from the server is icalendar data, then the XML-parsing will fail really fast anyway, with no harm done.
• Your logic can do debug-logging of the content but only if the XML-parsing is successful. I think it makes sense to do debug-logging also of icalendar data and text.
• According to the comments I wrote there, the server should not deliver anything else than XML and icalendar data, but may sometimes deliver some errors in using the text/plain content type. It would probably be appropriate to at least log a warning, probably an error if the server returns a 2xx code but delivering anything else than XML or icalendar data.

[tobixen]

So I'd like to see those two changes:

• Debug-logging of content should happen regardless of weather XML-parsing is successful or not
• We should have a warning logged if the server returns a 2xx-response but neither XML nor icalendar data

I can fix this myself, but I think it's cleaner with one commit from one author for one logical changeset :-)

[niccokunzmann]

@tobixen, cool, you did some changes... What would you like me to do still? Is it ok for you this way?

[tobixen]

I think it's done

[tobixen]

Oh, it would have been nice to squash the commits and make a better commit entry

[tobixen]

This has a side effect, the errors shown from the caldav client are horrible if the server delivers a 500-error or similar. I will have to work a bit more on it.

[niccokunzmann]

Please let me know when you plan to do your integration tests with the other servers. I would like to have a look.

[tobixen]

I've already started. I'd like to do a 1.5-release with what I have now, and then possibly do a payment request (I've had the possibility to work quite a bit over the last few days). There are some regressions that have sneaked in, working to fix it in #492