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

Unable to upload or display uploaded images when using UUIDs and Postgres #71

Closed
j-mcnally opened this issue Dec 8, 2023 · 17 comments · Fixed by #72
Closed

Unable to upload or display uploaded images when using UUIDs and Postgres #71

j-mcnally opened this issue Dec 8, 2023 · 17 comments · Fixed by #72
Labels
bug Something isn't working

Comments

@j-mcnally
Copy link

j-mcnally commented Dec 8, 2023

I've setup active_storage but uploads keep failing...

Started POST "/maglev/api/assets" for ::1 at 2023-12-07 21:35:31 -0500
Processing by Maglev::Api::AssetsController#create as JSON
  Parameters: {"asset"=>{"file"=>#<ActionDispatch::Http::UploadedFile:0x000000010b8925e0 @tempfile=#<Tempfile:/var/folders/fd/04qkytls0xxc3nhjyccdw3vm0000gn/T/RackMultipart20231207-22731-6ci014.jpg>, @content_type="image/jpeg", @original_filename="fabian-centeno-uY60pJUHqOo-unsplash.jpg", @headers="Content-Disposition: form-data; name=\"asset[file]\"; filename=\"fabian-centeno-uY60pJUHqOo-unsplash.jpg\"\r\nContent-Type: image/jpeg\r\n">}}
  Maglev::Site Load (0.5ms)  SELECT "maglev_sites".* FROM "maglev_sites" ORDER BY "maglev_sites"."id" ASC LIMIT $1  [["LIMIT", 1]]
  TRANSACTION (1.3ms)  BEGIN
  Maglev::Asset Create (1.4ms)  INSERT INTO "maglev_assets" ("filename", "content_type", "width", "height", "byte_size", "created_at", "updated_at") VALUES ($1, $2, $3, $4, $5, $6, $7) RETURNING "id"  [["filename", nil], ["content_type", nil], ["width", nil], ["height", nil], ["byte_size", nil], ["created_at", "2023-12-08 02:35:31.630188"], ["updated_at", "2023-12-08 02:35:31.630188"]]
  ActiveStorage::Attachment Load (2.1ms)  SELECT "active_storage_attachments".* FROM "active_storage_attachments" WHERE "active_storage_attachments"."record_id" = $1 AND "active_storage_attachments"."record_type" = $2 AND "active_storage_attachments"."name" = $3 LIMIT $4  [["record_id", nil], ["record_type", "Maglev::Asset"], ["name", "file"], ["LIMIT", 1]]
  ActiveStorage::Blob Create (1.6ms)  INSERT INTO "active_storage_blobs" ("key", "filename", "content_type", "metadata", "service_name", "byte_size", "checksum", "created_at") VALUES ($1, $2, $3, $4, $5, $6, $7, $8) RETURNING "id"  [["key", "002ab95u4hdex998jpobmw5cpc4u"], ["filename", "fabian-centeno-uY60pJUHqOo-unsplash.jpg"], ["content_type", "image/jpeg"], ["metadata", "{\"identified\":true}"], ["service_name", "local"], ["byte_size", 1583948], ["checksum", "f1y2gVRUElO71YtYCE0nZw=="], ["created_at", "2023-12-08 02:35:31.637841"]]
  ActiveStorage::Attachment Create (1.3ms)  INSERT INTO "active_storage_attachments" ("name", "record_type", "record_id", "blob_id", "created_at") VALUES ($1, $2, $3, $4, $5) RETURNING "id"  [["name", "file"], ["record_type", "Maglev::Asset"], ["record_id", nil], ["blob_id", "89625400-95c5-42d7-9e8d-b91cb8fdf3b3"], ["created_at", "2023-12-08 02:35:31.640474"]]
  TRANSACTION (0.4ms)  ROLLBACK
Completed 500 Internal Server Error in 51ms (ActiveRecord: 20.4ms | Allocations: 38835)
`


ActiveRecord::NotNullViolation (PG::NotNullViolation: ERROR:  null value in column "record_id" of relation "active_storage_attachments" violates not-null constraint
DETAIL:  Failing row contains (4c6ed3a7-1022-467d-a048-1cc0e427e416, file, Maglev::Asset, null, 89625400-95c5-42d7-9e8d-b91cb8fdf3b3, 2023-12-08 02:35:31.640474).
):

@j-mcnally
Copy link
Author

j-mcnally commented Dec 8, 2023

Ok i figured out part 1.

The maglev migrations / active storage do not work with uuids

For clarity, ive modified all the maglev migrations copied over to use uuids. This works fine now, uploads are fixed.

It might be worth looking at the migrations and changing to use the orm id generator types like active storage does.

@j-mcnally
Copy link
Author

But still failing to serve the uploaded images so digging into that.

@j-mcnally
Copy link
Author

Ok well i had to patch Maglev::Asset to make this work, but theres probably a better way, would love some help.

Maglev::Asset.class_eval do
  def self.find(friendly_name)
    # Extract uuid from friendly_name using a regex
    uuid = friendly_name.match(/(?<uuid>[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12})/)['uuid']
    super(uuid)
  end

end

@did did added the bug Something isn't working label Dec 8, 2023
@did
Copy link
Contributor

did commented Dec 8, 2023

hey @j-mcnally. sorry for being late in the game. Let me read everything :-)

@j-mcnally
Copy link
Author

No worried @did appreciate you taking a peek for me. I have an unideal working solution, but feels like im hacking too much of the Maglev::Asset model, not really clear how its "supposed to work".

@j-mcnally
Copy link
Author

The failing error on asset load was

Started GET "/maglev/assets/17841ed1-bb76-4a79-bb56-90e95238da5b-fabian-centeno-uy60pjuhqoo-unsplash-1.jpg" for ::1 at 2023-12-08 11:14:34 -0500
Processing by Maglev::AssetsController#show as JPEG
  Parameters: {"id"=>"17841ed1-bb76-4a79-bb56-90e95238da5b-fabian-centeno-uy60pjuhqoo-unsplash-1"}
  Maglev::Asset Load (0.8ms)  SELECT "maglev_assets".* FROM "maglev_assets" WHERE "maglev_assets"."id" = $1 LIMIT $2  [["id", nil], ["LIMIT", 1]]
Completed 404 Not Found in 4ms (ActiveRecord: 0.8ms | Allocations: 1704)



ActiveRecord::RecordNotFound (Couldn't find Maglev::Asset with 'id'=17841ed1-bb76-4a79-bb56-90e95238da5b-fabian-centeno-uy60pjuhqoo-unsplash-1):

@j-mcnally j-mcnally changed the title Unable to upload images Unable to upload or display uploaded images when using UUIDs and Postgres Dec 8, 2023
@did
Copy link
Contributor

did commented Dec 8, 2023

Thanks for the clear explanation. I'm confused because Rails is supposed to take care of "friendly" urls by parsing ids (either integer based or UUID base). Hmm..
And writing a spec for this specific use case, it's not an easy task...

@j-mcnally
Copy link
Author

its likely some edge case with the "Friendly part" and "UUIDs" i noticed that if i pass anything other than a UUID to find, it auto nullls it out, likely something rails added for safety with UUIDs without considering how normal id lookups work. So unfortunately this feels like an upstream bug.

@j-mcnally
Copy link
Author

j-mcnally commented Dec 8, 2023

I never really knew it worked this way. But look at this http://tutorials.jumpstartlab.com/topics/controllers/friendly-urls.html it appears the friendly thing works because of the way .to_i works

Loading development environment (Rails 7.1.2)
3.2.2 :001 > "1-abc-123.jpg".to_i
=> 1
3.2.2 :002 >

which explains why this works ints and not uuids if the find call is having the slug have .to_i called on it.

My hottest take would be to use something other than find for the this one specific lookup use case and then allow it to be more safely overwritten with other id implementations.

@j-mcnally
Copy link
Author

This article explains how UUIDs or non valid uuids are converted to nil

https://pawelurbanek.com/uuid-order-rails

@did
Copy link
Contributor

did commented Dec 8, 2023

I knew about the to_i trick regarding friendly urls. And I also read Pawel's article!

Actually, we need to 2 things to make Maglev support uuids.

@did
Copy link
Contributor

did commented Dec 8, 2023

in the meantime, I'm going to play with a new Maglev site and uuids.

@j-mcnally
Copy link
Author

ActiveStorage handles this well i think, if you want to take a look

# This migration comes from active_storage (originally 20170806125915)
class CreateActiveStorageTables < ActiveRecord::Migration[7.0]
  def change
    # Use Active Record's configured type for primary and foreign keys
    primary_key_type, foreign_key_type = primary_and_foreign_key_types

    create_table :active_storage_blobs, id: primary_key_type do |t|
      t.string   :key,          null: false
      t.string   :filename,     null: false
      t.string   :content_type
      t.text     :metadata
      t.string   :service_name, null: false
      t.bigint   :byte_size,    null: false
      t.string   :checksum

      if connection.supports_datetime_with_precision?
        t.datetime :created_at, precision: 6, null: false
      else
        t.datetime :created_at, null: false
      end

      t.index [ :key ], unique: true
    end

    create_table :active_storage_attachments, id: primary_key_type do |t|
      t.string     :name,     null: false
      t.references :record,   null: false, polymorphic: true, index: false, type: foreign_key_type
      t.references :blob,     null: false, type: foreign_key_type

      if connection.supports_datetime_with_precision?
        t.datetime :created_at, precision: 6, null: false
      else
        t.datetime :created_at, null: false
      end

      t.index [ :record_type, :record_id, :name, :blob_id ], name: :index_active_storage_attachments_uniqueness, unique: true
      t.foreign_key :active_storage_blobs, column: :blob_id
    end

    create_table :active_storage_variant_records, id: primary_key_type do |t|
      t.belongs_to :blob, null: false, index: false, type: foreign_key_type
      t.string :variation_digest, null: false

      t.index [ :blob_id, :variation_digest ], name: :index_active_storage_variant_records_uniqueness, unique: true
      t.foreign_key :active_storage_blobs, column: :blob_id
    end
  end

  private
    def primary_and_foreign_key_types
      config = Rails.configuration.generators
      setting = config.options[config.orm][:primary_key_type]
      primary_key_type = setting || :primary_key
      foreign_key_type = setting || :bigint
      [primary_key_type, foreign_key_type]
    end
end

@j-mcnally
Copy link
Author

@did i really do appreciate you digging into this, hopefully things will continue to work well for me atleast in the meantime. Maglev seems awesome so far.

@did
Copy link
Contributor

did commented Dec 8, 2023

@j-mcnally you're very welcome and thanks for taking the time to find a solution!

@did did linked a pull request Dec 10, 2023 that will close this issue
@did
Copy link
Contributor

did commented Dec 10, 2023

hey @j-mcnally!
Thanks to this PR #72 (not yet merged), I successfully managed to create a Maglev site within an application which uses UUIDs.
Let me know if you see any issues or if you've comments.

@j-mcnally
Copy link
Author

this is great, the comprehensive support across models for uuids will be a win win i think for users.

@did did closed this as completed in #72 Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants