Fix invalid direct upload form attribute

[gmcgibbon]

#38957

Closes #43698

[brenogazzola]

Found another possible problem (not sure if I should open another issue). If the form is not given a model, only a scope, it will not generate the data-direct-upload-token or data-direct-upload-attachment-name, and the upload will not work.

[gmcgibbon]

Found another possible problem (not sure if I should open another issue). If the form is not given a model, only a scope, it will not generate the data-direct-upload-token or data-direct-upload-attachment-name, and the upload will not work.

I think we should raise in that situation. Supplying the direct_upload key is clear intent that the developer wants to use Active Storage but we don't have a model to inflect on. I'll add that to this PR. Thanks!

[brenogazzola]

Might be a good idea to add this to the upgrade guide so others can prepare in advance, as well as a notice about the change in the JS library. If I had not checked the discussion in the original PR I’d have been pretty confused.

[brenogazzola]

I'm not sure if I'm missing something in the code, or if it was not considered in the original PR:

How do I handle form objects, which are simple POROs and not active record models?

[brenogazzola]

One more thing: Would it be possible to turn off the token, at least in the test env? Since we upgraded, some system tests that we never had a problem with are now flakky due to InvalidDirectUploadTokenError.

[gmcgibbon]

I'm not sure if I'm missing something in the code, or if it was not considered in the original PR:
How do I handle form objects, which are simple POROs and not active record models?

It wasn't considered. Since upload tokens are coupled to Active Record models, you need a model and attachment to tie an upload to. I don't think plain Active Model would work with currently either. Can you provide an example snippet or test case to show how this previously worked? I can add test coverage so these contracts are better enforced.

[gmcgibbon]

One more thing: Would it be possible to turn off the token, at least in the test env? Since we upgraded, some system tests that we never had a problem with are now flakky due to InvalidDirectUploadTokenError.

We can make multi-service uploads opt-in depending on how much it is breaking apps. The feature seems to still need some work on the action view integration end. I'll review that once this patch is in. The unreleased changes to JS libraries may be causing your app some issues too if you aren't linking them to your app.

[brenogazzola]

It wasn't considered. Since upload tokens are coupled to Active Record models, you need a model and attachment to tie an upload to. I don't think plain Active Model would work with currently either. Can you provide an example snippet or test case to show how this previously worked? I can add test coverage so these contracts are better enforced.

Previously direct upload made no assumptions about what its file was going to be attached to. It only uploaded the file to the configured service and returned to token that was used.

Here's the most bare bones use case that used to work and no longer does. Instead of using a form object, it uses a plain file_field_tag. I've also intentionally removed the attribute (nil) to ensure direct upload has absolutely no information about where the file is going to be attached.

View:

<%= form_with url: photos_path do |form| %>
  <%= file_field_tag nil, name: "photo[file]", direct_upload: true %>
  <%= form.submit "Save" %>
<% end %>

Controller:

def create
  Photo.create!(photos_params)
  head :ok
end

private
  def photo_params
    params.require(:photo).permit(:file)
  end

Model:

class Photo
  has_one_attached :file
end

[brenogazzola]

We can make multi-service uploads opt-in depending on how much it is breaking apps. The feature seems to still need some work on the action view integration end. I'll review that once this patch is in. The unreleased changes to JS libraries may be causing your app some issues too if you aren't linking them to your app.

I haven't read through the entire implementation, but would it be possible to modify the direct_upload attribute to take a string with the model/attachment instead of a bool? That would solve most of the problems:

<%= file_field_tag nil, name: "photo[file]", direct_upload: "photo#file %>

[gmcgibbon]

Here's the most bare bones use case that used to work and no longer does

Is the photo class an Active Model, Active Record, or something else? There's no superclass specified.

but would it be possible to modify the direct_upload attribute to take a string with the model/attachment instead of a bool?

Yes, that's what I have in this PR to fix the contract of file_field_tag, but in a hash. file_field can still use direct_upload: true because the model and attachment can be inferred from the options[:object]. The approach would be <%= file_field_tag nil, name: "photo[file]", direct_upload: { model: "Photo", attachment: :file } %>, though I had thought about just specifying a token. I'm open to suggestions.

[brenogazzola]

Sorry, Photo is a normal model. But in this case it does not make much difference. Since we are using a standard tag helper not a form builder one, direc upload does not have access to the forms object.

I’ve pulled the JS file from the gem into my JS folder and included it in my bundle. The uploads are all working now, but are still flakky in the CI.

[brenogazzola]

Thinking a bit, couldn’t the direct uplod controller assume that if no attachment name is provided it can simply upload to the default service? And then only create the attachment name/token if the attachment in question defined a service other than the default?

This would mean no migration burden for projects already using active storage, while supporting projects that need per attachment service.

[brenogazzola]

Hey @gmcgibbon anything I can help with this? I’m not skilled enough with JS to help there, but I’m fine with the ruby part.

[gmcgibbon]

Thinking a bit, couldn’t the direct uplod controller assume that if no attachment name is provided it can simply upload to the default service?

Yes, that's possible, but I'd rather it be consistent for all uploads to have tokens included. If this breaks apps, we could just make it opt-in behaviour while upgrading, but I'm not convinced that's necessary yet.

The uploads are all working now, but are still flakky in the CI.

If you could debug why the tests are flaky on your end, that would be helpful. I wouldn't expect them to be. I should have time to work on this next week.

[brenogazzola]

Yes, that's possible, but I'd rather it be consistent for all uploads to have tokens included.

Alright. With your PR it’s not as much extra work as we had with the original one, so it sounds like a good compromise between correctness and ease.

If you could debug why the tests are flaky on your end,

I’ll see if I can figure it out. They were ocasionallu breaking in production too. File uploads worked most of the time but every once in a while I’d get an invalid token error.

[brenogazzola]

A user in issue #43895 has mentioned another use case that is no longer supported: Using direct upload without a session. Seems he has a react app and disable CSRF tokens and sessions.

[gmcgibbon]

Sounds like we're rewriting this feature, and the implementation will change significantly, so I'm going to close this.