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.

Sign up for GitHub

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

Jump to bottom

Introduce ActiveModel::NestedAttributes #49637

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Introduce ActiveModel::NestedAttributes #49637

wants to merge 1 commit into from
+1,169 −0

Conversation

seanpdoyle
Copy link
Contributor

Motivation / Background

Largely inspired by the Active Record implementation, this commit introduces support for assigning instances of objects to classes that include ActiveModel::Model.

Detail

It aims to handle assignment of _attributes-suffixed values in an ActionView- and ActionPack-compliant way:

Author = Struct.new(:name)
Category = Struct.new(:name)

class Article
  include ActiveModel::Model
  include ActiveModel::NestedAttributes

  attr_accessor :author
  attr_accessor :tags

  accepts_nested_attributes_for :author
  accepts_nested_attributes_for :tags, class_name: Category
end

article = Article.new(
  author_attributes: { name: "Pseudo Nym" },
  tags_attributes: {
      0 => { name: "actionview" },
      1 => { name: "actionpack" },
  }
)

article.author.name # => "Pseudo Nym"
article.tags.pluck(:name) # => ["actionview", "actionpack"]

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@rails-bot rails-bot bot added the docs label Oct 14, 2023
@seanpdoyle seanpdoyle force-pushed the am-anaf branch 2 times, most recently from 6bd7f9f to 2140b20 Compare October 14, 2023 15:55
@seanpdoyle
Copy link
Contributor Author

@rafaelfranca this proposal is made in the same spirit as work like #49534. It aims to better integrate Active Mode with Action Pack and Action View in familiar and intuitive ways.

@seanpdoyle seanpdoyle mentioned this pull request Oct 14, 2023
Closed
@seanpdoyle seanpdoyle force-pushed the am-anaf branch 2 times, most recently from b833645 to 8dd3be8 Compare October 18, 2023 16:35
@rafaelfranca
Copy link
Member

I think this one makes sense, but we implemented a different API for it #44324.

I think we can build this one on top of #44324, so I'll add it to the list of PRs I need to review related to Active Model attributes.

@seanpdoyle
Copy link
Contributor Author

seanpdoyle commented Oct 18, 2023 •
edited
Loading

@rafaelfranca I'll subscribe to #44324.

I had considered adding test coverage for attribute :has_many_model, since it's possible to infer the class_name from the Attribute type, but wasn't sure about the direction Core was planning on taking Active Model Attributes. It's public API in that API Rdoc pages exist for the module, but its support level isn't clear from the documentation itself and its limited number of mentions in the Guides.

Are ActiveModel::Attributes public, but ActiveModel::Type and ActiveModel::Type.register private?

I'm happy to adjust the current implementation to respond to changes in #44324, but I think it's worth emphasizing that the current approach works with attr_accessor :has_many_model, in addition to attribute :has_many_model.

I think a final version of this code could support both, I just want to emphasize that attr_accessor support feels important to maintain.

@mansakondo
Copy link
Contributor

@seanpdoyle I opened this PR two years ago to solve the same problem. I'm curious to know what you think.

p8
p8 reviewed Nov 6, 2023
View reviewed changes
guides/source/configuring.md Outdated Show resolved Hide resolved
@seanpdoyle seanpdoyle force-pushed the am-anaf branch 2 times, most recently from 195909e to 9943b8a Compare November 6, 2023 16:16
seanpdoyle
seanpdoyle commented Nov 6, 2023
View reviewed changes
activemodel/lib/active_model/nested_attributes.rb
Comment on lines +295 to +315
# === Integration with ActiveModel::Attributes
#
# When attributes are declared with the +.attribute+ class method provided
# by the Attributes module, support
# for nested attributes will construct instances using available
# Type information. For example, the +accepts_nested_attributes_for+
# declarations in the following code sample will use the Type that corresponds
# to +:user+ when assigning to +author_attributes+ and the Type that
# correpsonds to +:tags+ when assigning to +tags_attributes+:
#
# class Article
# include ActiveModel::Model
# include ActiveModel::Attributes
# include ActiveModel::NestedAttributes
#
# attribute :author, :user
# attribute :tags, :tag
#
# accepts_nested_attributes_for :author
# accepts_nested_attributes_for :tags
# end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rafaelfranca in reference to #49637 (comment):

Does this style of definition match what you have in mind?

The implementation builds on top of #49910 (authored by @jonathanhefner and @p8) to first try utilizing defined Attributes before making any kind of inference to classes to build from Hashes:

@seanpdoyle seanpdoyle force-pushed the am-anaf branch 2 times, most recently from ef4db5c to 05bfcf0 Compare November 6, 2023 19:04
@rafaelfranca rafaelfranca mentioned this pull request Jan 3, 2024
Open
8 tasks
@seanpdoyle seanpdoyle force-pushed the am-anaf branch 3 times, most recently from c271fbe to 5b420d2 Compare March 8, 2024 13:31
@seanpdoyle
Introduce ActiveModel::NestedAttributes
d368653 
Largely inspired by the Active Record implementation, this commit
introduces support for assigning instances of objects to classes that
`include ActiveModel::Model`.

It aims to handle assignment of `_attributes`-suffixed values in an
ActionView- and ActionPack-compliant way:

```ruby
Author = Struct.new(:name)
Category = Struct.new(:name)

class Article
  include ActiveModel::Model
  include ActiveModel::NestedAttributes

  attr_accessor :author
  attr_accessor :tags

  accepts_nested_attributes_for :author
  accepts_nested_attributes_for :tags, class_name: Category
end

article = Article.new(
  author_attributes: { name: "Pseudo Nym" },
  tags_attributes: {
      0 => { name: "actionview" },
      1 => { name: "actionpack" },
  }
)

article.author.name # => "Pseudo Nym"
article.tags.pluck(:name) # => ["actionview", "actionpack"]
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@p8 p8 p8 left review comments

Assignees
No one assigned
Labels
activemodel activerecord docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@seanpdoyle @rafaelfranca @mansakondo @p8