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

Mapping JSON API spec / schema to AMS [ci skip] #1301

Merged
merged 4 commits into from
Dec 9, 2015
Merged

Mapping JSON API spec / schema to AMS [ci skip] #1301

merged 4 commits into from
Dec 9, 2015

Conversation

bf4
Copy link
Member

@bf4 bf4 commented Oct 26, 2015

This is something to think about. I'm not 100% sure if it should be
in the docs or in an issue, than a PR. But, I think it might be useful for discussion
As we work on JSON API.

View formatted

  1. Trying to surface which parts of the code map to the JSON API spec
  2. Use point 1 to consider design and naming of objects
  3. Track completeness of implementation
  4. TODO: Use schema/schema.json to validate and document our implementation
  5. TODO: use uri_template in link generation? use json pointer gem?

There are for sure formatting and possible correctness issues. I mean it as a point of discussion.

@beauby
Copy link
Contributor

beauby commented Oct 26, 2015

👍

@bf4 bf4 force-pushed the jsonapi_schema branch 3 times, most recently from c93005b to 804e195 Compare October 26, 2015 06:26
| failure.errors | UniqueArray(error) | | #1004
| meta | Object | |
| data | oneOf (resource, UniqueArray(resource)) | | AM::S::Adapter::JsonApi#serializable_hash_for_collection,#serializable_hash_for_single_resource
| resource | String(type), String(id),<br>attributes, relationships,<br>links, meta | type, id | AM::S::Adapter::JsonApi#primary_data_for
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the JSON API being represented by ActiveModel::Serializer is one of my larger motivations for this PR. The naming of the library as ActiveModelSerializers and its primary namespace being ActiveModel::Serializer and its resource wrapper being a class of the same name causes no end of confusion.

I'd like to at the least move towards naming ActiveModel::Serializer as ActiveModelSerializers::Resource, so that its naming is less confusing, more precise, and a better namespace. I know it's a big historical change, so it would be backwards compatible, deprecated for now.

I generally also want to revisit what we mean to Serializer (Resource?) vs. Adapter and what interfaces we'd like for those.

Also, consider how versioned resources might look ref: #1290 (comment)

# lib/active_model_serializers/v08/serializer.rb
# lib/active_model_serializers/v08/collection_serializer.rb
# lib/active_model_serializers/v08/adapter.rb
module ActiveModelSerializers::V08
  # Serializer might not need anything, but good to have defined
  Serializer = ActiveModel::Serializer
  CollectionSerializer = ActiveModel::Serializer::CollectionSerializer # well, it's Array now, but hopefully we'll get that renaming pr soon
  class Adapter < ActiveModel::Serializer::Adapter::Base
    # whatever methods you need
  end
end

And test like the other serializers and adapters

adapter = ActiveModelSerializers::V08::Adapter
serializer = ActiveModelSerializers::V08::CollectionSerializer
each_serializer = ActiveModelSerializers::V08::Serializer
collection_resource = Post.all
expected_response = [{ something }]
assert ActiveModel::SerializableResource.new(collection_resource, serializer: serializer, each_serializer: each_serializer, adapter: adapter).as_json, expected_response

# and

adapter = ActiveModelSerializers::V08::Adapter
serializer = ActiveModelSerializers::V08::Serializer
resource = Post.first
expected_response = { something }
assert ActiveModel::SerializableResource.new(collection_resource, serializer: serializer, adapter: adapter).as_json, expected_response

refs:

Keeping a well-defined interface, e.g. grape support: #1258 and serialization_context #1289

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added as a comment in #1298 (comment)

@NullVoxPopuli
Copy link
Contributor

this is fantastic. This will be great for people just getting in to JSON API (re: me, ;-) )

@bf4
Copy link
Member Author

bf4 commented Oct 27, 2015

ref: #1098

| error | String(id), links, String(status),<br>String(code), String(title),<br>String(detail), error.source, meta | |
| error.source | String(pointer), String(parameter) | |
| pointer | [JSON Pointer RFC6901](https://tools.ietf.org/html/rfc6901) | |

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reposting #1298 (comment) here with a few changes, is some thoughts on why we might organize things.

let AMS refer to ActiveModelSerializers, AM::S to ActiveModel::Serializer, and AM::S::A to ActiveModel::Serializer::Adapter for brevity in the below.

Current Desired Notes
require 'active_model_serializers' require 'active_model_serializers'
ActiveModelSerializers ActiveModelSerializers
ActiveModel::SerializableResource ActiveModel::SerializableResource
namespace ActiveModel::Serializer AMS::Current,
ActiveModelSerializers::V08, etc
versioned
class AM::S AMS::Current::Resource is a class that defines the properties and behavior of the data that you present to the user
AMS::Model AMS::Model e.g. ActiveRecord object but a PORO. Maybe should be AMS::Record A record is an instance of a model that contains data loaded from a server. Your application can also create new records and save them back to the server.
AM::S::Lint AMS::Model::Lint
ActiveModel::Serialization AMS::ActionController::Serialization
AM::S::CollectionSerializer AMS::Current::ResourceCollection
namespace AM::S::Adapter AMS::Adapter adapter: knows how to talk to your server. knows how to translate requests from AMS into requests on your server. object that translates requests from AMS/Rails/Grape etc (such as "find the user with an ID of 123") into a requests to a server. let you completely change how your API is implemented without impacting your application code.
class AM::S::A::JsonApi AMS::Adapter::JsonApi,
AMS::Adapter::V08::Json
not versioned
class ActiveModel::Serializer, AM::S::A::Attributes AMS::Formatter maps keys and values to desired format, see https://github.com/orbitjs/orbit.js/blob/master/lib/orbit-common/jsonapi-serializer.js
N/A AMS::Current::Store store is the central repository of records in your application, use the store to retrieve records, as well to create new ones. The store will automatically cache records for you.
N/A AMS::Adapter::JsonApi::Exceptions
AM::S::A::JsonApi::ApiObject::JsonApi AMS::Adapter::JsonApi::ApiObject::
{JsonApi,Links,Link,Resource,Error, etc}? vs.
JsonApiFormat or even schema validation
ActiveModelSerializers.logger ActiveModelSerializers.logger

re: ActiveModelSerializers::Current is just a thought.. perhaps to generate an alternative :)

see

@bf4
Copy link
Member Author

bf4 commented Nov 9, 2015

So, I think this PR got a bit sidetracked by the namespacing discussion in #1301 (comment) and elsewhere, but the actual contents of it should be good to go, no?

- [ ] info
- [ ] meta: `"$ref": "#/definitions/meta"`
- [ ] links: `"$ref": "#/definitions/links"`
- [ ] jsonapi: ` "$ref": "#/definitions/jsonapi"`
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Info document isn't really part of the spec json-api/json-api#440 (comment)

@bf4
Copy link
Member Author

bf4 commented Nov 13, 2015

@rails-api/ams any objections to merging?@beauby good to go?

bf4 added a commit that referenced this pull request Dec 9, 2015
Mapping JSON API spec / schema to AMS [ci skip]
@bf4 bf4 merged commit 614e349 into master Dec 9, 2015
@bf4 bf4 deleted the jsonapi_schema branch December 9, 2015 23:09
@bf4
Copy link
Member Author

bf4 commented Dec 9, 2015

Merged. Had two plus ones from collabs and no objections for 26 days

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants