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

Ability to specify a subset of properties for JSON representation. #264

Conversation

paulyoung
Copy link
Contributor

This pull request allows people to specify which properties are included in the JSON representation of their models on a per class basis.

This is different from mapping a property to NSNull.null since it still allows the values of JSON key paths to be represented in model properties, but not the other way around.

It does not currently provide the ability to specify which keys should be included on a per serialization basis (the use case being a HTTP PATCH request), but these changes could possibly be leveraged in providing support for that.

As discussed with @robb in #185 - I have a similar implementation which was achieved by extending MTLJSONSerializing and MTLJSONAdapter, and thought others might benefit from it being part of Mantle proper.

My use case was that I wanted to create a JSON representation of a newly created model so that I could send it in a POST request. Certain keys are managed by the server and therefore shouldn't have been present in a request. They were however expected to be present in a response, then their values assigned to properties of a model.

Implementing this new (optional) protocol method causes the specified
properties to be omitted when creating a JSON representation of a
model.

Includes:
* Header documentation describing usage.
* A spec which was written before the implementation.
* A new MTLTestModel subclass which supports the spec and demonstrates
  usage.
* An implementation which causes the spec to pass.
@paulyoung
Copy link
Contributor Author

I should add that I have been questioning whether or not excluding properties like this is an anti-pattern, and would love this PR to be as much a discussion about that.

I still think that excluding properties to support a PATCH request is valid.

@robb
Copy link
Member

robb commented Mar 9, 2014

I should add that I have been questioning whether or not excluding properties like this is an anti-pattern, and would love this PR to be as much a discussion about that.

With this change set, once you opt into +propertyKeysForJSONRepresentation you'll always deserialize an object from JSON using mapping A, while serializing it to JSON with mapping B.

It seems to me that in the majority of use-cases, clients would like to chose when to update a subset of keys and when to use the full mapping.

I still think that excluding properties to support a PATCH request is valid.

I totally agree.

@paulyoung
Copy link
Contributor Author

FWIW, I removed the changes I described above from my own project for the reasons we discussed.

I plan to provide something related to the PATCH request use case instead.

@jspahrsummers
Copy link
Member

Could we kill two 🐦s with one stone here, and make this an instance method? Then instances can determine when certain properties should be serialized.

@dcaunt
Copy link
Member

dcaunt commented Mar 13, 2014

I think this makes a lot more sense, especially with PATCH requests in
mind. At the moment we have the adapter support from #185 which requires
external knowledge of modified properties, and this new proposed protocol
which I don't think is flexible enough (being per class).

On Thursday, 13 March 2014, Justin Spahr-Summers notifications@github.com
wrote:

Could we kill two [image: 🐦]s with one stone here, and make this an
instance method? Then instances can determine when certain properties
should be serialized.


Reply to this email directly or view it on GitHubhttps://github.com//pull/264#issuecomment-37580115
.

@robb
Copy link
Member

robb commented Mar 14, 2014

My gut feeling is that this should be a concern of the adapter, since choosing a subset of keys seems dependent on the context of the serialization.

For example, the same model object may be

  • written to a disk cache with all its property keys, including derived but expensive data
  • be POSTed to the server only with the keys that server knows about
  • be PATCHed with an even smaller subset that could be determined in multiple ways (attributes changed since updatedAt for example).

@robb robb closed this Mar 14, 2014
@robb
Copy link
Member

robb commented Mar 14, 2014

whoops, hit close by accident. Apologies.

@robb robb reopened this Mar 14, 2014
@jspahrsummers
Copy link
Member

@robb That's a fair point. Should we close this out and focus more on that approach, then?

@robb
Copy link
Member

robb commented Mar 14, 2014

👌

@paulyoung feel free to close this if you don't want to continue from this branch.

@paulyoung
Copy link
Contributor Author

I'll probably continue from here.

@ryang1428
Copy link

Any updates on this? pre 2.0 I could just override dictionaryValue & remove the properties I did not want to pass up to the server. Updating to 2.0, this does not work. Looking through the docs I could not find a way to do this now. Any help? Thanks

@jspahrsummers
Copy link
Member

@ryang1428 You can use -serializablePropertyKeys:forModel: to implement that sort of behavior.

It looks like this PR has been resolved by the introduction of that method, too, so I'll close it out.

@jgrandelli
Copy link

@jspahrsummers I just added a new issue (#511) to talk about this some. Using serializablePropertyKeys:forModel: only handles some of the use cases. One that it can't handle at all is stripping (or transforming) null values.

Also, it feels pretty heavy handed to override the transformer just to remove a single key or handful of keys when there is already a subclass for that model that has a readymade method for manipulating the dictionary representation of the model. Given this update and change in functionality, what purpose does dictionaryValue: have as a overridable method?

I would love to continue the discussion on this but I think at bare minimum this should be mentioned in the change log. This is absolutely a breaking change.

@jspahrsummers
Copy link
Member

@jgrandelli I'll have to defer to @robb on most of the design decision questions, if for no other reason than I can't really recall the reasoning right now.

I agree this should be reflected in the changelog—its omission was just an oversight. Would you be willing to open a pull request to add it? 🙏

@jgrandelli
Copy link

Thanks @jspahrsummers. I'd be happy to open a PR for the changelog pending the outcome of #512.

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

Successfully merging this pull request may close these issues.

None yet

6 participants