Skip to content
This repository

Support optional types for computed attributes #170

Merged
merged 2 commits into from about 1 year ago

4 participants

Matt Rogish Jo Liss Steve Klabnik Evan Light
Matt Rogish

We're using Serializer.schema to auto-generate our Ember.js schema. We added a method that returns a string, but AMS is sending 'nil' since it's computed, which messes up the schema. This PR allows you to specify an optional type for a computed attribute so that your schema matches what you expect.

Evan Light elight commented on the diff December 26, 2012
lib/active_model/serializer.rb
@@ -73,7 +73,9 @@ def attributes(*attrs)
73 73
       end
74 74
 
75 75
       def attribute(attr, options={})
76  
-        self._attributes = _attributes.merge(attr => options[:key] || attr.to_s.gsub(/\?$/, '').to_sym)
  76
+        self._attributes = _attributes.merge(attr.is_a?(Hash) ? attr : {attr => options[:key] || attr.to_s.gsub(/\?$/, '').to_sym})
1
Evan Light
elight added a note December 26, 2012

Oof, that is a busy line if code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Jo Liss
Collaborator

I wonder if we should take back a step and think about if and how we want schema support to happen. Right now it feels a bit like we're heaping functionality onto a fairly obscure feature.

In general generated schemas seem to be great for DRYing up complex apps. In my experience, it doesn't scale well if you have to to keep server-side and client-side models in sync. AMS's schema method comes in handy to generate model definitions for your client-side framework. Matt and I have been using this, and it's quite convenient.

On the other hand, the way it's currently implemented, types are inferred from ActiveRecord, which requires an active DB connection. This means that you cannot use the schema in any asset files, since there's not necessarily a database around during precompilation. The alternative is generating a schema.js file using a rake task. That's what we've been doing, but it feels like a bit of a hack. For instance you have to remember to regenerate the schema. It's not something I'd recommend as a canonical solution.

I wonder if there's a way to make schemas work in the asset pipeline. Or maybe you guys have other ideas as to what to do with the schema?

Steve Klabnik
Owner

I am very, very against this change. :-1:. Types don't belong in JSON: schemas are an anti-pattern.

Matt Rogish

Steve,

I generally agree - I don't want JSON to turn into a slightly easier to read XML + XSD.

But -- as Jo said, we're using the AMS schema to auto-generate a lot of stuff for our Ember.js front-end. Losing schema support would relegate us to keeping manual mappings, which feels very un-Rails and Ember-like. Better living through automation.

I struggle to think of another way we could automate this without, basically, us copypasta'ing this code into our Rails app, which we would certainly do should schema support disappear.

That said, given that schema support is a "supported" bit of AMS, why :-1: something that makes it a bit more useful?

Matt

Steve Klabnik
Owner

Better living through automation.

I agree for sure. I need to better educate myself on the Ember side of things, which is why I wrote :-1: instead of closing. ;) Everything should automatically work; schemas aren't needed to make that happen properly.

Matt Rogish

Steve,

Thanks for the reply and the discussion. I understand and I agree with you that the AMS schema isn't necessary for Ember to work.

Ember "out of the box" totally doesn't need AMS schema; you just define your Ember models by hand, which is not impossible obviously but is a major bummer and a easy way to introduce bugs and other unanticipated behavior. :)

Thus, we decided that autogenerating our Ember model definitions from the Serializer schema was a net win in what we were doing. In the general case of {JS front end} and {Rails/Sinatra back-end}, AMS acts as the interface between, and code generation should enter into it somewhere.

Absolutely the AMS schema isn't a necessary component for that either; it just happens to be a very useful (and existing!) bit that we piggy back on. If AMS schemas go away, we'd need to just relocate that code into a helper; presumably the Ember-Rails project would be a better place for this interface to live, anyway.

I'd certainly be in favor of removing the AMS schema and then relocating the model definition generation (if they are amenable to it) to the Ember-Rails project.

In the interim, this change would help us remove some bit of manual work (since they're returning nil now) and further get us closer to complete automation, but I understand if it isn't furthering the goals of the AMS project. If so, I'd think some sort of deprecation of schemas ought to take place so we can look for a better place to put this stuff, anyway.

Perhaps the step I can do is extract our automation and the schema generation into something digestible by the Ember-Rails project, and continue the discussion over there?

Ref (Ember models):
http://emberjs.com/guides/models/defining-models/

Steve Klabnik
Owner

So.

Is this still useful? I need to investigate further into what the latest details are, but Ember has changed a lot in the last two months. Do we still want to get this in in some form?

Jo Liss
Collaborator

Not sure about this PR, but Ember still has model definitions that need to stay in sync with the serializer, yes.

Steve Klabnik
Owner

Do they need something like this to stay in sync? I really need to embed an ember app into AMS's test suite ;)

Jo Liss
Collaborator

Yes, we basically have a fundamental problem here that either you have to manually keep Rails serializers and Ember models in sync, or you have to write (slightly hacky) custom code to generate models from the AMS schema.

Jo Liss
Collaborator

I don't know how many people are actually using the AMS schema to generate Ember models. I imagine it's not super common.

Steve Klabnik
Owner

Ahhh, that's the part that wasn't clicking for me: it's not about when the app already exists, it's about generating files. Right.

Jo Liss
Collaborator

Ah, sorry I was unclear: The "generating" actually happens on a regular basis while you develop (semi-automatically) -- it's not just a helper for getting started. The purpose is to keep Ember models and Rails serializers DRY.

Matt Rogish

I think Jo said most of it: we're using this to dynamically generate the Ember schema on a semi-continuous basis (when we change something in the serializer) - not just at the Ember app creation time.

My thinking is:
The current #schema implementation feels almost, but not quite, done. Dynamic methods returning nil for types seems incomplete; this change (I don't care if it's done via this PR, or some other PR) would at least bring closure to the schema generation process and ensure that no unexpected nils wind up in there.

That said, I'm also completely happy to ignore it if it doesn't fit in with the philosophy (although, why have Schemas in that case?), it doesn't cause any observable problems in the Ember app if we leave this out.

Steve Klabnik steveklabnik merged commit e76a164 into from March 08, 2013
Steve Klabnik steveklabnik closed this March 08, 2013
Steve Klabnik
Owner

It's all good, everyone. I'm still getting up to speed on some of the details of Ember, so thank you for putting up with my n00bness. :)

Anyway, obviously this is something that's needed, so let's merge it in. :heart:

Matt Rogish

Thanks Steve! You rock!! :+1:

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

Showing 2 unique commits by 1 author.

Dec 10, 2012
Matt Rogish Adding optional type for attributes 25c564b
Dec 11, 2012
Matt Rogish Adding info to the readme on how to specify the type, adding an addit…
…ional

test to cover a computed property without a type
72652ce
This page is out of date. Refresh to see the latest.
14  README.md
Source Rendered
@@ -232,6 +232,20 @@ class PostSerializer < ActiveModel::Serializer
232 232
 end
233 233
 ```
234 234
 
  235
+The type of a computed attribute (like :full_name above) is not easily
  236
+calculated without some sophisticated static code analysis. To specify the
  237
+type of a computed attribute:
  238
+
  239
+```ruby
  240
+class PersonSerializer < ActiveModel::Serializer
  241
+  attributes :first_name, :last_name, {:full_name => :string}
  242
+
  243
+  def full_name
  244
+    "#{object.first_name} #{object.last_name}"
  245
+  end
  246
+end
  247
+```
  248
+
235 249
 If you would like the key in the outputted JSON to be different from its name
236 250
 in ActiveRecord, you can use the `:key` option to customize it:
237 251
 
12  lib/active_model/serializer.rb
@@ -73,7 +73,9 @@ def attributes(*attrs)
73 73
       end
74 74
 
75 75
       def attribute(attr, options={})
76  
-        self._attributes = _attributes.merge(attr => options[:key] || attr.to_s.gsub(/\?$/, '').to_sym)
  76
+        self._attributes = _attributes.merge(attr.is_a?(Hash) ? attr : {attr => options[:key] || attr.to_s.gsub(/\?$/, '').to_sym})
  77
+
  78
+        attr = attr.keys[0] if attr.is_a? Hash
77 79
 
78 80
         unless method_defined?(attr)
79 81
           define_method attr do
@@ -175,8 +177,12 @@ def schema
175 177
             attrs[key] = column.type
176 178
           else
177 179
             # Computed attribute (method on serializer or model). We cannot
178  
-            # infer the type, so we put nil.
179  
-            attrs[key] = nil
  180
+            # infer the type, so we put nil, unless specified in the attribute declaration
  181
+            if name != key
  182
+              attrs[name] = key
  183
+            else
  184
+              attrs[key] = nil
  185
+            end
180 186
           end
181 187
         end
182 188
 
7  test/serializer_test.rb
@@ -491,16 +491,17 @@ class << self; self; end.class_eval do
491 491
 
492 492
       # Computed attributes (not real columns or associations).
493 493
       def can_edit; end
  494
+      def can_view; end
494 495
       def drafts; end
495 496
 
496  
-      attributes :name, :age, :can_edit
  497
+      attributes :name, :age, {:can_edit => :boolean}, :can_view
497 498
       has_many :posts, :serializer => Class.new
498 499
       has_many :drafts, :serializer => Class.new
499 500
       has_one :parent, :serializer => Class.new
500 501
     end
501 502
 
502 503
     assert_equal serializer.schema, {
503  
-      :attributes => { :name => :string, :age => :integer, :can_edit => nil },
  504
+      :attributes => { :name => :string, :age => :integer, :can_edit => :boolean, :can_view => nil },
504 505
       :associations => {
505 506
         :posts => { :has_many => :posts },
506 507
         :drafts => nil,
@@ -1004,7 +1005,7 @@ def self.to_s
1004 1005
         :name => 'logo.png',
1005 1006
         :url => 'http://example.com/logo.png',
1006 1007
         :attachable => {
1007  
-          :type => :email, 
  1008
+          :type => :email,
1008 1009
           :id => 1
1009 1010
         }},
1010 1011
       :emails => [{
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.