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

Support jsonapi.org #445

Closed
conrad-vanl opened this issue Aug 6, 2014 · 90 comments
Closed

Support jsonapi.org #445

conrad-vanl opened this issue Aug 6, 2014 · 90 comments
Assignees

Comments

@conrad-vanl
Copy link

Is there any way, or plans, to configure how JSON responses (and therefore requests) are serialized? For instance, it would be very nice to support a standard, such as http://jsonapi.org/ so that clients can take advantage of generalized tooling. Looking at the current API, it would appear to me the biggest thing would be for models to be serialized in root objects...

This:

{
   "user": {
      "id": "...",
      "name": "Conrad"
   }
}

As opposed to:

{
   "id": "...",
   "name": "Conrad"
}

There would also be other considerations in how associations and relations are serialized.

@meng-zhang
Copy link

+1

@imjoshholloway
Copy link

+1 for this

@ritch
Copy link
Member

ritch commented Aug 8, 2014

You can modify sharedMethods to have data returned like you showed above. In 2.x this is possible by modifying sharedMethod.returns.

var sharedMethod = MyModel.sharedClass.find('findById', true);
sharedMethod.returns[0].root = false;

Although I don't think this is a good long term solution. I think we should add the ability to define a global response format setting. What would be helpful is to know what other formats are useful.

@ritch
Copy link
Member

ritch commented Aug 8, 2014

As far as supporting jsonapi, I'd like to see what else we need to change and if we can maintain computability with 2.x. Maybe a jsonapi flag that would modify all the right settings to support jsonapi.org conventions?

@ritch ritch added the idea label Aug 8, 2014
@eabruzzese
Copy link

+1

@bemosior
Copy link

+1 as well.

@bajtos bajtos removed the idea label Sep 4, 2014
@alagunambi
Copy link

+1, integration with ember apps would be cake walk if implemented.

@kaizhu256
Copy link

+1

@bajtos bajtos added the feature label Nov 3, 2014
@bradplank
Copy link

+1, and happy Ember developers we would be!

@bajtos
Copy link
Member

bajtos commented Nov 14, 2014

Does Ember include application/vnd.api+json in the accepts headers of the request?

strong-remoting can already serve different response depending on what the client accepts, it should be reasonably easy to add a new case for application/vnd.api+json - see strong-remoting/lib/http-context.js

The difficult part is how to determine the name of the root key (e.g. user for User.findById, users for User.find, but also category for Product.__get__category).

@bradplank
Copy link

By default, Ember uses the following in the request header:
Accept:application/json, text/javascript, */*; q=0.01

However, the RESTAdapter can easily be extended to add application/vnd.api+json as an Accept header.

For the root key, wouldn't the model's name / plural name be used? I see the difficulty with the hasManyThrough relationship, as the API explorer shows the incorrect model. (I've been meaning to see if I could fix that issue.)

@bajtos
Copy link
Member

bajtos commented Nov 18, 2014

For the root key, wouldn't the model's name / plural name be used? I see the difficulty with the hasManyThrough relationship, as the API explorer shows the incorrect model. (I've been meaning to see if I could fix that issue.)

I would make the initial implementation easier if we could use a constant key name. The jsonapi spec explicitly supports that:

The primary resource(s) SHOULD be keyed either by their resource type or the generic key "data".

Does Ember support this too?

@bradplank
Copy link

Unfortunately, it seems Ember does not treat the generic key 'data' as a special key. I reviewed the code, and tested just to make sure. The following error message is generated as there is no 'data' model defined:

Uncaught Error: Assertion Failed: Error: No model was found for 'datum'

[NOTE: I am currently using Ember version 1.7.0, however, I do not see any changes in 1.8.1 that would use the generic key 'data'.]

@sergiolepore
Copy link

@bradplank hmm... Seems like the Ember Inflector is trying to pluralize the word "data". Try this:

Ember.Inflector.inflector.uncountable('data');

@seriousben
Copy link
Contributor

👍

@seriousben
Copy link
Contributor

jsonapi will soon hit 1.0 and only backward compatible changes are planned after that.
If Loopback started supporting it, it would make it very interesting to companies.

@BenjaminHorn
Copy link

+1

@Panman82
Copy link

Now that json api is 1.0, is there any further news on loopback support? And to note, there is strong movement ATM to make Ember Data natively handle json api.

@sahin
Copy link

sahin commented Jul 22, 2015

+1

@sahin
Copy link

sahin commented Jul 22, 2015

IS there any update on this?

@Keeo
Copy link

Keeo commented Aug 10, 2015

👍

@raiskila
Copy link

Very interested in this!

@adamdilek
Copy link

+1

@digitalsadhu
Copy link
Contributor

+1 👍

@digitalsadhu
Copy link
Contributor

I've begun writing a bootscript to add jsonapi support. Couple questions: @bajtos @ritch

  • Is it possible to modify the list of content type options in the loopback explorer? For testing purposes it would be nice to be able to select application/vnd.api+json
  • JSON API specifies the use of PATCH instead of PUT. Is it possible to modify loopback explorer to send PATCHs in place of PUTs?

@bajtos
Copy link
Member

bajtos commented Sep 21, 2015

Is it possible to modify the list of content type options in the loopback explorer?

ATM, it's not possible to customise the list of content types at route (method) level - see https://github.com/strongloop/loopback-swagger/blob/8eb73acbaae2cd600d03a95dcf55aee6f7560980/lib/specgen/route-helper.js#L161-L162.

However, you can customise this list at global level via options.consumes and options.produces passed to loopback-component-explorer, see https://github.com/strongloop/loopback-swagger/blob/8eb73acbaae2cd600d03a95dcf55aee6f7560980/lib/specgen/swagger-spec-generator.js#L22-L36

JSON API specifies the use of PATCH instead of PUT. Is it possible to modify loopback explorer to send PATCHs in place of PUTs?

LoopBack explorer sends exactly the same HTTP verb which is specified in remoting metadata and used by the rest adapter. You should modify remoting data of your methods to specify a different HTTP verb (method), loopback-explorer will then pick it up automatically.

@digitalsadhu
Copy link
Contributor

Legend @bajtos, thanks!

@digitalsadhu
Copy link
Contributor

@bajtos earlier you mentioned:

Yes, it's not possible to change the list of content types at a boot script level now. The change should be pretty easy, we need to extend loopback-swagger to read consumes/produces from remoting metadata of each method (if it's provided).

I'd be happy to have a crack at submitting the PR for this. Any chance you can give me a bit of guidance?

@digitalsadhu
Copy link
Contributor

@bajtos @raymondfeng @ritch
I'm running into another issue that specifying 'application/vnd.api+json' as the accept type is not handled in strong remoting here:
https://github.com/strongloop/strong-remoting/blob/4575e92142aa32ee4a568862f93b394e7d034127/lib/http-context.js#L561-L593
I end up with a 406 Not Acceptable

The only option I can think of at the moment is to detect application/vnd.api+json and rewrite it to json or application/json

Any chance I can submit a PR to add support for application/vnd.api+json?

@ritch
Copy link
Member

ritch commented Oct 26, 2015

Any chance I can submit a PR to add support for application/vnd.api+json?

Sure. You should be able to add it as a case.

@digitalsadhu
Copy link
Contributor

@ritch awesome. Will do.

@digitalsadhu
Copy link
Contributor

@ritch PR submitted. strongloop/strong-remoting#249

@bajtos
Copy link
Member

bajtos commented Nov 5, 2015

@digitalsadhu

Yes, it's not possible to change the list of content types at a boot script level now. The change should be pretty easy, we need to extend loopback-swagger to read consumes/produces from remoting metadata of each method (if it's provided).

I'd be happy to have a crack at submitting the PR for this. Any chance you can give me a bit of guidance?

Awesome! You can start looking here: https://github.com/strongloop/loopback-swagger/blob/b6e39252640732e31c26d6831bf86c5b6593aff7/lib/specgen/route-helper.js#L161-L162. The default list of produces/consumes types is initialised here and then used here.

@digitalsadhu
Copy link
Contributor

Nice! On my list!

On Thu, 5 Nov 2015 at 17:02, Miroslav Bajtoš notifications@github.com
wrote:

@digitalsadhu https://github.com/digitalsadhu

Yes, it's not possible to change the list of content types at a boot
script level now. The change should be pretty easy, we need to extend
loopback-swagger to read consumes/produces from remoting metadata of each
method (if it's provided).

I'd be happy to have a crack at submitting the PR for this. Any chance you
can give me a bit of guidance?

Awesome! You can start looking here:
https://github.com/strongloop/loopback-swagger/blob/b6e39252640732e31c26d6831bf86c5b6593aff7/lib/specgen/route-helper.js#L161-L162.
The default list of produces/consumes types is initialised here
https://github.com/strongloop/loopback-swagger/blob/b6e39252640732e31c26d6831bf86c5b6593aff7/lib/specgen/swagger-spec-generator.js#L25-L35
and then used [here](
https://github.com/strongloop/loopback-swagger/blob/b6e39252640732e31c26d6831bf86c5b6593aff7/lib/specgen/swagger-spec-generator.js#L124-L1250
.


Reply to this email directly or view it on GitHub
#445 (comment)
.

@digitalsadhu
Copy link
Contributor

@bajtos I've been digging into this a bit. My first thought was to try something like this:

var entry = {
      path: routeHelper.convertPathFragments(route.path),
      method: routeHelper.convertVerb(route.verb),
      operation: {
        tags: tags,
        summary: typeConverter.convertText(route.description),
        description: typeConverter.convertText(route.notes),
        // [bajtos] We used to remove leading model name from the operation
        // name for Swagger Spec 1.2. Swagger Spec 2.0 requires
        // operation ids to be unique, thus we have to include the model name.
        operationId: route.method,
        // [bajtos] we are omitting consumes and produces, as they are same
        // for all methods and they are already specified in top-level fields
        parameters: accepts,
        responses: responseMessages,
        deprecated: !!route.deprecated,
        // TODO: security
      }
    };

    if (route.consumes) {
      entry.operation.consumes = route.consumes;
    }

    if (route.produces) {
      entry.operation.produces = route.produces;
    }

But 2 problems,

  1. produces and consumes aren't coming through from say a remoteMethod definition so I'm thinking I don't fully understand the nature of the route object. (Though it looks very similar to a remote method definition)
  2. Even if 1 above worked, boot scripts are run after the loopback-component-explorer is setup and so doing the following would have no effect:
module.exports = function (app) {
  let remotes = app.remotes();
  let methods = remotes.methods();
  methods.forEach(function (method) {
    method.consumes = ['application/vnd.api+json'];
    method.produces = ['application/vnd.api+json'];
  });
}

Perhaps I'm barking up the wrong tree and rather than trying to allow defining consumes and produces per method I should just be trying to allow a setting on app or strong-remoting setup?

Pointers, suggestions appreciated.

@bajtos
Copy link
Member

bajtos commented Nov 13, 2015

produces and consumes aren't coming through from say a remoteMethod definition so I'm thinking I don't fully understand the nature of the route object. (Though it looks very similar to a remote method definition)

strong-remoting's SharedMethod copies only a subset of properties from the original options, see the constructor: https://github.com/strongloop/strong-remoting/blob/65479ddb63524274193ea16d5e2ea5fbbc5c8b03/lib/shared-method.js#L81-L141 and also fromFunction method: https://github.com/strongloop/strong-remoting/blob/65479ddb63524274193ea16d5e2ea5fbbc5c8b03/lib/shared-method.js#L158-L169

Even if 1 above worked, boot scripts are run after the loopback-component-explorer is setup and so doing the following would have no effect:

This is a problem with remotes.methods() which creates a temporary short-lived SharedMethod instances. Any changes made to these instances are discarder :(

Perhaps I'm barking up the wrong tree and rather than trying to allow defining consumes and produces per method I should just be trying to allow a setting on app or strong-remoting setup?

Yes, that's what crossed my mind too. It looks like a good strategy, considering that JSON-API implementation almost certainly wants to change consumes/produces globally.

We already have two global options that are related to your work: rest.supportedTypes and rest.xml (docs). Perhaps it's time to add rest.jsonApi?

@digitalsadhu what do you think?

@digitalsadhu
Copy link
Contributor

Thanks @bajtos that makes sense, thanks for clarification.

How were you imagining rest.jsonApi would work?

Perhaps something like, if rest.jsonApi=true the following:

  • 'application/vnd.api+json' added to supportedTypes
  • 'application/vnd.api+json' added to consumes/produces list

Perhaps also:

  • bodyParser modified to treat Content-Type 'application/vnd.api+json' as json

@digitalsadhu
Copy link
Contributor

@bajtos also, how hard is it to get access to strong-remoting rest options inside loopback-swagger?

@bajtos
Copy link
Member

bajtos commented Nov 13, 2015

How were you imagining rest.jsonApi would work?

Your proposal above looks good to me.

how hard is it to get access to strong-remoting rest options inside loopback-swagger

I'd say it's easy. Swagger specgen has access to remotes and the rest handler, see
https://github.com/strongloop/loopback-swagger/blob/b6e39252640732e31c26d6831bf86c5b6593aff7/lib/specgen/swagger-spec-generator.js#L40-L41

The handler has options property, see
https://github.com/strongloop/strong-remoting/blob/32efdc433f8bd12f1ecc7872c855216b99c0911e/lib/rest-adapter.js#L40.

@digitalsadhu
Copy link
Contributor

Ok excellent. I'll make that my next push.
Thanks for the feedback

@teckays
Copy link

teckays commented Dec 28, 2015

I'm waiting for this feature more than for a Christmas present. +100

@digitalsadhu
Copy link
Contributor

@teckays
Have a look at the component we are working on here
https://github.com/digitalsadhu/loopback-component-jsonapi

It's not quite feature complete for use with ember. (Side loading in the works) and there are still most likely some bugs to work out but it's worth giving it a go. We'd certainly appreciate any big reports etc.

@teckays
Copy link

teckays commented Dec 28, 2015

@digitalsadhu I will definitely give it a try and report any bugs/improvements I find. Thank you.

@digitalsadhu
Copy link
Contributor

@teckays we released v0.11.0 of the component yesterday which adds support for side loading and that pretty much rounds out the main features needed for ember users.

For anyone else reading this, https://github.com/digitalsadhu/loopback-component-jsonapi is ready for anyone willing to try it out and submit bugs and such. We are pretty quick to respond and keen to find and fix any issues so please give it a go.

@digitalsadhu
Copy link
Contributor

@bajtos I'm wondering if theres any chance anyone from Strongloop would have the time to have a look through https://github.com/digitalsadhu/loopback-component-jsonapi and offer any feedback? (I know you guys are busy and understand if it's not possible)

Theres a lot of code improvement we'd like to do when we get a chance and we have a bank of integration tests in place so that we can refactor with confidence as needed.

We'd also like to continue adding additional configuration options to make the component more flexible for various real world use cases.

One thing i'm a bit worried about is that as loopback changes we may constantly end up fixing things that break (So far this hasn't happened) so in general i'm hoping theres ways we can make the code base a bit more robust.

@digitalsadhu
Copy link
Contributor

Just an update on loopback-component-jsonapi, we are getting pretty stable at version v0.17.2
We are using it in production in multiple projects at work now. I'd recommend people give it a go and give us feedback, report bugs etc.

@teckays
Copy link

teckays commented Feb 27, 2016

@digitalsadhu +1

@vforvalerio87
Copy link

+1

2 similar comments
@ashishtilara
Copy link

+1

@celicoo
Copy link

celicoo commented Jun 19, 2016

+1

@ritch ritch changed the title Any chance of supporting jsonapi.org? Support jsonapi.org Nov 29, 2016
@bajtos
Copy link
Member

bajtos commented Mar 3, 2017

Closing this issue as done - see https://www.npmjs.com/package/loopback-component-jsonapi

@bajtos bajtos closed this as completed Mar 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests