codenoble / cache-crispies

Speedy Rails JSON serialization with built-in caching
MIT License
156 stars 14 forks source link

Add :optional parameter to attributes #22

Closed floriandejonckheere closed 3 years ago

floriandejonckheere commented 4 years ago

Add an :optional condition to attributes, that serializes the attribute only if it is included in the cache_render call. It also supports a wildcard inclusion (include: '*').

Currently, the included attributes match just by name, so nested objects do not try and compare the attribute path (object.subobject.attribute). This feature could be expanded to implement this as well.

class CerialSerializer < CacheCrispies::Base
  key :food

  serialize :uid, from: :id, to: String
  serialize :name, optional: true
end
# Will not include :name
CerialSerializer.new(Cereal.first)

# Will include :name
CerialSerializer.new(Cereal.first, include: :name)

# Could also include client-defined params in controller (might be unsafe though)
CerialSerializer.new(Cereal.first, include: params[:include])
adamcrown commented 4 years ago

Hi @floriandejonckheere. Thanks for the good work here. Sorry it's taken me a little while to get to this. Life's been crazy lately as you can imagine.

A few thoughts about this.

First of all, I'm wondering if this is just another way to do what the show_if block already provides. I can see that this is a bit more structured way of handling this, but that also makes it less flexible. Could you explain how you see this fitting in?

Secondly, since the include: option should really always affect what data is rendered in a predictable way, I'd like to see those values added to the cache key to avoid issues with cached data for being accidentally reused with the wrong information included or excluded.

Thanks again for the PR, I do appreciate it. Please do let me know what you think about the above two points.

floriandejonckheere commented 4 years ago

Well, it's indeed syntactic sugar on what you can do with show_if. But since I'd see this pattern repeated many times in my code, I thought I'd abstract it and make it easier to use in the future.

It also mimics the include feature of activemodel-serializers, but allowing for more control on the level of serializers.

+1 for adding it to the cache key, I didn't think about that. I'll get on it this week or beginning of next.

codeclimate[bot] commented 3 years ago

Code Climate has analyzed commit e43e562b and detected 0 issues on this pull request.

View more on Code Climate.

adamcrown commented 3 years ago

Sorry again for the very late response. I'm going to try to do a better job staying on top of this project. Especially as we're starting to use it more with my 9-5 job.

I think this looks good now with the latest changes. Thanks!

adamcrown commented 3 years ago

FYI, I just pushed version 1.3.0 which includes this change.