Overriding default JSON GET/POST behavior #1522
Conversation
|
I've started looking into this, but don't have any working code yet. More to come soon, hopefully :) |
Until now, it returned Empty. This helps clarify what’s going on to consumers. Additionally, added a test to verify this, and some clarifying documentation to the method.
This reflects the motivation for using it a little better.
bodyAsJson ignores content type and forcibly tries to parse a request body as JSON. Failure is only reported when there is an issue doing the actual JSON parse.
This lays the groundwork for an easy mockXmlReq to test XML body types.
bodyAsXml ignores content type and forcibly tries to parse a request body as XML. Failure is only reported when there is an issue doing the actual XML parse.
|
Ok, I've attached some code here that implements |
|
Any feedback here? |
|
I don't really have a way to test this out right now... embarrassingly, I decided to switch back to pure servlets for my purposes because Lift was just too much hassle, undocument complexity and learning curve for my relatively small project, which wasn't alleviated by the fact that a lot of things in Lift happen with no real static typing, so the type system can't lead/guide you in most situations. Apologies for having stirred up dust and then sort of leaving the scene... |
|
That's fine. I think it's a valuable addition anyway, so if any committers have thoughts that would be appreciated, and then we can look at getting it in. |
|
I'm probably going to merge this in over the next week or so if no one objects. |
|
👀 |
| * Forcibly tries to parse the request body as JSON. Does not perform any | ||
| * content type checks, unlike the json method. | ||
| */ | ||
| lazy val bodyAsJson: Box[JsonAST.JValue] = { |
There was a problem hiding this comment.
Perhaps this is more aptly named bodyAsJosn_!?
There was a problem hiding this comment.
Hmmm… So this won't throw an exception, and there's generally nothing dangerous about it that I can see. What about it makes you want a _!?
There was a problem hiding this comment.
Ah, that's true, this is returning a Box. I wanted something to indicate that this is bypassing a guardrail you might otherwise want turned on. Unless I'm inclined to go hunting for the scaladoc, which not everyone is, the difference between json and bodyAsJson seems subtle enough for someone to very easily misunderstand what the difference is.
Maybe I'm just over-thinking it.
There was a problem hiding this comment.
I see @farmdawgnation 's point and I agree, we should make it a bit more obvious that most people should use json and not this new method, maybe forceBodyAsJson?
There was a problem hiding this comment.
Heh. Whoops! What I meant was… Hmmm…
I don't think we should discourage this method that strongly. I think it's more of making the “most standard” method easier than it is discouraging the less standard method. json follows the standard to a T, refusing to even attempt to parse a request that isn't explicitly marked as JSON as such. But not doing this isn't a security hole per se, in that we are about as certain as we are with any other part of the system that our JSON parser is secure.
That said, I do think forcedBodyAsJson is more descriptive of what's happening than just bodyAsJson. So I'm a little torn.
There was a problem hiding this comment.
At first I was going to suggest unsafeBodyAsJson but I think that really looks like discouraging it very strongly :)
There was a problem hiding this comment.
bodyConvertedToJsonRegardlessOfContentType
lol
There was a problem hiding this comment.
Let's do forcedBodyAsJson. I think that communicates enough of the difference to make that mental switch flip in somebody's head.
|
There don't appear to be specs demonstrating what the forcible methods do if the content can't actually be converted to that type. Do they return Empty or Failure? Can we add a spec for that case? |
|
Yeah, I can add specs. It'll be a |
Conflicts: web/webkit/src/test/scala/net/liftweb/http/ReqSpec.scala
We do this to more clearly indicate that you are bypassing built-in safeguards against incorrect Content-Types when you use these methods.
These specs clarify that we expect Failures when the content types are either correct or ignored but the contents of the request bodies are not valid XML or JSON bodies, depending on which method is being called.
|
Ok, made those two changes:
|
|
awesome! 👍 |
|
Going to verify locally, then merge. |
|
Let's ship it. |
Expose forcedBodyAsJson and forcedBodyAsXml to allow you to convert body content to those respective formats and bypass the Content-Type check.
Creating a ticket as per https://groups.google.com/d/msg/liftweb/E-XlChwJd10/-8Uix3KJDXEJ.
To summarize, the idea was to allow the customization of how JSON requests are handled; currently, the framework is hardcoded to require the Content-Type header of any JSON requests to always be
application/json. However, this is not how many JSON based web services work, and shouldn't be a hardcoded requirement in the case of e.g. web services that are designed to only use JSON anyway (i.e. the content-type selection is redundant).However YMMV, and perhaps for the sake of keeping the framework simple—although at least the HTTP REST code is not overly simple and easy to follow anyway—it might be worth to keep things how they are.