Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

play.mvc.Http.RequestImpl API is error-prone #6193

Closed
gmethvin opened this issue May 25, 2016 · 0 comments
Closed

play.mvc.Http.RequestImpl API is error-prone #6193

gmethvin opened this issue May 25, 2016 · 0 comments

Comments

@gmethvin
Copy link
Member

See https://github.com/playframework/playframework/blob/2.5.3/framework/src/play/src/main/java/play/mvc/Http.java#L667

There are a few issues I see:

  1. The constructor that takes a RequestHeader sets the underlying body to null. If we are going to set a null body it should probably be made explicit to the user.
  2. It's possible to accidentally call the RequestHeader constructor instead of the Request<RequestBody> one, if I happen to have a request with any other type of body.
  3. I'm not sure where the username feature is used. It doesn't seem like anything in Play uses it. It seems odd to have that as a built-in feature on the request.
richdougherty added a commit to richdougherty/playframework that referenced this issue Aug 30, 2016
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:
* Deprecated Java Request's username property, use attribute instead.
* Refactored Java's Request to implement it in Scala. Fixes playframework#6193.
* Change how request headers are modified ("tagged") during handling.
richdougherty added a commit to richdougherty/playframework that referenced this issue Aug 30, 2016
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:
* Deprecated Java Request's username property, use attribute instead.
* Refactored Java's Request to implement it in Scala. Fixes playframework#6193.
* Change how request headers are modified ("tagged") during handling.
richdougherty added a commit to richdougherty/playframework that referenced this issue Sep 1, 2016
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.
richdougherty added a commit to richdougherty/playframework that referenced this issue Sep 1, 2016
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.
wsargent pushed a commit to wsargent/playframework that referenced this issue Oct 25, 2016
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant