Skip to content
This repository

Single transformation API for both xml and json representations #2498

Closed
dchelimsky opened this Issue · 11 comments

7 participants

David Chelimsky Aaron Patterson Rafael Mendonça França Carlos Antonio da Silva Adam Hawkins Guilherme Simões Dean Brundage
David Chelimsky

I have an app that provides json and xml views of a database table, but with a subset of columns and some pre-processing (different column names and a couple of value transformations). In order to apply the same transformations to the json and xml output, I have to do the same thing twice:

  def as_json(options={})
    super((options || {}).merge(serialization_options))
  end

  def to_xml(options={})
    super((options || {}).merge(serialization_options))
  end

  def serialization_options
    {
     :only => [:first_name, :last_name],
     :methods => [:id, :username]
    }
  end

The fact that we have to use to_xml for xml and as_json for json is confusing, and imposes redundancy.

I propose a new method named serialization_options (as in my example above), which affects both xml and json (and can be used by other serializers like csv, etc).

I realize that there is already serializable_hash, but overriding that only impacts render :json => .... Changing render :xml => ... to use serializable_hash would solve the problem, but it might well break existing apps in the process.

Aaron Patterson
Owner

Yes. I am in favor and will make it so.

Rafael Mendonça França
Owner

Hey @dchelimsky, @tenderlove with options like ActiveModel::Serializers and jbuilder is this issue still applicable?

Thanks

David Chelimsky

@rafaelfranca the purpose of this request is to have one place to declare a conceptual serializable structure that can be used by any representation (json, xml, csv, html). From what I can see both of those only deal with json.

Rafael Mendonça França
Owner

I see. @dchelimsky thank you for the explanation. I agree too. We can make it.

Carlos Antonio da Silva

That sounds really weird, in my mind that was the purpose of serializable_hash, to provide a base structure to be used in any representation. I have to say that I've been using it mainly for json, but as I could see in the XML serializer implementation, it makes use of the serializable_hash method, which in theory should work fine for render :xml. Time to investigate it further in practice :).

Rafael Mendonça França
Owner

I don't know but I think that ActiveModel::Serializers can work with XML too cc/ @josevalim

Adam Hawkins

@rafaelfranca it can, but you have to include a root key otherwise you end up with invalid XML.

Guilherme Simões

As a relatively new Rails developer, I was using serializable_hash expecting it would affect both the xml and json, to no avail.
I ended up here after a few google searches and I am now using dchelimsky's solution. But I think using serializable_hash would make more sense and I too support it.

Dean Brundage

I too would like to see ActiveModel::Serializer render modifications moved over to ActiveModel::Serializers. So much cleaner.

Carlos Antonio da Silva carlosantoniodasilva referenced this issue from a commit
Carlos Antonio da Silva carlosantoniodasilva Fix serializable_hash with xml generation and default :except option
When generating xml with a custom implementation of serializable_hash,
if using the :except option, it was being overriden by the default AR
implementation that attempts to ignore the inheritance column from STI
automatically. So, if you have an implementation like this:

    def serializable_hash(options={})
      super({ except: %w(some_attr) }.merge!(options))
    end

The :except option was correctly being used for :json generation, but
not for :xml, because the options hash already contained the :except
key with the inheritance column, thus overriding the customization.

This commit fixes this problem by removing the :except logic from the
xml serializer, that happened before calling serializable_hash. Since
serializable_hash also does the same check for inheritance column, this
logic was duplicated in both places, thus it's safe to remove it from
xml serializer (see ActiveRecord::Serialization#serializable_hash).

This is an attempt to solve issue #2498, that claims for a
"Single transformation API for both xml and json representations".
8b173f3
Carlos Antonio da Silva

I was taking a look at this issue and confirmed that both json and xml generation actually use serializable_hash as their default implementation to share logic. What I found out, though, is that xml generation within Active Record had an issue with the :except option, related to the automatic removal of the inheritance column (STI type).

So, if you have an implementation like this:

def serializable_hash(options={})
  super({ except: %w(some_attr) }.merge!(options)
end

The :except option was correctly being used for json generation, but not for xml, because the options hash already contained the except key with the inheritance column, thus overriding the customization.

Based on this, I've added some more test coverage around json/xml serialization with serializable_hash in 965b779, and also fixed the aforementioned issue in 8b173f3.

I'd like to know if that was the root cause of the issue, @dchelimsky any thoughts?
Thanks!

Carlos Antonio da Silva

I'm going to mark this issue as closed, since it's been 3 months that I added some comments and there was no feedback so far, and also because I believe the changes introduced at that time fixed the existent problems.

If in some case someone thinks the issue still exists after the changes, please let me know and we can reopen for discussion, and try to fix whatever new related issue exists. Thanks everyone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.