Allow relationship ID to be overwritten for belongs_to relationships #2128
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bf4
reviewed
May 11, 2017
test/fixtures/poro.rb
Outdated
| @@ -146,6 +146,9 @@ class CustomBlogSerializer < ActiveModel::Serializer | |||
| attribute :special_attribute | |||
| has_many :articles | |||
| end | |||
| class ExternalBlog < Blog | |||
| attributes :external_id | |||
| end | |||
There was a problem hiding this comment.
This can go in the test/serializers/associations_test.rb. (This file is big enough :) )
|
This looks fine to me. I don't mind hiding the object behind the serializer a little more :) Just the one change |
If the `id` attribute for a class isn't taken directly from the object when
serializing it, it may be desirible for other classes that serialize a
relationship with that class to overwrite the relationship IDs they serialize.
For example, suppose we have:
```(ruby)
class Repo < Model
attributes :id, :github_id, :name
associations :configs
end
class Config < Model
attributes :id
belongs_to :repo
end
class RepoSerializer < ActiveModel::Serializer
attributes :id, :name
has_many :update_configs
def id
object.github_id
end
end
class ConfigSerializer < ActiveModel::Serializer
attributes :id
belongs_to :repo
end
```
In the above example, serializing a list of `Repo`s will give the `github_id`
for each one, but serializing a `Config` will give the `id` for its parent repo.
Ideally AMS would inspect the `RepoSerializer` when serializing the `Config`,
and realise it can't just output the foreign key. Unfortunately, getting the
serialization class for the child repo currently requires loading the record
(via evaluating `lazy_assocation`), and loses the performance benefit of the
existing `belongs_to?` path. Instead, I've opted to use
`read_attribute_for_serialization` instead of `object.send` to fetch the
serialized foreign key. This allows the serialized relationship ID to be
overwritten using
```(ruby)
class ConfigSerializer < ActiveModel::Serializer
...
def repo_id
object.repo.github_id
end
end
```
|
Would mind closing this PR and opening a new one against 0-10-stable? per #2121 |
|
Done. #2130. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
If the
idattribute for a class isn't taken directly from the object when serializing it, it may be desirable for other classes that serialize a relationship with that class to overwrite the relationship IDs they serialize.For example, suppose we have:
In the above example, serializing a list of
Repos will give thegithub_idfor each one, but serializing aConfigwill give theidfor its parent repo.Changes
I've updated the
belongs_to?path ofRelationship#data_for_oneto useread_attribute_for_serializationinstead ofobject.sendto fetch the serialized foreign key. This allows the serialized relationship ID to beoverwritten using:
Caveats
Ideally AMS would inspect the
RepoSerializerwhen serializing theConfig, and realise it can't just output the foreign key. Unfortunately, getting the serialization class for the child repo currently requires loading the record (via evaluatinglazy_assocation), and loses the performance benefit of the existingbelongs_to?path.Related GitHub issues
#2125. (Not resolved by this, but the ID issue is at least improved, for no performance cost.)
Additional helpful information