2

I am using the request-promise npm package to make multiple api calls in nodeJS. In order for me to get the data I require, I would have to cycle through all categories and then in each category cycle through the array of products to get the product info. Sample category response is as below

{
        "success": true,
        "response": [
          {
            "category_id": 9,
            "name": "Leather Wheeled luggage",
            "products": [
              "TL141911",
              "TL141888"
            ],
            "parent_category_id": 34
          },
          {
            "category_id": 10,
            "name": "Leather Luggage Weekender Duffles bags",
            "products": [
              "TL141794",
              "TL141658"

            ],
            "parent_category_id": 34
          }
        }

Because I have to loop through and make api calls, I am trying to use Promise.all, but it is not producing the correct results, it is throwing me a 404 not found error, can someone please help me where i am going wrong here? below is what i have tried

const rp = require('request-promise');

const requestUrl = "https://stage.tuscanyleather.it/api/v1/";
const categoryRequestUrl = requestUrl + 'categories';
let tuscanApiOptions = {
    uri: categoryRequestUrl,
    headers: {
        'Authorization': 'Bearer asd343'
    },
    method: 'Get',
    json: true 
};

rp(tuscanApiOptions)
    .then((categResponse) => {
        //res.end(categResponse);  
        let ps = [];
        categResponse.response.forEach((category) => {
            if (category.products !== undefined) {
                category.products.forEach((product) =>{
                    let productApiOptions = {
                        uri: requestUrl + `product-info?code=${product}`,
                        headers: {
                            'Authorization': 'Bearer asd343'
                        },
                        method: 'Get',
                        json: true 
                    };
                    ps.push(rp(productApiOptions));
                });
                Promise.all(ps)
                    .then((values) => {
                        console.log(values); //This is where things are going wrong
                  })
                    .catch((err) =>{
                        console.log(JSON.stringify(err,null,2));
                    })
            }

        })

    })
    .catch((error) => {
        console.log(error);
        //res.status(error.statusCode).send(error.error.error_description);
    });
sab
  • 338
  • 1
  • 3
  • 21
  • Related to your other question from an hour ago here: https://stackoverflow.com/questions/60358803/multiple-api-requests-using-request-promise-in-nodejs – jfriend00 Feb 23 '20 at 04:25
  • I thought, I did not frame the question properly, so I thought its better to post another one with additional information – sab Feb 23 '20 at 04:30
  • You should not have two questions on the same basic topic. When people have already engaged with your question and are working with you on it, if you want to clarify the question, then use the "edit" link to modify your question, not just open up a new one. Please delete one of these two questions. – jfriend00 Feb 23 '20 at 06:35
  • @jfriend00 i have deleted the other question , thanks – sab Feb 23 '20 at 06:37
  • Is your `request` variable shown in your code the `request-promise` module? If so, please add that to your code so that is shown. When I use the `request-promise` module (which has been deprecated by the way), I name it something different such as `rp` so anyone reading my code will not think it's the regular `request` module. – jfriend00 Feb 23 '20 at 06:41
  • I would suggest you log the exact URL that is producing a 404 error and then test that URL independently. – jfriend00 Feb 23 '20 at 06:43
  • @jfriend00 : I did log it and test, and they seem to work when I test it independently, it's only when it is used in Promise.all, it fails. – sab Feb 23 '20 at 07:00
  • Then, perhaps your target host has implemented some sort of rate limiting and they return a 404 when you send requests too fast. – jfriend00 Feb 23 '20 at 07:53
  • @jfriend00 : you are correct they do have a rate limit of 120 requests per min. This is where i m confused with async programming, how and where should i control the number of requests for a given timeline – sab Feb 23 '20 at 13:41
  • One idea for that here: [Rate limiting for a particular number of requests per second](https://stackoverflow.com/questions/36730745/choose-proper-async-method-for-batch-processing-for-max-requests-sec/36736593#36736593). You can try that at a low requests per second such as 1 request per second and see if it keeps you from getting blocked. – jfriend00 Feb 23 '20 at 15:38
  • 404 is a rather odd response code for rate limit violation. Not impossible though - servers aren't always configured as well as they might be. – Roamer-1888 Feb 23 '20 at 21:54

1 Answers1

3

This code should help you to debug the 404s:

const rp = require('request-promise');

const requestUrl = "https://stage.tuscanyleather.it/api/v1/";
const categoryRequestUrl = `${requestUrl}categories`;
const tuscanApiOptions = {
  uri: categoryRequestUrl,
  headers: {
    'Authorization': 'Bearer asd343'
  },
  method: 'Get',
  json: true 
};

// Promise of flat array of products (loses category data)
const productsP = rp(tuscanApiOptions))
  .then(({response}) => response
    .map(category => category.products) // array of arrays
      .flat() // flatten to array of products
      .filter(p => !!p) // remove empty 
  )

const prodUrl = `${requestUrl}product-info?code=`

const productDataP = productsP.then(products => 
  products.map(product => 
    rp({
      ...tuscanApiOptions,
      uri: `${prodUrl}${product}` 
    }).catch(e => `Error fetching ${prodUrl}${product} ${e.message}`)
  )
)

// Wait for all requests to resolve with data or error. 
// resultsP is an array of Promise<productdata|error>
const resultsP = Promise.all(productsDataP)

resultsP.then(console.log)             

A couple of suggestions, take 'em or leave 'em:

Take Array.forEach out of your programming vocabulary. It will cause you more problems than it solves, because it is side-effective (essentially a loop). Use Array.map to return a new array, then operate on that. Array.map separates the state machine from the data transformation, forEach mixes them together.

Don't use let or var for things that don't mutate. That tells other programmers and the machine not to rely on the value. Use const.

See this article for a more in-depth explanation.

It might be easier to do this with async/await, at least to debug. You have data structure traversal and async monads (Promise) you are grappling with here, and a 404 you need to debug. awaiting a promise extends the value out of the monad context, so it removes one layer of complexity.

When you have an array of Promises, you await Promise.all(arrayP) to get an array of the unwrapped values.

const rp = require('request-promise');

const requestUrl = "https://stage.tuscanyleather.it/api/v1/";
const categoryRequestUrl = `${requestUrl}categories`;
const tuscanApiOptions = {
  uri: categoryRequestUrl,
  headers: {
    'Authorization': 'Bearer asd343'
  },
  method: 'Get',
  json: true 
};

async function main() {
  const categories = await rp(tuscanApiOptions)
  const products = categories.response
    .map(category => category.products) 
    .flat() 
    .filter(p => !!p) // remove undefined

  console.log(products) // should be flat array of products

  const prodUrl = `${requestUrl}product-info?code=`

  const requestsP = products
    .map(product => 
      rp({
        ...tuscanApiOptions,
        uri: `${prodUrl}${product}` 
      }).catch(e => `Error fetching ${prodUrl}${product} ${e.message}`)
  )

  const results = await Promise.all(requestsP)

  console.log(results)
}

main()   
Josh Wulf
  • 4,727
  • 2
  • 20
  • 34
  • 1
    That's a pretty extreme position on good ol' forEach. I think your let and var advice in terms of “signaling” future readers is good. Maybe we should allow forEach to remain in the language as a signal that a procedure, rather than a function, is being applied. – danh Feb 24 '20 at 15:55
  • Have a look at the section "Custom State Machines: Loops" in [this article](https://www.joshwulf.com/blog/2020/02/just-say-no-to-loops-and-variables/) and see what you think. Replace `loop` with `forEach`, because it is the same thing, the only difference is that forEach encapsulates the counter. – Josh Wulf Feb 24 '20 at 15:59
  • 1
    Good article! It ought to be transformed (mapped!) into the syllabus of a *required* undergrad CS course. But I remain unconvinced about loops. Consider the example you gave, counting Nums in an array. Map and its variants (like filter, admittedly prettier) produce an intermediate array and a second iteration where none is needed. Didn't Confucius also say, "don't litter" and "just because you can't see it, that don't mean it ain't there" and "a long journey begins with one step, and no sideways steps"?? :-) – danh Feb 24 '20 at 16:29
  • After answering a bunch of questions in the JS and node tags these last couple of weeks, I'm convinced it's a foot gun. :-P Like this question - the intersection of state management, async, and data transform leaves you fatigued and at the effect of the complexity. You ain't got any brain power left after reasoning about that to debug. And your code is so fragile you are scared to touch it because it might break the state machine. – Josh Wulf Feb 24 '20 at 16:32