If validations set upsert lookup column value, existing record is not found

[wezm]

I have a scenario where my model has a url : String field, which is the lookup column upsert_lookup_columns :url. This field is set indirectly through an attribute on the save operation:

  attribute project : String

  before_save validate_format_of project, with: /^[a-z0-9_-]+\/[a-z0-9_-]+$/i
  before_save validate_project

  def validate_project
    # simplifed example
    url.value = "https://example.com/#{project.value}"
  end

For this model upsert is not working properly. It always attempts to insert a new record, resulting in a unique key violation.

The issue seems to be that the upsert method tries to retrieve the existing record before the validations have been run, and thus in my case url is still nil:

https://github.com/luckyframework/avram/blob/0ea2a6140f5f9ac770eeaa5badf1d479a5b60f69/src/avram/upsert.cr#L72-L82

It does this query:

SELECT feeds.id, feeds.created_at, feeds.updated_at, feeds.refreshed_at, feeds.title, feeds.url, feeds.etag, feeds.last_modified, feeds.last_refresh_hash, feeds.source_slug FROM feeds WHERE feeds.url IS NULL ORDER BY feeds.id ASC LIMIT 1

Note the WHERE feeds.url IS NULL bit.

It gets no record and then proceeds to call save, which attempts to insert a new record. However, as part of save it first calls valid?, which in turn populates url and then when it comes to persist it, a duplicate row is attempted to be inserted.

[jwoertink]

oh that's interesting. I've actually seen a few unique constraint errors in my app, and never knew where they were coming from, but I think it's from an upsert because we use these quite a bit.

I guess this is a bit of a tricky one... If you have a before_save validate_these_are_required, and you haven't looked up the record yet, you may get validation errors. But then on the flip side, if you do the lookup first, and haven't set your values yet, then you'll run in to this....

Does this need a new before_lookup callback? Or do we just say that in cases like this, you need to pass a named arg for the lookup column? :thinking:

[wezm]

Or do we just say that in cases like this, you need to pass a named arg for the lookup column? 🤔

This would certainly be the easiest way forward for now.

[jwoertink]

I was just looking in to this more and it's pretty tricky. We would have to first move the run_default_validations and before_save calls out of the save method, then add those in before every call to save that happens all over. Then we run the issue of validations being ran twice on upserts when a record is found. I think it would be a breaking change too since you could no longer do this

op = SaveThing.new
op.whatever.value = "something"
# no validations or before_save would get ran....
op.save

Alternatively, we could have the upsert work a little different, and duplicate some of the logic over just in a different order. That could get messy though. I think I want to sit on this a little longer until I can think of a cleaner way to handle this.