Too Many Requests Error from Google (a production environment issue) solved with rates caching for currency pairs

[olarotseyi]

Hi Paul, first I want to thank you for this contribution to the Nodejs community. I have been building with the script from this package that you sent to me via email a while ago. I recently noticed that there could be a serious issue when using this package in a production environment. Google has a rate limit feature and too many requests within a short period of time will lead to a "429 Too many requests" error. Luckily, I have figured out a solution to this issue and I hope you accept my pull request.

• I noticed that you stopped multiplying the rate by the amount because google now sends the converted value. My solution reverts to multiplying the amount by the rate so that you can cache the rates for an hour therefore any need for the rate of the same currency pair within an hour will not require a request to Google.
• I implemented the rates caching for an hour therefore any need for the rate of the same currency pair within an hour will not require a request to Google. I thought I should share this issue to contribute to the developer community.
• Lastly, I will appreciate if you make me a contributor to this project.

[olarotseyi]

I just realized some issues with my rate caching solution.

• In line 237, I should be returning a promise. You can update line 237 to this:
  return new Promise((resolve, _) => {
  resolve(this.ratesCache[currencyPair]);
  });
• For the rates caching to work properly, you might have to export an instance of the CurrencyConverter class rather than the CurrencyConverter class itself. If the CurrencyConverter class is instantiated every time it is needed, the cache will be cleared and the caching will not be useful. Since the CurrencyConverter already supports chaining, you can export an instance of the CurrencyConverter (see below) therefore, the same CurrencyConverter instance is used everywhere in a project and the cache does not get cleared. This change might require some updates to the documentation in the README of the project.
  // exporting the currency converter instance will look like this:
  let currencyConverter = new CurrencyConverter();
  module.exports = currencyConverter;

[olarotseyi]

Hi Paul, I just want to let you know that if you want, I can make all the changes including the changes to the README file in another pull request. Kindly let me know if you want to update the tool to include caching.

[paul-shuvo]

Hi @olarotseyi, thanks for your contribution. I think that's a great idea to address the "429 Too many requests". I've some suggestions:

1. Rather than caching the total converted value i.e. rates x amount, only caching rates should suffice.
2. I think it'd be useful to make the caching an optional parameter as some application needs the very latest rates.
3. Devs should be able to set the duration of caching; the default can be an hour.
4. Some tests (I guess you'd need to use mocking) need to be added, if possible.

Let me know what you think. I've also checked your contribution to my other project. Once, I merge your request, you'll automatically become a contributor. 😃

[olarotseyi]

Hi @paul-shuvo, thank you for considering my pull request.

1. The solution I created only caches the rate and not the converted amount
2. I do agree that caching should be optional and the duration of the cache should be optional. If the CurrencyConverter is instantiated once in an entire project, then the developer will have to setup the caching option with the duration of caching once, the first time they import the CurrencyConverter in their project.

[olarotseyi]

Hey @paul-shuvo, I have made some updates to my solution and I have updated the README and the test.js files to reflect the updates. I have also made the updates to the nodejs-crypto-converter

[olarotseyi]

Hey @paul-shuvo, I just wanted to follow up on this pull request and the similar pull request I made to the crypto converter package. Is there any thing you think I should include in the rates caching solution?

[paul-shuvo]

Hi @olarotseyi , I went through your pull request. Here are some issues:

1. The library needs to support node version 12 through the latest. I was checking for node -v 17, and I got the following issue SyntaxError: Lexical declaration cannot appear in a single-statement context on index.js:263. You can fix it by checking this link. Check for the node versions 12 to 17.
2. It didn't pass all tests. Here's the screenshot
   .

Let me know if you have any question.

[olarotseyi]

Hey @paul-shuvo, thank you for pointing out the tests that were failing. I was able to fix the issues and pass all the tests in both the node-currency-converter tool and the nodejs-crypto-converter tool.

[olarotseyi]

Hey @paul-shuvo, I noticed an issue with my rate caching. I just noticed that the setTimeout function does not have access to the global objects such as this.currencyFrom and this.ratesCache and therefore the rate would not be deleted from the cache as expected, instead the application crashes. I am currently working to fix this by making each rate an object that has one value representing it's expiry date.

[olarotseyi]

I fixed the issue with overwriting instead of deletion. Each rate is now stored as an object with two properties, the actual rate and an expiry date. The expiry date is calculated using the ratesCacheDuration that is set by the user. The rate for a currency pair will not be deleted from the rates cache after the expiry date instead, it will be overwritten with the new rate and new expiry date when a request is made for the currency pair after the old rate of the currency pair has expired.

I fixed the issue in the nodejs-crypto-converter tool also.

[toluolatubosun]

Any plans for this to get merged soon? It would really help out. Thanks @paul-shuvo

[olarotseyi]

Hi @toluolatubosun, were you able to find a working currency converter to use?

[olarotseyi]

Hi @paul-shuvo, I noticed an issue with the got package when I ran a test of my implementation of the currency converter tool. The got package was randomly giving a particular error that has not been fixed in the got package and it seems it will take some time to fix the error. I decided to replace the got package with the request package and I made the same change to the crypto converter tool.

[paul-shuvo]

Hi @olarotseyi, many thanks for your contribution and apologies for this late feedback/comment. I checked your pull request some time ago and it had some issues (at least at my end). I myself was thinking of replacing got as the newer versions are not compatible, but contains some security/bug fixes; request seems to be a good fit. Having that being said, the current version of this package has been very stable, and thus I don't want to add a feature (as it might introduce additional bugs) unless the feature is required/requested by a lot of other developers. However, I think this is a neat feature to have, and thus, I'll merge your other pull request to the crypto converter library and observe it for some time before merging this pull request. All this should happen by mid December. Let me know your thoughts.

[olarotseyi]

Hi @paul-shuvo, I made the changes in a way I thought would ensure seamless integration. I will appreciate it if you can run the tests again and provide some feedback on any issues if you find any issues. Also, if you run the tests again and there are no issues, you can approve the merge as a new major version. I think developers using the tool have to manually change the version of the tool in their .env files to enable the latest version and I would suggest that developers run some tests before updating to the newer version of this tool. I believe the update will integrate with smoothly with existing projects using the tool.

[paul-shuvo]

Hi @olarotseyi ,

I'm getting the following test fails.

[olarotseyi]

Hi @paul-shuvo, I have made a new commit to fix the issues and all the tests were passed on my end.

[paul-shuvo]

Could you add tests for the following lines of code:

[olarotseyi]

Hi @paul-shuvo, thanks for pointing that out. The tests needed for the setupRatesCache function are already added. I just added tests for the addRateToRatesCache function. The tests work in a way where by all the existing tests must be passed for the tool to be useful. So even if it looks like some of the lines are not covered, they are actually covered by tests in other functions. For example, while adding tests to the addRateToRatesCache function, I noticed that the additional errors that I was trying to catch would actually be caught by the tests for the rates function or the convert function.

[olarotseyi]

Hi @paul-shuvo, thanks for the merge!

[olarotseyi]

Hey @toluolatubosun I don't know if you got the email about the merge but this updated tool is now ready for everybody to use!