Move Response extraction logic out of init of DavException

[ArnyminerZ]

Moved all the code inside of init in DavException to its own function to initialize the contents, decoupling a bit better how the data is instantiated.

---

Depends on #82

[Copilot]

Pull Request Overview

This PR refactors the DavException class by extracting HTTP response processing logic from the constructor into a separate populateHttpResponse method. This improves separation of concerns and makes the exception instantiation more flexible.

Key changes:

• Extracted HTTP response processing logic from init block into populateHttpResponse method
• Made DavException constructor no longer accept httpResponse parameter
• Updated all usage sites to call populateHttpResponse after exception creation

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
DavException.kt	Removed httpResponse parameter from constructor, moved processing logic to populateHttpResponse method
HttpException.kt	Updated constructor to call populateHttpResponse instead of passing response to parent
DavResource.kt	Updated exception throwing to use fluent API with populateHttpResponse
DavExceptionTest.kt	Updated test to use new fluent API pattern

[sunkup]

It's bothersome to first create DavException and then populate with HttpResponse afterwards.

Is there a good reason for not adding a static fromResponse(response: HttpResponse) to the companion object of DavException, which would create DavException already populated with the needed properties from the response, like @rfc2822 suggested?

[ArnyminerZ]

Is there a good reason for not adding a static fromResponse(response: HttpResponse) to the companion object of DavException, which would create DavException already populated with the needed properties from the response, like @rfc2822 suggested?

Yes, DavException is an open class, and some other exceptions inherit from it, and thus need the constructor. Otherwise I think there's too much code duplication...

The other option is to use reflection or something like that, but I don't love that approach.

[sunkup]

Right. I meant this: why don't we make populateHttpResponse private (I would also rename the method to populateWithHttpResponse, as otherwise it sounds like it'd populate the HttpResponse) and just add fromResponse to the companion object for ease of access like this:

fun fromResponse(
    message: String,
    httpResponse: Response?,
    ex: Throwable? = null
): DavException = 
    DavException(message, ex).populateWithHttpResponse(httpResponse)

[rfc2822]

I also wonder about how a good API would look like for the (discussable) requirements that:

• there's a DavException that is thrown on failures that are not directly HTTP errors, like when
  • dav4jvm expects a 207 multi-status, but gets something else, or
  • there are too many HTTP redirects (other libraries seem to use ProtocolException for that – it's also an option to reuse existing exceptions, if they match)
  • XML processing errors in WebDAV XML
• HttpException for HTTP errors.

Both should be able to contain some context of the response body because servers often provide useful information in the response body. For instance, errors are often just 500 Internal Server Error, but the response body could contain more information. This information is shown for instance to DAVx5 users in the debug info screen.

Additionally there's the WebDAV XML error element that should be processed.

The classes probably shouldn't be coupled to a specific HTTP framework too tightly.

So some questions that come to my mind:

• Which exception classes do we actually need?
• New ones or can some of them be reused?
• In which relation to each other are they (should they really be derived)?
• How do we preserve the body excerpt properly (and without redundant code, i.e. where belongs the processing to and from where should it be called)?

[ArnyminerZ]

@sunkup please take a look and see if you like this more.

[sunkup]

@sunkup please take a look and see if you like this more.

mmh, no. Now there is a side effect in the constructor: It executes logic (reading/parsing HTTP body) that can throw exceptions or cause blocking I/O. It makes the constructor "unsafe" and violates the general principle that constructors should be light and free of side effects. It now has more than one responsibility (to simply construct and fill the object) so it violates the single-responsibility principle.

Also it is harder to test/mock now because you need to know which constructor you need to use in order to not invoke the population logic.

Using the factory method (fromResponse in companion object) proposed by @rfc2822 , will ensure cleaner separation of concerns: construction vs. population. It allows the constructor to remain side-effect free and it also improves clarity and intention (i.e., "I'm constructing from a response").

Sure, it's slightly more verbose in usage, but I think that's actually a benefit here.

[ArnyminerZ]

I've tried setting up some kind of templating but it's ugly... I honestly don't like it.

[rfc2822]

I've tried setting up some kind of templating but it's ugly... I honestly don't like it.

I agree. Replaced by #83, please follow up there.