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

Route refreshing TTL #827

Merged
merged 3 commits into from
Jun 13, 2024
Merged

Route refreshing TTL #827

merged 3 commits into from
Jun 13, 2024

Conversation

Udumft
Copy link
Contributor

@Udumft Udumft commented Jun 11, 2024

Added route response refresh TTL field and a DirectionsError to signal refreshing has expired

…nsError to signal refreshing has expired; Unit tests added.
@Udumft Udumft added the feature New feature request. label Jun 11, 2024
@Udumft Udumft self-assigned this Jun 11, 2024
Copy link

codecov bot commented Jun 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.32%. Comparing base (c277fff) to head (2701cc4).

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #827      +/-   ##
==========================================
+ Coverage   84.24%   84.32%   +0.08%     
==========================================
  Files          59       59              
  Lines        4906     4926      +20     
==========================================
+ Hits         4133     4154      +21     
+ Misses        773      772       -1     
Files Coverage Δ
Sources/MapboxDirections/Directions.swift 74.35% <100.00%> (+0.26%) ⬆️
Sources/MapboxDirections/DirectionsError.swift 87.40% <100.00%> (+1.69%) ⬆️
Sources/MapboxDirections/RouteResponse.swift 87.03% <100.00%> (+0.43%) ⬆️

@Udumft
Copy link
Contributor Author

Udumft commented Jun 12, 2024

List of accepted breaking changes:

  • 💔 API breakage: enumelement DirectionsError.refreshExpired has been added as a new enum case
    
  • PR adds handling of a new type of an error, I don't see a way how to avoid adding a new enum case, other than abusing .unknown.
  • 💔 API breakage: constructor DirectionsError.init(code:message:response:underlyingError:) has been removed
    
  • New argument added to this constructor with a default value -> won't require code changes on client's side
  • 💔 API breakage: constructor RouteResponse.init(httpResponse:identifier:routes:waypoints:options:credentials:) has been removed
    
  • New argument added to this constructor with a default value -> won't require code changes on client's side

@Udumft Udumft requested a review from a team June 12, 2024 08:46
@Udumft Udumft marked this pull request as ready for review June 13, 2024 07:41
Sources/MapboxDirections/DirectionsError.swift Outdated Show resolved Hide resolved
Sources/MapboxDirections/DirectionsError.swift Outdated Show resolved Hide resolved
credentials: BogusCredentials,
refreshTTL: refreshTTL)

XCTAssertNotNil(response.refreshTTL)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not check for equality?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Equality is checked below.

@@ -8,7 +8,11 @@ import FoundationNetworking
*/
public enum DirectionsError: LocalizedError {

public init(code: String?, message: String?, response: URLResponse?, underlyingError error: Error?) {
public init(code: String?, message: String?, response: URLResponse?, underlyingError error: Error?, refreshTTL: Int? = nil) {
guard refreshTTL == nil || refreshTTL != 0 else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure that in the case when refreshTTL == 0 and e.g. response.statusCode == 404 the error should be . refreshExpired?
As for me, it's better first to check the non-200 response code and only then check refreshTTL.
Btw, how does the Android platform handle this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refresh_ttl in the refresh response will be appended and =0 only in case of the refresh invalidation error. Android also does the check for this parameter to be 0

Docs corrections

Co-authored-by: Nastassia Makaranka <nastassia.makaranka@mapbox.com>
@Udumft Udumft enabled auto-merge (squash) June 13, 2024 08:47
@Udumft Udumft merged commit 9a23bdc into main Jun 13, 2024
11 checks passed
@Udumft Udumft deleted the vk/NAVIOS-1099 branch June 13, 2024 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants