Skip to content

Improve DavException construction #83

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

rfc2822
Copy link
Member

@rfc2822 rfc2822 commented Aug 13, 2025

After doing some research and thinking about

  • what exception classes we have and what they're used for,
  • how other HTTP / HTTP-using frameworks define their exceptions,
  • the questions mentioned earlier

I came to this proposal that shall close #80:

a HttpResponse body may or may not be consumable (think about a streaming transfer that is already finished)

Same for the request body. DavException handles those cases and doesn't set a request/response excerpt then. Also added the JSR 305 @WillNotClose to make clear that the DavException won't close the response (so it's the responsibility of the caller, who should always use use or try/finally).

it's not clear why the whole response is needed in the exception,

It's handy for constructing the response. This pattern is also used for instance in Ktor's ResponseException.

it could cause problems when we try to serialize the exception,

True → added test for JVM serialization, because Exception base class is Serializable.

it couples a HTTP-level data class with a specific HTTP implementation.

Only for construction. To make this clear, I changed the primary constructor so that it takes only simple values. HttpResponse parsing is moved to a secondary constructor.


Further notes / changes:

  • Simplified request / response body logic and XML error extraction.
  • Exceptions for specific HTTP error codes now validate the error code to avoid mistakes.
  • Make InvalidPropertyException a subclass of DavException.
  • Simplify DavResource.checkStatus().
  • Made Error a data class so that it prints the error names in toString() and errors can be compared.

@rfc2822 rfc2822 self-assigned this Aug 13, 2025
@rfc2822 rfc2822 added the refactoring Internal improvement of existing functions label Aug 13, 2025
@rfc2822 rfc2822 linked an issue Aug 13, 2025 that may be closed by this pull request
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR improves DavException construction by refactoring how HTTP responses are handled in exceptions and simplifying the exception hierarchy. The changes focus on making exception construction more type-safe and removing serialization concerns.

  • Restructured DavException to have a primary constructor with simple parameters and a secondary constructor for HTTP response parsing
  • Added validation to specific HTTP exception subclasses to ensure they're only created with the correct status codes
  • Simplified exception construction logic and removed serialization support

Reviewed Changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
src/main/kotlin/at/bitfire/dav4jvm/exception/DavException.kt Major refactoring of primary constructor and HTTP response parsing logic
src/main/kotlin/at/bitfire/dav4jvm/exception/HttpException.kt Simplified to only accept Response objects, removed status code constructor
src/main/kotlin/at/bitfire/dav4jvm/exception/*Exception.kt Added status code validation to specific HTTP exception subclasses
src/main/kotlin/at/bitfire/dav4jvm/DavResource.kt Updated exception construction and simplified checkStatus method
src/main/kotlin/at/bitfire/dav4jvm/Error.kt Made Error a data class for better toString and comparison
src/test/kotlin/at/bitfire/dav4jvm/exception/*Test.kt Updated tests to reflect new property names and construction patterns
gradle/libs.versions.toml Added SpotBugs annotations dependency

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@rfc2822 rfc2822 requested a review from Copilot August 13, 2025 15:05
@rfc2822 rfc2822 marked this pull request as ready for review August 13, 2025 15:05
@rfc2822 rfc2822 requested review from sunkup and ArnyminerZ August 13, 2025 15:18
Copilot

This comment was marked as outdated.

@rfc2822 rfc2822 requested a review from Copilot August 13, 2025 15:42
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the DavException construction to improve its design and usability. The main purpose is to decouple HTTP response parsing from the primary constructor while maintaining backwards compatibility and improving error handling.

Key Changes

  • Changed DavException primary constructor to accept simple values instead of HTTP response objects
  • Added validation to HTTP exception subclasses to ensure correct status codes
  • Made Error class a data class for better debugging and comparison capabilities

Reviewed Changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/main/kotlin/at/bitfire/dav4jvm/exception/DavException.kt Complete refactor of exception construction with new primary constructor and response parsing logic
src/main/kotlin/at/bitfire/dav4jvm/exception/HttpException.kt Simplified to only accept Response objects, removing dual constructor pattern
src/main/kotlin/at/bitfire/dav4jvm/exception/*.kt Added status code validation to specific HTTP exception classes
src/main/kotlin/at/bitfire/dav4jvm/Error.kt Converted to data class for better toString() and equality support
src/test/kotlin/at/bitfire/dav4jvm/exception/DavExceptionTest.kt Comprehensive rewrite of tests to cover new functionality
src/main/kotlin/at/bitfire/dav4jvm/DavResource.kt Updated to use new exception constructors and simplified checkStatus method

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Internal improvement of existing functions
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

DavException contains HTTP response
1 participant