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 format lookup when JSON or XML are defined as acronyms #361

Closed
wants to merge 1 commit into from
Closed

Fix format lookup when JSON or XML are defined as acronyms #361

wants to merge 1 commit into from

Conversation

odlp
Copy link

@odlp odlp commented Jun 14, 2021

When JSON or XML are defined as acronyms the format lookup fails:

ActiveSupport::Inflector.inflections do |inflect|
  inflect.acronym "XML"
  inflect.acronym "JSON"
end

ActiveResource::Formats[:xml]
# => NameError (uninitialized constant ActiveResource::Formats::XMLFormat)
# Did you mean?  ActiveResource::Formats::XmlFormat

ActiveResource::Formats[:json]
# => NameError (uninitialized constant ActiveResource::Formats::JSONFormat)
# Did you mean?  ActiveResource::Formats::JsonFormat

The proposed solution uses a hash to lookup built-in formats. For custom formats the implementation falls back to const_get after camelizing the MIME type reference. I didn't find many public examples of custom formats, so not sure these are widely used.

Here's an example of this issue in the wild, from the shopify_api gem:
Shopify/shopify-api-ruby#683

For custom 3rd-party formats the implementation falls back to
const_get after camelizing the MIME type reference.

Resolves Shopify/shopify-api-ruby#683
ActiveSupport::Inflector.inflections do |inflect|
# N.B. It's not yet possible to use the public ActiveSupport::Inflector::Inflections#clear
# API because it doesn't reset @acronyms to be a Hash (the default value),
# instead it sets an empty array.
Copy link
Author

@odlp odlp Jun 14, 2021

Choose a reason for hiding this comment

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

Planning to contribute a PR to Rails shortly to fix this.

Update: rails/rails#42475

@odlp
Copy link
Author

odlp commented Jun 14, 2021

Two of the builds are failing because activesupport-7.0.0.alpha requires ruby version >= 2.7.0

@rails-bot
Copy link

rails-bot bot commented Sep 13, 2021

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.

If it is an issue and you can still reproduce this error on the master branch,
please reply with all of the information you have about it in order to keep the issue open.

If it is a pull request and you are still interested on having it merged please make sure it can be merged clearly.

Thank you for all your contributions.

@rails-bot rails-bot bot added the stale label Sep 13, 2021
@rails-bot rails-bot bot closed this Sep 20, 2021
@odlp
Copy link
Author

odlp commented Oct 21, 2021

I think this is still an issue

@rafaelfranca
Copy link
Member

I don't think this is the right fix unfortunately. This removes the possibility of people to register they own format since they would have to modify that BUILT_IN constant. If we want to allow people to change the inflector of those formats and still have active resource working it would be better for us to stop using camelize all together to find the format. Does anyone want to write a new patch?

@odlp odlp deleted the fix-format-lookup-with-acronyms branch May 16, 2022 11:21
seanpdoyle added a commit to seanpdoyle/activeresource that referenced this pull request Jan 5, 2025
Related to rails#361

Add special-case treatment of JSON and XML acronyms by declaring aliases
for the `JsonFormat` and `XmlFormat` constants.

In addition to the aliases, this commit also includes test coverage to
cover existing behavior.

The original PR included a comment citing that
`ActiveSupport::Inflector::Inflections#clear` was not working as
expected. It was resolved by [a76344f][], which was merged into `main`
prior to the `7.0.0` release. Since the CI matrix currently includes
`7-0-stable` as the minimum version, the code can rely on the resolved
behavior.

[a76344f]: rails/rails@a76344f
@seanpdoyle
Copy link

I've opened #416 as a re-submission of this proposal.

seanpdoyle added a commit to seanpdoyle/activeresource that referenced this pull request Jan 5, 2025
Related to rails#361

Add special-case treatment of JSON and XML acronyms by declaring aliases
for the `JsonFormat` and `XmlFormat` constants.

In addition to the aliases, this commit also includes test coverage to
cover existing behavior.

The original PR included a comment citing that
`ActiveSupport::Inflector::Inflections#clear` was not working as
expected. It was resolved by [a76344f][], which was merged into `main`
prior to the `7.0.0` release. Since the CI matrix currently includes
`7-0-stable` as the minimum version, the code can rely on the resolved
behavior.

[a76344f]: rails/rails@a76344f
seanpdoyle added a commit to seanpdoyle/activeresource that referenced this pull request Jan 5, 2025
Related to rails#361

Add special-case treatment of JSON and XML acronyms by declaring aliases
for the `JsonFormat` and `XmlFormat` constants.

In addition to the aliases, this commit also includes test coverage to
cover existing behavior.

The original PR included a comment citing that
`ActiveSupport::Inflector::Inflections#clear` was not working as
expected. It was resolved by [a76344f][], which was merged into `main`
prior to the `7.0.0` release. Since the CI matrix currently includes
`7-0-stable` as the minimum version, the code can rely on the resolved
behavior.

[a76344f]: rails/rails@a76344f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants