Fix as_json call on ActiveModel::Type's child classes.

[nashby]

Motivation / Background

Right now since we have instance variable called itself_if_serialize_cast_value_compatible assigned to self when we run as_json we get stack too deep error because as_json calls as_json on every instance variable. And since @itself_if_serialize_cast_value_compatible references to self we run into recursion.

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.
• CI is passing.

[jonathanhefner]

What is the use case for calling as_json? Note that Type::Value and its descendents represent types, not values.

[nashby]

@jonathanhefner I noticed that something changed when we tried to upgrade Rails version in specs for enumerize gem. It has as_json method in the type class https://github.com/brainspec/enumerize/blob/d92c9e24d7b996e95369e0ed4e5ee92770323742/lib/enumerize/activerecord.rb#L125

[jonathanhefner]

@nashby Can you tell me why enumerize calls as_json, though? I see that you are an enumerize committer. How does enumerize use the result of as_json? From the code you linked, the JSON does not include the name of the type. So, for example, a Type::Boolean will look the same as a Type::Float:

{ attr: "my_attribute", subtype: ActiveRecord::Type.lookup(:boolean) }.as_json
# => {"attr"=>"my_attribute", "subtype"=>{"precision"=>nil, "scale"=>nil, "limit"=>nil}}

{ attr: "my_attribute", subtype: ActiveRecord::Type.lookup(:float) }.as_json
# => {"attr"=>"my_attribute", "subtype"=>{"precision"=>nil, "scale"=>nil, "limit"=>nil}}

More to the point, to the best of my knowledge, as_json is not part of the API for ActiveModel::Type::Value. If there were a strong enough use case, we could consider adding it. But if we did, the implementation would probably be explicit about what it returns; i.e. it would not simply return all the private instance variables.

[nashby]

@jonathanhefner honestly I don't really remember a real use-case for it in enumerize. Git history says it was needed for meta_request gem. But I see your point that calling to_json on a type doesn't show anything that useful. However I still think it makes sense to fix it because before that itself_if_serialize_cast_value_compatible it was working and somebody can rely on it to be working.

[rafaelfranca]

Thank you for the pull request @nashby and nice to see you contributing again to Rails! I don't think this object should respond to to_json, so maybe we should explicitly call undef :as_json?

[nashby]

@rafaelfranca hey hey! If "undefing" is common practice for Rails then it probably makes sense. I can change this PR is it's a way we choose to proceed

[rafaelfranca]

Yes. It is fine to use undef. In this case I see two option, either we implement as_json to return something useful (with your fix it would not) or we raise an error. I think "something useful" would be hard to implement since we don't know what is the use case. But we can raise a better error so people know to not use that method, so I think undef should be enough.

[nashby]

@rafaelfranca I thought about it a bit now and not sure if it's ok to potentially break someone's app with this undef change (I know it's unlikely that many people are trying to convert ActiveMode types to json but still). Wouldn't it be better to just return {} instead? (that's what to_json return when you try to call it on Object.new)

[rafaelfranca]

I think the point is that right now the application is already broken, only silently broken given the example that @jonathanhefner gave:

{ attr: "my_attribute", subtype: ActiveRecord::Type.lookup(:boolean) }.as_json
# => {"attr"=>"my_attribute", "subtype"=>{"precision"=>nil, "scale"=>nil, "limit"=>nil}}

{ attr: "my_attribute", subtype: ActiveRecord::Type.lookup(:float) }.as_json
# => {"attr"=>"my_attribute", "subtype"=>{"precision"=>nil, "scale"=>nil, "limit"=>nil}}

I don't think we should be returning anything is this object and the occasional breakage that this will cause to people is actually a good thing. They will realize their code isn't doing anything meaningful and will have to adapt. Of course I'd not backport this to a stable brach. If we need to do something in a stable branch, I'd go with this PR as it is right now. But for main, I think we should explicitly say to people to not try to convert a type to json because it isn't a operation you can roundtrip.

[nashby]

@rafaelfranca alright, it makes sense, thanks for explanation. I've updated the PR.

[rafaelfranca]

Ah, just saw the tests failed. It makes sense since as_json could be not defined when the undef executes. Let's define an as_json that raises a NoMethodError

[nashby]

@rafaelfranca it's updated now