Custom Nested AttrJson::Models Do Not Get Cast/Serialized Correctly when Reading from .attributes/.as_json/.serializable_hash

[bbarberBPL]

The Following Test Case Shows that there is an issue when reading the attributes when AttrJson:Model is nested inside other AttrJson::Model objects.

Note I used the faker gem to generate the test data for this case

_testcase.rb

require 'faker'
class Product
  include AttrJson::Model
  attr_json :name, :string, default: Faker::Commerce.unique.product_name
  attr_json :cost, :string, default: Faker::Commerce.price(range: 25..100, as_string: true)
  attr_json :material, :string, default: Faker::Commerce.material
  attr_json :color, :string, default: Faker::Commerce.color
end

class Brand
  include AttrJson::Model
  def self.build_test
    new(products: Array.new(5) { Product.new } )
  end
  attr_json :name, :string, default: Faker::Appliance.unique.brand
  attr_json :products, Product.to_type, array: true, default: []
end

class Department
  include AttrJson::Model

  def self.build_test
    new(brands: Array.new(5) { Brand.build_test } )
  end

  attr_json :name, :string, default: Faker::Commerce.unique.department
  attr_json :brands, Brand.to_type, array: true, default: []
end

dep = Department.build_test

dep.attributes
#=>
# {
#     "brands" => [
#         [0] #<Brand:0x00007fdc5a55d190 @attributes={"products"=>[#<Product:0x00007fdc5a55edd8 @attributes={"name"=>"G
# orgeous Granite Coat", "cost"=>"61.00", "material"=>"Wool", "color"=>"magenta"}>, #<Product:0x00007fdc5a55e6f8 @attri
# butes={"name"=>"Gorgeous Granite Coat", "cost"=>"61.00", "material"=>"Wool", "color"=>"magenta"}>, #<Product:0x00007f
# dc5a55e1a8 @attributes={"name"=>"Gorgeous Granite Coat", "cost"=>"61.00", "material"=>"Wool", "color"=>"magenta"}>, #
# <Product:0x00007fdc5a55dc58 @attributes={"name"=>"Gorgeous Granite Coat", "cost"=>"61.00", "material"=>"Wool", "color
# "=>"magenta"}>, #<Product:0x00007fdc5a55d708 @attributes={"name"=>"Gorgeous Granite Coat", "cost"=>"61.00", "material
# "=>"Wool", "color"=>"magenta"}>], "name"=>"KitchenAid"}>,
#         [1] #<Brand:0x00007fdc5ae0e280 @attributes={"products"=>[#<Product:0x00007fdc5a55cd30 @attributes={"name"=>"G
#
# orgeous Granite Coat", "cost"=>"61.00", "material"=>"Wool", "color"=>"magenta"}>, #<Product:0x00007fdc5a55c7e0 @attri
# butes={"name"=>"Gorgeous Granite Coat", "cost"=>"61.00", "material"=>"Wool", "color"=>"magenta"}>, #<Product:0x00007f
# dc5a55c290 @attributes={"name"=>"Gorgeous Granite Coat", "cost"=>"61.00", "material"=>"Wool", "color"=>"magenta"}>, #
# <Product:0x00007fdc5ae0f978 @attributes={"name"=>"Gorgeous Granite Coat", "cost"=>"61.00", "material"=>"Wool", "color
# "=>"magenta"}>, #<Product:0x00007fdc5ae0ee10 @attributes={"name"=>"Gorgeous Granite Coat", "cost"=>"61.00", "material
# "=>"Wool", "color"=>"magenta"}>], "name"=>"KitchenAid"}>,
#         [2] #<Brand:0x00007fdc5a563540 @attributes={"products"=>[#<Product:0x00007fdc5ae0dd58 @attributes={"name"=>"G
# orgeous Granite Coat", "cost"=>"61.00", "material"=>"Wool", "color"=>"magenta"}>, #<Product:0x00007fdc5ae0d330 @attri
# butes={"name"=>"Gorgeous Granite Coat", "cost"=>"61.00", "material"=>"Wool", "color"=>"magenta"}>, #<Product:0x00007f
# dc5ae0c958 @attributes={"name"=>"Gorgeous Granite Coat", "cost"=>"61.00", "material"=>"Wool", "color"=>"magenta"}>, #
# <Product:0x00007fdc5ae0c0e8 @attributes={"name"=>"Gorgeous Granite Coat", "cost"=>"61.00", "material"=>"Wool", "color
# "=>"magenta"}>, #<Product:0x00007fdc5a563ab8 @attributes={"name"=>"Gorgeous Granite Coat", "cost"=>"61.00", "material
# "=>"Wool", "color"=>"magenta"}>], "name"=>"KitchenAid"}>,
#         [3] #<Brand:0x00007fdc5a561768 @attributes={"products"=>[#<Product:0x00007fdc5a563220 @attributes={"name"=>"G
# orgeous Granite Coat", "cost"=>"61.00", "material"=>"Wool", "color"=>"magenta"}>, #<Product:0x00007fdc5a562cd0 @attri
# butes={"name"=>"Gorgeous Granite Coat", "cost"=>"61.00", "material"=>"Wool", "color"=>"magenta"}>, #<Product:0x00007f
# dc5a562780 @attributes={"name"=>"Gorgeous Granite Coat", "cost"=>"61.00", "material"=>"Wool", "color"=>"magenta"}>, #
# <Product:0x00007fdc5a562230 @attributes={"name"=>"Gorgeous Granite Coat", "cost"=>"61.00", "material"=>"Wool", "color
# "=>"magenta"}>, #<Product:0x00007fdc5a561ce0 @attributes={"name"=>"Gorgeous Granite Coat", "cost"=>"61.00", "material
# "=>"Wool", "color"=>"magenta"}>], "name"=>"KitchenAid"}>,
#         [4] #<Brand:0x00007fdc5ae03218 @attributes={"products"=>[#<Product:0x00007fdc5a561448 @attributes={"name"=>"G
# orgeous Granite Coat", "cost"=>"61.00", "material"=>"Wool", "color"=>"magenta"}>, #<Product:0x00007fdc5a560ef8 @attri
# butes={"name"=>"Gorgeous Granite Coat", "cost"=>"61.00", "material"=>"Wool", "color"=>"magenta"}>, #<Product:0x00007f
# dc5a5609a8 @attributes={"name"=>"Gorgeous Granite Coat", "cost"=>"61.00", "material"=>"Wool", "color"=>"magenta"}>, #
# <Product:0x00007fdc5a560458 @attributes={"name"=>"Gorgeous Granite Coat", "cost"=>"61.00", "material"=>"Wool", "color
# "=>"magenta"}>, #<Product:0x00007fdc5ae03e98 @attributes={"name"=>"Gorgeous Granite Coat", "cost"=>"61.00", "material
# "=>"Wool", "color"=>"magenta"}>], "name"=>"KitchenAid"}>
#     ],
#       "name" => "Shoes, Games & Toys"
# }

By Itself this doesn't seem like an issue(as_json and serializable_hash work perfectly fine on this case). However when adding these models to a json_column(Like the products table in the test app) with a container attribute like so

class Store < ActiveRecord::Base
  include AttrJson::Record

  self.table_name = "products"

  attr_json :departments, Department.to_type, container_attribute: :other_attributes, array: true, default: []
end

store = Store.new(departments: Array.new(3) { Department.build_test })

Then running either

store.as_json or store.serializable_hash

The output gets further messed up

#=>
# .... "other_attributes" => {
#          "departments" => [
              # [0] #<Department:0x00007f3221c22518 @attributes={"brands"=>[#<Brand:0x00007f3221c1fe30 @attributes={"products"=>[#<Produc
# t:0x00007f32229232d0 @attributes={"name"=>"Synergistic Linen Pants", "cost"=>"64.00", "material"=>"Cotton", "color"=>
# "blue"}>, #<Product:0x00007f3222922678 @attributes={"name"=>"Synergistic Linen Pants", "cost"=>"64.00", "material"=>"
# Cotton", "color"=>"blue"}>, #<Product:0x00007f3222921c78 @attributes={"name"=>"Synergistic Linen Pants", "cost"=>"64.
#]
#
# }```

Also doing this without `array: true` produces similar results. 

[jrochkind]

Thanks for report! Will try to look into this next week.

[jrochkind]

Is the problem just to manual/direct use of as_json or serializable_hash, the models actually work right as far as saving and fetching all data to/from the db?

Can you describe the nature of the problem in words -- I'm having trouble interpreting the pastes. Is it that you get the Model object (not serialized to json) in the output? Or do you actually get a string that looks like #<Department:0x00007f etc?

[bbarberBPL]

Its the former. Anytime you call those methods that hashify the top level object it doen't work recursively on nested models that are inside of a json column. If you call those on just the vanilla rails models ie department.as_json or department.serializable_hash it works recursively. It seems redundant in my use case to have to to check the value of something twice ie(if its a Hash or if its type is a kind_of AttrJson::Model).

It relates to a serializer I'm making for that project we discussed earlier. If there was a way to call store.departments with some arbitrary option or even just a reader method like department_attributes and have it return as a Hash or a collection of hashes that would make it easier in my case.

[jrochkind]

Sorry I confused myself, I'm not sure which "the former" is!

I think you're saying everything saves and fetches properly as ActiveRecord, but does not return what you want when you call as_json and serializable_hash directly yourself.

And I think you're saying that the manner in which it doesn't work how you want is that the hashes include ruby model objects, when you want them to include just "json-ish" data, hashes of hashes and arrays and primitives.

Do I have it right?

If so -- what if you call #to_json? I think you're supposed to get a serialized JSON string -- does it have what you want correctly? I know this is hacky and inefficient and not what you want, but both as a possible workaround and just to understand what's going on better: If you did JSON.parse(store.to_json), do you get what you want?

(I'm off tomorrow, but will try to get back to this next week)

[bbarberBPL]

Sorry I confused myself, I'm not sure which "the former" is!

"Is it that you get the Model object (not serialized to json) in the output? " - quoted from the above comment. This is what I meant by the former sorry for the confusion

I think you're saying everything saves and fetches properly as ActiveRecord, but does not return what you want when you call as_json and serializable_hash directly yourself.

Exactly.

And I think you're saying that the manner in which it doesn't work how you want is that the hashes include ruby model objects, when you want them to include just "json-ish" data, hashes of hashes and arrays and primitives.

Do I have it right?

Yes that's the gist of it. I would take a look at this post to give you some ideas on how to fix this. Take a look at the gems mentioned too at the end. Specifcally store_model. I'm not saying you should overhaul attr_json to be like these implementations(Like including the ActiveModel::Attributes API) but going through the code in store_model may help give you ideas on how to refactor/fix the AttrJson::Model module to fix - "the manner in which it doesn't work how you want is that the hashes include ruby model objects, when you want them to include just "json-ish" data, hashes of hashes and arrays and primitives."

If so -- what if you call #to_json? I think you're supposed to get a serialized JSON string -- does it have what you want correctly? I know this is hacky and inefficient and not what you want, but both as a possible workaround and just to understand what's going on better: If you did JSON.parse(store.to_json), do you get what you want?

Yes. Also works doing that with the oj gem via Oj.load(store.to_json) Using it with oj probably won't be as inefficient, but more redundant.

(I'm off tomorrow, but will try to get back to this next week)

Let me know if you still have any questions, or are still confused in anyway. I hope I was able to clarify everything.

[jrochkind]

Cool, I think I understand, thanks! I'll think on it and look at the code.

One of the design decisions that makes attr_json different was the goal that you should be able to edit and read via the actual attribute OR via the the underlying serialized json column, and either one would work fine. If you edit via the json_attributes attribute in your ActiveRecord, it is immediately seen by access via a "cover" attribute, and vice versa, and either one is serialized properly.

That may have been an unnecessary requirement/goal (not sure?), but I think it's what leads to this weird behavior, and different implementation than some other things. That the underlying storage is always in the Hash object ActiveRecord is using for the underlying JSON column, even for the attributes.

We should hopefully be able to fix this weird behavior somehow anyway though.

We actually DO use the ActiveModel::Attributes api in attr_json, in a bunch of ways -- it is definitely what makes possible this implementation. However, I started this in Rails 5.0-5.1 days, when the Attributes API wasn't as developed as it became in Rails 5.2 -- it possibly would/could have been done differently with full Rails 5.2 Attributes API and no need to support earlier Rails, the idea of taking another look is tracked in #18

[jrochkind]

@bbarberBPL

Trying to reproduce, I've found a few things:

1. I believe I've confirmed that as a workaround JSON.parse(store.to_json) works. I know that's inefficient and ugly, but if you need to get going with your code in the meantime, recommend using that as a hopefully temporary workaround.

2. However, actually, I am only able to reproduce with store.serializable_hash. I can see the problem you report with store.serializable_hash -- however, store.as_json seems to be different, and works as expected for me. Can you double check and confirm whether you are indeed seeing different behavior? If this is so, does it work for you just to use #as_json instead of #serializable_hash?

Here's my adaption of your test code committed to a branch of attr_json with tests in rspec verifying behavior, submitted as a PR just to make it easier to track and view.

https://github.com/jrochkind/attr_json/pull/80

Thanks for trying out attr_json, for the error report, and for your help tracking it down!

[jrochkind]

@bbarberBPL Here's an analagous example from stock Rails features, that leads me to believe present behavior is correct and as expected and consistent with Rails.

Create a Rails model with a postgres "decimal" column.

    create_table :widgets do |t|
      t.decimal :some_decimal
    end

That turns to a ruby BigDecimal on the ruby side.

In the serializable_hash output, the value is still a ruby BigDecimal, although in the as_json it turns into a String.

irb(main):007:0> widget = Widget.new(some_decimal: "4.99")
=> #<Widget id: nil, title: nil, some_decimal: 0.499e1, created_at: nil, updated_at: nil>
irb(main):008:0> widget.some_decimal.class
=> BigDecimal
irb(main):009:0> widget.serializable_hash
=> {"id"=>nil, "title"=>nil, "some_decimal"=>0.499e1, "created_at"=>nil, "updated_at"=>nil}
irb(main):012:0> widget.serializable_hash["some_decimal"].class
=> BigDecimal
irb(main):013:0> widget.as_json
=> {"id"=>nil, "title"=>nil, "some_decimal"=>"4.99", "created_at"=>nil, "updated_at"=>nil}
irb(main):015:0> widget.as_json["some_decimal"].class
=> String

This leads to me to conclude that specialized ruby domain objects (BigDecimal, a custom AttrJson::Model) are expected and appropriate in serializable_hash values; but not in as_json values.

As this is the behavior I am seeing in attr_json, it leads me to conclude attr_json is behaving as expected.

Do you have a read, @bbarberBPL ?

(If we/you did need to change this behavior as reported at top, I think I found it would be necessary to override read_attribute_for_serialization in the ActiveRecord::Model, to recognize attributes that were attr_json wrappers, and handle them differently by calling as_json on super. However, I don't believe this behavior is actually expected or consistent with stock ActiveRecord).

[bbarberBPL]

Hey @jrochkind thanks for looking into that. Sounds like a good solution overriding the read_attribute_for_serialization method. I was thinking overriding read_attribute_for_serialization might be a possible work around as well. Now I think I have a good idea of how to implement that override. If you don't think this is an issue with the gem could you by chance add something to the documentation for using nested attr_json models with json serializers ? I can give you my work around for read_attribute_for_serialization to add to the docs if you want.

[jrochkind]

@bbarberBPL

So, I'm not sure we're understanding each other properly.

You originally reported that both store.as_json and store.serializable_hash were not as you wanted.

However, I could only reproduce with serializable_hash, as_json seems to already be as you want. Can you confirm this please?

Also, my investigations of how Rails handles analogous things with serializable_hash without attr_json lead me to believe that what you are asking for in inappropriate for serializable_hash -- even without attr_json, Rails/ActiveRecord puts "domain objects" in serializable_hash -- like BigDecimal -- so I don't think it's appropriate to ask that to be any different for attr_json. I am curious if your understanding is differnet here, and how?

If it were necessary to change the behavior of serializable_hash for AttrJson::Model to be consistent with how Rails handles analagous things, I'd want attr_json itself to over-ride serializable_hash, I wouldn't make the developer-user do it. But since as far as I can tell what you are asking for is actually inconsistent behavior for serializable_hash, inconsistent with how stock Rails handles analagous things, and I don't understand why someone would want it, I don't see a reason for attr_json to document the behavior, it does not seem to me to be a common use case, or something attr_json should recommend.

What am I missing?

Sorry if I'm still being confusing, happy to talk about this further on Slack or a phone call or whatever.

[jrochkind]

I see you say "for use with JSON serializers" -- I guess you have a particular JSON serializer library for which this is causing a problem? i don't think it can possibly be universal for all "JSON serializers"? I have definitely serialized things to JSON using the current code. I wonder how your JSON serializer library would handle the BigDecimal case I demo'd above?

Is there a reason you can't use the as_json instead of serializable_hash? Can you actually call as_json on the results yourself? store.as_json, or even store.serializable_hash["other_attributres"].as_json should all get you what you want.

It's not clear to me that changing #serializable_hash to work differently than it does in stock Rails for analagous things (like BigDecimal) is the right solution, or one we'd want to recommend.

[bbarberBPL]

Hey @jrochkind sorry for the long delay but I made a spec for you that shows what is happening better. You should be able to put this under the spec folder. I also used Faker to generate dummy test data so just add

gem 'faker' to the Gemfile and require 'faker' to spec_helper.rb

One thing to note is when I ran it with the rails 6 appraisal via bundle exec appraisal rails-6-0 rspec spec/record_with_nested_models_spec.rb the spec passed. But using appraisal with 5-2 fails as expected. So it appears that the newer version of rails takes care of the problem.(See comments in Spec for specifics). That being said Upgrading to rails 6 seems like kind of havey undertaking for us at the moment and was wondering if there was a way you could mimic the rails 6 way of doing as_json in classes that include AttrJson::Record. Let me know what you think, in terms of solutions/ moving forward, once you have reviewed the specs.

record_with_nested_models_spec.rb.txt

[jrochkind]

Aha, thanks for identifying you were seeing differences between Rails 5.2 and 6.0. That could explain the confusion, I may have been testing in Rails 6.0.

It may take me some time to find time to look into this myself, but I want to get to it! Please ping me again if you haven't heard back in a while. I want to find some solution for you one way or another, I think we should be able to find a reasonable one that doesn't require you to upgrade to rails 6.0. (although it's not a terrible idea to upgrade to 6.0 :) )

[bbarberBPL]

I definitely agree that updating to Rails 6 is a good idea, but we're not quite there yet in terms of our project at the BPL. That being said I think I have a way to help give you a head start on this. I made the following snippet in this file on the line highlighted https://github.com/jrochkind/attr_json/blob/9577153ae37faeeb4dbca1ba36bb89a4418121ed/lib/attr_json/record.rb#L28

if Gem.loaded_specs['activerecord'].version.release < Gem::Version.new('6.0')

      def serializable_hash(options = {})
        hash = super(options)
        self.class.attr_json_registry.definitions.each do |attr_def|
          container_attribute, store_key = attr_def.container_attribute, attr_def.store_key
          next unless hash.key?(container_attribute) && hash.fetch(container_attribute, {}).key?(store_key)

          if attr_def.array_type?
            hash[container_attribute][store_key] = hash[container_attribute][store_key].map(&:serializable_hash)
          else
            hash[container_attribute][store_key] = hash[container_attribute][store_key].serializable_hash
          end
        end
        hash
      end

 end

 protected
  ...

I tested this out with the playground models in the console with appraisal set to rails-5-2 and it serialized the objects correctly for both single items (ie Product.find(1).as_json) and a collection of items(ie Product.all.as_json). Take your time working out for the solution for this issue. Hope this helps

[jrochkind]

I have reproduced your observation -- your test fails on rails 5.2 but not rails 6.0 for me too.

This is all so confusing. Your test is confusing to me, I don't understand your test setup or what you are testing. I don't understand why you are taking the results of serializable_hash and injecting things into it :(

I still believe that serializable_hash should not be changed -- Rails does not treat serializable_hash as you expect for it's own functionality, I don't believe you should expect serializable_hash to have already seriazed values intead of model objects, as Rails doesn't do this for it's own embedded-model functionality. I think it would be a mistake to change serializable_hash as you are asking. I think any software that is expecting it to is mistaken.

However, as_json should work as you are asking, and does appear to be a bug in Rails 5.2.

But... If you have a solution that works for you, what do you think about just monkey-patching it into your app locally for you? And leaving attr_json alone, since it appears to behave as expected on Rails 6.0.

[jrochkind]

OK, actually, here's a patch to just #as_json on just AttrJson::Record that makes your test pass!

Put this in like an initializer or something....

  class AsJsonNormalizer
    # just recursively calls #as_json on all values, to normalize
    # objects to as_json, to work around an apparent bug in Rails pre-6.0
    # #as_json , where it isn't calling as_json on all the values from serializable_hash. 
    def self.normalize_json(obj)
      case obj
      when Hash
        obj.transform_values { |v| normalize_json(v) }
      when Array
        obj.collect { |v| normalize_json(v) }
      else
        obj.respond_to?(:as_json) ? obj.as_json : obj
      end
    end
  end

  module AttrJson::Record
    def as_json(*args, **kwargs)
      AsJsonNormalizer.normalize_json(super)
    end
  end

I believe it makes your test pass! Can you see how it works for you?

You could just do it locally in your app. Or, I suppose, we could put it in attr_json, just used when Rails < 6.

It would be interesting to find the Rails commit or PR that changed/fixed this behavior in Rails 6.0, for some context on what's going on, and if it was done on purpose to fix something considered a bug. But my sense is what this is doing is working around a bug in Rails 5.2, fixed in Rails 6.0.

[jrochkind]

Oh, I better understand your patch to serializable_hash now too... it's possible that is an alternate (better?) fix for what I still believe is a Rails 5.2 bug too. Kind of similar to what i'm doing, but doing it in seriazable_hash instead of as_json.

there might be a better approach to the serializable_hash override if it is needed there though, maybe one with less code, that is more careful about not calling serializable_hash unless the method is available.

Trying to apply your serializable_hash patch though.. it actually BREAKS your test. I think because of a bug in your test. But it makes it hard for me to test if it succeeded.

After applying your serializable_hash patch, and then running your own test, I get:

     Failure/Error: vals[json_key] = val[json_key].map(&:serializable_hash)

     NoMethodError:
       undefined method `serializable_hash' for #<Hash:0x00007f98c05b9900>
     # ./spec/record_with_nested_models_spec.rb:74:in `map'
     # ./spec/record_with_nested_models_spec.rb:74:in `block (4 le

Is this what's happening for you? The goal of a test should be to demonstrate the broken behavior by failing, but then pass when the behavior is fixed. So this does not demonstrate that with your serializable_hash patch; my as_json patch does make your test go green.

[bbarberBPL]

Yes it was happening to me also. However, I needed to update the let!(:expected_hash) stub in the spec in order for it to work with both versions considering I was using serializable_hash to stub out the expected_hash before I applied my patch. If you change let!(expected_hash) to the following code it works on 5.2 and 6.

let!(:expected_hash) do
    recurse_hash_blk = ->(attributes_hash){
      attributes_hash.inject({}) do |ret, (attr, val)|
        case val
        when Hash
          ret.merge(attr => recurse_hash_blk.call(val))
        when Array
          ret_vals = val.map do |v|
            next v if v === Hash

            recurse_hash_blk.call(v.attributes)
          end
          ret.merge(attr => val.map {|v| recurse_hash_blk.call(v.respond_to?(:attributes) ? v.attributes : v) })
        else
          ret.merge(attr => val)
        end
      end
    }

    recurse_hash_blk.call(store.attributes)
  end

Since you pointed out that this is a bug in any version less than rails 6 with serializable hash you should consider adding my patch to attr_json. Even if we create an issue for the bug with serializable_hash, who knows if or when the Rails team will address it. Seeing how they are focusing their efforts on Rails 6 now I have my doubts they will fix it. You can make any modifications that make my patch "more careful about not calling serializable_hash unless the method is available" and with "less code". Otherwise I can add my patch or your patch(depending on how either of them perform) to the gem we have been working on that is using attr_json.

Ping me if there is still any confusion on slack and we can set up a call.

[jrochkind]

Hi @bbarberBPL, I'm sorry to have left this for so long.

I'm reluctant to introduce any code complexity to attr_json to work around a Rails bug which has already been fixed in recent versions of Rails -- especially when the code complexity will be there for even recent versions of rails, and perpetually too.

I'm just aware of the challenge of keeping what is already a somewhat complex codebase sustainably maintainable.

However, I also understand that your use case not working in older versions of Rails is a problem it would be good to solve!

I think ideally we'd provide a way for you to fix this locally just in your app; or to have attr_json fix it by changing code only in cases of old Rails versions, with code that's clearly fenced off for that, so can be removed later when we stop supporting those old Rails versions. Did you ever try the local patch I suggested? I'd be very interested to hear if that works for you, if you're willing to try it -- this would be super helpful feedback.

I'm going to try to look at this a bit more today, but it's confusing and my brain no longer remembers where it was at in January, so I might not make much progress.

If you still want to work on this (and understand if you don't), can you submit a PR that has:

1. In one commit, a test that fails in the current codebase. Ideally as simple pared-down example as possible to demontrate the thing considered broken with a failing test.
2. In a second commit, your proposed fix, that makes the test now pass. This second commit should not change the test added in (1) above, we should have a test that fails without the change, and passes with the change.

If as a result of discussion we change our mind about things or need to change them, then you can actually rebase/ammend the branch in the PR, to still have 1 and 2 as above with the new approach. We need to maintain a branch that has exactly two commits separated as above, to keep this comprehensible as we discuss it, I think.

That would make it a lot easier to investigate and understand and suggest changes or refactors, as well as to get one's head back into it if one forgets what's going on and wants to come back ot it like now. What we have here, a bunch of comments with code examples inline and as links as part of an evolving discussion, takes a lot more mental effort to figure out what's going on, and too easy for us not to be talking about the same thing.

Thanks for your interest and contributions!

[bbarberBPL]

Hey Jonathan. Sorry for not keeping in touch with you regarding this! After Code4lib with the covid situation things got hectic. Good news though we actually decided recently that we are going to update the gem to use rails 6 and above. This is more because ActiveStorage is more streamlined for our gems use case but this will also resolve the current reported issue with attr_json at the same time. So we can go ahead and close this out. Thank you again for your patience and support!

[jrochkind]

Now worries @bbarberBPL , I thought it was me who dropped it! These are tough times.

Now that I've got my head back in it... I actually kind of want to get it solved. At least for as_json , which I believe is broken in Rails 5.2 but not 6.0.

My understanding is still that serialized_hash is not actually broken, and if something is expecting serialized_hash to be different, that that thing with the expectations is broken. (And I can't promise it won't be under rails 6.0 too...)

I may get something in anyway, at least fixing as_json in Rails 5.2.. but may not.

[jrochkind]

Here's the commit that changed/fixed as_json in Rails 6.0: https://github.com/rails/rails/commit/2e5cb980a448e7f4ab00df6e9ad4c1cc456616aa#diff-531b52ff1432c7c9f9472188b39d6a43R96

Before as_json returned serializable_hash (with options), but it's changed to return serializable_hash.as_json -- this makes sure any values that were ruby objects in the results of serializable_hash get converted into json-compatible data structures. The commit was done for 'timestamps'; would also apply to BigDecimal as noted; also ends up applying to our custom internal models.

Which actually points out an even simpler implementation to make things work:

  module AttrJson::Record
    def as_json(*args, **kwargs)
      super.as_json
    end
  end

You could patch that into AttrJson::Record as above -- or you could just define that as_json on your relevant ActiveRecord models, and it would fix as_json.

I'm not totally sure this will be enough to make your ActiveModelSerializer case work -- I'm not sure what ActiveModelSerializer is doing. But I'd be inclined to call it a bug in ActiveModelSerializer if it doesn't.

If you really wanted (or needed) to change serializable_hash in this way too -- although I think it's inconsistent with what Rails normally does, so could have unexpected performance or other implications -- you could do something very similiar with serializable_hash:

  def serializable_hash(*args, **kwargs)
    super.as_json
  end

I would consider that working around a bug in ActiveModelSerializers, but it is an option.

This is easy to do without AttrJson actually supporting it, so a change in AttrJson may not be required, even if you do need to go down this route!