Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support JSON serialize #196

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yhirano55
Copy link
Contributor

@yhirano55 yhirano55 commented Feb 8, 2020

If it use active hash with web api responses, as_json behavior is different between active hash's instance and active model's instance.

  1) ActiveHash Base #as_json returns a hash
     Failure/Error: expect(country.as_json).to eq({"id" => 1, "name" => "US", "language" => "English"})

       expected: {"id"=>1, "name"=>"US", "language"=>"English"}
            got: {"attributes"=>{"id"=>1, "name"=>"US", "language"=>"English"}}

We don't need attributes route key.

If it includes ActiveModel::Serializers::JSON, behavior of active hash's instance would be as same as a behavior active model's instance.

Thanks :)


The reason to need this change

  • When we implement web api with using active hash model and active model serializers gem, we need include ActiveModel::Serializers::JSON module to all active hash's models.
  • It's not a little convenient for us. So I've opened this pull request.

If it use active hash with web api responses, `as_json` behavior is different between active hash's instance and active model's instance.

```
  1) ActiveHash Base #as_json returns a hash
     Failure/Error: expect(country.as_json).to eq({"id" => 1, "name" => "US", "language" => "English"})

       expected: {"id"=>1, "name"=>"US", "language"=>"English"}
            got: {"attributes"=>{"id"=>1, "name"=>"US", "language"=>"English"}}
```

We don't need `attributes` route key.

If it includes `ActiveModel::Serializers::JSON`, behavior of active hash's instance would be as same as a behavior active model's instance.

Thanks :)
@yhirano55 yhirano55 force-pushed the support_json_serialize branch from d393d3f to faa6f84 Compare February 8, 2020 12:05

it "returns a hash" do
country = Country.find(1)
expect(country.as_json.stringify_keys).to eq({"id" => 1, "name" => "US", "language" => "English"})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rails 6.0 or later returns stringified keys hash, but Rails 5.2 or earlier returns symbolized keys hash. So I used stringify_keys method.

@pfeiffer
Copy link
Contributor

pfeiffer commented May 13, 2020

This PR doesn't solve the underlying issue; namely that attributes return a hash with the keys as symbols, while ActiveModel::Serializer explicitly mentions that it expect keys to be strings (docs).

Even with this PR, the as_json doesn't work as expected:

Country.find(1).as_json
=> { id: 1, name: "US", .. }

Country.find(1).as_json(only: [:name])
=> {} # Huh!?

@yhirano55
Copy link
Contributor Author

@pfeiffer Thanks for the comment. Let me check it.

@yhirano55
Copy link
Contributor Author

ActiveHash's attributes are converted to symbolized keys in its constructor.

But in ActiveModel::Serialization#serializable_hash, it compares stringified keys and symbolised keys, so it seems it doesn't work only and except options.

https://github.com/zilkey/active_hash/blob/d2ccb223256b876a69b217ec70f314bdf71d2573/lib/active_hash/base.rb#L404-L411

ActiveModel::Serialization:

https://github.com/rails/rails/blob/ac65e560db598c7a8b1b6aaea1e5656ff3b27f08/activemodel/lib/active_model/serialization.rb#L123-L131

I consider a better solution. If you have an idea, let me know how resolve it. Thanks.

@pfeiffer
Copy link
Contributor

pfeiffer commented May 14, 2020

I think way to solve this is to migrate to having keys as strings in the attributes attribute. This is also how ActiveModel::Attributes and ActiveRecord behaves, and will allow full compatibility with ActiveModel mixins such as AM::Serialization etc.

# ActiveRecord
class User < ApplicationRecord; end

User.find(1).attributes
# => { "id" => 1, "name": "John", ... }

# ActiveModel
class Product
  include ActiveModel::Attributes

  attribute :sku, :string
end

Product.new(sku: "red_shirt").attributes
# => { "sku": "red_shirt" }

It's however a breaking change, since the keys of @attributes are currently forced to be symbols. Personally, I think having keys as strings is the way to go.

If needed, a potential migration path would be to store keys as strings with stringify_keys!, but still allow access to the attributes via symbol, but with a deprecation warning before dropping support for symbols as key in next major version, but this would probably require that @zilkey signs off on this migration plan.

What do you and @zilkey think?

@zilkey
Copy link
Collaborator

zilkey commented May 14, 2020

What about sort of a parallel track?

  • we push a version (maybe 3.1.1?) that deprecates symbol keys, but maintains 100% compatibility (still has symbols / still has attributes)
  • we push a new version 4.0.0 that makes all the breaking changes:
    • stores attributes as strings
    • returns attributes as strings
    • removes the attributes key from as_json

We'd created a 3.x.x branch here, and can release changes on both for a little while. Any reason to not do this?

@pfeiffer
Copy link
Contributor

What about sort of a parallel track?

Sounds like a great plan! Happy to contribute if needed :-)

@kbrock
Copy link
Collaborator

kbrock commented Aug 22, 2022

Looking back over this.

I think you can manually add this to each of your models to gain this functionality.

include ActiveModel::Serializers::JSON

Looks like this lost steam. I'm not sure if there is desire to get this in and beak compatibility.

@kbrock
Copy link
Collaborator

kbrock commented Aug 22, 2022

kicking this build

@kbrock kbrock closed this Aug 22, 2022
@kbrock kbrock reopened this Aug 22, 2022
@pfeiffer
Copy link
Contributor

pfeiffer commented Oct 4, 2022

Looking back over this.

I think you can manually add this to each of your models to gain this functionality.

include ActiveModel::Serializers::JSON

Looks like this lost steam. I'm not sure if there is desire to get this in and beak compatibility.

No the issue would still be here. The issue is that ActiveHash stores the attributes with symbols as keys, causing :only and :except options passed to as_json to not work, as it compares strings to symbols.

@kbrock
Copy link
Collaborator

kbrock commented Mar 9, 2023

@pfeiffer ugh. you had already clearly stated the problem. sorry about that.

Are the 2 branches going to be that much different?
Do you think it will be possibly to backport the same PRs to both branches while 2 live in parallel?

@kbrock
Copy link
Collaborator

kbrock commented Jul 14, 2024

@pfeiffer Sorry for lag on this one. (going over unresolved PRs)

So if we want this fixed we need to break compatibility and store strings as keys?
We just merged #312 which made sure all attributes were consistent - symbols for now, which seemed to be the way it was expected to work.

But looking at #310, it seems like at least to_json and column_names wants String names.

Just thinking out loud:

And visiting the source for as_json, it almost feels like we want to alias field to attribute and rearrange our internals to leverage attributes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants