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

Add exchange-rates-client fallback implementation #23

Merged
merged 1 commit into from
Jul 8, 2018

Conversation

solo-yolo
Copy link
Contributor

This PR tries to fix application build failure.
Currently, maven build failing due to failed ExchangeRatesClientTest test.
The test is failing while requesting exchange rates from Fixer API.

-------------------------------------------------------------------------------
Test set: com.piggymetrics.statistics.client.ExchangeRatesClientTest
-------------------------------------------------------------------------------
Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.895 sec <<< FAILURE! - in com.piggymetrics.statistics.client.ExchangeRatesClientTest
shouldRetrieveExchangeRates(com.piggymetrics.statistics.client.ExchangeRatesClientTest)  Time elapsed: 0.884 sec  <<< ERROR!
com.netflix.hystrix.exception.HystrixRuntimeException: getRates failed and no fallback available.

The root cause of this failure is discontinued Fixer API (since June 1st, 2018).
More details from the response:

Caused by: feign.FeignException: status 404 reading ExchangeRatesClient#getRates(Currency); content:
{
  "0": "#################################################################################################################################",
  "1": "#                                                                                                                               #",
  "2": "# IMPORTANT - PLEASE UPDATE YOUR API ENDPOINT                                                                                   #",
  "3": "#                                                                                                                               #",
  "4": "# This API endpoint is deprecated and has now been shut down. To keep using the Fixer API, please update your integration       #",
  "5": "# to use the new Fixer API endpoint, designed as a simple drop-in replacement.                                                  #",
  "6": "# You will be required to create an account at https://fixer.io and obtain an API access key.                                   #",
  "7": "#                                                                                                                               #",
  "8": "# For more information on how to upgrade please visit our Github Tutorial at: https://github.com/fixerAPI/fixer#readme          #",
  "9": "#                                                                                                                               #",
  "a": "#################################################################################################################################"
}

So, this PR adds a workaround in form of fallback implementation for ExchangeRatesClient, which returns empty ExchangeRatesContainer.

The motivation of this fix is the following:

  • fixing build failure for now
  • providing degraded service in case of third-party failure, instead of failing completely

Notes:
I've quickly looked at new Fixer API and would like to share with you what I've found :)

Fixer API now requires access key for any requests, which by itself not a big problem, as they are offering a free tier.
The worst part is that it's now limited up to 1000 requests per month and doesn't allow to specify the custom base currency.
As result, it seems to me, that Fixer SaaS API is not suitable anymore for this application.

Next steps:
As a further logical step to solve this issue, I would suggest to:

  • either switch to another exchange rates info provider
  • or deploy an instance of Fixer service as a part of the whole application ecosystem

What do you think? :)

Added exchange-rates-client fallback implementation, which returns
empty ExchangeRatesContainer.
@codecov-io
Copy link

codecov-io commented Jul 6, 2018

Codecov Report

Merging #23 into master will increase coverage by 0.16%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #23      +/-   ##
============================================
+ Coverage     80.86%   81.03%   +0.16%     
- Complexity      217      219       +2     
============================================
  Files            44       45       +1     
  Lines           575      580       +5     
  Branches         13       13              
============================================
+ Hits            465      470       +5     
  Misses          108      108              
  Partials          2        2
Impacted Files Coverage Δ Complexity Δ
...statistics/client/ExchangeRatesClientFallback.java 100% <100%> (ø) 2 <2> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eaec3c0...5763e90. Read the comment docs.

@sqshq
Copy link
Owner

sqshq commented Jul 8, 2018

Hi @q1nt! Thank you very much for the contribution. Looks good, I'll merge it.
As a next step, I agree, it would be great to switch to another exchange rates provider.

@sqshq sqshq merged commit 4935509 into sqshq:master Jul 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants