Add custom typed properties to requests #6321

richdougherty · 2016-07-13T23:44:21Z

RequestHeader and Request can now have custom properties added to
them. This lets plugins and users add their own data to requests.
Adding custom properties was sort of possible by using tags, but tag
values could only be strings. The new API adds support for typed
properties.

Typed properties are accessed using methods similar to Maps. For
example, the + method adds a new property and apply or get can
be used to retrieve a property.

I have intentionally restricted the API to hide internal details of
how properties are implemented. This is because I want to make it
possible to change the underlying implementation later if we want to
support more complicated features. For example, I've avoided exposing
the underlying map that's used to store the properties. This is
because we might want to support lazy or dynamic properties later, and
these might not be implemented by storing them in a simple map.

Fixes #5814.

TODO:

Scaladoc
Java API
More tests
Use typed tags for router

richdougherty · 2016-07-14T21:24:23Z

I need to fix up the document tests and maybe make minor changes to FakeRequest but this PR is ready for review now.

jroper · 2016-07-20T01:26:35Z

framework/src/play/src/main/scala/play/api/mvc/RequestHeader.scala

+    withProperties(properties.+(entry))
+  override def +(entry: TypedEntry[_], entries: TypedEntry[_]*): Repr =
+    withProperties(properties.+(entry, entries: _*))


Are properties that fundamental to the concept of a request header that they should be given the + method on RequestHeader? Why shouldn't the + method be used to add request headers to the request header? If I saw:

val newRequestHeader = requestHeader + (Foo -> fooValue)

I think I would assume that this was adding a foo header to the request, which I think would be a fairly intuitive reading of that code, but it's wrong. So I don't think this is a good use of an operator, I think the method should have a name that makes it clear that a property is being added.

The same goes for get, apply, contains, if these methods exist at all, they should be for headers, not properties.

jroper · 2016-07-20T01:55:31Z

What's the plan for typed request headers? Maintain properties in addition to headers? Both would probably be implemented the same way, I wonder if the distinction is worthwhile.

Another thing, is "properties" the best name? Properties has a meaning on the JVM, and generally refer to Strings. In contrast, "attributes" is not yet used in Play, or as far as I'm aware in a major way in any libraries that Play uses, and is familiar to anyone coming from the servlet API, since attributes is the analogous name for this feature there.

jroper · 2016-07-20T01:58:14Z

RequestHeader.tags should be deprecated, and the existing tags that are added to requests should also be added to the properties (or maybe added to properties instead, depending on how much backwards compatibility we want to maintain).

nafg · 2016-07-20T02:01:45Z

What's the use case for this?

On Tue, Jul 19, 2016 at 9:58 PM James Roper notifications@github.com
wrote:

RequestHeader.tags should be deprecated, and the existing tags that are
added to requests should also be added to the properties (or maybe added to
properties instead, depending on how much backwards compatibility we want
to maintain).

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#6321 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAGAUL6tunm5EXdTG5DIDSKgd4Fdf2oSks5qXYC4gaJpZM4JL9Lt
.

gmethvin · 2016-07-20T08:13:33Z

@jroper I agree on the naming.

@nafg The point is to be able to store rich data on the request. The use case is essentially the same as tags but broader: instead of strings it's now possible to store any type, and access the values in a type-safe way. We currently could use this within Play for the CSRF filter, where we store the CSRF token and some other data on the request so it can be later used by CSRF.getToken and the template helpers. It can also be used as a general mechanism to allow filters to cache data on the request to be used later by actions.

richdougherty · 2016-07-20T21:55:21Z

@jroper, thanks for the feedback. You may wish to look at my other (now closed) PR #6227 which went a lot further. That PR moved all the request vals into the map and supporting typed headers as properties. It also used normal method names instead of operators.

I think that other PR was too complex because it had a very flexible system for adding properties to requests, including supporting interdependencies between properties, lazy properties, etc. For this PR I decided to make things a bit simpler. There's no reason we can't bring back some of those more complicated features later though; the surface API for this PR is intentionally very similar.

I'll think about the other stuff, but for now I'll do some of the simpler things:

rename "properties" to "attributes"
use different method names, e.g. attr, withAttr, containsAttr.
convert a few of the existing request vals into attributes
deprecate tags, but use tags and attributes in parallel to maintain backwards compatibility

richdougherty · 2016-08-16T02:09:08Z

framework/src/play/src/main/scala/play/api/http/HttpRequestHandler.scala

  }

  /**
   * Apply any filters to the given handler.
   */
+  @deprecated("Use filterHandler(RequestHeader, Handler) instead", "2.6.0")
  protected def filterHandler(next: RequestHeader => Handler): (RequestHeader => Handler) = {


@jroper I deprecated this method because I couldn't see a good reason for requiring a function as an argument. Do you know the reason?

Ping @jroper?

richdougherty · 2016-08-26T02:15:52Z

...ork/src/play-akka-http-server/src/main/scala/play/core/server/akkahttp/ModelConversion.scala

    // Avoid clash between method arg and RequestHeader field
    val remoteAddressArg = remoteAddress

-    new RequestHeader {
+    new RequestHeader with WithAttrMap[RequestHeader] {


The WithAttrMap mixin is a private Play trait that provides a simple implementation of request attributes. Implementors just need to provide access to a TypedMap to store the attributes in.

richdougherty · 2016-08-26T02:52:02Z

framework/src/play/src/main/scala/play/api/mvc/RequestHeader.scala

-      override val clientCertificateChain = _clientCertificateChain
-      override val hasBody = _hasBody || super.hasBody
-    }
+    new RequestHeaderImpl(


Nothing special here, but notice we can reuse the RequestHeaderImpl class.

richdougherty · 2016-08-26T03:14:39Z

This is ready for review now. When reviewed I'll make small fixes, squash commits, etc. Then hopefully we can merge it!

nafg · 2016-08-26T03:25:40Z

framework/src/play/src/main/scala/play/api/libs/typedmap/TypedMap.scala

+/**
+ * A TypedKey is a key that can be used to get and set values in a
+ * [[TypedMap]] or any object with typed keys. Implementations of this
+ * object should ensure they have stable and well-behaving equals


Perhaps it should also mention that equals has to ensure there can't be a collision between different types, meaning for types A and B such that B is not <: A, a subclass of TypedKey[X] T[X] must obey that a T[A] must never equal a T[B]. The caveats mentioned apply to any map key, but we need this guarantee as well

I think if their types are different then keys should never be equal. But if their types are different then they're really different keys, and different keys should never be equal. I'm not sure if this is obvious or not, I'll add a note if I can think of a good way of expressing it.

What I mean is the current wording doesn't rule out, I don't know, say case class MyCustomTypedKey[A](msg: String, labels: String*) extends TypedKey[A] -- equals and hashCode will be defined "correctly" since it's a case class, but it won't behave correctly in TypedMap.

Do we really need to allow subclasses? The safest thing would be to make it sealed. With reference equality the only way to break the map is by casting deliberately.

Good point. Maybe subclassing is not needed. We could just make TypedKey a final class with reference equality semantics. That would prevent any misuse.

richdougherty · 2016-08-30T01:44:25Z

Fixes made, commits squashed, rebased to latest master to fix issue with NettyWebSocketSpec failures.

Now ready for merging when tests pass.

richdougherty · 2016-09-01T01:31:36Z

Hopefully tests pass now. The request rewriting changes that I'd made interfered with how JavaCompatibleHttpRequestHandler and JavaHandler work together to return a JavaAction. I've now fixed that up by changing JavaCompatibleHttpRequestHandler to return a Handler.Stage. The returned Handler.Stage deals with getting the JavaAction out of the JavaHandler…

(Handler.Stage is the new name for the new trait ModifyRequestHandler. I renamed the trait since it doesn't just allow request rewriting, it also allows handler rewriting. This makes Handler.Stage a fairly flexible system for adding custom logic into the handler processing pipeline.)

RequestHeader and Request can now have custom attributes added to them. This lets plugins and users add their own data to requests. Adding custom attributes was sort of possible by using tags, but tag values could only be strings. The new API adds support for typed attributes. I have intentionally restricted the API to hide internal details of how attributes are implemented. This is because I want to make it possible to change the underlying implementation later if we want to support more complicated features. In particular, I've avoided exposing the underlying map that's used to store the attributes. This is because we might want to support lazy or dynamic attributes later, and these types of attributes might not use a map. Fixes playframework#5814. Other misc changes: * Changed Java Request's username property to be an use attribute. * Refactored Java's Request to implement it in Scala. Fixes playframework#6193. * Use `Handler.Stage` for request and handler rewriting.

gmethvin · 2016-09-01T20:55:48Z

LGTM

RequestHeader and Request can now have custom attributes added to them. This lets plugins and users add their own data to requests. Adding custom attributes was sort of possible by using tags, but tag values could only be strings. The new API adds support for typed attributes. I have intentionally restricted the API to hide internal details of how attributes are implemented. This is because I want to make it possible to change the underlying implementation later if we want to support more complicated features. In particular, I've avoided exposing the underlying map that's used to store the attributes. This is because we might want to support lazy or dynamic attributes later, and these types of attributes might not use a map. Fixes playframework#5814. Other misc changes: * Changed Java Request's username property to be an use attribute. * Refactored Java's Request to implement it in Scala. Fixes playframework#6193. * Use `Handler.Stage` for request and handler rewriting.

jroper reviewed Jul 20, 2016
View reviewed changes

richdougherty force-pushed the custom-request-props branch 2 times, most recently from 279901d to 2aa1eda Compare August 16, 2016 02:02

richdougherty reviewed Aug 16, 2016
View reviewed changes

schmitch mentioned this pull request Aug 17, 2016

Refactor I18n API #6438

Merged

richdougherty force-pushed the custom-request-props branch from a37ff8f to c2ea972 Compare August 26, 2016 00:25

richdougherty reviewed Aug 26, 2016
View reviewed changes

nafg reviewed Aug 26, 2016
View reviewed changes

richdougherty force-pushed the custom-request-props branch 2 times, most recently from 0ece6a7 to 957bd97 Compare August 30, 2016 01:43

richdougherty force-pushed the custom-request-props branch from 957bd97 to 6b6d768 Compare September 1, 2016 01:20

richdougherty force-pushed the custom-request-props branch from 6b6d768 to 319dee6 Compare September 1, 2016 19:00

gmethvin merged commit 31a16bc into playframework:master Sep 1, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add custom typed properties to requests #6321

Add custom typed properties to requests #6321

richdougherty commented Jul 13, 2016 •

edited

richdougherty commented Jul 14, 2016

jroper Jul 20, 2016

jroper Jul 20, 2016

jroper commented Jul 20, 2016

jroper commented Jul 20, 2016

nafg commented Jul 20, 2016

gmethvin commented Jul 20, 2016

richdougherty commented Jul 20, 2016

richdougherty Aug 16, 2016

richdougherty Aug 26, 2016

richdougherty Aug 26, 2016

richdougherty Aug 26, 2016

richdougherty commented Aug 26, 2016

nafg Aug 26, 2016 •

edited

richdougherty Aug 26, 2016

nafg Aug 26, 2016

nafg Aug 26, 2016

richdougherty Aug 26, 2016

richdougherty commented Aug 30, 2016

richdougherty commented Sep 1, 2016

gmethvin commented Sep 1, 2016

Add custom typed properties to requests #6321

Add custom typed properties to requests #6321

Conversation

richdougherty commented Jul 13, 2016 • edited

richdougherty commented Jul 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jroper commented Jul 20, 2016

jroper commented Jul 20, 2016

nafg commented Jul 20, 2016

gmethvin commented Jul 20, 2016

richdougherty commented Jul 20, 2016

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

richdougherty commented Aug 26, 2016

nafg Aug 26, 2016 • edited

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

richdougherty commented Aug 30, 2016

richdougherty commented Sep 1, 2016

gmethvin commented Sep 1, 2016

richdougherty commented Jul 13, 2016 •

edited

nafg Aug 26, 2016 •

edited