0

I am currently trying to return a JSON object array that requires me to do one asynchronous function and then four nested asynchronous map functions in order to populate an array of entities. Basically, each user has an array of orders, each order has an array of items, each item has an array of options and each option has an array of values. I am using loopback4 framework and therefore cannot do res.send once all things have been populated. The function seems to return on the first await, but any await after that, it does not wait on it and instead runs to the end of the function. I have tried using Promises and .thens(), but cannot seem to figure out how to populate each entity fully nested, and then return the array of populated entities. I keep getting an empty array. Below is only one nest of maps, but I cannot get it to even populate up to the first nest and return this, so I decided not to go any further. This is the code:

async getUserOrders2(@param.path.number('id') id: number): Promise<any> {
      if ( !this.user) {
        throw new HttpErrors.Unauthorized(AuthErrorKeys.ClientInvalid);
      }
      else if (this.user.id != id) {
        throw new HttpErrors.Unauthorized(AuthErrorKeys.ClientInvalid);
      }
      else  {
        let restaurantId = this.user.restaurantId
        let orderFrameArray = new Array<OrderFrame>()
        return this.restaurantRepository.orders(restaurantId as string).find()
        .then(async orders => {
          orders.map(async (val, key)=> {
            let orderFrame = new OrderFrame(val)
            orderFrame.itemArray = await this.orderRepository.orderItems(val.id).find()
            orderFrameArray.push(orderFrame)
          })
          orderFrameArray = await Promise.all(orderFrameArray)
          return orderFrameArray
        })
      }
}

The function is returning before the orderFrameArray has been populated. I need four nested map loops and this first one is not working, so I am not sure how to do the rest. Any help would be extremely appreciated.

Based on @Tomalaks solution I tried the following, but its still only returning the top level array and nothing is nested:

    async getUserOrders2(@param.path.number('id') id: number): Promise<any> {
      if ( !this.user) {
        throw new HttpErrors.Unauthorized(AuthErrorKeys.ClientInvalid);
      }
      else if (this.user.id != id) {
        throw new HttpErrors.Unauthorized(AuthErrorKeys.ClientInvalid);
      }
      else  {
        let restaurantId = this.user.restaurantId
        let orderFrameArray = new Array<OrderFrame>()
        return this.restaurantRepository.orders(restaurantId as string).find()
          .then(orders => {Promise.all(orders.map(
          order => { 
          let orderFrame = new OrderFrame(order)
          orderFrame.itemArray = new Array<Item>()
          this.orderRepository.orderItems(order.id).find()
            .then(orderItems => Promise.all(orderItems.map(
            orderItem => {
            let itemFrame = new Item(orderItem)
            itemFrame.options = new Array<Option>()
            this.orderItemRepository.orderItemOptions(orderItem.id).find()
                .then(orderItemOptions => Promise.all(orderItemOptions.map(
                orderItemOption => { 
                let optionFrame = new Option(orderItemOption)
                optionFrame.values = new Array<Value>()
                this.orderItemOptionRepository.orderItemOptionValues(orderItemOption.id).find()
                    .then(orderItemOptionValues => Promise.all(orderItemOptionValues.map(
                    orderItemOptionValue => { 
                    let valueFrame = new Value(orderItemOptionValue)
                    optionFrame.values.push(valueFrame)})))
                itemFrame.options.push(optionFrame)})))
              orderFrame.itemArray.push(itemFrame)})))
            orderFrameArray.push(orderFrame)}))
          return orderFrameArray})
      }
    }

I apologize for the formatting I wasn't sure how best to format it. Is there something else I'm doing wrong?

Thanks to everyone for their response. The answer that was posted by @Tomalak was correct. I just had to surround the entire function in brackets, and put a .then to return the populated entity I had made

Vikram Khemlani
  • 555
  • 6
  • 17
  • What does `this.restaurantRepository.orders()` return? If the function returns actual orders, then it's superfluous to `.find()` each of them again. If the function does *not* return actual orders, then it should have a different name. – Tomalak Sep 30 '19 at 11:35
  • It is supposed to return all of the orders in an array. So is it wrong then? – Vikram Khemlani Sep 30 '19 at 12:00
  • ...that does not answer my question. – Tomalak Sep 30 '19 at 13:02
  • Oh sorry. restaurantRepository.orders takes in an argument and returns an object of the repository I believe. You use .find to return an array of all the orders with a foreign key associated with the argument that was used to created the object. – Vikram Khemlani Sep 30 '19 at 13:20
  • The background of the question was *"Is it necessary to hit the database **again** or are the objects that are returned already the objects you are looking for?"* – Tomalak Sep 30 '19 at 13:33
  • In arrow functions, returns are implicit only in the absence of curly braces around the function body. With curly braces, an explicit `return` is necessary. All of the `.map()` callbacks and some of the `.then()` callbacks above need to be addressed in this regard. – Roamer-1888 Sep 30 '19 at 13:54
  • After making those amendments, you will also need to rearrange the code very slightly to avoid having statements after returns have been made. – Roamer-1888 Sep 30 '19 at 14:04
  • @Tomalak Sorry, it is. It is necessary to do .find() according to the loopback4 API docs. I truly do appreciate your patience and taking the time to help me though. – Vikram Khemlani Sep 30 '19 at 14:17
  • @Roamer-1888, is it necessary to return stuff though? I thought by accumulating it into one object, I would not need to return during any of the .map() as I am modifying an object as I go along? – Vikram Khemlani Sep 30 '19 at 14:17
  • Yes, it's absolutely vital to return promises from your `.then()` callbacks (where they exist). That's the way the chain is kept informed of inner asynchronous activity. So you must make those returns even if you are not delivering data in that manner. – Roamer-1888 Sep 30 '19 at 14:19
  • And it's vital to make returns from the `.map()` callbacks otherwise you will be mapping to an array of undefineds. `Promise.all()` needs an array of Promises to bite on. – Roamer-1888 Sep 30 '19 at 14:37
  • 1
    Thanks @Roamer-1888! I realized that I needed return statements each time to make it work – Vikram Khemlani Sep 30 '19 at 15:33

1 Answers1

3

You only need to use async when you are using await in the same function. If there's await in a nested function, the parent function does not need async.

However, in your case, there is no function that should be made async in the first place.

  • There is no benefit in awaiting any results in your function, because no code inside depends on any intermediary result. Just return the promises as you get them.
  • There's no need for intermediary result variables like orderFrameArray, you're making things harder than they are with your approach of awaiting individual orders and pushing them to a top-level variable.
  • Using await in a loop like you do inside your .map() call is bad for performance. You are basically serializing database access this way – the next query will only be sent after the current one has returned. This kind of daisy-chaining nullifies the database's ability to process multiple concurrent requests.
  • getUserOrders2 is not Promise<any>, it's Promise<Array<OrderFrame>>.
  • throw terminates the function anyway, you can do multiple checks for error conditions without using else if. This reduces nesting.

So a fully asynchronous function would look like this:

getUserOrders2(@param.path.number('id') id: number): Promise<Array<OrderFrame>> {
  if (!this.user) throw new HttpErrors.Unauthorized(AuthErrorKeys.ClientInvalid);
  if (this.user.id != id) throw new HttpErrors.Unauthorized(AuthErrorKeys.ClientInvalid);

  return this.restaurantRepository
    .orders(this.user.restaurantId).find().then(
      orders => Promise.all(orders.map(
        order => this.orderRepository.orderItems(order.id).find().then(
          order => new OrderFrame(order)
        )
      ))
    );
}

The async/await equivalent of this function would be more complex.

You then would await the result in the calling code, as you would have to do anyway:

async test() {
  const orders = await foo.getUserOrders2(someUserId);
  // ...
}

// or

test() {
  foo.getUserOrders2(someUserId).then(orders => {
    // ...
  });
}
Tomalak
  • 332,285
  • 67
  • 532
  • 628
  • Array of promises still needs to be aggregated with `Promise.all()`. – Roamer-1888 Sep 30 '19 at 11:02
  • I updated it based on your solution but somehow still only the top level array is being returned. Please let me know if you have any advice – Vikram Khemlani Sep 30 '19 at 12:00
  • I don't know what you did, but you certainly did not update anything based on my solution. None of the code you show resembles the code I wrote. – Tomalak Sep 30 '19 at 13:01
  • I used the .then and Promise.all as you had said, but I created an orderframe which collects the data as it iterates through each map, to create one big object which all of the data in it can be returned – Vikram Khemlani Sep 30 '19 at 13:22
  • @Tomalak When I tried what you had said, it returned the inner most nested object, but every layer above was empty for some reason – Vikram Khemlani Sep 30 '19 at 15:07
  • @Tomalak Actually, I think I understand it now though. Thanks so much for the help – Vikram Khemlani Sep 30 '19 at 15:07
  • I can only recommend that you split up your large function into smaller ones. Make one called `getOrderItemOptionValues()` that returns `Promise>`, one called `orderItemOptions()` that returns `Promise>`, and so on. This way your code will become much more manageable, which would be a huge improvement. The current `getUserOrders2` function is too complex. – Tomalak Sep 30 '19 at 15:34
  • _"This kind of daisy-chaining nullifies the database's ability to process multiple concurrent requests."_ +1 - Nice way to outline the fact that such an approach would lead to absurdity ;) – iLuvLogix Apr 23 '21 at 12:52