get out of the way of rails 5.2 ActiveModel::Attributes

[jrochkind]

Rails 5.2 has a real attributes API for ActiveModel.

We're doing okay without it, and supporting Rails 5.0-5.2, but let's rename some of our methods and ivars in JsonAttribute::Model to step out of it's way and not conflict, to avoid any weirdness, and in case someone wants to use both.

(Jan 18 2024: We really should move to the models having ActiveModel attributes, and maybe include ActiveModel::API by defualt. See some things possible at https://betacraft.com/blog/08/06/2023/active-model-jsonb-coulmn.html, although it's not totally clear how this gets integrated with an AR container, as they suggest. https://www.reddit.com/r/ruby/comments/199yvhl/progressively_adopt_activemodelapi_for_your_poros/).

[jrochkind]

Actually, can we deprecate AttrJson::Model entirely, in favor of ActiveModel::Attributes? Possibly provide a custom type that can serialize AttrJson::Attributes properly in the context of AttrJson::Record serialization and model-inside-model nesting.

[jrochkind]

Might mean giving up some features. Ones identified include:

• store_key -- let key stored in db be different than method name. Does this need to be a feature, you can easily provide your own wrapper methods?
• configurable bad_cast -- is this necessary? What would "natural" behavior be with ActiveModel::Model, would that be enough?

Maybe we don't need these. Or maybe we could replace with a light wrapper over ActiveModel::Model,Attributes,Serialization.

[jrochkind]

Yep, returning to this.... seems to mean giving up .attr_json_config(unknown_key: :allow), which is kind of important after all I think.

store_key probably is too, although there might be. a workaround. Not sure about attr_json_config(bad_cast: :as_nil).

Not quite sure what to do. Coming back to this, I forgot about these notes, and thought this would be pretty easy. I am doing this for a future 2.x release, so features can be dropped, but I don't think we want to drop store_key or unknown_key config. Hmm.

MAYBE we could drop unknown_key: allow specifically.

[jrochkind]

Another thing that ActiveModel::Attributes does differently than our implementation:

• in our implementation, an attribute you never set at all (including no default), does not show up in the attributes hash at all, and does not get serialized to json. In ActiveModel::Attributes, effectively all things default to nil, and all attributes are in hash and serialization to json.
  • This might be a fine (major version breaking) change?
    • But it also seems to break a bunch of our other tests, including things around our json querying. Have to look into it. It's proving much more work than I had hoped.

[jrochkind]

OK I delved into an include ActiveModel::Attributes based structure, and ran into some architectural pain points. I am curious for feedback for those who previously expressed interest in this -- if you're still using attr_json, what is your interest/use case involving AM::Attributes?

Right now, AttrJson::Model can distinguish between "unset value" and "value set to nil" (since it is using a Hash where these things are different).

ActiveModel::Attributes can not do that. By default, it's JSON represnetation includes keys for all defined attributes, including with nil values. That seemed maybe a bit annoying to have in the DB?

I can change it to compact nils... but then, every time you de-serialize you have all default values applied on top of anything that was missing, including things you meant to explicitly set to nil instead of their default.

I am considering abandoning attempt to actually base on or be compatible with ActiveModel::Attributes. And instead just writing some more code to "look like" AM::Attributes a bit more. (Fix dup and freeze based on AM::Attributes implementation).

AM::Attributes itself is some very convoluted code including things that I think are used by ActiveRecord but just aren't necessarily and I don't know what the're doing there in a model context. I suspect there were plans for more that never went anywhere, AM::Attributes hasnt' really been touched since sgrif left rails.

I will take the liberty of tagging those who thumbs-upped this issue over the years. @stevenharman, @swrobel, @rainerborene, @aried3r, @TylerRick, @jits, @huobazi, @exsemt, @gilbert, @thomasklemm

[jrochkind]

So I realized we had to/wanted to take care of that nil serialization issue either way. So did so in #175

So theoretically we could base the implementation on ActiveModel::Attributes now... but it still makes me nervous. I'm not sure if it's a good idea; I think we have semantics basically the same now -- which means we could probably switch to it later, even without a major version, if there's a reason to.

If the upcoming (prob Rails 7.1) normalizes feature were going to be useable in an ActiveModel::Attributes, that might be a reason that it mattered to be actual ActiveModel::Attributes, but that does not look in the cards for now.

For now I think I will leave things as they are -- in our upcoming 2.x release the things should be more-or-less indistinguishable from actual ActiveModel::Attributes, but won't actually use that module.

But always interested in feedback, or use cases that are made a problem by not actually being ActiveModel::Attributes.

I will perhaps leave this ticket open?

[paulodeon]

A use case I just encountered where we are using attr_json to fetch things out of the DB but we wanted to use AM:Attributes to add temporary accessors while running the code with the benefits of defaults, typecasting etc, normalizing will also be great once out.

I wasnt aware of this issue and saw that all the attr_jsons were set to nil. There are plenty of workarounds, but as attributes become more useful, this will eventually be needed

[jrochkind]

Hi, thanks for the feedback @paulodeon, I appreciate it!

Hm. Well, maybe we can get this in there before I release attr_json 2.0, I'll mess with it some more.

Can you explain more about:

I wasnt aware of this issue and saw that all the attr_jsons were set to nil

I don't understand what you mean there. "all the attr_jsons were set to nil"? Ah, are you saying you tried to manually include ActiveModel::Attributes into your class that also had include AttrJson::Model in it, and you found this made AttrJson::Model break? That I would expect, if so. Hmm.

normalizing will also be great once out.

Be aware that as far as I can tell, there's not currently anything committed to Rails -- or as far as I know planned -- that will let you use the new normalization features with ActiveModel::Model classes. they are only at present for ActiveRecord::Base.

Now... here's the thing though, about your use case... if I do what I am thinking of to merge ActiveModel::Attributes in with AttrJson::Model... you would still not be able to add temporary accessors that are not persisted , if that's what you meant? If I actually merge things, then anything you declare with ActiveModel::Attributes would end up serialized and persisted too.

If your use case requires the ability to declare attributes that are NOT persisted.... that requires more thought to figure out how to meet. Just using ActiveModel::Attributes in the way I was thinking wouldn't do it. If I met that use case in AttrJson in a way that did not use ActiveModel::Attributes, would that still do for you? Say, if you set store_key: false on the attr_json, perhaps.

(Note that other alternative solutions to AttrJson that have nested models that are ActiveModel::Attributes based -- also I think don't meet your use case , I think they will all serialize all attributes! That is the most 'natural' way to implement it).

[paulodeon]

Hey @jrochkind, thanks for the reply. Here's a little code as an example, but yes, you got it right about the nils.

class AccessGroup
  include AttrJson::Model
  include ActiveModel::Attributes

  attr_json :id, :string, default: -> { SecureRandom.alphanumeric(8) }
  attr_json :name, Name.to_type

  attribute :for_option_model, :boolean, default: false
end

class App < ApplicationRecord
  attr_json :access_group, AccessGroup.to_type
end

App.new.access_group.id # => nil

The for_option_model is an attribute I'll use during the lifecycle of the object which is not persisted

Also thanks for the info about the normalization, but I would guess given past history AR Features tend to work their way to AM, over time.

Now I recognise that my use case may be an unusual one and as you say store_key: false would certainly cater for it. But allow me to make an argument against merging with ActiveModel::Attributes.

ActiveModel::Attributes is specifically designed for attributes that are not persisted. By merging with it you are in fact using it to persist attributes. In addition you would be removing a piece of rails functionality, which isnt ideal either.

Conceptually attribute and attr_json are in fact different things.

I can see that it would make it more difficult to implement all the AM features here but surely there is also some value from being a non-AM solution?

Perhaps adding some info to the docs suggesting why this is better than using AM directly?

[jrochkind]

OK thanks @paulodeon ! I appreciate the discussion! (Also some history is that the original attr-json implementation actually precedes ActiveModel::Attributes existing!)

The thing is, that the solution I was thinking of wouldn't meet your use case either....

ActiveModel::Attributes is specifically designed for attributes that are not persisted

I don't see it that way. ActiveModel::Attributes is designed for adding typed attributes anywhere; you can use it in ActiveRecords including on top of your persisted attributes to provide different typing; you can also use it in a pure-ruby model with typed attributes.

AttrJson is designed for taking pure-ruby models and letting you serialize them to json (in nested ways).

The thing is that if I completely stay away from AM::Attributes without merging with it... our AttrJson::Models become much less conventional ruby/Rails objects. Like... if you call model.to_h, right now you get a hash of the AttrJson attributes. If you call model.as_json or to_json or serializable_hash, right now, you get a hash of your AttrJson attributes. In the 99% of cases that don't need the non-persisted "virtual" attributes you do -- I think this is right and makes the thing behave like a well-behaved ruby and rails citizen. (the actual internal implementation also depends on as_json giving you the thing you are going to serialize in attr_json -- which makes sense, right?!)

BUT, in a straight ActiveModel::Attributes hash, to_h, as_json, to_json all give you the hash of your ActiveModel::Attributes. Which should it give you if you have both involved? One, the other, a merged set of both? Should it change depending on what you have included? (Heavens no!) Would we somehow use ActiveModel::Attributes, but override enough stuff that all your AM::Attributes are left out of to_h, as_json, to_json, etc, even though normal AM::Attributes would behave differently? That sounds awfully messy and confusing.

I don't think what you are asking for is realistic, if you think through all the edge cases. (In another alternate implementation I know of for JSON serialization to DB, which is based on AM::Attributes -- all AM::Attributes are serialized. https://github.com/DmitryTsepelev/store_model -- that makes sense to me)

BUT, as far as your actual use case....

Would you be interested in a:

      attr_json :for_option_model, :boolean, default: false, store_key: false

That would let you define an attr_json that is a "virtual" attribute that is not persisted. (store_key already exists, to let you define a json key different from the ruby attribute/method name. Currently store_key: false would be an error).

If you are interested in that, I think it's actually pretty easy to add! Do you want to file a separate issue on it?