Question about Ref

[mbukosky]

Hey,

I have been trying to follow the advanced examples for falcor. I am trying to recreate the "all" route but I am running into odd results. I am not sure if it is a bug with the falcor.net project or my client code. Here is what I have.
Any ideas what I am doing wrong?

In NetflixFalcorRouter

Get["titles[{ranges:indices}]"] = async parameters => {
    List<int> titleIds = parameters.indices;

    var ratings = await ratingService.GetRatingsAsync(titleIds.Select(x => Guid.NewGuid()), userId);
    var results = titleIds.Select(idx => {
        var rating = ratings.ElementAtOrDefault(idx);

        return Path("titles", idx)
            .Ref("titlesById", rating.TitleId.ToString());
    });

    return Complete(results);
};

Client index.html

model.get('titles[0..2]["rating", "userRating"]').then(logJson, logJsonError);

Output

{
   "json": {
      "titles": {
         "0": {
            "\u001epath": [
               "titlesById",
               "aca9317a-d813-4e35-8f22-428652c495c8"
            ],
            "rating": 0,
            "userRating": 0
         },
         "1": {
            "\u001epath": [
               "titlesById",
               "f40b6c24-be45-4969-b541-2f4491d1300a"
            ],
            "rating": 4,
            "userRating": 0
         },
         "2": {
            "\u001epath": [
               "titlesById",
               "656f9ed3-2973-4756-9f82-5d286244dfc7"
            ],
            "rating": 4,
            "userRating": 2
         }
      }
   }
}

To make things more confusing. When looking at the network traffic, I am getting errors. By it resolves somehow? Not really sure.

{
    "jsonGraph": {
        "titles": {
            "0": {
                "$type": "ref",
                "value": [
                    "titlesById",
                    "b697c926-cfd8-4564-8025-7252a3ec4599"
                ]
            },
            "1": {
                "$type": "ref",
                "value": [
                    "titlesById",
                    "7b833be2-ef51-46f4-9de6-77c427b8dce1"
                ]
            },
            "2": {
                "$type": "ref",
                "value": [
                    "titlesById",
                    "e32e733b-f21d-4842-a5f2-a7a21818f6a9"
                ]
            }
        },
        "titlesById": {
            "b697c926-cfd8-4564-8025-7252a3ec4599": {
                "Falcor.KeySet": {
                    "$type": "error",
                    "value": {
                        "message": null
                    }
                }
            },
            "7b833be2-ef51-46f4-9de6-77c427b8dce1": {
                "Falcor.KeySet": {
                    "$type": "error",
                    "value": {
                        "message": null
                    }
                }
            },
            "e32e733b-f21d-4842-a5f2-a7a21818f6a9": {
                "Falcor.KeySet": {
                    "$type": "error",
                    "value": {
                        "message": null
                    }
                }
            }
        }
    }
}

[mbukosky]

I think I tracked down the issue to the RoutingEngine.cs. It looks like the Ref path on line 43 is building a path as ["titleById", "e32e733b-f21d-4842-a5f2-a7a21818f6a9", KeySet("rating", "userRating")] I don't think the titleById works with only one id. I ran into that issue a few days ago. if that guid was a KeySet, I think it would work.

[mbukosky]

I have been trying to track down this issue but coming up short. Here is a unit test that I believe proves my point. I think we need to treat any string after the first seed as a KeySet (if not numeric range or set).

What you think?

var path10 = @"[""genrelist"", ""51f2928f34"", ""name""]";
var path10Expected = Path("genrelist", new KeySet("51f2928f34"), "name");

[craigsmitham]

Haven't read through everything here in detail yet, but at first glance it does appear their might be a couple bugs ..... looking into it now

[craigsmitham]

Ok, see above commit. I added a failing case that reproduces the error. The issue is not how paths are parsed, but the behavior of the KeysPattern path matcher. Should be an easy fix - let me know if you want to try to put a fix in, otherwise I can do it soon.

[mbukosky]

Thanks, I should be able to take a stab at it today. I'll replay back with my results.

[mbukosky]

I have been poking around with this for a few hours and I am little confused about the test condition itself. The logic is a little complex here so let me try and explain why I think this is not working. (please correct me if I am wrong)

For this test above, we have 3 parts

1. Router Pattern: I think this is working correctly because I can clearly see that it is picking up the Keys Pattern

2. KeySegment inputs: ShouldMatch("foo", "id1"), I think there is a possible problem here because when this code runs, it is calling KeySegment implicit operator and converting to StringKey. If you change the test case to .ShouldMatch("foo", new KeySet("id1")) it passes.

3. PatternPathMatcher runs: Here is where I think the problem is. The keyTest is only checking if the key is a KeySet. But in this case, it will always be a StringKey because we define it through the implicit operator.

private static PathMatcher PatternPathMatcher<TValue>(Predicate<KeySegment> keyTest,
    Func<KeySegment, TValue> getValue, string name) =>
        key => keyTest(key) ? new MatchingResult(name, getValue(key)) : Unmatched;

Am I on the correct track here? Or am I missing something?

[craigsmitham]

1. I think this is the core issue - yes it is working correctly by picking up KeySets, but should it also match against SimpleKeys? That is the question. (I'll come back to this)

2. The KeySegment inputs are working fine. Strings should implicitly convert to StringKeys. Again, the question is whether or not the KeysPattern should match against a StringKey (or other SimpleKeys ... probably usually a StringKey).

3. Yes - this is much closer to the issue. The PatternPathMatcher code is still correct, the question is whether the KeysPattern factory method (

   falcor.net/src/Falcor.Server/Routing/PathParser/PathMatchers.cs

   Line 51 in 09cc726

   public static PathMatcher KeysPattern(string name = null)

   ) should have keyTest parameter it passes to the PatternMathMatcher also check if the key is a SimpleKey in addition to a KeySet. The AsKeySet() to convert the incoming key into a KeySet already works for simple Keys - see here (

   falcor.net/src/Falcor/SimpleKey.cs

   Line 5 in 09cc726

   public override KeySet AsKeySet() => new KeySet(this);

   ).

I do think this is the right approach, but I've gone back and forth a bit. This is absolutely the right approach if the falcor client code collapses a keyset with a single key to just a key (did you observe it doing that?), but also makes reasonable sense apart from that. In the NetflixFalcorRouter example, you could argue that the Ref that is returned in the initial route should specify the ID inside a KeySet. What do you think?

[mbukosky]

Need to think about this a bit. As for the client, it collapses the keyset into a single key.

model.get('titlesById["fc32c7b1-1213-4296-8e8a-65f846774333"]["rating", "userRating"]').then(logJson, logJsonError)

//*** Server gets
[["titlesById","fc32c7b1-1213-4296-8e8a-65f846774333",["rating","userRating"]]]

[mbukosky]

How about this. I was able to get the tests passing and client code rendering with this change in KeySegment.cs. My thinking is that StringKey == KeySet, so lets just check for everything else besides a StringKey.

//public bool IsKeySet() => KeyType == KeyType.KeySet;
public bool IsKeySet() => !(IsNumericSet() || IsNull() || IsBoolean());

With this change the client also returned

model.get('titlesById["fc32c7b1-1213-4296-8e8a-65f846774333"]["rating", "userRating"]').then(logJson, logJsonError);

//returns

{
   "json": {
      "titlesById": {
         "fc32c7b1-1213-4296-8e8a-65f846774333": {
            "rating": 1,
            "userRating": 2
         }
      }
   }
}

Also, this kinda works

model.get('titles[0..2]["rating", "userRating"]').then(logJson, logJsonError);

//returns
{
   "json": {
      "titles": {
         "0": {
            "\u001epath": [
               "titlesById",
               "036dbaea-929a-4017-a632-d41eae06507b"
            ],
            "rating": 1,
            "userRating": 3
         },
         "1": {
            "\u001epath": [
               "titlesById",
               "cc24b49f-d9fb-4ecf-9d20-d5778cc568ad"
            ],
            "rating": 1,
            "userRating": 3
         },
         "2": {
            "\u001epath": [
               "titlesById",
               "98a2efbc-49cd-4469-ab66-7c34be8424e0"
            ],
            "rating": 0,
            "userRating": 1
         }
      }
   }
}

BUT, the network traffic says that the JSON looks like (below). From the jgraph it looks like we got it working, but I am not sure why the console is dumping out a different output. This is odd. I think we are on the correct path though. What you think?

{
    "jsonGraph": {
        "titles": {
            "0": {
                "$type": "ref",
                "value": [
                    "titlesById",
                    "036dbaea-929a-4017-a632-d41eae06507b"
                ]
            },
            "1": {
                "$type": "ref",
                "value": [
                    "titlesById",
                    "cc24b49f-d9fb-4ecf-9d20-d5778cc568ad"
                ]
            },
            "2": {
                "$type": "ref",
                "value": [
                    "titlesById",
                    "98a2efbc-49cd-4469-ab66-7c34be8424e0"
                ]
            }
        },
        "titlesById": {
            "036dbaea-929a-4017-a632-d41eae06507b": {
                "userRating": {
                    "$type": "atom",
                    "$timestamp": 635826040572507800,
                    "$expires": -3000,
                    "value": 3
                },
                "rating": {
                    "$type": "atom",
                    "$timestamp": 635826040572507800,
                    "value": 1
                }
            },
            "cc24b49f-d9fb-4ecf-9d20-d5778cc568ad": {
                "userRating": {
                    "$type": "atom",
                    "$timestamp": 635826040572507800,
                    "$expires": -3000,
                    "value": 3
                },
                "rating": {
                    "$type": "atom",
                    "$timestamp": 635826040572507800,
                    "value": 1
                }
            },
            "98a2efbc-49cd-4469-ab66-7c34be8424e0": {
                "userRating": {
                    "$type": "atom",
                    "$timestamp": 635826040572517800,
                    "$expires": -3000,
                    "value": 1
                },
                "rating": {
                    "$type": "atom",
                    "$timestamp": 635826040572517800,
                    "value": 0
                }
            }
        }
    }
}

[mbukosky]

Oh snap, I figured out the odd path thing. It had to do with the log js file

if you change

var stringifyReplacer = function (key, value) {
    if (key === '\u001Eparent' || key === '\u001Ekey' || key === '\u001Epath') {
        return undefined;
    } else {
        return value;
    }
};

Then you get the correct results.

{
   "json": {
      "titles": {
         "0": {
            "rating": 0,
            "userRating": 3
         },
         "1": {
            "rating": 4,
            "userRating": 2
         },
         "2": {
            "rating": 4,
            "userRating": 3
         }
      }
   }
}

If you agree with these changes, I can put together a pull request.

[craigsmitham]

We shouldn't modify IsKeySet(), instead we should probably add an IsSimpleKey property that is false on KeySet and true for SimpleKeys. We should then modify the KeysPattern at the bottom of PathMatchers to check for both IsKeySet || IsSimpleKey

[mbukosky]

Yeah that sounds like a good idea. Want me to put a pr together?

[craigsmitham]

Yeah you bet. I know it's a simple change but I really appreciate the thought you're putting into this - addressing these edge cases will really help in the long run. I'm on vacation the next two weeks so my response time may be delayed but I should be able respond within 12 hours at least. Thanks for the continued help. Please don't hesitate to add new issues or ask questions.