kjellmorten / hapi-json-api

Hapi plugin for enabling/enforcing JSON-API specification.
MIT License
44 stars 5 forks source link

Issue with Accept header validation #3

Closed jamesdixon closed 9 years ago

jamesdixon commented 9 years ago

Hi,

First off, thanks for creating the lib!

I'm using Ember on the front-end, which heavily uses jquery. Unfortunately, there seems to be an issue with setting the 'Accept' header for ajax calls. I've tried a number of solutions listed here, but none actually change the accept header, which is causing problems when communication with Hapi while your plugin is installed.

That said, would you be willing to make accept header validation optional via configuration? The way I read the spec seems to indicate adding the application/vnd.api+json media type to the Accept header is optional. Specifically, it says Clients that include the JSON API media type in their Accept header MUST specify the media type there at least once without any media type parameters. The fact that it doesn't say Clients MUST include... indicates to me that it is optional. Thoughts?

Thanks! James

wraithgar commented 9 years ago

Yeah good point, I guess I didn't read that as closely as I could have. Looks like Accept is definitely optional, but if it's there it needs to follow the conventions outlined in the spec. I don't even think configuration is needed, if no Accept header is set.

I'm glad someone else is using the lib, thanks for taking the time to file this issue.

wraithgar commented 9 years ago

This should be fixed in v1.0.4. Give it a spin and let me know.

wraithgar commented 9 years ago

Gonna go at this one more time, after re-reading the spec with another friend.

wraithgar commented 9 years ago

Ok try v1.0.5 which will let your Accept header be application/json.

Big thanks to @paddyforan for explaining media header parsing to me.

jamesdixon commented 9 years ago

thanks @wraithgar! I'll give it a shot.

Somewhat unrelated question for you: given this plugin, I'm assuming you're using JSON API with Hapi. How are you handling serializing your models? We're using bookshelf and ended up writing something custom to build the correct options to pass to the serializer from this project.

I'll report back shortly!

jamesdixon commented 9 years ago

@wraithgar unfortunately, this still doesn't work for me because Jquery sends the following Accept header by default:

application/json, text/javascript

As I mentioned previously, unfortunately, I cannot find a solution to override it.

Reading the spec again, it doesn't seem to say that you can't have other accepted media types, just that if you use the JSON API media type, you must specify it without any params. Unless I'm misreading it, do you think it would be possible to just accept any Accept header, but if it is the JSON API media type, ensure there are no params?

wraithgar commented 9 years ago

Ah, it's sending multiple accepts. Yes that's a valid thing and will take a little more work to parse for 'at least one has to be application/*json'

I'll try to get at that today if I can.

As far as bookshelf, I have a mixin that I haven't published yet, that it's looking like I should soon. Here's the latest gist of it though: https://gist.github.com/wraithgar/1c1932533f92bbe45581

wraithgar commented 9 years ago

Ok phew, Accept parsing is already complicated enough, but adding that one tiny super-specific condition from the json-api spec proved to be too much. I'm just gonna punt on this, remove accept parsing for now (hapi already takes care of the 'client must accept json of some sort') And publish v1.0.6 and hopefully get you on your way

jamesdixon commented 9 years ago

lol. yeah and honestly, im not sure if it provides that much value in the real world. sorry to cause you all this trouble. blame it on damn jquery!

also, your bookshelf code looks similar to what i've done. i'd honestly love to move the code outside of bookshelf and just have adapters that can serialize based on the type of the model being passed in. any thoughts on that? maybe a possibility to collaborate on something?

wraithgar commented 9 years ago

You're talking about decoupling serialization from bookshelf (or any ORM for that matter) into hapi? I thought about that but the ORM has knowledge that hapi can't by the time the data is traditionally serialized, i.e. 'type'.

Or are you talking about an adapter that your ORM would call separately in its serialize function, moving the code out of the custom-written approach in that gist? That could work because at that point it's still an object you could pull a 'type' attribute out of.

Nothing in that gist is anywhere close to set in stone, and I'd love to explore this further.

wraithgar commented 9 years ago

Closing issue for now as the Accept issue is solved

jamesdixon commented 9 years ago

I was thinking something along the lines of a Hapi plugin with adapters for various ORMs. So nothing would be written into any of the models, but instead, the adapter would have knowledge of the type of Model (ex: Bookshelf, Sequelize, etc.) and be able to pull the necessary information from that model and serialize it. Ideally, you'd be able to call something like return reply(model).jsonApi() or something like that. Granted, I haven't fully thought this out, but I like the idea of not having to modify the model.

jamesdixon commented 9 years ago

Also, part of the idea would be to implement a JSON API Serializer that's active and well-maintained. The project I mentioned above seems to be a good choice for the time being.

jamesdixon commented 9 years ago

@wraithgar just an FYI that I ran into another issue with Content-Type validation. I'm not sure if I'm just doing things incorrectly or not, but for example, I have a simple signup form that serializes the form fields to JSON and posts to my Hapi endpoint. When that form posts, its Content-Type is set as application/x-www-form-urlencoded; charset=UTF-8. I also tried converting the POST body to JSON and setting the Content-type to application/json, but because it's specifically looking for application/vnd.api+json, it doesn't work.

This media type validation is a real pain in the ass, ey? I think for now, I may just fork your repo and disable all validation for the time being as I'd still like to take advantage of the Boom rewrite.

Have you run into any of these issues at all?

wraithgar commented 9 years ago

No I'm interacting w/ my API purely through xhr. I don't know how you'd get it to work from an actual html form without some sort of 'submit' event handler in javascript interpreting things for you.

I think the content/type has to be set to the full json-api spec if there's a payload.

jamesdixon commented 9 years ago

I'm doing the same, using Ember's jQuery.ajax(). Not sure what you're working with, but are you setting the content type on each request or using a preFilter?

wraithgar commented 9 years ago

My front end is in ampersand, which means I just set ajaxConfig in my base model, setting it on each request.

jamesdixon commented 9 years ago

Ok, I ended up doing something similar:

  Ember.$.ajaxSetup({
    beforeSend: function(xhr) {
      // prepends the API url to all AJAX requests
      this.url = config.APP.API_URL + this.url;

      // send valid JSON in the request body
      this.data = JSON.stringify(this.data);
    },
    headers: {
      'Content-Type': 'application/vnd.api+json'
    },
    processData: false
  });

Even though some of my requests technically aren't always JSON API spec (for example, a login request), I don't think it's doing any harm.

One other very minor thing is that for a response with an empty payload, it should return null. Hapi does this automatically for type application/json, but I'm guessing you need to set it manually since you're dealing with a different media type.

Ok, thanks a lot man. Appreciate all of your help/hard work!

paddycarver commented 9 years ago

Chiming in just because I'm tagged, but JSON API does not permit null responses: http://jsonapi.org/format/#document-top-level

If the response is empty, it should return an empty JSON API body. If you truly want a null response that has no content, that should be a 204 status code, have a content length of 0, and technically doesn't have a Content-Type (as there's no content).