feat: Add URL extraction support to AFError#3976
Conversation
- Add public url property to AFError for extracting URLs from error contexts - Implement extractURL helper method for consistent URL extraction from underlying errors - Add url properties to ResponseValidationFailureReason and ResponseSerializationFailureReason - Support URL extraction from URLError instances via failingURL property - Handle nested error types including multipart, validation, and serialization failures - Delegate to reason-specific URL extraction for complex error types - Return nil for creation/encoding/lifecycle errors as per design decision - Add comprehensive test suite with 34 test cases covering all 17 AFError types - Update Usage.md with documentation on extracting URLs from AFError instances - Note architectural limitation that errors don't automatically include request URLs
jshier
left a comment
There was a problem hiding this comment.
Thanks for the PR! I have a few points of cleanup, and I'll review the contents of the tests once that occurs.
|
|
||
| /// The `URL` associated with the error. | ||
| /// Helper method to extract URL from an error if it's URLError or AFError | ||
| private func extractURL(from error: (any Error)?) -> URL? { |
There was a problem hiding this comment.
This seems better expressed as an extension on Error, which you can added in the extension section later in this file.
extension Error {
var url: URL? {
...
}
}Additionally, it seems like should be more cases where URLs are available from Foundation errors, please do a more thorough examination.
| case let .dataFileReadFailed(at: url): | ||
| return url | ||
| case let .customValidationFailed(error): | ||
| if let urlError = error as? URLError { |
There was a problem hiding this comment.
This should simply be the Error.url extension property.
| } | ||
| ``` | ||
|
|
||
| #### Extracting URLs from `AFError` |
There was a problem hiding this comment.
This documentation doesn't seem all that useful right now, please remove it.
| /// Extracts URL information from error contexts where applicable. Due to Alamofire's architecture, | ||
| /// errors don't automatically include request URLs. | ||
| /// | ||
| /// **URL extraction by error type:** |
There was a problem hiding this comment.
No need for any bold treatment here, and please extract the error explanations into a Note section so that it renders independently in the markdown.
| case let .requestAdaptationFailed(error): | ||
| return extractURL(from: error) | ||
| case let .requestRetryFailed(retryError, originalError): | ||
| return extractURL(from: originalError) ?? extractURL(from: retryError) |
There was a problem hiding this comment.
This isn't a valid fallback, the URL of a retry failure should only come from the retryError.
| // | ||
| // AFErrorURLExtractionTests.swift | ||
| // | ||
| // Copyright (c) 2014-2018 Alamofire Software Foundation (http://alamofire.org/) |
There was a problem hiding this comment.
All new source files should have the current year as copyright.
|
Thanks for your patience! I finally have time to review the pending PRs now that I've gotten the important 5.11 release out, so I should be getting these reviewed and merged over the holidays. |
Issue Link 🔗
#3965
Goals ⚽
Enhance
AFError.urlproperty to extract URL information from all applicable error types, improving debugging and error tracking capabilities. Previously only supported multipart encoding failures.Implementation Details 🚧
Core Changes:
AFError.urlto support 17 different error types through comprehensive switch statementextractURL(from:)helper method for consistent URL extraction from nested errorsSupported Error Types:
.multipartEncodingFailed,.responseValidationFailed,.responseSerializationFaileddelegate to reason's URL.sessionTaskFailed,.sessionInvalidated,.requestAdaptationFailed,.requestRetryFailedextract from underlyingURLErrornil- useresponse.request?.urlfor request URLUsage:
Backward compatibility: Existing
AFError.urlbehavior for multipart errors unchanged.Testing Details 🔍
New Tests: 461 lines of comprehensive test cases in
AFErrorURLExtractionTestsDocumentation: Added Usage.md section with examples and architectural notes.
Results: All tests pass, existing AFError functionality unaffected.