AR Serialization shouldn't fall back to base opts for includes #1344
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.
It seems wrong that when I do the following:
that
:only => :first_name
will apply to to:posts
as well.The present behavior has been around essentially forever, and the current nuances of the behavior arrived in two phases, apparently initially to preserve backward compatibility when it became possible to specify opts for includes in the first place, and then when app developers got the ability to override to_xml on their models.
As far as I can tell it was just initially decided that passing the base's options along to the include is better than having the included model be serialized in an unconstrained way, but the remote likelihood that the same column names are likely to usefully exist on the two models makes me think that's worth reconsidering.
The current behavior isn't covered by tests, so I'm hoping nobody is intentionally relying on it. That said, because this patch is less restrictive than the old approach, this could cause security issues for anybody who was relying on it unwittingly (included models without explicit options hashes would start dumping all attributes out over JSON and XML REST APIs instead of the seemingly random intersection of their column and method names with those from the base opts). That may make this pull request a no go until AR (or really, AM) serialization has a mandatory security layer analogous to
attr_accessible
(mandatory may be far-fetched given the cool reception to mandatoryattr_accessible
, but I'd be in favor of it), or this behavior change comes with a configuration option that must be turned on consciously by an upgrading app maintainer in order to take effect.I'm working on a generic AM serialization security layer for a project (and this patch is a prerequisite for it, in fact -- otherwise it's challenging to keep opts on base from trickling down to included models). If there's interest in pulling that into core, I'd be happy to clean it up and submit. Especially if it would make this patch more palatable.
Thanks for considering.