Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for root-relative path URLs to the disk service #32139

Conversation

pixeltrix
Copy link
Contributor

Also caches the url helpers module since that was being created every time an Active Storage url was generated.

@georgeclaghorn
Copy link
Contributor

We can’t do this by default. Content type identification for direct uploads requires that ActiveStorage::Blob#service_url return a fully-qualified URL.

@pixeltrix
Copy link
Contributor Author

@georgeclaghorn what if we moved identifiable_chunk to come from the service directly? Would be more efficient for the disk service then because it could do a file read instead of a http request.

@georgeclaghorn
Copy link
Contributor

I considered that before and was loath to add to the Service API, but I suppose it’s best.

The catch is that the official Google Cloud Storage client doesn’t support range downloads, so the GCS service will have to do roughly what Identification does now: generate a URL and manually issue a download request with a Range header.

@georgeclaghorn
Copy link
Contributor

If we go down that path, we can do away with the :host option altogether and always generate root-relative paths in the disk service. Identification was the only reason for switching to fully-qualified URLs.

@georgeclaghorn
Copy link
Contributor

georgeclaghorn commented Mar 5, 2018

@pixeltrix, I took the liberty of implementing your proposal in anticipation of tomorrow’s release candidate.

@pixeltrix
Copy link
Contributor Author

@georgeclaghorn thanks - was going to look at it at the weekend but ended up having to work. Do the docs need updating?

@pixeltrix pixeltrix deleted the add-support-for-root-relative-path-urls-to-the-disk-service branch March 6, 2018 12:57
@stevenharman
Copy link
Contributor

stevenharman commented Mar 7, 2018

UPDATE: I now realize that this makes host optional for the disk service. And when not provided, root-relative url values will be created. But if a host option is specified, a fully-qualified url value is generated. If so, please ignore the rest of this message.

ORIGINAL COMMENT:

If we go down that path, we can do away with the :host option altogether and always generate root-relative paths in the disk service. Identification was the only reason for switching to fully-qualified URLs.

I'm sure I'm missing something, but this will be problematic for any non-HTML forms (or Rails-provided JS lib) clients, no? For example, a native iOS/Android client.

We have a Rails UI which uses the built in direct upload stuff. But we also have an iOS client that needs to upload files. Our iOS client first creates a Blob via POST /api/uploads (which is a controller that inherits from ActiveStorage::DirectUploadsController). As of Rails 5.2.0.rc1 the response looked like this:

{
  "id": 1,
  "key": "AHPU4cV8yHWYzoGfaaRuTQzg",
  "filename": "some-file.png",
  "content_type": "image/png",
  "metadata": {},
  "byte_size": 15844,
  "checksum": "dHJ4QIEDJy7mxfm0IE09xw==",
  "created_at": "2018-03-07T20:17:36.756Z",
  "signed_id": "eyJfcmFpbHMiOnsibWVzc2FnZSI6IkJBaHBCZz09IiwiZXhwIjpudWxsLCJwdXIiOiJibG9iX2lkIn19--5e5775d4c690a6a4171abb69e25e50e91f8a61cd",
  "direct_upload": {
    "url": "http://localhost:5000/rails/active_storage/disk/eyJfcmFpbHMiOnsibWVzc2FnZSI6IkJBaDdDVG9JYTJWNVNTSWRRVWhRVlRSalZqaDVTRmRaZW05SFptRmhVblZVVVhwbkJqb0dSVlE2RVdOdmJuUmxiblJmZEhsd1pVa2lEbWx0WVdkbEwzQnVad1k3QmxRNkUyTnZiblJsYm5SZmJHVnVaM1JvYVFMa1BUb05ZMmhsWTJ0emRXMUpJaDFrU0VvMFVVbEZSRXA1TjIxNFptMHdTVVV3T1hoM1BUMEdPd1pVIiwiZXhwIjoiMjAxOC0wMy0wN1QyMDoyMjozNi43ODBaIiwicHVyIjoiYmxvYl90b2tlbiJ9fQ==--c639a4499a4159ae7c6d3abcf4ce8a89b23ba964",
    "headers": {
      "Content-Type": "image/png"
    }
  }
}

The client then pulls out the direct_upload.url to know where to PUT the file. Will that URL be changing to root-relative with this PR? If so, that's quite a change for clients. They now have to know about the storage backed being used by Rails, rather than blindly PUT-ing to the direct_upload.url.

Am I understanding this correctly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants