yosiat / panko_serializer

High Performance JSON Serialization for ActiveRecord & Ruby Objects
https://panko.dev
MIT License
577 stars 35 forks source link

Aliases for custom method #66

Open xchi opened 4 years ago

xchi commented 4 years ago

We want to implement a custom method with payload key name as metricValues, but the proper ruby method name should be snake_case.

class MetricPeriodSerializer < Panko::Serializer
        attributes :period, :metricValues

        def metricValues
           [{
            'name' => 'metric1',    
            'value' => object.metric_1  
          },    
           {    
             'name' => 'metric2',   
             'value' => object.metric_2 
           }]
        end

end

We have tried the following way, but having no luck. Just wondering does panko support alias for custom method by any chance?

class MetricPeriodSerializer < Panko::Serializer
        attributes :period, :metricValues
        aliases metric_values: :metricValues

        def metric_values
           [{
            'name' => 'metric1',    
            'value' => object.metric_1  
          },    
           {    
             'name' => 'metric2',   
             'value' => object.metric_2 
           }]
        end

end
yosiat commented 4 years ago

Hi @xchi !

Looks like what you need is support for casing, so you can write something like this:

class MetricPeriodSerializer < Panko::Serializer
  attributes :period, :metric_values, :first_name

  def metric_values
      [{
      'name' => 'metric1',  
      'value' => object.metric_1    
    },  
      { 
        'name' => 'metric2',    
        'value' => object.metric_2  
      }]
  end
end

and the output of the keys will be period, metricValues and firstName.

Now the only question is how we want to support it, currently, Panko don't have global configuration and I want to delay it.

So we can have: specify it on the serializer level like attributes or when serializing. I prefer on the serializer level.

If this makes sense to you, let me know and I'll implement it.

xchi commented 4 years ago

Hey @yosiat ,

It will be very helpful if you can support casing for our project, as we basically need to create aliases for almost all fields we want to parse.

On the serializer level as one attribute is fine for us as well. Thanks a lot for your replying.

Vivian

yosiat commented 4 years ago

@xchi let's make sure the api makes sense to you and I'll implement it.

class MetricPeriodSerializer < Panko::Serializer
  keys_format :camel_case

  attributes :period, :metric_values, :first_name

  def metric_values
      [{
      'name' => 'metric1',  
      'value' => object.metric_1    
    },  
      { 
        'name' => 'metric2',    
        'value' => object.metric_2  
      }]
  end
end

We will start by defining the serializer class and maybe in the future we will create a global configuration. Until then, you can create a base class for serializer (like ApplicationSerializer) which will have only the key_format definition.

mikebaldry commented 4 years ago

In my mind, the simplest and most obvious way to achieve this is to just not use snake_case in your serializers. No other code should be referencing the attribute methods in your serializer, and I think the clearest possible way of doing it, is just using the attribute names as you expect them to come out after serialization.

Also I think global configuration should not be a thing. It's just another name for a global variable that affects the result of something that was pure before.

Using a base serializer should be all that is needed to support that.

Side note @yosiat: I'd like to pick this feature up, if you disagree with my first statement and think it has value.

yosiat commented 4 years ago

@mikebaldry about global configuration, I totally agree with you.

About specifying the names of the attributes as snake case - this can be really problematic for a developer - database column names are not snake case and linters will yell that methods are not snake case.

In addition to that, think about serializer that is used for external API and some specific integration needs to be output as snake case with a subset of fields. In today's case, you need another serializer for this, in ideal place - you will re-use the same serializers and pass options for filters and key format.

I am ok with you taking this feature up (and really happy with that)! but let's make sure we are aligned on the public API for panko and let's get @xchi on board with us.

mikebaldry commented 4 years ago

Sure, I see your points :)

Happy to implement like the example above and we can go from there?

yosiat commented 4 years ago

@mikebaldry cool, let's go for it!

as how I see the implementation, there is no need to change something in the C-extension part (except aliases I think, need to test it). since each attribute have name_sym and name_str , the name_sym used for accessing the data and name_str for the key - see this function - https://github.com/yosiat/panko_serializer/blob/master/ext/panko_serializer/attributes_writer/common.c#L3

Please make sure that your PR includes testing for - attributes (both methods and simple attributes), aliases and associations (both has_one and has_many) and docs. If you want me to join and help with docs & tests, let me know.

mikebaldry commented 4 years ago

I've had a little first attempt working.. https://github.com/yosiat/panko_serializer/commit/9d352a45aab5e0fce5091d7b7b267c4436059fc5

I have a few things:

Many thanks

xchi commented 4 years ago

Hey @mikebaldry ,

I had look the implementation and it seems can fix our problem. The only question I had is whether you need to provide all support for key_transformer, or this one is going to be an user defined thing?

mikebaldry commented 4 years ago

@xchi I don't know yet. Waiting on @yosiat for guidance :)

yosiat commented 4 years ago

@mikebaldry hi! sorry, another long work-week :)

If you can make a PR and it will be simpler to discuss on the code.

Some notes:

yosiat commented 4 years ago

@mikebaldry hi! any update on this? can I help you?

oyeanuj commented 4 years ago

@yosiat Just adding +1 to the camelize support, that would be super helpful!

xchi commented 4 years ago

Just want to check any update to this one?