Add support for reverse relations to the ARI engine #147

bryankane · 2016-11-27T19:25:43Z

While forward relations are very useful in a majority of cases (i.e. courses -> instructors, where the course model has an instructorId on it), there are some cases where a relation is necessary where the first model doesn't have any reference to the second. For instance, while a Membership model has a CourseId available on it, the Course model has no reference to all of the memberships available.

In order to create this course -> membership relationship though, we can dynamically fetch the relationship via a query to the membership resource, which, given information about the course, can return a list of memberships + ids for the relationship. This is referred to as a "reverse relationship", where the relationship data is stored on the related model.

These relationships are defined in the Naptime resource (via the Fields attribute), like so:

override implicit lazy val val Fields = Fields
  .withReverseRelations(
    "memberships" -> ReverseRelation[MembershipId](
      resourceName = ResourceNames.membershipResourceName,
      arguments = Map(
        "q" -> "meByCourse",
        "courseId" -> "$id",
        "showHidden" -> "true")))

The $id string is treated like string interpolation, and will be replaced with the value of the "id" field on the fetched course.

During execution, the engine will make the API call as described on the relationship (appending all interpolated arguments listed), and will make that response available. It will also create a new dynamic field on the original model (in this case, Course), with the ids of the related resources for easier traversal / interop with forward relations.

PTAL @yifan-coursera -- let me know if you want to go over this one in person.

And if you're still interested, cc @saeta

saeta · 2016-11-27T19:27:53Z

Wahoo!

The original engine doesn’t support related resources on unions, nested records, nested lists, or anything that isn’t a top-level record. This change uses the Pegasus dataiterator to iterate through all fields of a datamap and find related resources anywhere on the object.

Refactors things a bit to make testing easier

yifan-coursera

Not quite done but have some preliminary comments / questions

yifan-coursera · 2016-11-30T06:31:37Z

examples/src/main/pegasus/org/coursera/example/Course.courier

@@ -13,4 +13,6 @@ record Course {
  description: string?

  extraData: AnyData
+
+  courseMetadata: union[CourseMetadata]


just for testing that relations on unions work. i can remove this field if you want

yifan-coursera · 2016-11-30T06:34:07Z

naptime-graphql/src/main/scala/org/coursera/naptime/ari/graphql/SangriaGraphQlParser.scala

      case inlineFragment: InlineFragment =>
-        inlineFragment.selections.flatMap(selection => parseSelection(selection, document))
+        inlineFragment.selections.flatMap { selection =>
+          println(inlineFragment.typeCondition)


Remove println.

yifan-coursera · 2016-11-30T06:34:19Z

naptime-graphql/src/main/scala/org/coursera/naptime/ari/graphql/SangriaGraphQlParser.scala

      case fragmentSpread: FragmentSpread =>
        (for {
          fragment <- document.fragments.get(fragmentSpread.name).toList
          selection <- fragment.selections
        } yield {
-          parseSelection(selection, document)
+          println(fragment.typeConditionOpt)


Remove println.

yifan-coursera · 2016-11-30T06:41:08Z

...l/src/main/scala/org/coursera/naptime/ari/graphql/schema/NaptimePaginatedResourceField.scala

@@ -107,3 +154,7 @@ object NaptimePaginatedResourceField {
  }

 }
+
+case class FieldRelation(


I think this should be a union or sealed trait. Is there a case where a field is both a forward relation and a backward relation at the same time?

yifan-coursera · 2016-11-30T06:44:26Z

naptime/src/main/scala/org/coursera/naptime/Types.scala

+            s"'$name', but that field is already defined on the model")
+        case None =>
+          val errorMessageBuilder = new StringBuilder
+          val newField = new RecordDataSchema.Field(new ArrayDataSchema(new StringDataSchema)) // TODO(bryan): fix type here


What happens with this code if the key is a non-string? Do things still work reasonably?

i believe it still works, because ids are serialized to strings in naptime output. I need to investigate this more though and try cases with integers and stuff. this is why i put the key type on the reverse relation annotation (FinderReverseRelation[KeyType](...)), but i don't know if that's necessary or if i can always assume the id will be a string.

i'm going to keep this todo in here and investigate when there's more time, but i don't think it'll break anything now.

yifan-coursera · 2016-11-30T08:02:02Z

naptime/src/main/scala/org/coursera/naptime/ari/engine/EngineHelpers.scala

+    var dataElement: DataElement = null
+    while ( {
+      dataElement = it.next(); dataElement
+    } != null) {


More idiomatic to write this as

Iterator .continually(it.next) .takeWhile(_ != null) .filter(_.path.toSeq.map(_.toString) == path.dropRight(1)) .foreach(_.getValue.asInstanceOf[DataMap].put(path.last, data)

oh woah. i hadn't used scala iterators before... that's really nice though :)

yifan-coursera · 2016-11-30T08:08:03Z

naptime/src/main/scala/org/coursera/naptime/ari/engine/EngineHelpers.scala

+    data.foreach { element =>
+      val it = Builder.create(element, schema, IterationOrder.PRE_ORDER).dataIterator()
+      var dataElement: DataElement = null
+      while ( {


Do an iterator here too, as above.

Bryan Kane added 3 commits November 25, 2016 16:23

Initial approach to reverse relations

4123bb9

Fix a few small things with reverse relations

9745c46

Optimize requests to batch after reverse includes

0dbaed5

jnwng mentioned this pull request Nov 28, 2016

We should support dynamic arguments for reverse relations. #148

Open

Bryan Kane added 6 commits November 27, 2016 18:52

Add types for reverse relation definitions

4126545

Merge connection-arguments branch

f98d359

Pull arguments on related finder for finder-based reverse relations

f1c0569

Fix tests

fde328b

Fix tests, pull helpers out to own class, and add docs

32917ec

Refactors things a bit to make testing easier

yifan-coursera suggested changes Nov 30, 2016

View reviewed changes

Clean-ups from code review

3b1421d

bryankane merged commit 2b7341a into coursera:master Nov 30, 2016

bryankane deleted the reverse-relations branch November 30, 2016 20:31

bryankane mentioned this pull request Nov 30, 2016

Add arguments from multiget handlers on forward relations #149

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for reverse relations to the ARI engine #147

Add support for reverse relations to the ARI engine #147

bryankane commented Nov 27, 2016

saeta commented Nov 27, 2016

yifan-coursera left a comment

yifan-coursera Nov 30, 2016

bryankane Nov 30, 2016

yifan-coursera Nov 30, 2016

yifan-coursera Nov 30, 2016

yifan-coursera Nov 30, 2016

yifan-coursera Nov 30, 2016

bryankane Nov 30, 2016

yifan-coursera Nov 30, 2016

bryankane Nov 30, 2016

yifan-coursera Nov 30, 2016

Add support for reverse relations to the ARI engine #147

Add support for reverse relations to the ARI engine #147

Conversation

bryankane commented Nov 27, 2016

saeta commented Nov 27, 2016

yifan-coursera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment