Skip to content

Route refreshing TTL#827

Merged
Udumft merged 3 commits intomainfrom
vk/NAVIOS-1099
Jun 13, 2024
Merged

Route refreshing TTL#827
Udumft merged 3 commits intomainfrom
vk/NAVIOS-1099

Conversation

@Udumft
Copy link
Copy Markdown
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
@codecov
Copy link
Copy Markdown

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
Copy Markdown
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
credentials: BogusCredentials,
refreshTTL: refreshTTL)

XCTAssertNotNil(response.refreshTTL)
Copy link
Copy Markdown
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
Copy Markdown
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.


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
Copy Markdown
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
Copy Markdown
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
@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