Have consistent names for adding, removing and clearing data in multiple APIs

[marcospereira]

Purpose

We don't have good/consistent names for APIs that manipulates headers, session, flash, and cookies. For example, we have discardCookies (it is discarding in other places), removingFromSession and clearlingLang. And in some other places just remove or -.

How to make the change

To enable a smooth migration, we need to deprecate the existing methods and add the new methods. Just renaming or removing will break binary compatibility which won't give users a change to migrate at their own pace.

Methods to rename

Here is a (not extensive) compilation:

play.api.mvc.Session {
  def get(key: String): Option[String]
  def +(kv: (String, String)): Session
  def -(key: String): Session
  def apply(key: String): String = data(key)
}

play.api.mvc.Flash {
  def get(key: String): Option[String]
  def +(kv: (String, String)): Flash
  def -(key: String): Flash
  def apply(key: String): String
}

play.api.mvc.Result {
    def withHeaders(headers: (String, String)*)
    def withDateHeaders(headers: (String, ZonedDateTime)*)
    def discardingHeader(name: String)
    def withCookies(cookies: Cookie*)
    def discardingCookies(cookies: DiscardingCookie*)
    def withSession(session: Session)
    def withSession(session: (String, String)*)
    def withNewSession
    def flashing(flash: Flash)
    def flashing(values: (String, String)*)
    def as(contentType: String)
    def session(implicit request: RequestHeader): Session = newSession getOrElse request.session
    def addingToSession(values: (String, String)*)(implicit request: RequestHeader)
    def removingFromSession(keys: String*)(implicit request: RequestHeader)
}

play.mvc.Result {
    public Optional<String> header(String header)
    public Map<String, String> headers()
    public Result withFlash(Flash flash)
    public Result withFlash(Map<String, String> flash)
    public Result withNewFlash()
    public Result flashing(Map<String, String> values)
    public Result flashing(String key, String value)
    public Result removingFromFlash(String... keys)
    public Session session()
    public Session session(Http.Request request)
    public Result withSession(Session session)
    public Result withSession(Map<String, String> session)
    public Result withNewSession()
    public Result addingToSession(Http.Request request, Map<String, String> values)
    public Result addingToSession(Http.Request request, String key, String value)
    public Result removingFromSession(Http.Request request, String... keys)
    public Cookie cookie(String name)
    public Optional<Cookie> getCookie(String name)
    public Cookies cookies()
    public Result withCookies(Cookie... newCookies) {
    public Result discardCookie(String name)
    public Result discardCookie(String name, String path)
    public Result discardCookie(String name, String path, String domain)
    public Result discardCookie(String name, String path, String domain, boolean secure)
    public Result withHeader(String name, String value)
    public Result withHeaders(String... nameValues)
    public Result discardHeader(String name)
    public Result withLang(Lang lang, MessagesApi messagesApi)
    public Result clearingLang(MessagesApi messagesApi)
}

play.api.mvc.Request {
    def queryString: Map[String, Seq[String]] = target.queryMap
    def headers: Headers
    def withHeaders(newHeaders: Headers): RequestHeader =
    def attrs: TypedMap
    def withAttrs(newAttrs: TypedMap): RequestHeader =
    def addAttr[A](key: TypedKey[A], value: A): RequestHeader =
    def removeAttr(key: TypedKey[_]): RequestHeader =
    def getQueryString(key: String): Option[String] = target.getQueryParameter(key)
    def cookies: Cookies = attrs(RequestAttrKey.Cookies).value
    def session: Session = attrs(RequestAttrKey.Session).value
    def flash: Flash = attrs(RequestAttrKey.Flash).value
    def rawQueryString: String = target.queryString
    def withTransientLang(lang: Lang): RequestHeader =
    def withTransientLang(code: String): RequestHeader =
    def withTransientLang(locale: Locale): RequestHeader =
    def clearTransientLang(): RequestHeader =
    def transientLang(): Option[Lang] =
}

play.mvc.Request {
    TypedMap attrs();
    RequestHeader withAttrs(TypedMap newAttrs);
    <A> RequestHeader addAttr(TypedKey<A> key, A value);
    RequestHeader removeAttr(TypedKey<?> key);
    Map<String,String[]> queryString();
    String getQueryString(String key);
    Cookies cookies();
    Cookie cookie(String name);
    default Session session()
    default Flash flash()
    Headers getHeaders();
    default Optional<String> header(String headerName)
    default boolean hasHeader(String headerName) {
    default RequestHeader withTransientLang(Lang lang) {
    default RequestHeader withTransientLang(String code) {
    default RequestHeader withTransientLang(Locale locale) {
    default RequestHeader clearTransientLang() {
    default Optional<Lang> transientLang() {
}

play.api.mvc.Headers {
  def hasHeader(headerName: String): Boolean = get(headerName).isDefined
  def add(headers: (String, String)*): Headers = new Headers(this.headers ++ headers)
  def apply(key: String): String = get(key).getOrElse(scala.sys.error("Header doesn't exist"))
  def get(key: String): Option[String] = getAll(key).headOption
  def getAll(key: String): Seq[String] = toMap.getOrElse(key, Nil)
  def keys: Set[String] = toMap.keySet
  def remove(keys: String*): Headers = {
  def replace(headers: (String, String)*): Headers = remove(headers.map(_._1): _*).add(headers: _*)
}

play.mvc.Http.Headers {
    public Map<String, List<String>> toMap()
    public boolean contains(String headerName)
    public Optional<String> get(String name)
    public List<String> getAll(String name)
    public Headers addHeader(String name, String value)
    public Headers addHeader(String name, List<String> values)
    public Headers remove(String name)
}

Originally posted by @marcospereira in https://github.com/playframework/playframework/pull/8768#issuecomment-436084836

[yuuri111]

@marcospereira Hi, I agree with you and fix method names on the following pull request.(At first rename flashing to flash) https://github.com/playframework/playframework/pull/8803

Although The Travis CI build failed, what do I have to fix next? Would you give advice?

[marcospereira]

Hi @yuuri111, thanks for your contribution!

This comment fron @ignasi35 explains why the build is broken and how to fix it:

https://github.com/playframework/playframework/pull/8803#discussion_r233984845

I've also edited the issue description to make it clear how we handle such situations.

[yuuri111]

@marcospereira Thank you for your reply. I kept the old code and created new function whose name is flash. https://github.com/playframework/playframework/pull/8803#discussion_r234088903

[gmethvin]

@marcospereira

I don't have that strong an opinion on removing the -ing suffix, but it's important we change all APIs at once or else it'll cause more confusion. So we should back out #8803 if more changes are not going to happen for that before 2.7.0. Or else we are actually taking a step backwards in consistency, and are just going to annoy people who are migrating their code to the new version.

In general I think the -ing suffix makes sense for immutable APIs, especially when there is a mix of immutable and mutable APIs. The -ing suffix just sounds more like you're getting a new value returned, versus mutating an existing value. For example "result remove foo" sounds like the "remove" is taking some mutative action, whereas "result removing foo" sounds like you're getting a new result, removing some value. Note that Response still exists, as do some other mutable Java APIs that don't use the -ing suffixes, so in my opinion having the suffix adds clarity that it's an immutable API.

I think if we keep flashing and change discardCookie and discardHeader to discardingCookie and discardingHeader, that would still make sense to me, and would be a good API change to make now since it's new API and doesn't require deprecation. We should make this decision before the Play 2.7 release when we have to lock in the changes.

I'm happy to make a PR with whatever changes we decide.

[marcospereira]

I don't have that strong an opinion on removing the -ing suffix, but it's important we change all APIs at once or else it'll cause more confusion. So we should back out #8803 if more changes are not going to happen for that before 2.7.0. Or else we are actually taking a step backwards in consistency, and are just going to annoy people who are migrating their code to the new version.

Agree with that. My opinion is that removing -ing is consistent with Java & Scala APIs, so I would follow that "convention".

In general I think the -ing suffix makes sense for immutable APIs, especially when there is a mix of immutable and mutable APIs. The -ing suffix just sounds more like you're getting a new value returned, versus mutating an existing value. For example "result remove foo" sounds like the "remove" is taking some mutative action, whereas "result removing foo" sounds like you're getting a new result, removing some value.

That makes sense to me, but I think developers are more used to have APIs without the -ing. For example, an immutable.List in Scala has map, filter, drop, take etc. and this idiom looks more consistent for Play as well.

I'm happy to make a PR with whatever changes we decide.

Appreciate that. What do you think about my considerations above?

Best.

[gmethvin]

Agree with that. My opinion is that removing -ing is consistent with Java & Scala APIs, so I would follow that "convention".

If you're designing an API in Scala from scratch, I think that makes sense. If all your APIs are all immutable, then users will assume any method that looks like a mutator is actually returning a new value.

Do you think the current method names are unclear, though? Considering that we have to make breaking changes, what benefit does the developer using Play get from the new API? I think the current naming is totally clear, though perhaps not especially common.

That makes sense to me, but I think developers are more used to have APIs without the -ing. For example, an immutable.List in Scala has map, filter, drop, take etc. and this idiom looks more consistent for Play as well.

Methods with those names are also conventionally understood to return new values. They are common in functional programming languages and libraries and don't appear often in mutable APIs.

When it comes to the new methods added so far in 2.7, their names are relatively uncommon in general. discard is not commonly used in method names, for example. Especially in the Java API, I think having the extra hint that it's returning a new object is useful as long as we still have some mutable APIs. For those reasons I'd vote to keep the newly-added methods consistent with the old API, and wait for Play 3 to make more sweeping API changes.

[marcospereira]

Do you think the current method names are unclear, though? Considering that we have to make breaking changes, what benefit does the developer using Play get from the new API? I think the current naming is totally clear, though perhaps not especially common.

I don't think it is unclear, although I believe that if we need to pick a standard to have consistency, then using an established one is better. But to be honest, I don't have a strong opinion here about removing -ing. Have consistency is the important factor of this issue.

For those reasons I'd vote to keep the newly-added methods consistent with the old API, and wait for Play 3 to make more sweeping API changes.

You mean just revert #8803 or are you suggesting some additional changes to the new APIs?

[gmethvin]

I mean #8803 plus updating the existing APIs for consistency. I didn't really explain in detail so let's look at some of the specific APIs here. I added some additional thoughts in my explanations below.

First I'd change some new APIs:

• discardCookie -> discardingCookie in the new Java API. The naming is not my favorite but it's consistent with the Scala API, and we are pretty committed to that naming since we have a DiscardingCookie case class for cookies to discard.
• discardHeader/discardingHeader -> withoutHeader (both Java and Scala) - This actually could be changed to varargs withoutHeaders :thinking:. It's my understanding that we use discarding for cookies because the result actually needs to explicitly set a Set-Cookie header to make the browser discard the cookie. I'd rather use with/without if we're just setting a property of the response.

I also think we can change Result#clearingLang to withDefaultLang for clarity. I'm thinking something like this for both Java and Scala APIs:

• clearingLang (deprecate Scala API) -> withDefaultLang
• withTransientLang stays the same
• clearTransientLang -> withoutTransientLang

So that'd involve some deprecation, but arguably for good reason this time since we are adding new APIs for the transient lang and it'll make the API more clear.

[marcospereira]

@gmethvin sounds like a good plan. Is that something you plan to work for 2.7?

[gmethvin]

I should be able to get a PR out this week.

I also noticed some inconsistent APIs added in play.api.mvc.Session and play.api.mvc.Flash. I think we should have ++ and -- take Seqs like Scala collections rather than using varargs, and I don't see the need for the extra named methods.

[gmethvin]

Actually it just occurred to me that withLang is on Result so I don't think it's that easily confused with withTransientLang on RequestHeader. So we can keep it the same. I think we should change clearingLang to withDefaultLang since that's the effect it actually has.

[brownbonnie]

I'd like to work on this - what changes are still left to do?

[marcospereira]

Hey, @brownbonnie, Thanks for your interested here!

I'm not sure about all the changes. Better to inspect the code and see where we need to change the APIs.