Netflix / falcor-router

A Falcor JavaScript DataSource which creates a Virtual JSON Graph document on your app server.
http://netflix.github.io/falcor
Apache License 2.0
104 stars 46 forks source link

Router fails when jsong response includes arrays #59

Open bcardi opened 9 years ago

bcardi commented 9 years ago

When the jsong response includes arrays, the router mistakenly converts these to undefined atoms. I have created a test to demonstrate the issue. I wanted input from the team before submitting a pull request with my test.

it('should allow arrays in jsong response', function (done) {
    var routeResponse = {
       "jsong":{
          "ProductsById":{
             "KMW39254":{
                "SellingPoints":[
                   "Charges USB powered devices with micro or mini ports.",
                   "Charges in the car or on AC wall outlet.",
                   "Ultra-small and low profile.",
                   "Car charger is one amp-ready for quick smartphone charging."
                ]
             }
          }
       }
    };
    var router = new R([
        {
            route: "ProductsById[{keys}][{keys}]",
            get: function (pathSet) {
                return Observable.of(routeResponse);
            }
        }
    ]);
    var obs = router.get([["ProductsById", "KMW39254", "SellingPoints"]]);
    var called = false;
    obs.subscribe(function (res) {
        expect(res).to.deep.equals(routeResponse);
        called = true;
    }, done, function () {
        expect(called, 'expect onNext called 1 time.').to.equal(true);
        done();
    });
});

When I run the test, it fails as shown below.

1) Router Unit Core Specific should allow arrays in jsong response:

  AssertionError: expected { Object (jsong) } to deeply equal { Object (jsong) }
  + expected - actual

   {
     "jsong": {
       "ProductsById": {
         "KMW39254": {
  -        "SellingPoints": {
  -          "undefined": {
  -            "$type": "atom"
  -          }
  -        }
  +        "SellingPoints": [
  +          "Charges USB powered devices with micro or mini ports."
  +          "Charges in the car or on AC wall outlet."
  +          "Ultra-small and low profile."
  +          "Car charger is one amp-ready for quick smartphone charging."
  +        ]
         }
       }
     }
   }
jhusain commented 9 years ago

Hey Bob this doesn't look like a bug. You cannot return an array or object from a route. Every route must evaluate to a value type (string, number, boolean, reference, atom, error). If you want to, you can return an array. However you have to wrap the Array inside of an atom.

{ $type: "ref", value: [...] }

Let me ask you this: do you really want to return an entire array all the time? Or would you like to be able to page over it? If you want to return the entire array use an atom, otherwise match a range in the route and page over it.

@mpaulson If this is indeed the issue, we probably need to add a better error message here. Seems like an object or an array where a value type was expected would be a good thing to give a descriptive error message on, similar to the great ones we have for the parser.

bcardi commented 9 years ago

Selling points and several other "simple" arrays are not separate entities with key structures that we can reference; thus there is no route for these. They are stored with the product object in the data store (elastic search). To answer your question, yes, we return the entire contents of these arrays.

You mention wrapping the arrays in an atom, can you give more details or provide an example?

Below is an example of the full JSON Product object we read from elastic search. You will see the object contains several simple arrays, which all fail to return as part of the "ProductsById[{keys}][{keys}]" route.

{  
   "Sku":"ADC BFSM-950",
   "LongDescription":"Mesh Fry Basket, Steel, 9 1/2\" Dia x 5 3/4\"",
   "ShortDescription":"FRY BSKT,STEEL,9.5\"",
   "Status":1,
   "VendorId":1573,
   "Height":0.8000,
   "Length":1.0000,
   "Width":1.0000,
   "Weight":1.7000,
   "Uom":"EA",
   "UpcRetail":"            ",
   "UpcCarton":"              ",
   "AssemblyRequired":false,
   "MadeFromRecycled":false,
   "Returnable":true,
   "MinOrderQty":1,
   "Hazmat":false,
   "BrandName":"Adcraft®",
   "BrandLogoUrl":"http://cdn.facility.supplies/WebImages/Logos/ADCRAFT_LOGO.JPG",
   "BrandWebsiteUrl":"",
   "Name":"Adcraft® Fryer Baskets",
   "SummarySellingPoint":"Steel mesh fryer basket.",
   "SellingPoints":[  
      "Steel mesh fryer basket.",
      "Nickel plated steel.",
      "Made to be used when frying with oil."
   ],
   "SellingCopy":{  
      "Long":"This round wire fry basket is ideal for light to heavily-breaded food products. This basket can handle the most rigorous of conditions. It can dive right into the fryer and create something delicious. It is nickel-plated to resist corrosion and is easy to clean.",
      "Short":"This round wire fry basket is ideal for light to heavily-breaded food products. This basket can handle the most rigorous of conditions. It can dive right into the fryer and create something delicious. It is nickel-plated to resist corrosion and is easy to clean.",
      "Medium":"This round wire fry basket is ideal for light to heavily-breaded food products. This basket can handle the most rigorous of conditions. It can dive right into the fryer and create something delicious. It is nickel-plated to resist corrosion and is easy to clean.",
      "Catalog":""
   },
   "Keywords":[  
      "Fry Basket",
      "Steel",
      "Storage",
      "Cooking",
      "Baking",
      "Breakrooms",
      "Chefs",
      "Cookery",
      "Culinary",
      "Kitchens",
      "Packages",
      "Restaurants"
   ],
   "PackageIncludes":[  
      "Includes one basket."
   ],
   "Thumbnails":[  

   ],
   "SmallImages":[  

   ],
   "LargeImages":[  

   ],
   "Videos":[  

   ],
   "Attributes":[  
      {  
         "Name":"Global Product Type",
         "Value":"Cookware-Fryer Basket"
      },
      {  
         "Name":"Cookware Type",
         "Value":"Mesh Fry Basket"
      },
      {  
         "Name":"Material(s)",
         "Value":"Steel"
      },
      {  
         "Name":"Color(s)",
         "Value":"Silver"
      },
      {  
         "Name":"Handle Material",
         "Value":"Steel"
      },
      {  
         "Name":"Diameter",
         "Value":"9 1/2\""
      },
      {  
         "Name":"Height",
         "Value":"5 3/4\""
      }
   ]
}
jhusain commented 9 years ago

When I say wrap arrays and objects in atoms I mean this:

{ route: "ProductsById[{integers:numbers}].Thumbnails", (pathSet) => // lets say it's just one integer (5) { jsong: { ProductsById: { 5: { Thumbnails: { $type: "atom", value: [ // actual thumbnails ] } } } } }

Is that clear? I just mean take each array and wrap it inside of an Atom.

Sent from my iPad

On Jun 14, 2015, at 8:47 PM, Bob Cardenas notifications@github.com wrote:

Selling points and several other "simple" arrays are not separate entities with key structures that we can reference; thus there is no route for these. They are stored with the product object in the data store (elastic search). To answer your question, yes, we return the entire contents of these arrays.

You mention wrapping the arrays in an atom, can you give more details or provide an example?

Below is an example of the full JSON Product object we read from elastic search. You will see the object contains several simple arrays, which all fail to return as part of the "ProductsById[{keys}][{keys}]" route.

{
"Sku":"ADC BFSM-950", "LongDescription":"Mesh Fry Basket, Steel, 9 1/2\" Dia x 5 3/4\"", "ShortDescription":"FRY BSKT,STEEL,9.5\"", "Status":1, "VendorId":1573, "Height":0.8000, "Length":1.0000, "Width":1.0000, "Weight":1.7000, "Uom":"EA", "UpcRetail":" ", "UpcCarton":" ", "AssemblyRequired":false, "MadeFromRecycled":false, "Returnable":true, "MinOrderQty":1, "Hazmat":false, "BrandName":"Adcraft®", "BrandLogoUrl":"http://cdn.facility.supplies/WebImages/Logos/ADCRAFT_LOGO.JPG", "BrandWebsiteUrl":"", "Name":"Adcraft® Fryer Baskets", "SummarySellingPoint":"Steel mesh fryer basket.", "SellingPoints":[
"Steel mesh fryer basket.", "Nickel plated steel.", "Made to be used when frying with oil." ], "SellingCopy":{
"Long":"This round wire fry basket is ideal for light to heavily-breaded food products. This basket can handle the most rigorous of conditions. It can dive right into the fryer and create something delicious. It is nickel-plated to resist corrosion and is easy to clean.", "Short":"This round wire fry basket is ideal for light to heavily-breaded food products. This basket can handle the most rigorous of conditions. It can dive right into the fryer and create something delicious. It is nickel-plated to resist corrosion and is easy to clean.", "Medium":"This round wire fry basket is ideal for light to heavily-breaded food products. This basket can handle the most rigorous of conditions. It can dive right into the fryer and create something delicious. It is nickel-plated to resist corrosion and is easy to clean.", "Catalog":"" }, "Keywords":[
"Fry Basket", "Steel", "Storage", "Cooking", "Baking", "Breakrooms", "Chefs", "Cookery", "Culinary", "Kitchens", "Packages", "Restaurants" ], "PackageIncludes":[
"Includes one basket." ], "Thumbnails":[

], "SmallImages":[

], "LargeImages":[

], "Videos":[

], "Attributes":[
{
"Name":"Global Product Type", "Value":"Cookware-Fryer Basket" }, {
"Name":"Cookware Type", "Value":"Mesh Fry Basket" }, {
"Name":"Material(s)", "Value":"Steel" }, {
"Name":"Color(s)", "Value":"Silver" }, {
"Name":"Handle Material", "Value":"Steel" }, {
"Name":"Diameter", "Value":"9 1/2\"" }, {
"Name":"Height", "Value":"5 3/4\"" } ] } — Reply to this email directly or view it on GitHub.

ThePrimeagen commented 9 years ago

@jhusain is this resolved?

parvezk commented 8 years ago

Thanks @jhusain $type: "atom" helped

designeng commented 8 years ago

@bcardi, I had the same problem, here's decision: https://gist.github.com/designeng/f00de5e04f27773eaf5c#file-itemsrouter_improved-js-L1-L20 Thanks @jhusain

designeng commented 8 years ago

@jhusain, +1 for a better error message. Otherwise we have MaxRetryExceededError.