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

Add a readOnly option to DS.attr #303

Closed
wants to merge 2 commits into from
Closed

Add a readOnly option to DS.attr #303

wants to merge 2 commits into from

Conversation

bradleypriest
Copy link
Member

This adds a readOnly option to DS.attr which causes the attribute not to be serialized to json.
I'm not sure if readOnly is the right name because it doesn't stop you changing the value in any way it just makes sure the changes aren't persisted.

My main use case is for created_at and updated_at fields. I'm currently filtering them out of the json in my Rails controllers at the server end. But it would be nice to not send them at all.

@wagenet
Copy link
Member

wagenet commented Jul 9, 2012

@bradleypriest I like this idea, but it looks like the PR doesn't merge anymore.

@bradleypriest
Copy link
Member Author

@wagenet rebased and pushed

@wagenet
Copy link
Member

wagenet commented Jul 12, 2012

@bradleypriest Do you think it makes sense to warn if a user tries to set a readOnly attribute?

@wagenet wagenet mentioned this pull request Jul 13, 2012
@bradleypriest
Copy link
Member Author

Yeah, I think that'd be a good idea. To be honest, this feature was just called excludeFromJson right up until I created the PR :)

@ppcano
Copy link
Contributor

ppcano commented Sep 14, 2012

@wagenet , @bradleypriest. One specific case of a similar "readOnly" feature, require the following implementation:

  • excludeFromJson
  • doNotDirty: when user changes property, its value is updated, but setProperty is skipped, so the record is not on dirty state and won't be committed to the server.

This allows the user changes the value to provide a more responsiveness UI, but the value will only be really updated by the server.

@bradleypriest
Copy link
Member Author

@ppcano That seems like a pretty specific situation to me. Would you mind giving a specific example of where that would be useful

@ppcano
Copy link
Contributor

ppcano commented Sep 14, 2012

hasLiked property defines if the current authorized/logged user "likes" the following produc and cannot be updated with the product REST API, even though the info can be redundant/unnecesary, it has been defined to improve UIs.

Yn.Product = DS.Model.extend({

  hasLiked: DS.attr('boolean', {key: 'has_voted', doNotDirty: true, exclude:true} )

})

When the user fires the like button, the property will be updated immediately ( i think, this a common approach to manage async UIs ), and it will be updated based on server action response.

product.set('hasLiked', true);

var like = Yn.Like.createRecord({ user: currentUser, product: product};);
like.on('didCreate', function(item) {
  // refresh product if necessary
});

like.on('didCreateError', function(item) {
  product.set('hasLiked', false);
  //show error ui
});

App.store.commit();

@lewang
Copy link

lewang commented Oct 3, 2012

How does the PR relate to the relationship-improvements branch? Which AFAIU is the future of Ember-data.

@bradleypriest
Copy link
Member Author

It doesn't yet, I'll update once that branch gets merged, relationship-improvements is still too buggy to recommend that anyone uses.

@wagenet
Copy link
Member

wagenet commented Oct 18, 2012

@bradleypriest Can you revisit this now?

@bradleypriest
Copy link
Member Author

Have got it working locally, but it's broken a handful of unrelated tests that were naively stubbing the attributes property. Will push it up once I've sorted

@kevinansfield
Copy link
Contributor

Would this also cover the use case where you need to define a BelongsTo on a model so that it can be tied to a user but you don't want the user_id to be sent back to the server because it should only ever be set to the logged-in user and so is protected against mass assignment?

@ppcano
Copy link
Contributor

ppcano commented Oct 23, 2012

@kevinansfield, i already mentioned this case, doNotDirty + exclude.

@wagenet, @wycats, Wasn't this feature mentioned in other cases?

@bradleypriest
Copy link
Member Author

I think they were talking about whether associations could be marked as read only as well. Not currently as it uses a different path than serializing an attribute. It's definitely doable though.

Bradley Priest

On Tuesday, 23 October 2012 at 9:11 PM, Pepe Cano wrote:

@kevinansfield (https://github.com/kevinansfield), i already mentioned this case (#303 (comment)), doNotDirty + exclude.
@wagenet (https://github.com/wagenet), @wycats (https://github.com/wycats), Wasn't this feature mentioned in other cases?


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

@bradleypriest
Copy link
Member Author

Pepe, I agree there's definitely still some discussion to be had around the final API

Bradley Priest

On Tuesday, 23 October 2012 at 11:12 PM, Bradley Priest wrote:

I think they were talking about whether associations could be marked as read only as well. Not currently as it uses a different path than serializing an attribute. It's definitely doable though.

Bradley Priest

On Tuesday, 23 October 2012 at 9:11 PM, Pepe Cano wrote:

@kevinansfield (https://github.com/kevinansfield), i already mentioned this case (#303 (comment)), doNotDirty + exclude.
@wagenet (https://github.com/wagenet), @wycats (https://github.com/wycats), Wasn't this feature mentioned in other cases?


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

@bradleypriest
Copy link
Member Author

From the talk done at the Addepari meetup 2 weeks ago, I think this is being handled in the core of Ember-Data now?
Should I leave it? /cc @wycats

@mtwestra
Copy link

It would be great to have this feature.

My current use case is that I have a list of items in a table, which I can sort by clicking on the header (this changes the sortProperties on the controller). In the first column I have a select box, so I can select numerous items, and then perform a common action on them, such as set a property. To be able to do this, I have added an 'isSelected' property to the item model. However, I don't want this (local) selection to be persisted to the server.

I cannot use views for this, as the views get recreated when the sortProperties is changed and the table is rebuild.

any ideas are welcome!

@spencer516
Copy link

@mtwestra — I have a nearly identical implementation for what you're describing (Table of data with sort options; checkboxes to check multiple rows, etc).

In my Model, the 'isSelected' property is set to false (instead of being set to DS.attr("boolean") )...it behaves as expected and is excluded when serializing toData / toJSON.

Not sure if this is how it's supposed to work...but haven't had any issues yet. ;-)

@mtwestra
Copy link

@spencer516, have tried it, and it works like a charm. Thanks for the quick reaction!
For local object state this makes a lot of sense: have attributes for things the server cares about, and simple properties for local state, such as 'selected'.

@markprzepiora
Copy link
Contributor

Here is my short-and-sweet solution to this problem. (Sorry about the CoffeeScript)

DS.RESTAdapter.map 'App.Post'
  readOnlyKeys: ['createdAt']

FilteredSerializer = DS.RESTSerializer.extend
  addAttributes: (data, record) ->
    record.eachAttribute ((name, attribute) ->
      isReadOnly = @mappings.get(record.constructor)?['readOnlyKeys']?.indexOf(name) >= 0
      @_addAttribute data, record, name, attribute.type unless isReadOnly
    ), this

App.store = DS.Store.create
  revision: 10
  adapter: DS.RESTAdapter.create
    serializer: FilteredSerializer

In this example, when you commit an App.Post object, the createdAt attribute will be ignored.

The only downside is that you will not be prevented from performing aPost.set('createdAt', ...), but I'm sure this could easily be done as well.

@workmanw
Copy link

workmanw commented Dec 6, 2012

+1 . We've implemented this in our app in a very simular fashion. I'd definitely appreciate it being a native piece of functionality.

I understand the desire to warn users about changing a read-only property, but it would be ideal if we could turn that off conditionally or maybe via an ENV setting. Our use case is we have a noSQL backend. We can't do joins and other things to produce complex counts. So we have stat jobs that de-normalizes this data for us asynchronously. When a count is updated we use PubNub to push that down to our clients. Lastly, the adapter then updates those counts on the effected models. Those count attributes are stored read-only because they would never be sent up to the server from the client.

Blah, that was a long description. Sorry for that, I thought it was necessary to provide a valid use case.

@shilov
Copy link

shilov commented Dec 7, 2012

@markprzepiora your solution worked great, thanks for sharing.

@workmanw I'm in a similar situation as you. Would be nice to have support for this natively.

@bradleypriest
Copy link
Member Author

Here's the slides from the talk I mentioned where Tom mentions readOnly attrs. https://speakerdeck.com/tomdale/30

@tomdale
Copy link
Member

tomdale commented Dec 11, 2012

This is definitely on the roadmap. We want to this feature to have runtime checking, so that changing a read-only attribute immediately raises an exception.

That being said, I would be willing to accept a "lightweight" PR that implements some of the API read-only proposal (without the runtime checking), so long as it was not too intrusive. The API proposal is here: https://gist.github.com/4263171

@Nthalk
Copy link

Nthalk commented Jan 2, 2013

Here is my CoffeeScript fix for this:

# This is oddly an INSTANCE, not an extendable class... Hence, reopen
DS.RESTSerializer.reopen
  #####
  # This filters attributes so that properties with read_only:true options
  # are not sent back on the wire:
  # App.User = DS.Model.extend
  #   created_at: DS.attr('date', read_only:true )
  addAttribute: (hash, record, attributeName, attributeType)->
    unless record.constructor.metaForProperty(attributeName).options.read_only
      @_super hash, record, attributeName, attributeType

@Nthalk
Copy link

Nthalk commented Jan 2, 2013

And if you really care about assertions on setting read only data, then you can add an assert under this existing one:

Ember.assert("You may not set `id` as an attribute on your model. Please remove any lines that look like: `id: DS.attr('<type>')` from " + this.toString(), key !== 'id');
Ember.assert("Cannot set `" + key + "` because it is declared read_only on your model", options.read_only);

Unfortunately, this must be modified within your ember-data.js, because it's pretty much hidden by closures.

@Nthalk
Copy link

Nthalk commented Jan 17, 2013

It is currently impossible for addAttribute to do what I just mentioned because the context getting the property's meta was removed:

https://github.com/emberjs/data/blob/master/packages/ember-data/lib/system/serializer.js#L759

At this point, you'd have to reopen and overwrite _addAttribute.

@stephanos
Copy link

Anyone have a functioning workaround for this?

@bradleypriest
Copy link
Member Author

@stephanos I'm currently using this

Serializer = DS.RESTSerializer.extend
  _addAttribute: (data, record, attributeName, attributeType) ->
    options = Ember.get(record, "constructor.attributes").get(attributeName).options
    if options.readOnly
      return
    else
      key = @_keyForAttributeName(record.constructor, attributeName)
      value = get(record, attributeName)
      @addAttribute(data, key, @serializeValue(value, attributeType))

@kdemanawa
Copy link

@stephanos I override addAttributes:

App.RESTSerializer = DS.RESTSerializer.extend({
 addAttributes: function(data, record) {
    record.eachAttribute(function(name, attribute) {
      if (!attribute.options.readOnly) this._addAttribute(data, record, name, attribute.type);
    }, this);
  }
});

and in my model, App.Post = DS.Model.extend({createdAt: DS.attr('date', {readOnly: true})});

@stephanos
Copy link

Thanks @kdemanawa and @bradleypriest !

@slindberg
Copy link
Contributor

@stephanos This is what the addAttributes hook looks like if you want to use mappings ala @tomdale's proposal:

CustomSerializer = DS.RESTSerializer.extend({
  addAttributes: function(data, record) {
    record.eachAttribute(function(name, attribute) {
      // Skip serializing the attribute if 'readOnly' is true in its mapping
      if (!this.mappingOption(record.constructor, name, 'readOnly')) {
        this._addAttribute(data, record, name, attribute.type);
      }
    }, this);
  }
};

And to add the mapping:

DS.RESTAdapter.map('App.SomeModel', {
  someField: { readOnly: true }
});

@tomdale
Copy link
Member

tomdale commented May 10, 2013

What is the status of this?

@bradleypriest
Copy link
Member Author

@tomdale is https://gist.github.com/tomdale/4263171 still a good doc to use for API preference.
Currently everyone has seems to be using their own custom hacks to stop returning attributes in the JSON, I can't say I've seen any implementations of the other features yet.
I'll start looking at getting this step reimplemented on top of master

@ssoroka
Copy link

ssoroka commented Jun 18, 2013

+1

@stas
Copy link

stas commented Jul 12, 2013

Looking forward to see this merged. Thanks.

@wagenet
Copy link
Member

wagenet commented Aug 9, 2013

@tomdale, @bradleypriest Updates?

@ghost ghost assigned tomdale Aug 9, 2013
@bradleypriest
Copy link
Member Author

Ok, I'm taking a look at this now that 1.0beta is out.

Tom's proposal is obviously quite out-of-date, but I've tried to see how it translates
Correct me if I'm wrong, but it looks like the configure and map APIs have disappeared.

At first glance, there's a couple of basic API choices.

Firstly, an updated version of the proposal.

App.Post = DS.Model.extend({
  published: attr("boolean")
})
App.PostSerializer = DS.JSONSerializer.extend({
  attrs: {
     published: { readOnly: true }
  }
})

This one is based on the commented out tests used for embedded records here. Currently the attrs hash is used for customizing keys, but I haven't seen any documentation about it.

Although this makes sense for serializing, it doesn't really make sense with the warning step. The model would need to ask it's serializer if it can set a value?

Secondly, declaratively on the Model. This allows warning when setting readOnly attributes, but is putting serialization knowledge on the model itself.

App.Post = DS.Model.extend({
  published: attr("boolean", { readOnly: true })
})

I have basic versions of both working locally, would love to get some feedback.

Also @tomdale is all of the previous feature set on the wishlist within the new lighter weight ED?

  1. Read-Only Records
  2. Read-Only Attributes
  3. Read-Only attributes with warning when calling set
  4. Read-Only Associations

@wycats
Copy link
Member

wycats commented Sep 4, 2013

A simpler implementation based on more recent Ember might just be to mark the computed property as .readOnly.

@bradleypriest want to open another PR with that implementation against 1.0?

@wycats wycats closed this Sep 4, 2013
@bradleypriest
Copy link
Member Author

@wycats Will do, however IMO the much more important use-case of readonly attributes is excluding them from the JSON.
Thoughts on adding the config to the model vs the serializer?

@kevinansfield
Copy link
Contributor

Is there a canonical example of excluding attributes from the JSON returned to the server? Perhaps it would make a good cookbook example. I'm finding I have to write a lot of workarounds server-side to prevent mass-assignment errors.

@bradleypriest
Copy link
Member Author

With ED 1.0+ the basic override to exclude from JSON is:

App.ApplicationSerializer = DS.RESTSerializer.extend({
  serializeAttribute: function(record, json, key, attribute) {
    if (!attribute.options.readOnly) {
      return this._super(record, json, key, attribute);
    }
  }
});

App.User = DS.Model.create({
  admin: DS.attr('boolean', { readOnly: true })
})

@mitchlloyd
Copy link
Contributor

@bradleypriest Could we reopen this issue? I was working with someone new to Ember and this was a stumbling point. He needed a way to exclude an attribute from the JSON response as you're describing and overriding a serializer was a little intimidating.

I'd be happy to take a shot at another implementation that works with ED 1.0 beta, but I'm not sure where this functionality should go. Would it be appropriate to include in the serializeAttribute function of the JSONSerializer?

@abarax
Copy link

abarax commented Apr 1, 2014

👍

@workmanw
Copy link

+1 Upgrading my version of ember data and I found this was no longer a feature. Would be nice, and seemingly easy, to add back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ feat This PR introduces a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet