Add json options

[c910335]

1. as: the name to replace
2. filter: a lambda for filtering the value
3. hidden: hiding this field if true

class Post < Granite::ORM::Base
  adapter pg
  field user_id : Int64, json: {as: author_id}
  field title : String, json: {filter: ->(title : String?) { title.try &.upcase }}
  field content : String
  field created_at : Time, json: {hidden: true}
end

post = Post.new(user_id: 910335_i64, title: "Add json options", content: "ry", created_at: Time.now)
post.to_json # => "{\"id\":null,\"author_id\":910335,\"title\":\"ADD JSON OPTIONS\",\"content\":\"ry\"}"

[drujensen]

Nice addition. The change from type to options is very clean and opens up many new possibilities.

I am a little concerned that the model object has more than one responsibility (breaking the Single Responsibility principle). It's only responsibility should be to handle the Object to Relational Mapping. By adding to_json we have added the Serialization concern as well.

However, Rails model objects have to_json that supports only:, except:, and methods:. Adding the as:, filter: and hidden: field options gives us the same flexibility and I find it to be very useful for quick prototyping.

I think the desire by the community is to simplify moving from Rails to Amber, and to that end, I think this change makes sense.

[robacarp]

I share the same concern about keeping the functionality of ORM::Base simple, but wanting to provide a simple way for basic JSON configuration.

A lot of the work here (and in Granite in general) is done in macro-land and I'm not sure it has to be. Muddling this concern so deeply with the field macro feels like it's opening up to some maintainability issues in the future. I have the same concern of addressing too much with the params macro.

The same functionality provided here could be given by splitting up json rendering into two methods: rendered_hash and to_json. ActiveSupport provides similar functionality with as_json and to_json.

Here is a simplified example, some of which is a little psuedocode-ish:

# generated as part of __process_fields
def rendered_fields : JSON::Any
  {
    field_1_name => field_1,
    field_2_name => field_2
  }
end

def to_json(json : JSON::Builder)
  rendered_fields.to_json
end

Then, when a presentation of that model is desired:

# handwritten by dev
def rendered_fields : JSON::Any
  {
    "other_name" => field_1,
    field_2_name => field_2,
    related_object_name => related_object.rendered_fields
  }
end

Dividing the problem like this allows developers to do whatever they want in their override of rendered_fields. They can introduce calculated values, render sub-objects, adhere to JSON-api or other standards, etc. It also allows other serialization formats to be easily hooked into the system, eg xml, yaml, protobuf, etc.

[drujensen]

@robacarp Yes, good point. The field mapping is getting cluttered with too many concerns. The mapping should only focus on the database mapping.

@c910335 What are your thoughts? Do you agree with @robacarp recommended changes?

[drujensen]

@c910335 I would like to keep the type to options change. That will help with supporting default and other ORM specific options.

[c910335]

I started this PR because It is very useful and convenient for Web API developing if Amber has such kind of feature and Granite reaches this nearly.

But I agree that ORM should not do too much work besides database.
Maybe we can open another project to handle this, like grape-entity does.

As for the design of rendered_fields and to_json, in practice, there are only a few fields which need to be changed, I don't want to implement the whole rendered_fields just for a few fields.

[drujensen]

@c910335 I like the presenter or serializer pattern. We use grape-entity at work and I find it provides a pretty clean approach for exposing an API.

To your point, if you are only looking to change a couple fields, I can see overriding rendered_fields requires redefining every field instead of targeting just the ones that need to be changed. When you're trying to quickly prototype a solution, the ability to quickly hide the timestamps or rename a field can be done easily with this change.

However, I can see this being abused and the mapping could look pretty ugly if you add logic to every field.

There are two conflicting goals here. We want to provide an easy prototyping platform like rails provides but we also want to promote good coding practices and design patterns.

@amberframework/core-team WDYT?

[eliasjpr]

Chipping in here. I agree with all above. We should build a presenter layer that will handle ViewModel and Serialization in Amber.

[robacarp]

I’m 👍 for a serializers library.

However, you needn’t rewrite the entire rendered_fields- method to make one change. Call previous_def and mutate the result. I can’t provide a sample now as I’m on mobile.

[Jens0512]

Is there anything apart for the conflicting files, that keeps this PR from being merged?

[robacarp]

@Jens0512 unfortunately, yes. Per the discussion on this pull I think we've decided to go a different direction.