FlattenJson adapter no longer inherits Json adapter, renamed to Attributes #1117

bf4 · 2015-09-04T09:31:25Z

FlattenJson adapter no longer inherits Json adapter
Rename FlattenJson to Attributes (allow plural adapter names)
Consistently refer to the 'JSON API' and the 'JsonApi' adapter

TODO

Add to changelog

beauby · 2015-09-04T09:56:17Z

👍

bf4 · 2015-09-04T10:31:21Z

@beauby Thanks. I followed a somewhat non-linear path to get there, which is why I extracted CacheCheck along the way, but decided is was fine :)

bf4 · 2015-09-04T10:33:35Z

README.md

@@ -10,7 +10,7 @@ AMS does this through two components: **serializers** and **adapters**.
 Serializers describe _which_ attributes and relationships should be serialized.
 Adapters describe _how_ attributes and relationships should be serialized.

-By default AMS will use the **Flatten Json Adapter**. But we strongly advise you to use **JsonApi Adapter** that follows 1.0 of the format specified in [jsonapi.org/format](http://jsonapi.org/format).
+By default AMS will use the **Attributes Adapter**. But we strongly advise you to use **JsonApi Adapter** that follows 1.0 of the format specified in [jsonapi.org/format](http://jsonapi.org/format).
 Check how to change the adapter in the sections bellow.


One thing that I don't like about calling the adapter attributes is that it is harder to grep for, esp if we want to rename it again. But, we liked the name in the Slack channel, so until something better..

yup, I'm not sure about the name as well, but have no suggestions so far

AttributeSerialization?

Fields, Properties?? idk. ha

joaomdmoura · 2015-09-07T20:28:20Z

lib/active_model/serializer/adapter.rb

      autoload :Null
      autoload :FragmentCache

+      include CacheCheck


We are using autoload and including it in the sequence, shouldn't we go with require instead then?

good point :)

joaomdmoura · 2015-09-07T20:52:44Z

Nice work here @bf4, really fast! Can you check the comments and rebase it please ? 😄

bf4 · 2015-09-07T22:29:38Z

cleaned up

NullVoxPopuli · 2015-09-12T14:49:52Z

needs rebase

NullVoxPopuli · 2015-09-13T10:42:56Z

Why not inherit from the json adapter?

bf4 · 2015-09-13T13:29:52Z

Can't. Would need to undef methods

NullVoxPopuli · 2015-09-13T13:31:39Z

why?

this is just so clean:

    def serializable_hash(options = {})
      super.each_value.first
    end

my concern is that duplicating code is prone to errors and repeated bugs

bf4 · 2015-09-13T14:21:07Z

@NullVoxPopuli asked:

Why not inherit from the json adapter?

Because, if anything, FlattenJson/Attributes is the super class. It's a more general case.

I knew someone who preferred inheritance over composition. He was a super guy.

+        def serializable_hash(options = {})
+          { root => Adapter::Attributes.new(serializer).serializable_hash(options) }

Nice eh?

Aside: adapters and serializers should both have their own meta, the adapter's going at the top level. serializer.meta should be part of Attributes. http://jsonapi.org/format/#document-resource-objects

NullVoxPopuli · 2015-09-13T14:29:37Z

Because, if anything, FlattenJson/Attributes is the super class. It's a more general case.

Maybe the common functionality could be pulled out in to a module then? or a class that both inherit from?

I know inheritance isn't the answer for everything, but I do hate duplicate code.

also, I hate combining that sort of stuff in to one line..
I would have done:

def serializable_hash(options = {})
  adapter = Adapter::attributes.new(serializer)
  serialized = adapter.serializable_hash(options)

  { root => serialized }

much cleaner :-) and easier to debug.

But anyway, with the upcoming Adapter::Base changes, I'm hoping to eventually move JSON stuff in there as well.

I think there is a way to use json api and json such that they pass blocks to 'Base' for there specific different formats

bf4 · 2015-09-13T14:41:31Z

Maybe the common functionality could be pulled out in to a module then? or a class that both inherit from?

Modules are inheritance. I'm not even sure how that would work without having two subclasses that include such a module if we want to keep the abstract methods in Adapter.

I know inheritance isn't the answer for everything, but I do hate duplicate code.

Where's the duplicate code. I'm not seeing it :)

bf4 · 2015-09-13T14:51:21Z

Rebased

bf4 · 2015-09-13T14:52:30Z

lib/active_model/serializer/adapter/json_api.rb

-          resource[:relationships][name] ||= { data: [] }
-          resource[:relationships][name][:data] += serializers.map { |serializer| { type: serializer.json_api_type, id: serializer.id.to_s } }
-        end
-


Was accidentally re-introduced in a rebased PR

NullVoxPopuli · 2015-09-18T14:31:03Z

needs rebase.

But.. I think I am ok with these changes -- I like the separation of responsibility for attributes. Hopefully this will help cleanup #1127 as well.

NullVoxPopuli · 2015-09-18T16:14:52Z

👍

FlattenJson adapter no longer inherits Json adapter, renamed to Attributes

jfelchner · 2015-09-18T22:41:06Z

Hey guys, I just tried rebasing #1029 and ran into this. Can someone explain to me what benefit was provided by doing this as opposed to either:

a) Leaving it as it was. After all FlattenJson was a more specific Json adapter and therefore inheritance made sense IMO.
b) Creating a module that both adapters inherit from with the common functionality.

In all three scenarios (including the changes in this PR) there is no duplicated code anywhere.

Now effectively any adapter-specific options/defaults need to be explicitly parsed before passing them into the Attributes adapter. Whereas if we'd used inheritance everything would Just Work because it would be using the appropriate values for whatever class it was instantiated as.

I favor composition the majority of the time, but I don't think this one is appropriate.

I personally think we should instead break the serializable_hash method down into about a dozen smaller methods, and keep them on the Json adapter. If the Json adapter's root is nil then there's no root (and no allowed meta). Then the Attributes adapter can be completely removed.

As a concrete example, this is causing major problems for me rebasing #1029 and I don't like the solution nearly as much as I did before. IMO it's much less elegant.

bf4 · 2015-09-20T19:32:00Z

@jfelchner

I personally think we should instead break the [Adapter#]serializable_hash method down into about a dozen smaller methods,

Totally agree

and keep them on the Json adapter.

I get what you're saying, but the Json adapter is, I think, the wrong place

If the Json adapter's root is nil then there's no root (and no allowed meta). Then the Attributes adapter can be completely removed.

True. I agree there's got to be a better division of reponsibilities, but is the Json adapter the same as Attributes, but only with a root and meta data? That's certainly how the code looks now, but will it continue to be true?

For the moment, it's simpler, I think, for the Json adapter to use the Attributes adapter than to worry about which adapter should inherit which. I'm also not convinced that mixing in an 'AttributesSerialization' module to both the Attributes Adapter and the Json Adapter improves on this.

Here's how I'm thinking of the pieces now:

A model/resource has attributes that are 'readable for serialization'.
- (We should really document that read_attribute_for_serialization(name) could be used for any user-controlled transformations in the model...
A Serializer for the model that expresses the desired attributes and any transformations of the model's attributes
The Attributes adapter more or less passes through the serializer attributes into the serializable hash, except that it also handles collections.
The other adapters use the serialization (via the Attributes adapter) of the models and any other input or options to build the response document aka serializable_hash

(or something like that)

bf4 reviewed Sep 4, 2015
View reviewed changes

bf4 force-pushed the remove_flatten_json_inheriting_json branch from fcbae45 to 40a1daa Compare September 6, 2015 14:04

bf4 mentioned this pull request Sep 7, 2015

Support nested associations for Json and Attributes adapters + Refactor Attributes adapter #1127

Merged

joaomdmoura reviewed Sep 7, 2015
View reviewed changes

bf4 force-pushed the remove_flatten_json_inheriting_json branch from 40a1daa to fb92d25 Compare September 7, 2015 22:23

beauby mentioned this pull request Sep 10, 2015

Get rid of unnecessary instance variables, and implied dependencies. #1132

Merged

bf4 changed the title ~~FlattenJson adapter no longer inherits Json adapter~~ FlattenJson adapter no longer inherits Json adapter, renamed to Attributes Sep 11, 2015

bf4 mentioned this pull request Sep 11, 2015

Add basic support for xml #1133

Closed

bf4 force-pushed the remove_flatten_json_inheriting_json branch from fb92d25 to b592ebc Compare September 13, 2015 14:51

bf4 reviewed Sep 13, 2015
View reviewed changes

beauby added V: 0.10.x C: Cleanup S: Needs Team Discussion labels Sep 14, 2015

bf4 mentioned this pull request Sep 16, 2015

Remove legacy method accidentally reintroduced in #1017 #1150

Merged

bf4 force-pushed the remove_flatten_json_inheriting_json branch from b592ebc to a144f55 Compare September 17, 2015 20:45

bf4 added 2 commits September 18, 2015 10:15

FlattenJson adapter no longer inherits Json adapter

ceef214

Rename FlattenJson to Attributes (allow plural adapter names)

c6f8d0f

bf4 force-pushed the remove_flatten_json_inheriting_json branch from a144f55 to c6f8d0f Compare September 18, 2015 15:17

NullVoxPopuli added a commit that referenced this pull request Sep 18, 2015

Merge pull request #1117 from bf4/remove_flatten_json_inheriting_json

a309132

FlattenJson adapter no longer inherits Json adapter, renamed to Attributes

NullVoxPopuli merged commit a309132 into rails-api:master Sep 18, 2015

bf4 deleted the remove_flatten_json_inheriting_json branch September 18, 2015 16:56

bf4 added the S: Changelog Missing label Sep 20, 2015

This was referenced Jan 6, 2016

Update for the CHANGELOG for merged pull requests #1415

Closed

Update Changelog [ci skip] #1416

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FlattenJson adapter no longer inherits Json adapter, renamed to Attributes #1117

FlattenJson adapter no longer inherits Json adapter, renamed to Attributes #1117

bf4 commented Sep 4, 2015

beauby commented Sep 4, 2015

bf4 commented Sep 4, 2015

bf4 Sep 4, 2015

joaomdmoura Sep 7, 2015

bf4 Sep 7, 2015

NullVoxPopuli Sep 18, 2015

joaomdmoura Sep 7, 2015

bf4 Sep 7, 2015

bf4 Sep 10, 2015

joaomdmoura commented Sep 7, 2015

bf4 commented Sep 7, 2015

NullVoxPopuli commented Sep 12, 2015

NullVoxPopuli commented Sep 13, 2015

bf4 commented Sep 13, 2015

NullVoxPopuli commented Sep 13, 2015

bf4 commented Sep 13, 2015

NullVoxPopuli commented Sep 13, 2015

bf4 commented Sep 13, 2015

bf4 commented Sep 13, 2015

bf4 Sep 13, 2015

NullVoxPopuli commented Sep 18, 2015

NullVoxPopuli commented Sep 18, 2015

jfelchner commented Sep 18, 2015

bf4 commented Sep 20, 2015

FlattenJson adapter no longer inherits Json adapter, renamed to Attributes #1117

FlattenJson adapter no longer inherits Json adapter, renamed to Attributes #1117

Conversation

bf4 commented Sep 4, 2015

beauby commented Sep 4, 2015

bf4 commented Sep 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaomdmoura commented Sep 7, 2015

bf4 commented Sep 7, 2015

NullVoxPopuli commented Sep 12, 2015

NullVoxPopuli commented Sep 13, 2015

bf4 commented Sep 13, 2015

NullVoxPopuli commented Sep 13, 2015

bf4 commented Sep 13, 2015

NullVoxPopuli commented Sep 13, 2015

bf4 commented Sep 13, 2015

bf4 commented Sep 13, 2015

Choose a reason for hiding this comment

NullVoxPopuli commented Sep 18, 2015

NullVoxPopuli commented Sep 18, 2015

jfelchner commented Sep 18, 2015

bf4 commented Sep 20, 2015