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

[Fix #51463] Raise an error when invalid :on or :except options are given to #resource or #resources #51464

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

Conversation

joshuay03
Copy link
Contributor

@joshuay03 joshuay03 commented Apr 2, 2024

Motivation / Background

Fixes #51463.

Detail

Raises an ArgumentError when a ActionDispatch::Routing::Mapper::Resources::Resource is initalized with :on or :except options that aren't default (index, create, new, show, update, and destroy) actions .

Additional information

See my comment below as to why this seems like more changes than it needs to be.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Unrelated changes 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 actionpack label Apr 2, 2024
@joshuay03 joshuay03 changed the title [Fix #51463] Raise an error when invalid :on or :except options are given to #resource or #resouces [Fix #51463] Raise an error when invalid :on or :except options are given to #resource or #resouces Apr 2, 2024
@joshuay03 joshuay03 force-pushed the raise-error-on-invalid-resources-on-or-except-args branch 2 times, most recently from f0fb46c to ad8ac7b Compare April 2, 2024 04:34
@joshuay03 joshuay03 force-pushed the raise-error-on-invalid-resources-on-or-except-args branch 2 times, most recently from 475d14a to 8b6140c Compare April 2, 2024 04:42
@joshuay03 joshuay03 changed the title [Fix #51463] Raise an error when invalid :on or :except options are given to #resource or #resouces [Fix #51463] Raise an error when invalid :on or :except options are given to #resource or #resources Apr 2, 2024
@joshuay03 joshuay03 force-pushed the raise-error-on-invalid-resources-on-or-except-args branch 3 times, most recently from 4a6ac93 to fc2da04 Compare April 2, 2024 05:37
@joshuay03
Copy link
Contributor Author

joshuay03 commented Apr 2, 2024

Hmm the test failures are scenarios where the scope's :only or :except includes :index which is then merged into the a singleton resource's options. I think a good solution is to only merge in scoped actions that are applicable to the resource type.

Addressed in these changes.

…ons are given to `#resource` or `#resources`
@joshuay03 joshuay03 force-pushed the raise-error-on-invalid-resources-on-or-except-args branch from fc2da04 to a0d7574 Compare April 2, 2024 06:28
attr_reader :controller, :path, :param

def initialize(entities, api_only, shallow, options = {})
if options[:param].to_s.include?(":")
raise ArgumentError, ":param option can't contain colons"
end

if (invalid_actions = (options.values_at(:only, :except).flatten.compact - default_actions)).any?
raise ArgumentError, "expected :only and :except to be one or more of #{default_actions}, got #{invalid_actions}"
end
Copy link
Contributor

Choose a reason for hiding this comment

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

The method assert_valid_keys would be a better approach as it's used elsewhere within Rails to perform the same task, e.g.

def to_sentence(array, options = {})
options.assert_valid_keys(:words_connector, :two_words_connector, :last_word_connector, :locale)
default_connectors = {
words_connector: ", ",
two_words_connector: " and ",
last_word_connector: ", and "
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it works here since we're asserting the values and not the keys.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants