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

Use validates_with rather than validates to validate URI #1662

Merged
merged 2 commits into from Jun 22, 2023
Merged

Use validates_with rather than validates to validate URI #1662

merged 2 commits into from Jun 22, 2023

Conversation

ravron
Copy link
Contributor

@ravron ravron commented Jun 20, 2023

Summary

Today my project upgraded from Doorkeeper 5.3.2 to 5.6.6. In the process, we started encountering errors like the following:

ArgumentError: Unknown validator: 'Doorkeeper::RedirectURIValidator'

After much investigating, we determined that we have an inflection configured in a Rails initializer for the string URI:

ActiveSupport::Inflector.inflections(:en) do |inflect|
  inflect.acronym 'URI' # Uniform Resource Identifier
end

This causes ActiveModel to attempt to load Doorkeeper::RedirectURIValidator rather than the correct Doorkeeper::RedirectUriValidator, here. The fix I'm suggesting in this PR is to avoid ActiveModel's attempt to guess the proper constant name, and instead use the constant directly by providing it to validates_with, which is what validates does anyways. I'm absolutely open to other suggestions on how to make this more resistant to custom inflection configurations.

ActiveModel's validates method uses string manipulation, in particular
camelize, to identify the class constant to be used for validation. If
inflections have been modified or added, it may attempt to load a class
that does not exist. By using validates_with and passing the exact class
to be used, we can avoid this issue.
@ravron ravron marked this pull request as ready for review June 20, 2023 23:56
Copy link
Member

@nbulaj nbulaj left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!
🙇

@nbulaj nbulaj merged commit b3d6787 into doorkeeper-gem:main Jun 22, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants