Skip to content

Add url calculation to attachment resource - #12

Merged
zachdaniel merged 3 commits into
ash-project:mainfrom
sodapopcan:add-url-calculation-to-blob
Apr 26, 2026
Merged

Add url calculation to attachment resource#12
zachdaniel merged 3 commits into
ash-project:mainfrom
sodapopcan:add-url-calculation-to-blob

Conversation

@sodapopcan

@sodapopcan sodapopcan commented Apr 25, 2026

Copy link
Copy Markdown
Contributor

Contributor checklist

Leave anything that you believe does not apply unchecked.

  • I accept the AI Policy, or AI was not used in the creation of this PR.
  • Bug fixes include regression tests
  • Chores
  • Documentation changes
  • Features include unit/acceptance tests
  • Refactoring
  • Update dependencies

First off, sorry @StephanH90 if you were planning on doing this, I just wanted this ASAP so I put this together. If you have WIP you were hoping to PR, I'm happy to close this in favour of anything you've done (not a loss for me as I learned a thing or two!)


Adds url directly to the attachment resource as per #5.

Design Trade-off

This implementation threads the parent resource through the calculation in order to get the base_url. Alternatively, we could store the base_url in AshStorage.Service.Disk.service_opts_fields however this would be a breaking change. Note that it would only be breaking for users who would want to use this new functionality and in order to do so, would need to back-fill.

Basically this:
defmodule MyApp.Repo.Migrations.BackfillBlobBaseUrl do
  use Ecto.Migration

  def up do
    execute """
    UPDATE storage_blobs
    SET service_opts = jsonb_set(COALESCE(service_opts, '{}'), '{base_url}', '"/storage"')
    WHERE service_name = 'Elixir.AshStorage.Service.Disk'
    AND (service_opts IS NULL OR NOT service_opts ? 'base_url')
    """
  end

  def down do
    execute """
    UPDATE storage_blobs
    SET service_opts = service_opts - 'base_url'
    WHERE service_name = 'Elixir.AshStorage.Service.Disk'
    """
  end
end

I want to note that I use AI heavily to produce these changes. I spent around 1.5 hours on this PR going back and forth with Calude Code, gaining a decent grasp of what is going on but I cannot say that I yet have a deep understanding of the inner workings of ash_storage (having built things like this before I have a very good high level understanding I'm just talking more Ash stuff, like Spark).

I have also tried this out in my current project and all seems good there.

@sodapopcan

Copy link
Copy Markdown
Contributor Author

Also, small nitpick I noticed: all calculations define an init/1 that doesn't do anything different than the default. I just did the same though if they are unnecessary this seems like a decent PR to remove them in.

@zachdaniel

Copy link
Copy Markdown
Contributor

Down to remove the unnecessary callbacks. Was using them at some point and removed the check w/o removing the callback.

1 similar comment
@zachdaniel

Copy link
Copy Markdown
Contributor

Down to remove the unnecessary callbacks. Was using them at some point and removed the check w/o removing the callback.

@sodapopcan

Copy link
Copy Markdown
Contributor Author

Down to remove the unnecessary callbacks. Was using them at some point and removed the check w/o removing the callback.

Done!

@sodapopcan
sodapopcan force-pushed the add-url-calculation-to-blob branch from a44a8f9 to 6963385 Compare April 25, 2026 18:26
@sodapopcan

Copy link
Copy Markdown
Contributor Author

I'm working through the CI issues.

@sodapopcan
sodapopcan force-pushed the add-url-calculation-to-blob branch from 84c39ec to 821e2fc Compare April 25, 2026 19:08
@sodapopcan

Copy link
Copy Markdown
Contributor Author

I'm having a very hard time understanding what is wrong with CI. if Code.ensure_loaded?(Phoenix.PubSub) is directly above the line that fails on a call to Phoenix.PubSub.broadcast. It seems to be happening in main as well, though, at least locally. I'm a bit at a loss atm the moment but I can come back to it if this isn't familiar to you.

@sodapopcan
sodapopcan force-pushed the add-url-calculation-to-blob branch from 821e2fc to 7d6e4d1 Compare April 26, 2026 13:35
@zachdaniel
zachdaniel merged commit e456fae into ash-project:main Apr 26, 2026
23 of 24 checks passed
@zachdaniel

Copy link
Copy Markdown
Contributor

🚀 Thank you for your contribution! 🚀

(we just need to use apply there)

@sodapopcan

Copy link
Copy Markdown
Contributor Author

Oh, I could have made that change before you merged! AI actually suggested exactly this change but I wasn't sure about it 😅

@zachdaniel

Copy link
Copy Markdown
Contributor

It's all good, was easy enough to update! I was excited to get this in 😆

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.

2 participants