Just a question here, not a real issue =)

Does naptime have any sort of system that can process your resources and automatically generate documentation?

StringKey.asOpt throws an exception instead of returning none if StringKeyCodec on input. This breaks the contract that this method should return None upon encountering an error.

import org.coursera.common.stringkey.StringKey
import org.coursera.naptime.courier.CourierFormats
import org.coursera.courier.data.IntArray

implicit val stringKeyFormat = CourierFormats.arrayTemplateStringKeyFormat[IntArray]
val key = StringKey("!~").asOpt[IntArray]

Expected:
None

Actual:

java.io.IOException: `,' expected but ! found line: 1 column: 1
  at org.coursera.naptime.courier.StringKeyCodec$StringListParser.handleParseErrors(StringKeyCodec.scala:382)
  at org.coursera.naptime.courier.StringKeyCodec$StringListParser.parse(StringKeyCodec.scala:353)
  at org.coursera.naptime.courier.StringKeyCodec$Parser.parseList(StringKeyCodec.scala:182)
  at org.coursera.naptime.courier.StringKeyCodec.readList(StringKeyCodec.scala:144)
  at org.coursera.naptime.courier.StringKeyCodec.bytesToList(StringKeyCodec.scala:140)
  at org.coursera.naptime.courier.CourierFormats$$anon$4.reads(CourierFormats.scala:342)
  at org.coursera.common.stringkey.StringKey.asOpt(StringKey.scala:32)
  ... 43 elided

Index.md:

We also define the default fields we want to return for our resource. See [Fields]({{ BASE_PATH }}/projects/naptime/fields.html)

Not sure if this ever existed.

We have a resource like

@Singleton
class EnterpriseVoucherResource @Inject() (
    manager: EnterpriseVoucherManager,
    siteAuths: SiteAuths,
    superuserAuths: SuperuserAuths)
    (implicit ec: ExecutionContext)
  extends CourierCollectionResource[EnterpriseVoucherId, EnterpriseVoucher] {

  override def resourceName = "enterpriseVouchers"
  override def resourceVersion = 1
}

When printing out the documentation for this resource, I'd expect the paths to look like enterpriseVouchers.v1/$id. Instead, they look like enterpriseVouchers/$id.

It looks like it must be hitting this branch:
https://github.com/coursera/naptime/blob/master/naptime/src/main/scala/org/coursera/naptime/router2/NaptimePlayRouter.scala#L164

Original conversation at: https://coursera.slack.com/archives/dragon/p1484855939000114

500's are currently returned as the default Play Internal Server Error page -- unfortunately, this causes downstream response parsing to fail.

Ideally, 500's should return a standard error JSON response with the proper error code.

in regards to #147, based on some of the changes to the resources themselves, it looks like arguments passed to the reverse relation are hardcoded; however, we're definitely going to want to be able to manipulate those reverse relations like any other resource / finder.

under the hood, we could potentially take a reference to the designated finder and infer the arguments from there.

here's an example of what i'm talking about:

query {
  courses {
    memberships(someParameter: true, someOtherParameter: false) {
      id 
    }
  }
}

where memberships is designated as a reverse relation on the courses resource.

/cc @bryan-coursera

Suppose we have a custom class TestPositiveInt that represents positive integers and has a coercer to Int.

TestPositiveInt.scala:

package org.coursera.naptime.courier

import com.linkedin.data.template.Custom
import com.linkedin.data.template.DirectCoercer

case class TestPositiveInt(value: Int) extends AnyVal

object TestPositiveInt {
  object Coercer extends DirectCoercer[TestPositiveInt] {
    override def coerceInput(obj: TestPositiveInt): AnyRef = {
      Int.box(obj.value)
    }

    override def coerceOutput(obj: Any): TestPositiveInt = {
      obj match {
        case value: java.lang.Integer =>
          if (value > 0) {
            TestPositiveInt(value)
          } else {
            throw new IllegalArgumentException(s"$value is not positive")
          }
        case _: Any =>
          throw new IllegalArgumentException()
      }
    }

    def registerCoercer(): Unit = {
      Custom.registerCoercer(new TestPositiveIntCoercer, classOf[TestPositiveInt])
    }
  }

  Coercer.registerCoercer()
}

TestPositiveInt.courier:

namespace org.coursera.naptime.courier

@scala.class = "org.coursera.naptime.courier.TestPositiveInt"
@scala.coercerClass = "org.coursera.naptime.courier.TestPositiveInt.Coercer"

typeref TestPositiveInt = int

TestPositiveIntKeyTest.courier:

namespace org.coursera.naptime.courier

import org.coursera.naptime.courier.TestPositiveInt

record TestPositiveIntKey {
  value: TestPositiveInt
}

Now we try to parse a negative integer as this key type:

import org.coursera.common.stringkey.StringKey
import org.coursera.naptime.courier.CourierFormats
import org.coursera.naptime.courier.TestPositiveIntKey

implicit val stringKeyFormat = CourierFormats.arrayTemplateStringKeyFormat[TestPositiveIntKey]
val key = StringKey("-1").asOpt[TestPositiveIntKey]

Expected:
key is None

Actual:
key is instantiated of type Some[TestPositiveIntKey]. It has a lazy val called value. Upon accessing value (such as by printing key to the console), TestPositiveInt.Coercer.coerceOutput is run and causes an exception to be thrown.

The same error would have occurred if the validation was performed in the constructor of TestPositiveInt instead of the coercer.

As discussed with @saeta and @josh-newman -- gets are basically just a MultiGet of one id, so it doesn't make sense to write both Gets and MultiGets. We should add support to Naptime to convert a get to a multiGet of one element, and deprecate gets going forward. Developers will then be able to only write a multiget, and get support for both gets and multiget.

There are a couple places where we use our own execution context. Instead, we should use a provided execution context.

naptime/src/main/scala/org/coursera/naptime/actions/RestAction.scala
62:  import play.api.libs.concurrent.Execution.Implicits.defaultContext

naptime/src/main/scala/org/coursera/naptime/actions/RestActionBuilder.scala
98:      import play.api.libs.concurrent.Execution.Implicits._

naptime-testing/src/main/scala/org/coursera/naptime/actions/RestActionTester.scala
44:      import play.api.libs.concurrent.Execution.Implicits.defaultContext

For example,

record Example {
  foo: string
  bar: int
}

implicit val f  = CourierFormats.recordTemplateFormats[Example]
Json.parse("""{"foo": 1, "bar": 1}""").validate[Example]

outputs

JsError(List((,List(JsonValidationError(List(Unsupported JSON type: class play.api.libs.json.JsNumber),WrappedArray())))))

This does not contain the path where the problem happened. In complicated models, this makes it very hard to figure out the reason for the error.

I believe it's being wrapped inside a Future.successful

We should instead use a blocking context:
http://docs.scala-lang.org/overviews/core/futures.html#in_a_future

This allows users to customize how blocking calls are handled. For Coursera, this allows the pool to add more parallelism via ManagedBlocker.

in a query like this:

query ($bundleName: String!) {
  RapidashAppsV1Resource {
    bundleName(bundleName: $bundleName, limit: 1) {
      elements {
        deploys: deployIds(limit: 10) {
          elements {
            id
            buildId {
              buildDetails {
                ... on org_coursera_rapidash_ProductionBuildDetailsMember {
          -->  org_coursera_rapidash_ProductionBuildDetails {
                    commitId {
                      summary
                      message
                    }
                  }
                }
              }
            }
          }
        }
      }
    }
  }
}

the nested field org_coursera_rapidash_ProductionBuildDetails is unnecessary since we've already subselected the particular union type via the inline fragment. at this point, we should have resolved the RapidashBuildDetails. ideally this should look like:

buildDetails {
  ... on org_coursera_rapidash_ProductionBuildDetailsMember {
    commitId {
      summary
      message
    }
  }
}

or (stretch goal) renaming the union as well.

buildDetails {
  ... on productionBuildDetails {
    commitId {
      summary
      message
    }
  }
}

/cc @bryan-coursera

A 404 that is triggered within Naptime seems to return content-type text/html and the "Action not found" Play page.

Instead, these should return JSON with the proper error code.

Reproducible by hitting any GET request with a malformed ID

If you use Courier records as query parameters, Naptime uses InlineStringCodec / StringKeyCodec to parse that parameter instead of whatever StringKeyFormat you specify.

We call into CourierQueryParsers here which calls into those codecs here.

If the Courier record query parameter contains any typedef fields that have a custom StringKeyFormat defined, that StringKeyFormat is ignored. Suppose, if I have a Courier model containing a UUID field and a base64 encoded field. The base64 encoded field uses a custom StringKeyFormat. If the record is in the ID position for a GET, the base64 decoding happens correctly. If the record is in the query param position in a finder, the decoding does not happen. Also, the decoding works properly if a Scala case class is used instead of a Courier record.