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

Allow has_one nil association #48

Merged
merged 6 commits into from Feb 7, 2018
Merged

Allow has_one nil association #48

merged 6 commits into from Feb 7, 2018

Conversation

jeremyjung
Copy link
Contributor

Attempt to fix issue #35 .

Any feedback or suggestions on where to add a test would be appreciated. I created a sample rails app to verify functionality.

@grossadamm
Copy link
Contributor

Thanks @jeremyjung! If you could, could you add some tests? For guidance, I would suggest at least adding a test to object_serializer_spec

@jeremyjung
Copy link
Contributor Author

The existing movie context (Movie, Actor, etc.) doesn’t have any has_one relations in it. I wasn’t sure if I should modify the existing models in there or create new models specifically to test has_one.

@shishirmk
Copy link
Collaborator

shishirmk commented Feb 7, 2018

@jeremyjung @grossadamm This issue could be fixed by doing a nil check in line 12 of this file https://github.com/Netflix/fast_jsonapi/blob/master/lib/extensions/has_one.rb

Something like this

association(:#{name}).reader&.id

@jeremyjung
Copy link
Contributor Author

jeremyjung commented Feb 7, 2018

@shishirmk Good call with the dig method. I'm having a little trouble figuring out the appropriate tests for this.

Putting this test in object_serializer_spec.rb doesn't interact with ActiveRecord and so it never touches the has_one extension method.

    it 'returns correct json when has_one returns nil' do
      supplier.account_id = nil
      json = SupplierSerializer.new(supplier).serialized_json
      serializable_hash = JSON.parse(json)
      expect(serializable_hash['data']['relationships']['account']['data']).to be nil
    end

Another possibility could be just testing the extension method in the active_record_spec.rb to make sure it returns nil instead of raising an exception:

    it 'has account_id method return nil if not present' do
      expect(Supplier.find(@supplier_id_without_account).account_id).to eq nil
    end

In addition, any thoughts on removing the has_one extension method completely? Without the extension method I think we could still get similar behavior by using:

    public_send(:object_method_name)&.id

@shishirmk
Copy link
Collaborator

@jeremyjung I think the test can go to active_record_spec.rb. The test you have in the comment looks good.

Throughout the gem we try to support serialization with just the id_method_name so that we dont have to create an object unnecessarily when we call object_method_name. It is also really helpful when we just have the id in our database and the record is in another micro service. In such cases we can serialize the record by avoiding a network call. Hope that explains why we have chosen to use this approach.

@shishirmk
Copy link
Collaborator

The &. Operator is available only ruby 2.3 onwards. That's why the builds are failing on 2.2.* version.

Do you mind using something more verbose?

@shishirmk
Copy link
Collaborator

@jeremyjung Thank you for making all the necessary changes

@shishirmk shishirmk merged commit e7caa7a into Netflix:dev Feb 7, 2018
andyjeffries pushed a commit to andyjeffries/fast_jsonapi that referenced this pull request May 25, 2018
* Allow has_one nil association

* add test for when has_one returns nil

* modify has_one extension method, add active record test

* Use try operator to support old rubies
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.

None yet

3 participants