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

[issue-48] Ability to serialize a hash object #50

Merged
merged 1 commit into from
Jul 31, 2019
Merged

[issue-48] Ability to serialize a hash object #50

merged 1 commit into from
Jul 31, 2019

Conversation

mikebaldry
Copy link
Contributor

If a Hash is passed as object, it will attempt to find attributes using Hash#[].

PankoSerializer must at some point turn attributes :blah from a Symbol to a String, as it only works with String keys. I think this is good enough as it'd be hard to detect which to use, and might cause confusion.

I attempted to make it work with ActiveSupport::HashWithIndifferentAccess but was unable to get the hash lookup working in ext

    it "hash (with indifferent access)" do
      foo = ActiveSupport::HashWithIndifferentAccess.new(
        "name" => Faker::Lorem.word,
        "address" => Faker::Lorem.word
      )

      expect(foo).to serialized_as(FooSerializer,
                                   "name" => foo[:name],
                                   "address" => foo[:address])
    end
void hash_attributes_writer(VALUE obj, VALUE attributes,
                             EachAttributeFunc func, VALUE writer) {
  long i;
  for (i = 0; i < RARRAY_LEN(attributes); i++) {
    volatile VALUE raw_attribute = RARRAY_AREF(attributes, i);
    Attribute attribute = attribute_read(raw_attribute);
    volatile VALUE value = rb_funcall(obj, rb_intern("[]"), 1, attribute->name_id);

    func(writer, attr_name_for_serialization(attribute), value);
  }
}

I expect rb_hash_lookup is faster than rb_funcall with [] but that might bypass overridden [] method on Hash subclasses like ActiveSupport::HashWithIndifferentAccess. Either way, it works for a plain Hash but not for ActiveSupport::HashWithIndifferentAccess and I can't figure out why (my ext experience is limited)

Fixes #48

@mikebaldry
Copy link
Contributor Author

Also I've seen that rb_hash_aref is used to access Hash value. That doesn't have any different result.

Copy link
Owner

@yosiat yosiat left a comment

Choose a reason for hiding this comment

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

@mikebaldry thanks for the PR!

Supporting other types of hash is possible, but it's complicated because of the reason you posted.

If you are ok with your result, I'll merge this.

@mikebaldry
Copy link
Contributor Author

I'm happy with this. It could be improved upon in the future without breaking backwards compatibility.

Thanks

@mikebaldry
Copy link
Contributor Author

This actually did not work at all - my test was incorrect, I had it working at some point but then with the trying to get it working with different types of hash, I broke it but the tests still passed.

I was asserting that serialized values were equal to hash[:blah] where the hash had string keys, so it was returning nil and therefore the tests passed because the code in hash.c was incorrectly returning nil.

Once I realised my mistake I was able to track down the reason it wasn't working and make it work with HashWithIndifferentAccess also.

This is now ready to merge :)

@yosiat yosiat merged commit b501b46 into yosiat:master Jul 31, 2019
@yosiat
Copy link
Owner

yosiat commented Jul 31, 2019

@mikebaldry merged, will release a version later this week.

@mikebaldry mikebaldry deleted the issue-48/serialize-a-hash branch July 31, 2019 11:07
@mikebaldry
Copy link
Contributor Author

Great, thanks!

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.

Using with a Hash object?
2 participants