Continuation of #1217: Support creating polymorphic has-many relationships #1231

st0012 · 2019-03-15T16:33:07Z

This is a continuation of #1217, which fixes its failing tests (thanks for @saravanak's hard work!).

The cause of #1217's failure is data mutation between test cases: the integration test's post request creates a new record, and it fails other tests (under certain execution order). So my additional commit is to keep the database clean (with database_cleaner before/after each integration test suite.

All Submissions:

I've checked to ensure there aren't other open Pull Requests for the same update/change.
I've submitted a ticket for my issue if one did not already exist.
My submission passes all tests. (Please run the full test suite locally to cut down on noise from travis failures.)
I've used Github auto-closing keywords in the commit message or the description.
I've added/updated tests for this change.

New Feature Submissions:

I've submitted an issue that describes this feature, and received the go ahead from the maintainers.
My submission includes new tests.
My submission maintains compliance with JSON:API.

Bug fixes and Changes to Core Features:

I've included an explanation of what the changes do and why I'd like you to include them.
I've provided test(s) that fails without the change.

Test Plan:

Reviewer Checklist:

Maintains compliance with JSON:API
Adequate test coverage exists to prevent regressions

If we create/modify/delete records (via requests) in integration tests, it might affect other tests' test result. The ideal way is to keep integration test data isolated. And by adding database_cleaner, we can make sure database is reset between each test cases.

lgebhardt

Thanks @saravanak and @st0012 This is a good start, but it's not thoroughly tested and further work is needed downstream in the Resource for creating the relationship links.

lib/jsonapi/request_parser.rb

lgebhardt · 2019-03-17T19:04:22Z

test/integration/requests/request_test.rb

+
+    assert_jsonapi_response 400, msg: "Submitting a thing as a vehicle should raise a type mismatch error"
+  end
+


Let's test cases where multiple vehicles are provided. Tests should include mismatches of types, such as

'relationships' => { 'vehicles' => {'data' => [{'type' => 'car', 'id' => '1'}, {'type' => 'car', 'id' => '2'}]}, #vehicle 2 is actually a boat }

Test should probably be repeated for updates, though as of now they should go through the same path.

test/integration/requests/request_test.rb

We do this in following steps 1) Get the sub-resource's model class with `Resource.model_hint`'s info 2) Query the records with subclass instead of polymorphic class 3) Return 404 if we get any record not found error The reason to raise RecordNotFound instead of TypeMismatch is that type mismatch means requires 2 conditions: - the id can't be used to find any record in current subclass (Car) - but in the meantime, it does exists for the superclass (Vehicle) And this means we need to perform additional query to prove the second condition, which is kind of a waste.

lib/jsonapi/resource.rb

st0012 · 2019-03-18T10:16:36Z

lib/jsonapi/resource.rb

+        begin
+          related_records = relationship_klass.find(relationship_key_values[:ids])
+        rescue ActiveRecord::RecordNotFound
+          fail JSONAPI::Exceptions::RecordNotFound.new(relationship_key_values[:ids])


The reason to raise RecordNotFound instead of TypeMismatch is that type mismatch requires 2 conditions:
- the id can't be used to find any record in current subclass (Car)
- but in the meantime, it does exists for the superclass (Vehicle)
And this means we need to perform additional query to prove the second condition, which is kind of a waste.

I tend to agree it's kind of a waste, but from a JSON API perspective it's not known that the backing table is sharing a common set of IDs. So there really isn't a Car with id 2 so the system should raise an error.

Let's use find_by_keys, like https://github.com/cerebris/jsonapi-resources/blob/master/lib/jsonapi/resource.rb#L251. That way any overrides will get applied. This might be the case if people are doing permission checking for these related resources.

We need to use Resource.records as our query base to invoke user customizations. And because the ivar inheritance, we need to manually call model_name on subclasses to make this work.

st0012 · 2019-03-18T14:14:43Z

lib/jsonapi/resource.rb

+        ids = relationship_key_values[:ids]
+
+        related_records = relationship_resource_klass
+          .records


@lgebhardt for supporting customizations, I think using records is enough. Use find_by_keys will also run includes for this query, which is unncessary.

I missed this earlier: records needs to be called as records(options)

st0012 · 2019-03-19T03:14:32Z

@lgebhardt added tests for update, is there anything missing for merging?

lgebhardt

@st0012 There are a few test failures masked by using assert instead of assert_equal.

lgebhardt · 2019-03-19T13:28:14Z

lib/jsonapi/resource.rb

+        ids = relationship_key_values[:ids]
+
+        related_records = relationship_resource_klass
+          .records


I missed this earlier: records needs to be called as records(options)

lgebhardt · 2019-03-19T13:30:51Z

lib/jsonapi/resource.rb

+          .records
+          .where({relationship_resource_klass._primary_key => ids})
+
+        missed_ids = ids - related_records.map(&:id)


We need to account for alternate primary keys, and I think we can skip creating the models so pluck will save a few cycles.

missed_ids = ids - related_records.pluck(relationship_resource_klass._primary_key)

lgebhardt · 2019-03-19T13:51:50Z

test/integration/requests/request_test.rb

+    person = Person.find(body.dig("data", "id"))
+
+    assert "Reo", person.name
+    assert 2, person.vehicles.count


These asserts should be assert_equal. Once fixed this test is failing with only the last vehicle type being created.

ah sorry 😅

st0012 · 2019-03-19T15:39:44Z

@lgebhardt all updated!

closes gh-1231 closes gh-1217

lgebhardt · 2019-03-19T18:57:27Z

@st0012 Thanks for working on this. I squashed your changes and pushed them to release-0-9

saravanak and others added 2 commits January 28, 2019 13:18

Support fetching polymorphic has-many relationships

b8f3948

st0012 changed the title ~~Fix creating polymorphic has many relations~~ Continuation of #1217: Support creating polymorphic has-many relationships Mar 15, 2019

lgebhardt requested changes Mar 17, 2019

View reviewed changes

st0012 added 3 commits March 18, 2019 15:00

Improve request test cases.

d0f7a29

Refactor polymorphic has many link creation.

ea85dda

st0012 commented Mar 18, 2019

View reviewed changes

Improve _replace_to_many_links' polymorphic check

b0fe00b

We need to use Resource.records as our query base to invoke user customizations. And because the ivar inheritance, we need to manually call model_name on subclasses to make this work.

st0012 commented Mar 18, 2019

View reviewed changes

Add test cases for update

3d4e41a

lgebhardt requested changes Mar 19, 2019

View reviewed changes

Correct test cases and fixed implementation.

2081af6

st0012 force-pushed the fix_creating_polymorphic_has_many_relations branch from a0ec607 to 2081af6 Compare March 19, 2019 15:26

Refactor RequestParser#parse_to_many_relationship

53b4026

lgebhardt pushed a commit that referenced this pull request Mar 19, 2019

Refactor polymorphic has many link creation

f6e3183

closes gh-1231 closes gh-1217

lgebhardt closed this Mar 19, 2019

st0012 mentioned this pull request Mar 20, 2019

Fix creating polymorphic has many relations #1233

Closed

12 tasks

lgebhardt mentioned this pull request Mar 22, 2019

Reset _model_class cache when calling model_name #1237

Closed

9 tasks


		assert_jsonapi_response 400, msg: "Submitting a thing as a vehicle should raise a type mismatch error"
		end

Continuation of #1217: Support creating polymorphic has-many relationships #1231

Continuation of #1217: Support creating polymorphic has-many relationships #1231

Uh oh!

Conversation

st0012 commented Mar 15, 2019

All Submissions:

New Feature Submissions:

Bug fixes and Changes to Core Features:

Test Plan:

Reviewer Checklist:

Uh oh!

lgebhardt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

st0012 commented Mar 19, 2019

Uh oh!

lgebhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

st0012 commented Mar 19, 2019

Uh oh!

lgebhardt commented Mar 19, 2019

Uh oh!

Uh oh!

lgebhardt left a comment •

edited

Loading