Bad date input lost before validations can execute

[gap777]

Context

I have a jsonb attribute on my model that contains an array of dates:

class MyModel < ApplicationRecord
  include AttrJson::Record
  attr_json :recurrence_dates,
              :date,
              array: true,
              default: [],
              store_key: 'rdates',
              container_attribute: "schedule_rules"
  validate :recurrence_dates_valid

  def recurrence_dates_valid
      return if recurrence_dates.blank?

      if recurrence_dates_changed?
        recurrence_dates_before_type_cast.reject { |r| valid_iso_date?(r) }.each do |invalid_date|
          errors.add(:recurrence_dates, :invalid, message: "#{invalid_date} is invalid")
        end
      end
  end

The bug

My rspec test passing a bad value for recurrence_date during initialization:

RSpec.describe MyModel, type: :model do
    context 'validations' do
      it 'validates recurrence dates' do
        other = MyModel.create(name: 'test', recurrence_dates: ['bad date'])
        expect(other).not_to be_valid
        expect(other.errors.full_messages).to include('Recurrence dates bad date is invalid')
      end

when I create a new model with invalid dates, I want my custom error message to inform my user of what they typed which was invalid (in this case, ISO date format only).

Unfortunately, both recurrence_dates_before_type_cast and recurrence_dates contains an array w/ 1 element, which is a nil.

Analysis

It seems to stem from attr_json/record.rb L209 - L214:

super(value) # should write to rails attribute

# write to container hash, with value read from attribute to try to keep objects
# sync'd to exact same object in rails attribute and container hash.
attribute_def = self.class.attr_json_registry.fetch(name.to_sym)
public_send(attribute_def.container_attribute)[attribute_def.store_key] = read_attribute(name)

in an attempt to keep the Rails attribute for recurrence_dates in sync with the JSONB Rails attribute (schedule_rules):

1. the bad value is sent to the recurrence_dates setter. At this point, recurrence_dates has [nil] and recurrence_dates_before_type_cast has ['bad value'].

2. the [nil] is stored in the JSONB attribute (read_attribute(name) is used instead of value on L214.

Request

Can you provide a fix, so that the recurrence_dates_before_type_cast correctly reports ['bad value'] by the time my validation function is invoked?

[jrochkind]

Hi, thanks for reporting this!

So I think the way activerecord typing works, the value is coerced to the type on assignment.

Like, you want to say recurrence_dates is of type date (or in this case an array of date), but sometimes you want to look at .recurrence_dates and get back an array that includes things that are not dates, you want the array to include a string that is not a date?

I think that's just impossible with how ActiveRecord works! If you say it's a date, it can only include objects of class Date, it can't ever, including in memory before saving, include a string that isn't a date.

Let me know if I'm not understanding your case right?

The goal here is to make attr_json values behave as much like ordinarily ActiveRecord attributes as we can. To compare, can you create an actual column in your database of type date, and see how it behaves? If you assign an "invalid" value to it, what happens? Is there any way to see the original input before validation in that case?

If I'm misunderstanding, and you want to do a thing you can do with an "ordinary" ActiveRecord attribute, I'll take another look! Having the "ordinary" ActiveRecord example to compare would be useful. But otherwise, I think this is just part of the design of ActiveRecord attribute typing maybe! It might be interesting for us to look up how people handle validation of manually entered data in "ordinary" ActiveRecord date/time columns -- I'm not sure!

[jrochkind]

Ah wait, I see your use of before_type_cast now. Hmm.

If you could provide the example of an "ordinary" Rails attribute that works doing what you want, that would still be helpful?

Maybe you can provide a complete Rails app (or try to do a rails app in a single file like Rails docs show you how to do somewhere, can't find right now) with a passing test case for the ordinary rails attribute and failing one for att_json?

I am not sure what's going on, or how hard it might be to fix this! I have not looked at this kind of thing before.

I would also be very curious as to whether it works with any of our peer apps doing smiilar things, like https://github.com/madeintandem/jsonb_accessor or https://github.com/DmitryTsepelev/store_model

If you or anyone else wants to do more research on those questions, it would be a contribution to this open source! Or certainly if you have a suggestion for PR! I am not certain when I will have time to do the research.

I appreciate you pointing out the way the syncing between the ActiveModel attribute and the json value is maybe causing the problem... not sure what can be done about that though, hm.

[gap777]

@jrochkind Thanks for thinking about this with me!

For context, we're currently using jsonb_accessor gem. We first experienced this bug there, and his response was to check out the peer apps like you. ;-).

Note also that the behavior I'm desiring works for the update cycle. Why I consider this a bug is because it only fails during the create cycle. That is, given an existing model, model.update(recurrence_dates: ['bad value']) results in the desired behavior (the validation error message which includes the bad value itself accessed via before_type_cast).

[jrochkind]

I think I understand better now... but i'm not sure how feasible it will be to fix it, or if it might take major redesigns (with backwards incompat and giving up on some features)...

I often look to jsonb_acessor to see if things broken here work right there, cause if THEY can do it i"m like okay I can copy what they did!

But if it's broken there too...

We kind of have to do some hacky things to do what we want with AR, which generally works amazingly smoothly, but occaisonally gets us into a roadblock. Here in attr_json originally I wanted to keep the value in the actual underlying JSON always "live", along with the value in teh rails attribute... I have sometimes wondered if giving up on that, and sync'ing only on save would make integration smoother. Like if you look at the in-memory json before a save, it won't actually have your changes, it'll just get updated to have your changes on save. But I think that may be what jsonb_accessor does, which I guess didn't save it from this!

If you felt like looking at https://github.com/DmitryTsepelev/store_model , I'd be very curious if it has the same problem or not!

I'm honestly not sure when/if i'll have time to really dig into this -- I think it would take some serious digging -- but will definitely leave the issue open. if you were able to come up with a PR, I'd be interested! Although if it breaks existing tests/functionality... would have to think about how/if to roll that out.